Repository navigation
CSTACKEX-262: Support for Compute + Volume migration and Volume migration - #112
sandeeplocharla wants to merge 17 commits into
Conversation
15f8e29 to
ddcebda
Compare
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.86% |
| Branch coverage | 18.99% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
e5c27c5 to
9416f32
Compare
9416f32 to
7bca4ca
Compare
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
575b714 to
26664a6
Compare
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 25.96% |
| Branch coverage | 20.26% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
26664a6 to
be02af2
Compare
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
piyush5netapp
left a comment
piyush5netapp
left a comment
There was a problem hiding this comment.
Approving with some comments which can be taken in future.
Will wait for one comment reply on file LibvirtMigrateCommandWrapper |
- LunMap: drop property-level NON_DEFAULT JsonInclude on logicalUnitNumber - KVMStorageProcessor: prefer copy details over command options, narrow not-found handling to managed volumes - OntapDataMotionStrategy: add disconnectVolumeFromHost helper used by failed and post-migration cleanup - LibvirtMigrateCommandWrapper: remove the disconnect wrapper method and its test - Add unit tests for the copy-details precedence and the new cleanup path
…ration QoS Offline migration calls createAsync with a null callback and reads no result, so a failed create must throw instead of returning silently. The post-migration QoS reapply now retries twice more with a short wait, then logs and continues, because the VM has already moved and a failed patch must not expunge its disks.
| * source volume's IOPS so the policy attached after {@code copy.source} matches the disk. | ||
| */ | ||
| private void copyQosIopsFromSourceWhenDestinationHasNone(VolumeInfo srcVolumeInfo, VolumeInfo destVolumeInfo) { | ||
| if (srcVolumeInfo == null || destVolumeInfo == null |
There was a problem hiding this comment.
in what circumstances destination would have IOps available ?
There was a problem hiding this comment.
Destination by itself wouldn't have any existing IOPS. Cloudstack orchestration does a copy within the DB from source volume entity to destination and the IOPS is not one of the entries which get copied. So, in that case this method would set the source IOPS to destination volume. Also, in case of an exception cloudstack goes for a retry and this method would not go through copying the same again as the destination volume would have the details already.
|
|
||
| success = true; | ||
| handlePostMigration(srcVolumeInfoToDestVolumeInfo, srcHost); | ||
| } catch (AgentUnavailableException | OperationTimedoutException | CloudRuntimeException ex) { |
There was a problem hiding this comment.
we should have another super set of exception handling to have smooth exit with errorMsg value assigned to the variable, right?
There was a problem hiding this comment.
Gone through a bit in depth for this. From my understanding, there's only a DAO error from 311 which might not be under the purview of any of the exceptions mentioned. If it's a big issue like the DB is down completely, then the call shouldn't even reach here, but if its intermittent, then this workflow should go through in one of the API Cmd retries by cloudstack orchestration.
I feel adding a generic exception might not add much here, but please let me know your thoughts on this.
There was a problem hiding this comment.
it is more about in worst cases also are we ensuring that product is not misbehaving and it remian in-sync with internal functionality and the responses coming out for user. It can be dao specific exception now, but it is good practice to handle cases for any probability.
There was a problem hiding this comment.
Sure, will add it. This file has majority code borrowed from StorageSystemDataMotionStrategy class, so was only checking why they've added specific exceptions instead of handling 'Exception' overall.
Anyways, how do you think this should be handled?
Shall I replace the 'AgentUnavailableException | OperationTimedoutException | CloudRuntimeException' with just 'Exception', as all that catch block is doing is to log an error and throw a CloudRuntimeException with the errMsg.
If we add additional catch block for 'Exception' with similar code, it would look duplicate.
| sendModifyTargetsCommand(getModifyTargetsCommand(storagePoolId, iqn, false), host); | ||
| } | ||
|
|
||
| private ModifyTargetsCommand getModifyTargetsCommand(long storagePoolId, String iqn, boolean add) { |
There was a problem hiding this comment.
this method would help in cluster-to-cluster migration, but VM is being migrated from one host to another with in the same cluster then post migration, lun may be disconnected, can you validate this ?
There was a problem hiding this comment.
I've cross checked the automation tests for relevant tests. Tests 6-10 of suite 1 (test_01_live_vm_with_storage_migration) in the PR #113 verifies this.
There was a problem hiding this comment.
Are we not calling this method if migration is happening between the hosts from the same cluster ?
There was a problem hiding this comment.
Yes, same cluster or different cluster, flow remains the same. The flow only changes based on whether there's a VM attached or not.
| _volumeDao.update(volumeVO.getId(), volumeVO); | ||
| // Host-side rewrite clears NFS file QoS; re-attach from the destination volume's IOPS. | ||
| reapplyOntapQosAfterContentRewrite(destVolumeInfo); | ||
| } catch (Exception ex) { |
There was a problem hiding this comment.
We should check for path for null, if it is not null then we should perform rollback by deleting the migrated object.
There was a problem hiding this comment.
The createAsync that runs from ln 162 would make sure that the (path != null) on success. In case of its failure, there's already handler in the finally.
FYI (Additional info)
Also, one thing to highlight, there's no rollback of the migrated volume in cloudstack orchestration, once the migration of volume is complete and there's some issue later. This is one of the major reasons why ontap related checks were added in core cloudstack code. The checks would verify the compatibility in migrating both ROOT and DATA disks, as this class would receive migration request per volume. Say, if ROOT migrated but DATA couldn't be due to compatibility issues, the ROOT volume wouldn't move back.
There was a problem hiding this comment.
My intention is to have this method level rollback. And in case of copy would have been created at the destination and post to that exception occurs, do we need to clean that storage object created at the destination in catch block?
I did not find desired code at line number 162 of this class. Yeah, you added the remove details from the cloudstacj database in recent commit, do we need to delete the storage object also if path is having value?
There was a problem hiding this comment.
ln 162 ==> 1destVolumeInfo.getDataStore().getDriver().createAsync(destVolumeInfo.getDataStore(), destVolumeInfo, null);`, I'm referring to this.
Yes, there's already rollback mechanism from the orchestration. If copyAsync fails then the finally method would pass the failure message back to the corresponding callback, triggering the cleanup.
So, even the additional scenario I added previously, say ROOT is successful while DATA failed, the DATA disk's rollback happens. Earlier I just wanted to highlight the overall flow.
The driver treated any unattached volume in Ready, Allocated or Creating state as an offline migration destination, so an ordinary volume about to be attached could get the migration-only host connect and NFS export policy update. OntapDataMotionStrategy now tags the offline destination with the ontap.migration.destination volume detail right after createAsync and clears it in the finally block. The driver connects the host for migration only when the volume is Migrating or carries that tag. The live path is unchanged because its destination is already Migrating.
Description
This PR...
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Live VM with volumes (
migrateVirtualMachineWithVolume)migrateVolumewith livemigrate on a running volume is rejectedStopped VM storage migration (
migrateVirtualMachineWithVolume, no destination host)Volume migration (
migrateVolume)