-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fixed volume stats #3798
Fixed volume stats #3798
Conversation
@Spaceman1984 is this meant to go in 4.14, 4.13.1 or else? |
@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
@andrijapanicsb Looks like 4.13.1 |
@Spaceman1984 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
@Spaceman1984 pls don't quote the reply with the blue monkey, it triggers re-build or whatever command was there for "him"... |
@blueorangutan package |
@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
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.
@Spaceman1984 , though it looks like an improvement to me, i have one worry:
- we are iterating over volumes
- we get the owning vm
- we get the file layout for the vm
- and we than iterate over the files for that vm.
it seems to me that we might count doubles if multiple volumes are attached to a vm.
any thoughts?
VirtualMachineFileLayoutEx fileLayout = vmMo.getFileLayout(); | ||
List<VirtualMachineFileLayoutExFileInfo> files = fileLayout.getFile(); | ||
for(VirtualMachineFileLayoutExFileInfo fileInfo: files){ | ||
if (fileInfo.getName().contains("ROOT")){ |
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.
on second thought, there is no way here we know that we "only" want "ROOT" while iterating over a list of cmd.getVolumeUuids()
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-558 |
@blueorangutan test |
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Trillian test result (tid-738)
|
There is an issue here, this fix doesn't take into account when the volume has a different name than root, and it also doesn't work for volumes that are not attached to a vm. A rework is in progress |
Description
This fixes a bug where the volume statistics for the physical size is incorrect
Fixes: #3416
Types of changes
Screenshots (if appropriate):
How Has This Been Tested?
This was tested by verifying that the vmdk file size in vcenter corresponds to the physical volume size in the ui of the management server. Please note that vcenter displays kilobytes where
Cloudstack management server displays kibibytes on linux.