kvm: ref-count storage pool usage by rp- · Pull Request #9498 · apache/cloudstack · GitHub

Description

If a storage pool is used by e.g. 2 concurrent snapshot->template actions, if the first action finished it removed the netfs mount point for the other action.
Now the storage pools are usage ref-counted and will only deleted if there are no more users.

Fixes: #8899

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)
build/CI
test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

BLOCKER
Critical
Major
Minor
Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Run several snapshot to template actions, that are executed on the same host.

How did you try to break this feature and the system with this change?

Codecov Report

Attention: Patch coverage is 6.06061% with 31 lines in your changes missing coverage. Please review.

Project coverage is 15.10%. Comparing base (03bdf11) to head (20735be).
Report is 2 commits behind head on 4.19.

Files with missing lines	Patch %	Lines
.../hypervisor/kvm/storage/LibvirtStorageAdaptor.java	7.40%	24 Missing and 1 partial ⚠️
...hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java	0.00%	1 Missing ⚠️
.../hypervisor/kvm/storage/KVMStoragePoolManager.java	0.00%	1 Missing ⚠️
...pervisor/kvm/storage/ManagedNfsStorageAdaptor.java	0.00%	1 Missing ⚠️
...pervisor/kvm/storage/MultipathSCSIAdapterBase.java	0.00%	1 Missing ⚠️
.../hypervisor/kvm/storage/ScaleIOStorageAdaptor.java	0.00%	1 Missing ⚠️
...hypervisor/kvm/storage/StorPoolStorageAdaptor.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##               4.19    #9498     +/-   ##
===========================================
  Coverage     15.10%   15.10%             
- Complexity    11220    11225      +5     
===========================================
  Files          5404     5404             
  Lines        473460   473486     +26     
  Branches      57728    59047   +1319     
===========================================
+ Hits          71525    71541     +16     
- Misses       393941   393948      +7     
- Partials       7994     7997      +3

Flag	Coverage Δ
uitests	4.30% <ø> (ø)
unittests	15.82% <6.06%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

clgtm, you do have a good test scenario for this, do you @rp- ? Or is it only intermitted (i.e. not automatable)

clgtm, you do have a good test scenario for this, do you @rp- ? Or is it only intermitted (i.e. not automatable)

I'm not sure it is easy to reproducible automate that, as it is a timing/parallelism issue.
I didn't even try yet if an NFS primary storage uses the same code paths, but I might do that this week to see if it would also be affected somehow.

But we have 2 customers who didn't report any issues with this yet.

@blueorangutan package

@DaanHoogland a [SL] 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.

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

@blueorangutan test

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

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

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	Error	0.33	test_primary_storage.py
test_01_primary_storage_nfs	Error	0.37	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	Error	0.63	test_primary_storage.py
test_01_primary_storage_scope_change	Error	0.21	test_primary_storage_scope.py
ContextSuite context=TestCpuCapServiceOfferings>:setup	Error	0.00	test_service_offerings.py
test_02_list_snapshots_with_removed_data_store	Error	8.74	test_snapshots.py
test_02_list_snapshots_with_removed_data_store	Error	8.75	test_snapshots.py
test_01_deploy_vm_on_specific_host	Error	0.11	test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster	Error	0.14	test_vm_deployment_planner.py
test_01_migrate_VM_and_root_volume	Error	83.40	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	Error	50.91	test_vm_life_cycle.py
test_01_secure_vm_migration	Error	134.41	test_vm_life_cycle.py
test_01_secure_vm_migration	Error	134.42	test_vm_life_cycle.py
test_08_migrate_vm	Error	0.06	test_vm_life_cycle.py

@blueorangutan package

@rohityadavcloud a [SL] 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.

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10927

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

I guess the failed packaging is nothing related to this PR?

I guess the failed packaging is nothing related to this PR?

11:02:25 [ERROR] Failures: 
11:02:25 [ERROR]   VMSchedulerImplTest.testScheduleNextJobScheduleCurrentSchedule:262 expected:<Wed Sep 04 09:02:00 UTC 2024> but was:<Wed Sep 04 09:03:00 UTC 2024>

looks like a test was too slow , so might have to do with too busy container. retrying @rp-

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

@blueorangutan test

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

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

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	Error	0.66	test_primary_storage.py
test_01_primary_storage_nfs	Error	0.33	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	Error	0.62	test_primary_storage.py
test_01_primary_storage_scope_change	Error	0.22	test_primary_storage_scope.py
ContextSuite context=TestCpuCapServiceOfferings>:setup	Error	0.00	test_service_offerings.py
test_02_list_snapshots_with_removed_data_store	Error	9.77	test_snapshots.py
test_02_list_snapshots_with_removed_data_store	Error	9.77	test_snapshots.py
test_01_volume_usage	Failure	848.98	test_usage.py
test_01_deploy_vm_on_specific_host	Error	0.10	test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster	Error	0.13	test_vm_deployment_planner.py
test_01_migrate_VM_and_root_volume	Error	87.68	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	Error	52.01	test_vm_life_cycle.py
test_01_secure_vm_migration	Error	316.92	test_vm_life_cycle.py
test_02_unsecure_vm_migration	Error	459.21	test_vm_life_cycle.py
test_08_migrate_vm	Error	0.09	test_vm_life_cycle.py
test_06_download_detached_volume	Error	310.28	test_volumes.py

@blueorangutan package

@DaanHoogland a [SL] 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.

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

@blueorangutan test

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

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

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	Error	0.31	test_primary_storage.py
test_01_primary_storage_nfs	Error	0.30	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	Error	0.60	test_primary_storage.py
test_01_primary_storage_scope_change	Error	0.21	test_primary_storage_scope.py
ContextSuite context=TestCpuCapServiceOfferings>:setup	Error	0.00	test_service_offerings.py
test_02_list_snapshots_with_removed_data_store	Error	8.63	test_snapshots.py
test_02_list_snapshots_with_removed_data_store	Error	8.63	test_snapshots.py
test_01_deploy_vm_on_specific_host	Error	0.09	test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster	Error	0.14	test_vm_deployment_planner.py
test_01_migrate_VM_and_root_volume	Error	82.29	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	Error	50.76	test_vm_life_cycle.py
test_01_secure_vm_migration	Error	134.37	test_vm_life_cycle.py
test_01_secure_vm_migration	Error	134.37	test_vm_life_cycle.py
test_08_migrate_vm	Error	0.08	test_vm_life_cycle.py

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

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	Error	0.36	test_primary_storage.py
test_01_primary_storage_nfs	Error	0.34	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	Error	0.55	test_primary_storage.py
test_01_primary_storage_scope_change	Error	0.20	test_primary_storage_scope.py
ContextSuite context=TestCpuCapServiceOfferings>:setup	Error	0.00	test_service_offerings.py
test_02_list_snapshots_with_removed_data_store	Error	10.74	test_snapshots.py
test_02_list_snapshots_with_removed_data_store	Error	10.74	test_snapshots.py
test_01_deploy_vm_on_specific_host	Error	0.09	test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster	Error	0.13	test_vm_deployment_planner.py
test_01_migrate_VM_and_root_volume	Error	84.39	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	Error	55.00	test_vm_life_cycle.py
test_01_secure_vm_migration	Error	175.92	test_vm_life_cycle.py
test_08_migrate_vm	Error	0.07	test_vm_life_cycle.py

@JoaoJandre
So yeah, this doesn't really work with primary storage NFS (maybe others handled with libvirt too).
Currently ModifyStoragePool would call createStoragePool, but not deleteStoragePool. which means it would just increase the refCount but never actually remove it from libvirt. (but the HA code would already unmount it....)
So the next createStoragePool would ask libvirt if it knows about the pool and it still does, but later the new "isNFSreallymounted" check would fail.

Adding a deletStoragePool to ModifyStoragePool has the negative effect that there would never be a mounted primary storage on the agent, which is currently assumed and creating instances/copy templates will fail.

I'm not really sure how to move forward with this then, maybe don't let the management server create 2 jobs with the same secondary storage pool to the same agent in parallel.

@JoaoJandre So yeah, this doesn't really work with primary storage NFS (maybe others handled with libvirt too). Currently ModifyStoragePool would call createStoragePool, but not deleteStoragePool. which means it would just increase the refCount but never actually remove it from libvirt. (but the HA code would already unmount it....) So the next createStoragePool would ask libvirt if it knows about the pool and it still does, but later the new "isNFSreallymounted" check would fail.

Adding a deletStoragePool to ModifyStoragePool has the negative effect that there would never be a mounted primary storage on the agent, which is currently assumed and creating instances/copy templates will fail.

I'm not really sure how to move forward with this then, maybe don't let the management server create 2 jobs with the same secondary storage pool to the same agent in parallel.

We could keep the ref-count for the secondary storage only. The com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager#createStoragePool(java.lang.String, java.lang.String, int, java.lang.String, java.lang.String, com.cloud.storage.Storage.StoragePoolType, java.util.Map<java.lang.String,java.lang.String>, boolean) already takes a parameter to determine if the pool is primary or not, we could pass it to com.cloud.hypervisor.kvm.storage.LibvirtStorageAdaptor#createStoragePool to determine if we should keep the ref-count or not.

@JoaoJandre
We could keep the ref-count for the secondary storage only. The com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager#createStoragePool(java.lang.String, java.lang.String, int, java.lang.String, java.lang.String, com.cloud.storage.Storage.StoragePoolType, java.util.Map<java.lang.String,java.lang.String>, boolean) already takes a parameter to determine if the pool is primary or not, we could pass it to com.cloud.hypervisor.kvm.storage.LibvirtStorageAdaptor#createStoragePool to determine if we should keep the ref-count or not.

And on delete? only check if the uuid is part of the refcount map?

@JoaoJandre
We could keep the ref-count for the secondary storage only. The com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager#createStoragePool(java.lang.String, java.lang.String, int, java.lang.String, java.lang.String, com.cloud.storage.Storage.StoragePoolType, java.util.Map<java.lang.String,java.lang.String>, boolean) already takes a parameter to determine if the pool is primary or not, we could pass it to com.cloud.hypervisor.kvm.storage.LibvirtStorageAdaptor#createStoragePool to determine if we should keep the ref-count or not.

And on delete? only check if the uuid is part of the refcount map?

@rp- Yeah, if it is not part of the map, we assume that it can be deleted. If it is, we check the refcount.

@blueorangutan package

@JoaoJandre This seem to work in my tests now

If a secondary storage pool is used by e.g. 2 concurrent snapshot->template actions, if the first action finished it removed the netfs mount point for the other action. Now the storage pools are usage ref-counted and will only deleted if there are no more users.

@blueorangutan package

@rp- a [SL] 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.

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

@DaanHoogland can you start tests here, I would want to see if they succeed now

@blueorangutan test

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

CLGTM, I'll try to test this in the coming days

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

Test	Result	Time (s)	Test File

Tested:

Template creation from snapshot;
Taking multiple snapshots at once;
Removing a primary-storage;
Attaching and Detaching ISOs;

All operations succeeded and the storage was umounted afterwards (only once detached for test 4).

If a secondary storage pool is used by e.g. 2 concurrent snapshot->template actions, if the first action finished it removed the netfs mount point for the other action. Now the storage pools are usage ref-counted and will only deleted if there are no more users.

boring-cyborg Bot added the component:kvm label Aug 7, 2024

rp- self-assigned this Aug 7, 2024

DaanHoogland approved these changes Aug 12, 2024

View reviewed changes

rp- requested a review from harikrishna-patnala August 29, 2024 11:05

yadvr added this to the 4.19.2.0 milestone Sep 3, 2024

DaanHoogland added the status:needs-testing label Sep 9, 2024

rp- force-pushed the 4.19-kvm-refcount-storagepool-usage branch from b896bbc to 427ed5a Compare November 8, 2024 14:24

boring-cyborg Bot added component:iscsi component:primary-storage component:storage labels Nov 8, 2024

rp- force-pushed the 4.19-kvm-refcount-storagepool-usage branch from 427ed5a to 7064436 Compare November 8, 2024 14:36

rp- force-pushed the 4.19-kvm-refcount-storagepool-usage branch from 7064436 to 20735be Compare November 8, 2024 17:53

rp- linked an issue Nov 11, 2024 that may be closed by this pull request

Create template from snapshot parallel allocating same NFS secondary storage on agent #8899

Closed

JoaoJandre approved these changes Nov 11, 2024

View reviewed changes

JoaoJandre added type:bug and removed status:needs-testing labels Nov 13, 2024

JoaoJandre merged commit dfe4a67 into apache:4.19 Nov 13, 2024

rp- deleted the 4.19-kvm-refcount-storagepool-usage branch November 13, 2024 13:39

DaanHoogland mentioned this pull request Dec 6, 2024

Create template from snapshot parallel allocating same NFS secondary storage on agent #8899

Closed

kvm: ref-count storage pool usage by rp- · Pull Request #9498 · apache/cloudstack · GitHub

Repository navigation

kvm: ref-count storage pool usage - #9498

kvm: ref-count storage pool usage#9498JoaoJandre merged 1 commit intoapache:4.19apache/cloudstack:4.19from LINBIT:4.19-kvm-refcount-storagepool-usageLINBIT/cloudstack:4.19-kvm-refcount-storagepool-usageCopy head branch name to clipboard

Conversation

rp- commented Aug 7, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

codecov Bot commented Aug 7, 2024 • 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!

rp- commented Aug 12, 2024

Uh oh!

DaanHoogland commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 12, 2024

Uh oh!

DaanHoogland commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 13, 2024

Uh oh!

yadvr commented Sep 3, 2024

Uh oh!

blueorangutan commented Sep 3, 2024

Uh oh!

blueorangutan commented Sep 3, 2024

Uh oh!

blueorangutan commented Sep 4, 2024

Uh oh!

rp- commented Sep 5, 2024

Uh oh!

DaanHoogland commented Sep 5, 2024

Uh oh!

blueorangutan commented Sep 5, 2024

Uh oh!

DaanHoogland commented Sep 5, 2024

Uh oh!

blueorangutan commented Sep 5, 2024

Uh oh!

blueorangutan commented Sep 6, 2024

Uh oh!

DaanHoogland commented Sep 6, 2024

Uh oh!

blueorangutan commented Sep 6, 2024

Uh oh!

blueorangutan commented Sep 6, 2024

Uh oh!

DaanHoogland commented Sep 6, 2024

Uh oh!

blueorangutan commented Sep 6, 2024

Uh oh!

blueorangutan commented Sep 7, 2024

Uh oh!

blueorangutan commented Nov 8, 2024

Uh oh!

rp- commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoaoJandre commented Nov 8, 2024

Uh oh!

rp- commented Nov 8, 2024

Uh oh!

JoaoJandre commented Nov 8, 2024

Uh oh!

rp- commented Nov 8, 2024

kvm: ref-count storage pool usage#9498
JoaoJandre merged 1 commit into
apache:4.19from
LINBIT:4.19-kvm-refcount-storagepool-usage

codecov Bot commented Aug 7, 2024 •
edited
Loading

rp- commented Nov 8, 2024 •
edited
Loading