[E2E] Validation after admincli vmgroup and config ops #1568
Conversation
pshahzeb
changed the title
| } | ||
|
|
||
| // Verify the vmgroup removal | ||
| if IsVmgroupPresent(ip, name) != true { |
There was a problem hiding this comment.
When "delete_vol" is set to true, should also check volume that associated with that vmgroup have been removed.
There was a problem hiding this comment.
Currently this util doesn't have knowledge about which volumes fall into the given VMgroup. So directly we cannot verify.
What we can do is retrieve the list of volumes from admin cli and in verification make sure those volumes are deleted. But I would prefer doing it out of this PR depending on the need of such verification.
tests/utils/admincli/vmgroupmgmt.go
Outdated
| } | ||
|
|
||
| // verify the DB init | ||
| if GetDBmode(ip) != admincli.DBSingleNode { |
There was a problem hiding this comment.
This logic seems not exactly consistent with what we are doing in above CreateVmGroup and DeleteVmGroup. If we follow the same style, the logic here should be something like this:
if GetDBmode(ip) == admincli.DBSingleNode {
return out, nil
}
msg := ...
return msg, fmt.Errorf(msg)Not a big concern, but I'd like to know if there are any specific reason for this difference.
There was a problem hiding this comment.
No specific reason. Just happened to write the if condition in that way. Would you want it to be changed as per style in CreateVmGroup and DeleteVmGroup?
There was a problem hiding this comment.
The logic in CreateVmGroup and DeleteVmGroup makes more sense to me because we just want to add one extra verification, but don't really want to change the original successful command output.
There was a problem hiding this comment.
Sure. Will make it consistent.
tests/utils/admincli/vmgroupmgmt.go
Outdated
| } | ||
|
|
||
| // verify the DB removal | ||
| if GetDBmode(ip) != admincli.DBNotConfigured { |
tests/utils/admincli/vmgroupmgmt.go
Outdated
|
|
||
| // verify the DB init | ||
| if GetDBmode(ip) == admincli.DBSingleNode { | ||
| return "DB init successful on esx " + ip, nil |
There was a problem hiding this comment.
Shouldn't we return original command output in this case?
return out, nilBasically the point is: we don't want to change the original command output if the extra verification passes.
tests/utils/admincli/vmgroupmgmt.go
Outdated
|
|
||
| // verify the DB removal | ||
| if GetDBmode(ip) == admincli.DBNotConfigured { | ||
| return "DB remove successful on esx " + ip, nil |
This PR adds verification step in following 4 utils
CreateVMgroup()
DeleteVMgroup()
ConfigInit
ConfigRemove
The intention is to catch the regression early with such verification and avoid misleading during debugging as to which test failed and the reason for the same
Testing:
test-e2e passes
Fixes #1458