Destroyvm also removes volumes by houthuis · Pull Request #2793 · apache/cloudstack

houthuis · 2018-08-06T13:00:20Z

Description

Currently DestroyVMCmd does not have the option to delete any attached volumes (Data Disks), resulting in unused orphan volumes sitting in CloudStack, unless the user manually deletes them or attach them to a different VM. This FR will introduce functionality to allow users of any type (who are owners of the VM) to delete the attached volumes either via the API or the UI. The UI will have the option to select which volumes should be deleted, or they can select the option to delete none. CloudStack will then detach and delete the selected volumes after destroying the VM.
DestroyVMCmd will be modified to accept a comma separated list of volume UUIDs that will be deleted. CloudStack will then validate this list, making sure that the volumes are indeed attached to the VM. If not, the operation will fail. Checks are also done to make sure the volumes are data disks.
Before destroying the VM, CloudStack will first detach the volumes, and after the VM has been destroyed, delete the volumes.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

borisstoyanov · 2018-08-07T07:05:16Z

@blueorangutan package

blueorangutan · 2018-08-07T07:05:56Z

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-08-07T07:32:41Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2231

borisstoyanov · 2018-08-07T07:34:48Z

@blueorangutan test

blueorangutan · 2018-08-07T07:34:55Z

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

wido · 2018-08-07T11:44:02Z

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

+
+        checkForUnattachedVolumes(vmId, volumes);
+        validateVolumes(volumes);
+        detachVolumesFromVm(volumes);


Shouldn't we detach after the VM has been stopped? As the detach might not work, let's say libvirt (KVM) hangs or something else.

If the VM is stopped the detach is a lot easier and you can remove them then.

We don't stop the VM for a regular detach cmd? This has been tested on KVM (with a small and large amount of volumes) and it worked fine. I'm happy to make the change if required though

@houthuis Yes, I know. I was just thinking of it, what implications it might have. The stop of the VM just make sures the detach works even better, it saves additional commands to the Agent to detach the data volumes. This is because the KVM Agent needs to talk to libvirt to detach the disks.

@wido Can you explain the risk here? It seems no different than a regular detach. Do these fail a lot?

@DaanHoogland My point is that it's extra calls towards the KVM Agent, it needs to talk to libvirt, etc, etc. Since we are stopping the VM a few seconds later it would be best to do the stop first.

This would make it less calls and thus less error prone.

@wido this change has been made. please give the PR another review

blueorangutan · 2018-08-07T14:43:45Z

Trillian test result (tid-2914)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 24138 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2793-t2914-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_primary_storage.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 63 look OK, 6 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	`Error`	0.59	test_primary_storage.py
test_01_primary_storage_nfs	`Error`	0.10	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.18	test_primary_storage.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	1100.52	test_privategw_acl.py
test_02_list_snapshots_with_removed_data_store	`Error`	1.14	test_snapshots.py
test_01_secure_vm_migration	`Error`	52.05	test_vm_life_cycle.py
test_02_unsecure_vm_migration	`Error`	91.62	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	40.28	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	14.69	test_vm_life_cycle.py
test_01_cancel_host_maintenace_with_no_migration_jobs	`Failure`	0.10	test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	1.25	test_host_maintenance.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	3.52	test_hostha_kvm.py
test_hostha_kvm_host_degraded	`Failure`	636.48	test_hostha_kvm.py
test_hostha_kvm_host_fencing	`Failure`	615.87	test_hostha_kvm.py
test_hostha_kvm_host_recovering	`Failure`	613.98	test_hostha_kvm.py

borisstoyanov · 2018-08-08T06:52:28Z

@blueorangutan test

blueorangutan · 2018-08-08T06:52:56Z

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2018-08-08T16:56:19Z

Trillian test result (tid-2916)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34312 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2793-t2916-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_primary_storage.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 63 look OK, 6 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	`Error`	0.84	test_primary_storage.py
test_01_primary_storage_nfs	`Error`	0.13	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.25	test_primary_storage.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	1239.32	test_privategw_acl.py
test_02_list_snapshots_with_removed_data_store	`Error`	1.18	test_snapshots.py
test_01_secure_vm_migration	`Error`	164.66	test_vm_life_cycle.py
test_02_unsecure_vm_migration	`Error`	83.01	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	48.64	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	22.09	test_vm_life_cycle.py
test_01_cancel_host_maintenace_with_no_migration_jobs	`Failure`	0.12	test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	2.37	test_host_maintenance.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	3.61	test_hostha_kvm.py

borisstoyanov · 2018-08-09T11:40:54Z

@houthuis can you please rebase the branch agains master, I've tried but there's some conflicts. Once you do we'll be able to get proper test results

dhlaluku · 2018-08-15T14:00:22Z

@houthuis could you please resolve the conflicts and do a force push to restart Jenkins.

yadvr · 2018-08-16T10:12:14Z

Henko, can you remove extra newlines and check Travis failures? Also see marvin test failures, and regression test on vmware and xenserver hypervisors.

houthuis · 2018-08-17T10:05:39Z

@blueorangutan package

blueorangutan · 2018-08-17T10:05:58Z

@houthuis a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-08-17T10:37:36Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2257

houthuis · 2018-08-17T10:55:52Z

@blueorangutan test

blueorangutan · 2018-08-17T10:55:58Z

@houthuis a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

DaanHoogland · 2018-08-17T11:23:07Z

ui/scripts/instances.js

                        hiddenTabs.push("securityGroups");
                    }
-					
+                                       


can you clean this?

yes, will clean after tests. intellij community edition keeps on formatting js

dhlaluku

LGTM, please rebase against the latest master in order to eliminate the failing VM migration integration tests from this PR

blueorangutan · 2018-08-17T20:22:06Z

Trillian test result (tid-2939)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32031 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2793-t2939-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 67 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	1239.93	test_privategw_acl.py
test_02_unsecure_vm_migration	`Error`	3668.22	test_vm_life_cycle.py
test_02_unsecure_vm_migration	`Error`	3806.36	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	137.25	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	136.29	test_vm_life_cycle.py
ContextSuite context=TestVMLifeCycle>:setup	`Error`	230.85	test_vm_life_cycle.py

borisstoyanov

LGTM

DaanHoogland · 2018-08-20T08:19:03Z

ping @houthuis , don't forget the empty lines please.

blueorangutan · 2018-10-23T20:57:06Z

Trillian test result (tid-3109)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31210 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2793-t3109-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_public_ip_range.py
Intermittent failure detected: /marvin/tests/smoke/test_templates.py
Intermittent failure detected: /marvin/tests/smoke/test_usage.py
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 64 look OK, 5 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	35.37	test_multipleips_per_nic.py
test_04_extract_template	`Failure`	128.27	test_templates.py
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
test_06_download_detached_volume	`Failure`	137.61	test_volumes.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	443.32	test_vpc_redundant.py

yadvr · 2018-10-25T01:27:47Z

@blueorangutan package

blueorangutan · 2018-10-25T01:28:42Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-10-25T02:18:47Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-2380

DaanHoogland · 2018-10-29T09:33:47Z

@blueorangutan package

blueorangutan · 2018-10-29T13:26:22Z

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

GabrielBrascher · 2018-11-21T17:37:55Z

@houthuis can you please resolve the conflict and rebase to latest master? Thanks!

dhlaluku · 2018-11-21T18:28:33Z

@GabrielBrascher I resolved the conflicts and rebased against latest master

dhlaluku · 2018-11-21T18:28:49Z

@blueorangutan package

blueorangutan · 2018-11-21T18:28:59Z

@dhlaluku a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-11-21T18:49:05Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2451

dhlaluku · 2018-11-21T18:56:09Z

@blueorangutan test matrix

blueorangutan · 2018-11-21T18:56:59Z

@dhlaluku a Trillian-Jenkins matrix job (centos6 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

blueorangutan · 2018-11-22T02:01:04Z

Trillian test result (tid-3201)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 23954 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2793-t3201-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 68 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	34.26	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	503.90	test_vpc_redundant.py

blueorangutan · 2018-11-22T02:37:52Z

Trillian test result (tid-3200)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 26317 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2793-t3200-xenserver-71.zip
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_scale_vm.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 67 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	57.63	test_multipleips_per_nic.py
test_01_scale_vm	`Error`	14.32	test_scale_vm.py
test_02_redundant_VPC_default_routes	`Error`	0.73	test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	538.20	test_vpc_redundant.py

blueorangutan · 2018-11-22T03:54:49Z

Trillian test result (tid-3202)
Environment: vmware-65 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30939 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2793-t3202-vmware-65.zip
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 68 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	86.21	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	528.50	test_vpc_redundant.py

dhlaluku · 2018-11-22T11:37:54Z

It seems we are ready to get this merged.
@wido @borisstoyanov @GabrielBrascher @rhtyd do you have any more suggestions for this?

wido · 2018-11-22T11:53:41Z

LGTM from me!

borisstoyanov

LGTM @dhlaluku

dhlaluku · 2018-11-26T12:53:06Z

@rafaelweingartner @DaanHoogland @rhtyd could you please merge this PR?

rafaelweingartner

I think we can for sure merge it. Can you just check a very minor detail?

rafaelweingartner · 2018-11-26T14:07:20Z

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

-    private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
+    @ActionEvent(eventType = EventTypes.EVENT_VOLUME_DETACH, eventDescription = "detaching volume")
+    public Volume detachVolumeViaDestroyVM(long vmId, long volumeId) {
+        Volume result = orchestrateDetachVolumeFromVM(vmId, volumeId);


you can use directly return orchestrateDetachVolumeFromVM(vmId, volumeId);

dhlaluku · 2018-11-29T14:49:52Z

@rafaelweingartner @nvazquez @GabrielBrascher @rhtyd any last thoughts with this PR?

nvazquez

LGTM

nvazquez · 2018-11-30T13:27:18Z

Merging based on reviews and approvals, thanks @dhlaluku

wido added component:management-server component:api component:UI labels Aug 7, 2018

wido reviewed Aug 7, 2018

View reviewed changes

houthuis force-pushed the destroyvm-also-removes-volumes branch from 2d34762 to 6303fbe Compare August 10, 2018 10:13

houthuis force-pushed the destroyvm-also-removes-volumes branch from 6303fbe to 906c400 Compare August 16, 2018 12:11

DaanHoogland reviewed Aug 17, 2018

View reviewed changes

DaanHoogland approved these changes Aug 17, 2018

View reviewed changes

dhlaluku suggested changes Aug 17, 2018

View reviewed changes

borisstoyanov approved these changes Aug 20, 2018

View reviewed changes

DaanHoogland added this to the 4.12.0.0 milestone Aug 20, 2018

Allow user to detach and delete volumes when destroyinh VMs

0962e74

dhlaluku force-pushed the destroyvm-also-removes-volumes branch from 9b3cfaa to 0962e74 Compare November 21, 2018 18:26

borisstoyanov approved these changes Nov 26, 2018

View reviewed changes

rafaelweingartner reviewed Nov 26, 2018

View reviewed changes

Minor code refactoring

e159220

nvazquez approved these changes Nov 29, 2018

View reviewed changes

nvazquez merged commit 525ddfb into apache:master Nov 30, 2018

dhlaluku deleted the destroyvm-also-removes-volumes branch November 30, 2018 14:26

dhlaluku mentioned this pull request Jan 7, 2019

Rework for destroy VM with volumes UI #3120

Merged

5 tasks

Conversation

houthuis commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

borisstoyanov commented Aug 7, 2018

Uh oh!

blueorangutan commented Aug 7, 2018

Uh oh!

blueorangutan commented Aug 7, 2018

Uh oh!

borisstoyanov commented Aug 7, 2018

Uh oh!

blueorangutan commented Aug 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Aug 7, 2018

Uh oh!

borisstoyanov commented Aug 8, 2018

Uh oh!

blueorangutan commented Aug 8, 2018

Uh oh!

blueorangutan commented Aug 8, 2018

Uh oh!

borisstoyanov commented Aug 9, 2018

Uh oh!

dhlaluku commented Aug 15, 2018

Uh oh!

yadvr commented Aug 16, 2018

Uh oh!

houthuis commented Aug 17, 2018

Uh oh!

blueorangutan commented Aug 17, 2018

Uh oh!

blueorangutan commented Aug 17, 2018

Uh oh!

houthuis commented Aug 17, 2018

Uh oh!

blueorangutan commented Aug 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhlaluku left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Aug 17, 2018

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Aug 20, 2018

Uh oh!

blueorangutan commented Oct 23, 2018

Uh oh!

yadvr commented Oct 25, 2018

Uh oh!

blueorangutan commented Oct 25, 2018

Uh oh!

blueorangutan commented Oct 25, 2018

Uh oh!

DaanHoogland commented Oct 29, 2018

Uh oh!

houthuis commented Aug 6, 2018 •

edited

Loading