Fix: Volumes on lost local storage cannot be removed by nvazquez · Pull Request #7594 · apache/cloudstack

nvazquez · 2023-06-06T02:11:07Z

Description

This PR fixes the following:

When destroying a local storage pool during host force deletion: then also destroy the volumes on the pool from DB
When expunging a destroyed volume: check if the volume's storage pool exists (not removed). In case its removed, skip the volume removal from storage

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Create a local storage KVM zone
Deploy a VM to local storage
Force remove the host
Delete the VM -> This succeeds but its volumes remain forever in "Destroy" (before the fix) -> After the fix: volumes are destroyed

…volume from storage

nvazquez · 2023-06-06T02:11:52Z

@blueorangutan package

blueorangutan · 2023-06-06T02:14:03Z

@nvazquez a [LL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-06-06T03:06:09Z

Packaging result [LL]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6086

nvazquez · 2023-06-06T03:20:02Z

@blueorangutan test

blueorangutan · 2023-06-06T03:22:03Z

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

codecov · 2023-06-06T03:34:13Z

Codecov Report

Merging #7594 (f086bec) into 4.18 (0941d01) will increase coverage by 0.01%.
The diff coverage is 38.88%.

@@             Coverage Diff              @@
##               4.18    #7594      +/-   ##
============================================
+ Coverage     12.98%   12.99%   +0.01%     
- Complexity     8981     9001      +20     
============================================
  Files          2716     2716              
  Lines        256307   256391      +84     
  Branches      39967    39988      +21     
============================================
+ Hits          33280    33317      +37     
- Misses       218864   218904      +40     
- Partials       4163     4170       +7

Impacted Files	Coverage Δ
...main/java/com/cloud/storage/dao/VolumeDaoImpl.java	`23.23% <0.00%> (-0.39%)`	⬇️
...cloud/storage/resource/VmwareStorageProcessor.java	`0.32% <0.00%> (-0.01%)`	⬇️
...n/java/com/cloud/resource/ResourceManagerImpl.java	`0.00% <0.00%> (ø)`
...ain/java/com/cloud/storage/StorageManagerImpl.java	`1.59% <65.00%> (+0.70%)`	⬆️
...n/java/com/cloud/storage/VolumeApiServiceImpl.java	`15.30% <80.00%> (+0.27%)`	⬆️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DaanHoogland

mostly sensible but I found one strangity

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

blueorangutan · 2023-06-06T16:48:59Z

[SF] Trillian test result (tid-6683)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46978 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7594-t6683-kvm-centos7.zip
Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

nvazquez · 2023-06-07T13:58:37Z

@blueorangutan package

blueorangutan · 2023-06-07T14:00:03Z

@nvazquez a [LL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-06-07T15:15:12Z

Packaging result [LL]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6105

nvazquez · 2023-06-07T15:34:33Z

@blueorangutan test

blueorangutan · 2023-06-07T15:36:03Z

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

DaanHoogland

clgtm

blueorangutan · 2023-06-08T04:58:32Z

[SF] Trillian test result (tid-6703)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46712 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7594-t6703-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_list_vms_metrics_admin	`Error`	3608.74	test_metrics_api.py
test_list_vms_metrics_history	`Error`	7.42	test_metrics_api.py
test_list_volumes_metrics_history	`Error`	7.38	test_metrics_api.py

weizhouapache · 2023-06-09T07:14:07Z

server/src/main/java/com/cloud/resource/ResourceManagerImpl.java

                    final Long poolId = pool.getPoolId();
                    final StoragePoolVO storagePool = _storagePoolDao.findById(poolId);
                    if (storagePool.isLocal() && isForceDeleteStorage) {
+                        destroyLocalStoragePoolVolumes(poolId);


this should be good 👍

weizhouapache · 2023-06-09T07:17:33Z

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

+        InternalIdentity pool = role == DataStoreRole.Primary ?
+                _storagePoolDao.findById(volume.getPoolId()) :
+                imageStoreDao.findById(volume.getPoolId());


use switch/case ?

shwstppr

Code LGTM

kiranchavala

Hi @nvazquez

Observing the following issue

Create a disk offering with storage type as local
Create a compute offering with storage type as local
Launch a vm with data disk , wait till its in running state
Disable the host
Force delete the host
Found the vm gets into destroyed state and is not expunged
The data disk is still in ready state

Ideally the vm and data disk on the local storage should also get remvoed automatically if the local storage is removed

Currently the end user has to manually expunge the vm and the destory data disk

nvazquez · 2023-06-20T15:48:18Z

Thanks @kiranchavala, have pushed a fix for that scenario

nvazquez · 2023-06-20T15:48:32Z

@blueorangutan package

blueorangutan · 2023-06-20T15:50:03Z

@nvazquez a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-06-20T16:45:05Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6276

kiranchavala

LGTM, Tested the data disk fix and it's working fine

kiranchavala · 2023-06-22T07:45:43Z

@blueorangutan test

blueorangutan · 2023-06-22T07:46:03Z

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

yadvr

LGTM didn't test it

blueorangutan · 2023-06-22T22:16:38Z

[SF] Trillian test result (tid-6811)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 50678 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7594-t6811-kvm-centos7.zip
Smoke tests completed. 104 look OK, 4 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_migrate_VM_and_root_volume	`Error`	78.71	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	52.38	test_vm_life_cycle.py
test_02_unsecure_vm_migration	`Error`	225.86	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	140.91	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	150.16	test_vm_life_cycle.py
test_01_verify_ipv6_vpc	`Error`	601.41	test_vpc_ipv6.py
test_02_redundant_VPC_default_routes	`Failure`	632.33	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	497.28	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	497.30	test_vpc_redundant.py
test_02_VPC_default_routes	`Failure`	172.52	test_vpc_router_nics.py

nvazquez added 3 commits June 5, 2023 22:56

Remove volumes on local storage pool when destroying the pool

6e9f91c

Check the volume storage pool exists before attempting to remove the …

29e7c10

…volume from storage

Refactor and fix unit tests

d0c1f0d

nvazquez added type:bug component:local-storage labels Jun 6, 2023

nvazquez added this to the 4.18.1.0 milestone Jun 6, 2023

nvazquez requested review from DaanHoogland, shwstppr and weizhouapache June 6, 2023 02:11

boring-cyborg bot added the component:storage label Jun 6, 2023

nvazquez requested review from borisstoyanov and yadvr June 6, 2023 02:11

DaanHoogland reviewed Jun 6, 2023

View reviewed changes

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java Outdated Show resolved Hide resolved

Refactor exception message

1396b91

DaanHoogland approved these changes Jun 7, 2023

View reviewed changes

weizhouapache reviewed Jun 9, 2023

View reviewed changes

shwstppr approved these changes Jun 9, 2023

View reviewed changes

kiranchavala reviewed Jun 19, 2023

View reviewed changes

Fix datadisks not removed

f086bec

kiranchavala approved these changes Jun 21, 2023

View reviewed changes

weizhouapache added the status:ready-for-merge label Jun 22, 2023

yadvr marked this pull request as ready for review June 22, 2023 09:18

yadvr approved these changes Jun 22, 2023

View reviewed changes

DaanHoogland merged commit c809201 into apache:4.18 Jun 23, 2023

DaanHoogland removed the status:ready-for-merge label Dec 4, 2024

Conversation

nvazquez commented Jun 6, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

nvazquez commented Jun 6, 2023

Uh oh!

blueorangutan commented Jun 6, 2023

Uh oh!

blueorangutan commented Jun 6, 2023

Uh oh!

nvazquez commented Jun 6, 2023

Uh oh!

blueorangutan commented Jun 6, 2023

Uh oh!

codecov bot commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blueorangutan commented Jun 6, 2023

Uh oh!

nvazquez commented Jun 7, 2023

Uh oh!

blueorangutan commented Jun 7, 2023

Uh oh!

blueorangutan commented Jun 7, 2023

Uh oh!

nvazquez commented Jun 7, 2023

Uh oh!

blueorangutan commented Jun 7, 2023

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jun 8, 2023

Uh oh!

weizhouapache Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

weizhouapache Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

kiranchavala left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Jun 20, 2023

Uh oh!

nvazquez commented Jun 20, 2023

Uh oh!

blueorangutan commented Jun 20, 2023

Uh oh!

blueorangutan commented Jun 20, 2023

Uh oh!

kiranchavala left a comment

Choose a reason for hiding this comment

Uh oh!

kiranchavala commented Jun 22, 2023

Uh oh!

blueorangutan commented Jun 22, 2023

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jun 22, 2023

Uh oh!

Reviewers

Assignees

Labels

codecov bot commented Jun 6, 2023 •

edited

Loading