-
Notifications
You must be signed in to change notification settings - Fork 95
Fix to match full volume names in refcount map. #1512
Conversation
CI failed with govc error, 2017/06/29 11:19:30 Adding VM [photon.vmfs] to a vmgroup [vmgroup_test1] on esx [192.168.31.62] FAIL: vmgroups_test.go:53: VmGroupTest.SetUpSuite vmgroups_test.go:79: 2017/06/29 11:19:34 Removing VM [photon.vmfs] from a vmgroup [vmgroup_test1] on esx [192.168.31.62] |
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.
Thanks for fixing this Govindan.
This fix will work. However, this logic can be optimized a bit.
The idea of introducing GetNameFromRefmap was to avoid a trip to ESX vmdkops service to get volume meta. Hence the call to it was placed before actually doing a GetVolume ( L139 in the earlier change)
With this bug, looks like this optimization is not valid in all cases.
With this change, since we first endup doing a GetVolume, we are bound to construct a full volume name after that. We can just call makeFullVolName and return the VolumeInfo
Call to GetNameFromRefmap won't get us anything useful after that. Infact this entire thing can be cleaned up. I am fine if you think this should be cleaned up in a separate PR.
I was wondering if the GetNameFromRefmap() was also for checking duplicates. If that's not a needed check then I'll remove that all together. And the SplitVolume() function too. This fix needs to go in before #1500. |
Yes. You can remove. The fix itself looks good. |
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.
LGTM
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.
LGTM.
The build failed is likely due to the issue fixed by #1519. Let's rebase and get CI pass and we are good to merge. |
0c2c2dd
to
0e6f19b
Compare
@govint - please add a few words to description directly explaining what is the problem and what is the fix. As of now, there is a reference to another (long and not super clear) issue so I got lost really fast. |
@msterin, sure, below is the issue: The issue is how refcounting handles volumes with the same name on different datastores. When the plugin restarts and begins refcount'ing volumes it does this:
|
Make of plugin failed, mkdir -p /tmp/plugin/rootfs |
The changes are added to the changes submitted for #1500 and being tested there for the docker restart tests.