-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Vmware] Improve listing of Vmware Datacenter VMs for migration to KVM #10770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.19
Are you sure you want to change the base?
Conversation
@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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #10770 +/- ##
==========================================
Coverage 15.16% 15.16%
- Complexity 11332 11335 +3
==========================================
Files 5412 5414 +2
Lines 475033 475197 +164
Branches 57963 57990 +27
==========================================
+ Hits 72048 72073 +25
- Misses 394930 395063 +133
- Partials 8055 8061 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13166 |
@blueorangutan test ol8 vmware-70u3 |
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + vmware-70u3) has been kicked to run smoke tests |
.../vmware/src/main/java/org/apache/cloudstack/api/command/admin/zone/ListVmwareDcHostsCmd.java
Outdated
Show resolved
Hide resolved
[SF] Trillian test result (tid-13127)
|
…/api/command/admin/zone/ListVmwareDcHostsCmd.java Co-authored-by: Suresh Kumar Anaparti <[email protected]>
@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 13184 |
@blueorangutan test ol8 vmware-70u3 |
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + vmware-70u3) has been kicked to run smoke tests |
[SF] Trillian test result (tid-13159)
|
.../vmware/src/main/java/org/apache/cloudstack/api/command/admin/zone/ListVmwareDcHostsCmd.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one remark, otherwise LGTM - this needs testing
Hi @sureshanaparti @DaanHoogland @rohityadavcloud I could improve the listing reducing the time significantly by excluding the disks and NICs information on the initial listing and then obtaining them when the user selects the VM (I have updated the PR description to explain the new behavior). Can you please re-review? |
@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]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13265 |
@blueorangutan test ol8 vmware-70u3 |
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + vmware-70u3) has been kicked to run smoke tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgg,
powerState == VirtualMachinePowerState.POWERED_OFF ? UnmanagedInstanceTO.PowerState.PowerOff : UnmanagedInstanceTO.PowerState.PowerUnknown; | ||
} | ||
|
||
protected List<UnmanagedInstanceTO> convertVmsObjectContentsToUnmanagedInstances(List<ObjectContent> ocs, String keyword) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
complexity of this method is a bit high, would be nice to modularise it a bit.
as this functionality was implemented in 4.19.2 (but with side effects) and thus would be a regression if not merged, I marked this as BLOCKER. |
[SF] Trillian test result (tid-13205)
|
Description
This PR improves the listing of Vmware VMs for KVM migration tool to reduce the timeout observed while listing VMs on a datacenter. The improvement is performed in two stages:
listVmwareDcVms
API setting thedatacentername
parameter only)listVmwareDcVms
API setting thedatacentername
,hostname
andvirtualmachinename
parameter)It was originally added by PR: #9925 but reverted as it caused unintended regression affecting the VM console access (#10634).
Fixes: #9782
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Tested against Vmware vCenter datacenter with 200+ VMs, listing times reduced significantly from 30 seconds to <1 second for the entire datacenter VMs
Same command time difference against the same vCenter: (
time cmk list vmwaredcvms vcenter=XXXX datacentername=XXXX username=XXXX password=XXXX page=1 pagesize=10 filter=name
)Before:
After: