Made CLI return proper code to shell

[msterin]

Partially addresses #1411

CLI code used to print error message and exit, without passing proper code to shell.
This PR fixes that.
There may still be some functions which do not return proper status , but at least this fixes the framework

Functions which do not return proper code: example.
set_vol_opts() should return None on success and return "err msg" on error. Currently it prints stuff and always returns success. A quick review of CLI code is needed to make sure all return proper code.

Tested: manually (below) plus CI

[root@msterin-esx-136:/usr/lib/vmware/vmdkops/bin] ./vmdkops_admin.py config status ; echo $?
DB_Mode: SingleNode (local DB exists)
DB_SharedLocation: N/A
DB_LocalPath: /etc/vmware/vmdkops/auth-db
0
[root@msterin-esx-136:/usr/lib/vmware/vmdkops/bin] ./vmdkops_admin.py volume ls ; echo $?
Volume             Datastore      VMGroup   Capacity  Used  Filesystem  Policy          Disk Format  Attached-to  Access      Attach-as         Created By     Created Date    
-----------------  -------------  --------  --------  ----  ----------  --------------  -----------  -----------  ----------  ----------------  -------------  ----------------
default_tenant_..  vsanDatastore  _DEFAULT  100MB     8MB   ext4        [VSAN default]  thin         detached     read-write  independent_pe..  test-vm_55592  Wed Jun 21 22:..

0
[root@msterin-esx-136:/usr/lib/vmware/vmdkops/bin] ./vmdkops_admin.py volume lsxx ; echo $?
usage: vmdkops_admin.py volume [-h] {set,ls} ...
vmdkops_admin.py volume: error: invalid choice: 'lsxx' (choose from 'set', 'ls')
2

[shaominchen]

LGTM. Thanks for addressing this!

[shuklanirdesh82]

Great! The fix is working and helping as expected.

There is a CI failure, I believe test step needs to be fixed till we have to hold this PR merge.

2017/06/23 01:20:18 Deleting a vmgroup [T1] on esx [192.168.31.62]

----------------------------------------------------------------------
FAIL: basic_test.go:155: BasicTestSuite.TestVmGroupVolumeIsolation

basic_test.go:199:
    c.Assert(err, IsNil, Commentf(out))
... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc4200d0460), Stderr:[]uint8(nil)} ("exit status 2")
... All Volumes will be removed
Cannot delete non-empty vmgroup T1. Remove VMs from the vmgroup before deleting it.

[msterin]

@shuklanirdesh82 - so the failure is expected? It is not obvious from go test code..

[shuklanirdesh82]

@msterin The step was added recently and it seems slipped during the review (#1454)

Test is creating a vmgroup and adding a vm to it > later deletes the vmgroup (which is expected to fail as vm is still part of it).

Correct way of doing is to remove vm from vmgroup and invoke vmgroup deletion.

Test is having config remove step hence not observed so far or no side effect so far.

In short there is a missing step in the test case to remove vm from vmgroup and then delete.

/CC @shaominchen

[shaominchen]

Yes, as @shuklanirdesh82 mentioned, we are missing one step in that test case, which is exposed by the current change.

@msterin Can you try to add the following 2 lines into the test case and see if it works?

++ out, err = admincli.RemoveVMFromVMgroup(s.esx, vmgroup, s.vm1Name)
++ c.Assert(err, IsNil, Commentf(out))
out, err = admincli.DeleteVMgroup(s.esx, vmgroup)
c.Assert(err, IsNil, Commentf(out))

[shaominchen]

This fix is already in. So just rebase with master and you'll get it work.

[pshahzeb]

As per the latest CI run log, one pass was successful but the second pass failed with following error on BasicTestSuite

FAIL: basic_test.go:159: BasicTestSuite.TestVmGroupVolumeIsolation

basic_test.go:164:
    c.Assert(err, IsNil, Commentf(out))
... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc4200ae6a0), Stderr:[]uint8(nil)} ("exit status 2")
... Warning: this feature is EXPERIMENTAL
Config DB  is already initialized. Use 'rm --local' or 'rm --unlink' to reset
Additional information: {'DB_Mode': 'SingleNode (local DB exists)', 'DB_LocalPath': '/etc/vmware/vmdkops/auth-db', 'DB_SharedLocation': 'N/A'}

[shaominchen]

@pshahzeb Yes, Mark has filed an issue for this: #1508. Currently the e2e test case's assumption is to start with a clean setup, and end with a clean setup. We are making sure the test case itself should clean up everything it creates and restore the setup to a clean state. But we are not checking in the beginning if the setup is clean or not. We should add a bootstrap for e2e test suite to handle this.

[msterin]

Currently the e2e test case's assumption is to start with a clean setup, and end with a clean setup.

Looks like it does not , thus the first pass leaves it configured and the second pass fails. It used to pass because nobody paid attention to action success of failure of CLI (since it was not telling :-))

[govint]

Possibly, add a test group that cleans up the config DB (at start, something like a_cleanup_test.go)

[shaominchen]

Possibly, add a test group that cleans up the config DB (at start, something like a_cleanup_test.go)

Yes, we should have done that. Actually, it looks like some tests are not stable enough. Even if we start with a clean setup, this setup might still be messed up if some unstable tests failed unexpectedly. So to be safe, we may want to clean up the config DB before running each test group.

We can use issue #1508 to discuss this improvement.

[pshahzeb]

. So to be safe, we may want to clean up the config DB before running each test group.

+1

[pshahzeb]

Lets merge this.