Fix KVM import and list unmanaged VMs by slavkap · Pull Request #8445 · apache/cloudstack · GitHub

Description

This PR fixes part (issues 2 and3) of #8439 and depends on #8433

1 - Fix of VM insert for VMs with StorPool volumes
2 - Fix of the list/insert unmanaged VMs with RBD 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)
build/CI

Bug Severity

BLOCKER
Critical
Major
Minor
Trivial

How Has This Been Tested?

manually tested

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (d6ac91f) 30.74% compared to head (8921119) 22.98%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
...er/LibvirtGetUnmanagedInstancesCommandWrapper.java	0.00%	11 Missing ⚠️
.../apache/cloudstack/vm/UnmanagedVMsManagerImpl.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8445      +/-   ##
============================================
- Coverage     30.74%   22.98%   -7.77%     
+ Complexity    33930    23732   -10198     
============================================
  Files          5341     5209     -132     
  Lines        374918   352870   -22048     
  Branches      54534    50543    -3991     
============================================
- Hits         115286    81100   -34186     
- Misses       244374   260108   +15734     
+ Partials      15258    11662    -3596

Flag	Coverage Δ
simulator-marvin-tests	24.62% <0.00%> (-0.01%)	⬇️
uitests	4.39% <ø> (ø)
unit-tests	?

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.

@blueorangutan package

@shwstppr 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]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8202

@blueorangutan test

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

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

Test	Result	Time (s)	Test File
test_02_trigger_shutdown	Failure	336.70	test_safe_shutdown.py
test_07_deploy_kubernetes_ha_cluster	Failure	3611.63	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	Failure	0.04	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	Failure	0.04	test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster	Failure	42.62	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	Error	90.87	test_kubernetes_clusters.py

aren't we replacing a bug with a bug here (nitpicking maybe but genuine question)
path.endswith("bla") is different from StringUtils.containsAny(path, "bla", "blob")
considder paths:

"bla/blah"
"bla/blob"
"bla/bleb"
Neither would match the first condition, and though the first is supposed to match the second condition (as I understand) the other two seem false positives to me.

Am I misinterpreting this @slavkap ?

Adding to @DaanHoogland's comment, perhaps we can add a condition to keep the current check for non RBD disks? Raising a concern since the Vmware import of unmanaged instances uses the same code

thanks @DaanHoogland, @nvazquez! I've changed it to check only path vs dsPath. Neither for KVM/VMware we need to check for the datastore name in the primary storage path

Adding to @DaanHoogland's comment, perhaps we can add a condition to keep the current check for non RBD disks? Raising a concern since the Vmware import of unmanaged instances uses the same code

@blueorangutan package

@shwstppr 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.

1 - Fix of VM insert for VMs with StorPool volumes 2 - Fix of list/insert unmanaged VMs with RBD volumes

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

Chech if the pool path contains the datastore path. There is no need to check for the datastore name because for VMware it get the datastore UUID which isn't relevant with the primary storage path in CS

@blueorangutan package

@nvazquez 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]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8256

LGTM - tested OK on a mixed Vmware+KVM env and observe no regressions on KVM or Vmware import

clgtm and did similar tests to @nvazquez (not well communicated but very well tested.) of course we might have missed some corner cases still. Let's merge however.

VM import fixes 1 - Fix of VM insert for VMs with StorPool volumes 2 - Fix of list/insert unmanaged VMs with RBD volumes

boring-cyborg Bot added the component:kvm label Jan 4, 2024

shwstppr added this to the 4.19.0.0 milestone Jan 5, 2024

DaanHoogland reviewed Jan 5, 2024

View reviewed changes

nvazquez reviewed Jan 8, 2024

View reviewed changes

VM insert fixes

3484044

1 - Fix of VM insert for VMs with StorPool volumes 2 - Fix of list/insert unmanaged VMs with RBD volumes

address changes

8921119

Chech if the pool path contains the datastore path. There is no need to check for the datastore name because for VMware it get the datastore UUID which isn't relevant with the primary storage path in CS

slavkap force-pushed the fix-vm-ingestion branch from 309c09b to 8921119 Compare January 9, 2024 13:58

nvazquez approved these changes Jan 9, 2024

View reviewed changes

DaanHoogland approved these changes Jan 10, 2024

View reviewed changes

DaanHoogland added the status:ready-for-merge label Jan 10, 2024

shwstppr removed the status:ready-for-merge label Jan 10, 2024

shwstppr merged commit c569fe9 into apache:main Jan 10, 2024

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jan 12, 2024

Fix KVM import and list unmanaged VMs (apache#8445 )

ec55cbb

VM import fixes 1 - Fix of VM insert for VMs with StorPool volumes 2 - Fix of list/insert unmanaged VMs with RBD volumes

sureshanaparti mentioned this pull request Feb 28, 2024

Vmware Import Problem #8712

Closed

                           pools.addAll(primaryDataStoreDao.listByDataCenterId(zone.getId()));
                           for (StoragePool pool : pools) {
-                              if (pool.getPath().endsWith(dsName)) {
+                              if (StringUtils.containsAny(pool.getPath(), dsName, dsPath)) {

                           pools.addAll(primaryDataStoreDao.listByDataCenterId(zone.getId()));
                           for (StoragePool pool : pools) {
-                              if (pool.getPath().endsWith(dsName)) {
+                              if (StringUtils.containsAny(pool.getPath(), dsName, dsPath)) {

Fix KVM import and list unmanaged VMs by slavkap · Pull Request #8445 · apache/cloudstack · GitHub

Repository navigation

Fix KVM import and list unmanaged VMs - #8445

Fix KVM import and list unmanaged VMs#8445shwstppr merged 2 commits intoapache:mainapache/cloudstack:mainfrom storpool:fix-vm-ingestionstorpool/cloudstack:fix-vm-ingestionCopy head branch name to clipboard

Conversation

slavkap commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Bug Severity

How Has This Been Tested?

Uh oh!

codecov Bot commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shwstppr commented Jan 4, 2024

Uh oh!

blueorangutan commented Jan 4, 2024

Uh oh!

blueorangutan commented Jan 4, 2024

Uh oh!

shwstppr commented Jan 4, 2024

Uh oh!

blueorangutan commented Jan 4, 2024

Uh oh!

blueorangutan commented Jan 5, 2024

Uh oh!

DaanHoogland Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

nvazquez Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

slavkap Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

nvazquez Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shwstppr commented Jan 9, 2024

Uh oh!

blueorangutan commented Jan 9, 2024

Uh oh!

blueorangutan commented Jan 9, 2024

Uh oh!

nvazquez commented Jan 9, 2024

Uh oh!

blueorangutan commented Jan 9, 2024

Uh oh!

blueorangutan commented Jan 9, 2024

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fix KVM import and list unmanaged VMs#8445
shwstppr merged 2 commits into
apache:mainfrom
storpool:fix-vm-ingestion

slavkap commented Jan 4, 2024 •
edited
Loading

codecov Bot commented Jan 4, 2024 •
edited
Loading