vmware: fix volume stats logic by yadvr · Pull Request #3473 · apache/cloudstack

yadvr · 2019-07-05T09:58:12Z

During volume stats calculation, if a volume has more than one disk in
the chain-info it is not used to sum the physical and virtual size
in the loop, instead any previous entry was overwritten by the last disk.

Fixes #3416

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)

During volume stats calculation, if a volume has more than one disk in the chain-info it is not used to sum the physical and virtual size in the loop, instead any previous entry was overwritten by the last disk. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

yadvr · 2019-07-05T09:59:16Z

@blueorangutan package

blueorangutan · 2019-07-05T09:59:45Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2019-07-05T11:25:28Z

Packaging result: ✖centos6 ✖centos7 ✔debian. JID-91

yadvr · 2019-07-05T11:40:41Z

@blueorangutan package

blueorangutan · 2019-07-05T11:41:46Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2019-07-05T12:03:01Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-95

yadvr · 2019-07-05T12:09:40Z

@blueorangutan test centos7 vmware-65u2

blueorangutan · 2019-07-05T12:10:46Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-65u2) has been kicked to run smoke tests

anuragaw

LGTM. Haven't tested but code looks solid.
Would be great, @andrijapanic if you can quickly test and LGTM this.

andrijapanicsb · 2019-07-05T12:37:49Z

Can't comment atm, but @rhtyd this was an issue with both full clones and linked clones.

I'll be able to test this after 17th...

blueorangutan · 2019-07-05T21:48:57Z

Trillian test result (tid-107)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33310 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3473-t107-vmware-65u2.zip
Smoke tests completed. 66 look OK, 6 have error(s)
Only failed tests results shown below:

shwstppr · 2019-07-08T05:57:56Z

@blueorangutan package

blueorangutan · 2019-07-08T05:58:58Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2019-07-08T06:20:31Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-97

yadvr · 2019-07-08T06:24:34Z

@blueorangutan test centos7 vmware-65u2

blueorangutan · 2019-07-08T06:24:58Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-65u2) has been kicked to run smoke tests

shwstppr · 2019-07-08T09:30:30Z

@rhtyd Still not seeing correct results. With VMware 65u2, I don't think code is taking care of -delta.vmdk. Please see screenshots,
From NFS primary storage. -delta.vmdk file size increases as I write data in instance.

Instance storage is almost full

But UI and API does not show correct physical size

blueorangutan · 2019-07-08T16:05:32Z

Trillian test result (tid-113)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33304 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3473-t113-vmware-65u2.zip
Smoke tests completed. 68 look OK, 4 have error(s)
Only failed tests results shown below:

yadvr · 2019-07-09T07:23:01Z

@blueorangutan package

blueorangutan · 2019-07-09T07:24:03Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2019-07-09T07:45:28Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-102

yadvr · 2019-07-15T11:33:36Z

@anuragaw @shwstppr anyone wants to take this one?

shwstppr · 2019-07-15T11:41:56Z

@rhtyd will be looking into this

shwstppr · 2019-07-16T12:55:33Z

As reported in #3473 (comment), ROOT-*-delta.vmdk file size increases whenever something is written to instance disk. But this vmdk file is not taken into account while calculating volumes' physical size.
I get following volume info for one such volume,
{"diskDeviceBusName":"ide0:1","diskChain":["[729e08870a5d311c92d455db46da45d1] i-2-3-VM/ROOT-3.vmdk","[729e08870a5d311c92d455db46da45d1] 3ca2d374b8083bfda1591a3e3757db70/3ca2d374b8083bfda1591a3e3757db70.vmdk"]}
Dring my debugging I couldn't find any ACS code that deals with ascertaining this disk chain and it appears it is done by VMware library class VirtualMachineDiskInfoBuilder.
Any help here @nvazquez @rhtyd

shwstppr · 2019-07-18T09:04:47Z

From vCenter,

From storage,

I've verified with debugging https://github.com/apache/cloudstack/blob/master/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java#L2597-L2609, VirtualDisk points to ROOT-5.vmdk and not to ROOT-5-delta.vmdk. Therefore only two vmdk files in the chain,
{"diskDeviceBusName":"ide0:1","diskChain":["[b5fe57d739a33733bdd825fa62e86d4d] i-2-5-VM/ROOT-5.vmdk","[b5fe57d739a33733bdd825fa62e86d4d] 9b41f3d52a7f36678e302280f7ca2bbb/9b41f3d52a7f36678e302280f7ca2bbb.vmdk"]}

Even with ROOT-5.vmdk file size is different from vCenter and storage. Do we need to explore a different method to get volume usage on VMware?

yadvr · 2019-07-22T11:57:36Z

Merging this based on lgtms and reviews. It's possible that more fixes need to be done, but at least this change ensures that multi-chained disks are not ignored. I'll keep the original issue open but close/merge this PR.

DaanHoogland

logic looks sane, i hope we can rely on the vmware logic to be as sane (i.e. chainInfo works as intuitively and consistently as it seems from this code)

yadvr added the component:vmware label Jul 5, 2019

yadvr added this to the 4.13.0.0 milestone Jul 5, 2019

yadvr requested review from DaanHoogland, GabrielBrascher, PaulAngus, andrijapanicsb, borisstoyanov, nvazquez and sateesh-chodapuneedi July 5, 2019 09:58

nvazquez approved these changes Jul 5, 2019

View reviewed changes

anuragaw approved these changes Jul 5, 2019

View reviewed changes

yadvr merged commit e1fa270 into apache:master Jul 22, 2019

DaanHoogland approved these changes Jul 22, 2019

View reviewed changes

yadvr mentioned this pull request Jul 22, 2019

VMware: volume statistics show wrong values #3416

Closed

Conversation

yadvr commented Jul 5, 2019

Types of changes

Uh oh!

yadvr commented Jul 5, 2019

Uh oh!

blueorangutan commented Jul 5, 2019

Uh oh!

blueorangutan commented Jul 5, 2019

Uh oh!

yadvr commented Jul 5, 2019

Uh oh!

blueorangutan commented Jul 5, 2019

Uh oh!

blueorangutan commented Jul 5, 2019

Uh oh!

yadvr commented Jul 5, 2019

Uh oh!

blueorangutan commented Jul 5, 2019

Uh oh!

anuragaw left a comment

Choose a reason for hiding this comment

Uh oh!

andrijapanicsb commented Jul 5, 2019

Uh oh!

blueorangutan commented Jul 5, 2019

Uh oh!

shwstppr commented Jul 8, 2019

Uh oh!

blueorangutan commented Jul 8, 2019

Uh oh!

blueorangutan commented Jul 8, 2019

Uh oh!

yadvr commented Jul 8, 2019

Uh oh!

blueorangutan commented Jul 8, 2019

Uh oh!

shwstppr commented Jul 8, 2019

Uh oh!

blueorangutan commented Jul 8, 2019

Uh oh!

yadvr commented Jul 9, 2019

Uh oh!

blueorangutan commented Jul 9, 2019

Uh oh!

blueorangutan commented Jul 9, 2019

Uh oh!

yadvr commented Jul 15, 2019

Uh oh!

shwstppr commented Jul 15, 2019

Uh oh!

shwstppr commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shwstppr commented Jul 18, 2019

Uh oh!

yadvr commented Jul 22, 2019

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shwstppr commented Jul 16, 2019 •

edited

Loading