-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vmware: fix stopped VM volume migration #4758
vmware: fix stopped VM volume migration #4758
Conversation
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@@ -2683,15 +2683,14 @@ public Answer deleteVolume(DeleteCommand cmd) { | |||
List<VirtualDisk> virtualDisks = vmMo.getVirtualDisks(); | |||
List<String> managedDatastoreNames = getManagedDatastoreNamesFromVirtualDisks(virtualDisks); | |||
|
|||
// Preserve other disks of the VM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these changes @shwstppr.
Can you please test destroy VM and expunge VM cases with VM having disks. Because this particular block is removed to address disk detachments in case of destroy VM. If those operations are not effected, this looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harikrishna-patnala I've verified the destroy case and have seen any issue.
Will try to share API responses if needed
@blueorangutan package |
@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2887 |
Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 60 |
@blueorangutan test centos7 vmware-67u3 |
@shwstppr a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests |
[S] Trillian test result (tid-64)
|
Trillian test result (tid-3683)
|
|
@nvazquez @harikrishna-patnala can you review this? |
if (!blankVmCreated) { | ||
throw new Exception("Failed to create VM. vmName: " + vmInternalCSName); | ||
} | ||
deployAsIs = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please include a log line explaining why not deploying-as-is in this case? Otherwise will look confusing as the StartCommand may receive deployAsIs = true
and not honouring it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nvazquez I've added a log message. Please see if there needs to be any change in the text.
Also, I've found that systemvms use the same flow with the default systemvm template, i.e., register a blank VM and then start it. Though deployAsIs was already false
in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - as long a new system VM template is not registered it uses the non-deploy-as-is behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shwstppr possible to consider user input through some strictness flag for deployAsIs, and then fallback to not deploying-as-is case if strictness flag is false? (so that user will be aware of it already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shwstppr, a small concern here. Since we are starting the VM as not deployAsIs for a VM which is actually a deployAsIs, will there be any problem while applying vApp properties as it is. Can you please test that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing it out @harikrishna-patnala . There seem an issue vApp appliances. Re-working on it.
Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 264 |
@blueorangutan test centos7 vmware-67u3 |
@shwstppr a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests |
Trillian test result (tid-271)
|
@blueorangutan package |
@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
@blueorangutan test centos7 vmware-67u3 |
@shwstppr a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests |
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
Show resolved
Hide resolved
...s/vmware/src/main/java/org/apache/cloudstack/storage/motion/VmwareStorageMotionStrategy.java
Outdated
Show resolved
Hide resolved
Trillian test result (tid-435)
|
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@blueorangutan package |
@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 415 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shwstppr LGTM
@blueorangutan test centos7 vmware-67u3 |
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests |
Trillian test result (tid-469)
|
@borisstoyanov @nvazquez are you lgtm on this? |
Yes, LGTM after various rounds of review and manual testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested with both standard template and multi-disk one.
@blueorangutan package |
@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 455 |
@blueorangutan test centos7 vmware-67u3 |
@shwstppr a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests |
Trillian test result (tid-514)
|
Description
Fixes #4674
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?