RFE: handle oc binary doesn't exist

[vasukulkarni]

probably check in the runner and bail out sooner...

2019-04-23 10:51:07,465 - test_ocs_basic_install - INFO - Executing command: oc cluster-info
2019-04-23 10:51:07,480 - __main__ - ERROR - Traceback (most recent call last):
  File "run.py", line 198, in run
    rc = test_mod.run(**test_kwargs)
  File "/Users/vasukulkarni/ocs-ci/tests/test_ocs_basic_install.py", line 80, in run
    run_cmd("oc cluster-info")
  File "/Users/vasukulkarni/ocs-ci/tests/test_ocs_basic_install.py", line 111, in run_cmd
    r = subprocess.run(cmd.split(), stdout=sys.stdout, stderr=sys.stderr, **kwargs)
  File "/Users/vasukulkarni/ocs-ci/venv2/lib/python3.7/site-packages/gevent/subprocess.py", line 1672, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/Users/vasukulkarni/ocs-ci/venv2/lib/python3.7/site-packages/gevent/subprocess.py", line 627, in __init__
    restore_signals, start_new_session)
  File "/Users/vasukulkarni/ocs-ci/venv2/lib/python3.7/site-packages/gevent/subprocess.py", line 1505, in _execute_child
    raise child_exception
FileNotFoundError: [Errno 2] No such file or directory: 'oc'

[clacroix12]

We can do this pretty easily with shutil.which("oc"). If we don't find anything, we can log and raise an exception.

[shylesh]

This could be problematic. In my case i downloaded oc-tools (which had oc binary) to a non-standard binary path which could lead to shutil.which("oc") fail. If we enforce restriction saying oc binary should be present in /usr/bin then this might work.

[clacroix12]

@shylesh shutil.which() will return it if it is on the path. if it's not on the path, the oc commands we execute will end up failing anyways.

[shylesh]

If this discussion is all about just fail if it doesn't exist then Yes it makes sense. What I meant is oc could be in custom path which may not be noticed by which() unless its in paths scanned by shutil.which(). Either we can document the path where we expect oc binary should be (either /usr/bin OR any path which could be added later in python path. Also if oc-tools has different versions how to tell end users about this ? I feel its better we handle it inside out script.

[clacroix12]

I was mostly just trying to get after vasu's proposed solution to check in the runner and bail out sooner. However, I think this conversation changes a bit if we plan on using something like openshift-client-python instead of manually executing oc commands. For now we are manually executing cmds so if we recognize that it's not there we either need to fail or install it. If it is installed, we can also do a version check if there is functionality that we utilize in later versions that isn't supported in older ones.

[vavuthu]

We need to leave installing of "oc' to the users and move to /usr/bin/ , so that it can access from anywhere instead of handling in framework.

Above should go to documentation( in pre-requisites for framework)

[shylesh]

One of the downside of this leaving it to the user could be:
What if all the users are launching automation from same machine (ex: ci slave) and each one of them testing different versions of oc ? /usr/bin/ won't work here .
May be we need something like virtualenv !!

[mbukatov]

This could be problematic. In my case i downloaded oc-tools (which had oc binary) to a non-standard binary path which could lead to shutil.which("oc") fail. If we enforce restriction saying oc binary should be present in /usr/bin then this might work.

Why? This will be a problem only when your non-standard binary path is not in PATH variable.

And when you actually put oc to a directory which is not in PATH, the ocs-ci code won't work, because it assumes just that. So checking this via shutils.which() seems quite reasonable to me.

Eg. I have oc in ~/bin, which is in my PATH and so shutils.which() works as expected:

>>> shutil.which("oc")
'/home/ocsqe/bin/oc'

What if all the users are launching automation from same machine (ex: ci slave) and each one of them testing different versions of oc ? /usr/bin/ won't work here .
May be we need something like virtualenv !!

If you actually want to put oc somewhere else, eg. because you want to have multiple versions installed, you could just redefine PATH just for ocs-ci's run.py, eg.:

[ocsqe@localhost ~]$ mkdir -p somewhere/else
[ocsqe@localhost ~]$ cp ~/bin/oc somewhere/else
[ocsqe@localhost ~]$ python3 -c 'import shutil; print(shutil.which("oc"))'
/home/ocsqe/bin/oc
[ocsqe@localhost ~]$ PATH=somewhere/else:$PATH python3 -c 'import shutil; print(shutil.which("oc"))'
somewhere/else/oc

[shylesh]

This could be problematic. In my case i downloaded oc-tools (which had oc binary) to a non-standard binary path which could lead to shutil.which("oc") fail. If we enforce restriction saying oc binary should be present in /usr/bin then this might work.

Why? This will be a problem only when your non-standard binary path is not in PATH variable.

And when you actually put oc to a directory which is not in PATH, the ocs-ci code won't work, because it assumes just that. So checking this via shutils.which() seems quite reasonable to me.

Eg. I have oc in ~/bin, which is in my PATH and so shutils.which() works as expected:

>>> shutil.which("oc")
'/home/ocsqe/bin/oc'

What if all the users are launching automation from same machine (ex: ci slave) and each one of them testing different versions of oc ? /usr/bin/ won't work here .
May be we need something like virtualenv !!

If you actually want to put oc somewhere else, eg. because you want to have multiple versions installed, you could just redefine PATH just for ocs-ci's run.py, eg.:

[ocsqe@localhost ~]$ mkdir -p somewhere/else
[ocsqe@localhost ~]$ cp ~/bin/oc somewhere/else
[ocsqe@localhost ~]$ python3 -c 'import shutil; print(shutil.which("oc"))'
/home/ocsqe/bin/oc
[ocsqe@localhost ~]$ PATH=somewhere/else:$PATH python3 -c 'import shutil; print(shutil.which("oc"))'
somewhere/else/oc

I Agree, but the point we wanted to converge on is whether to leave this oc util configuration to user OR to handle it in framework(sorry if I was not clear in my previous comment). If user's oc version is deprecated or not supported and unknowingly if user uses it, since we don't have any control on such version check it may lead to silent bugs.
So considering all these factors I was just supporting to handle oc binary management inside framework itself.User can just specify oc version in config and we will handle it inside framework (may not be required at first cut of framework but for long term).

my 2 cents.

[mbukatov]

So considering all these factors I was just supporting to handle oc binary management inside framework itself. User can just specify oc version in config and we will handle it inside framework (may not be required at first cut of framework but for long term).

Do we consider oc to be one of the components under test? If yes, we may need to be able to use oc which matches OCP/OCS build which is being tested.

Also, it would be a good idea to use the same approach for both oc and openshift-install binaries.

[dahorak]

I agree with Martin, that we should handle the oc the same way as openshift-install:

• we should use the same version of both binaries
• there is already logic to download the right version of openshift-install, so it doesn't make much sense to handle oc differently and have two places where to configure which version should be downloaded and used...
• we can call the oc the same way as the openshift-install.... like: ./oc ... so we don't need to add it to PATH for the automated testing
  • Martin's suggestion: small improvement here would be to move both oc and openshift-install binaries to ./bin subdirectory... then we can configure PATH easily for manual testing/debugging etc...

[dahorak]

If you are not against the idea suggested in the previous comment, I can create PR for that.

[clacroix12]

Do we consider oc to be one of the components under test?

I don't think we do, at least not currently. However I still think that we should be using an oc version that matches the version of OCP, and handling it the same way we handle openshift-install makes sense.

If you are not against the idea suggested in the previous comment, I can create PR for that.

My vote is go ahead with this. I think it's a great idea and something we should be handling properly.

[vasukulkarni]

@dahorak sure lets go with the same approach, but eventually we need the flexibility to test with mixed oc versions by changing the parameter(or iterating few tests with different oc versions), we haven't reached that state yet, so probably what you suggested is good.