Skip to content

vmware: fix volume stats logic#3473

Merged
yadvr merged 1 commit into
apache:masterfrom
shapeblue:vmware-volume-stats-logic
Jul 22, 2019
Merged

vmware: fix volume stats logic#3473
yadvr merged 1 commit into
apache:masterfrom
shapeblue:vmware-volume-stats-logic

Conversation

@yadvr

@yadvr yadvr commented Jul 5, 2019

Copy link
Copy Markdown
Member

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

yadvr commented Jul 5, 2019

Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

@yadvr

yadvr commented Jul 5, 2019

Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

@yadvr

yadvr commented Jul 5, 2019

Copy link
Copy Markdown
Member Author

@blueorangutan test centos7 vmware-65u2

@blueorangutan

Copy link
Copy Markdown

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

@anuragaw anuragaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@andrijapanicsb

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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

shwstppr commented Jul 8, 2019

Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

@yadvr

yadvr commented Jul 8, 2019

Copy link
Copy Markdown
Member Author

@blueorangutan test centos7 vmware-65u2

@blueorangutan

Copy link
Copy Markdown

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

@shwstppr

shwstppr commented Jul 8, 2019

Copy link
Copy Markdown
Contributor

@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.
Screenshot from 2019-07-08 14-54-30
Instance storage is almost full
Screenshot from 2019-07-08 14-51-05
But UI and API does not show correct physical size
Screenshot from 2019-07-08 14-53-29

@blueorangutan

Copy link
Copy Markdown

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

yadvr commented Jul 9, 2019

Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

@yadvr

yadvr commented Jul 15, 2019

Copy link
Copy Markdown
Member Author

@anuragaw @shwstppr anyone wants to take this one?

@shwstppr

Copy link
Copy Markdown
Contributor

@rhtyd will be looking into this

@shwstppr

shwstppr commented Jul 16, 2019

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

From vCenter,
Screenshot from 2019-07-18 13-42-50
Screenshot from 2019-07-18 13-53-04
Screenshot from 2019-07-18 14-30-41

From storage,
Screenshot from 2019-07-18 14-28-35

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

yadvr commented Jul 22, 2019

Copy link
Copy Markdown
Member Author

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.

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

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VMware: volume statistics show wrong values

7 participants