-
Notifications
You must be signed in to change notification settings - Fork 95
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 looks good.
I have 2 comments
tests/e2e/vmlistener_test.go
Outdated
// 2. Setup: Attach it to a container | ||
// 3. Kill the VM | ||
// 4. Verification: volume status should be detached | ||
// 5. Attach it to a different container |
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 think it should be attach to a container on different VM.
Thats what it looks like from line 157
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.
Yes, will update the comments.
// Power on VM | ||
govc.PowerOnVM(s.vm1Name) | ||
isStatusChanged := misc.WaitForExpectedState(govc.GetVMPowerState, s.vm1Name, properties.PowerOnState) | ||
c.Assert(isStatusChanged, Equals, true, Commentf("VM [%s] should be powered on state", s.vm1Name)) |
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 isn't part of original test steps but I think it would be a good idea to add a check here (after L172) to ensure that status of the volume in detached. The reason being that container from VM2 is removed and VM1 has come up too.
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.
Will add.
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.
With this change, I think the existing TestKillVM can be removed. What do you think? @pshahzeb
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.
Agree that there is an overlap. I had a discussion with @shuklanirdesh82 in such cases. We should have it for now since we do not want to miss any bug in any of the possible permutation.
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 don't think it's a permutation in the current case. Anyway, I will keep it for now.
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
d61c1d8
to
1462b13
Compare
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
Added a new failover e2e test and also refactored existing test cases.
Fixes issue #1379
Testing Done:
=> Running target e2e-test-runalways Wed Jun 7 18:49:10 UTC 2017
--- PASS: Test (194.08s)
PASS
OK: 6 passed
=> Running target e2e-test-runonce Wed Jun 7 18:52:27 UTC 2017
--- PASS: Test (540.44s)
PASS
OK: 18 passed