-
Notifications
You must be signed in to change notification settings - Fork 95
Use vm shutdown instead of power off. Fixes issue #1505. #1585
Conversation
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.
Overall good fix, but I can you elaborate why the action are being done via ssh instead of govmomi ?
tests/utils/esx/govc.go
Outdated
// govc vm.power -s=true photon | ||
func ShutDownVM(vmName string) string { | ||
log.Printf("Shuting down VM [%s]\n", vmName) | ||
cmd := esx.ShutDownVM + vmName |
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.
why it is done with ssh, and not govmomi ?
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.
This is done with govmomi. The util go file name (ssh.go) is confusing I think - it used to contain a util function to call ssh, but later added a new function to call local shell. The current ssh.InvokeCommandLocally() is calling govc command in local shell.
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.
Yeas, but it's still go code calling shell to call local command line (govc) to use govmomi to talk to VC. Why not cut the middleman and call govmomi directly ? It's a generic question to all test, not to your PR - I understand you are just being consistent.
The rest LGTM ; I just want to understand the above
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.
I see your point. So you are asking why we are not using govmomi API/library directly. @shuklanirdesh82 did the early investigation. My guess is that that govc CLI seems easier to integrate with. Let me file a issue to track this so that the current fix will not be blocked.
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.
Filed issue #1586.
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
I'm holding on this PR because of the CI failure: https://ci.vmware.run/vmware/docker-volume-vsphere/1220 In CI there are two different setups. The same test (TestFailoverAcrossSwarmNodes) passed during 1st run, but failed during 2nd run. Need to discuss with @shuklanirdesh82 to figure out the difference between those 2 CI setups. |
@shaominchen No, that's not correct. This test fails on both the setups - sometimes on 6.0 and other times on 6.5. Failed run on 6.5 esx build: https://ci.vmware.run/vmware/docker-volume-vsphere/1224 |
@ashahi1 So far the test passed 100% in my local run on ESX 6.5, and failed 100% on CI ESX 6.0. To debug this issue, I'd suggest to use ESX 6.0 and enable TRACE logging. That will help us to figure out the root cause more easily. |
@shaominchen Twice I ran the test locally with 6.0 and test passed. And since test failed on CI even with 6.5 build, so test failure has nothing to do with esx version. Looks like we need to revisit the test steps here. Failed CI run with 6.5 esx build: https://ci.vmware.run/vmware/docker-volume-vsphere/1224 |
@ashahi1 As you have analyzed, it may not be related to esx versions. So please go ahead to debug the issue further. You'll need to enable TRACE logging to make debugging easier. |
Please refer to the issue for more information about the root cause and proposed solution.
Please refer to the issue #1505 for more information about the root cause and proposed solution.
Testing Done:
OK: 46 passed
--- PASS: Test (1830.20s)
PASS
ok github.com/vmware/docker-volume-vsphere/tests/e2e 1830.211s