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

slavkap · 2024-01-04T17:09:37Z

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

How Has This Been Tested?

manually tested

codecov · 2024-01-04T17:18:07Z

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.

shwstppr · 2024-01-04T17:43:59Z

@blueorangutan package

blueorangutan · 2024-01-04T17:46:03Z

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

blueorangutan · 2024-01-04T18:48:18Z

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

shwstppr · 2024-01-04T18:52:54Z

@blueorangutan test

blueorangutan · 2024-01-04T18:54:03Z

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

blueorangutan · 2024-01-05T08:11:38Z

[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

DaanHoogland · 2024-01-05T09:21:25Z

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


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

nvazquez · 2024-01-08T14:02:35Z

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


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

shwstppr · 2024-01-09T12:14:48Z

@blueorangutan package

blueorangutan · 2024-01-09T12:16:03Z

@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

blueorangutan · 2024-01-09T13:22:59Z

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

nvazquez · 2024-01-09T14:30:08Z

@blueorangutan package

blueorangutan · 2024-01-09T14:32:03Z

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

blueorangutan · 2024-01-09T15:32:41Z

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

nvazquez

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

DaanHoogland

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-cyborgBot 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 8921119Compare 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:mainJan 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

Uh oh!

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!

codecovBot 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!

DaanHooglandJan 5, 2024

Choose a reason for hiding this comment

Uh oh!

nvazquezJan 8, 2024

Choose a reason for hiding this comment

Uh oh!

slavkapJan 9, 2024

Choose a reason for hiding this comment

Uh oh!

nvazquezJan 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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

slavkap commented Jan 4, 2024•
edited
Loading

codecovBot commented Jan 4, 2024•
edited
Loading