testing container in OpenShift #164

phracek · 2017-11-08T12:53:23Z

This Pull Request brings possibility to test containers in OpenShift environment.

MTF provides two scripts for preparing environment like MODULE=openshift mtf-env-set and MODULE=openshift mtf-env-clean

Usage is like:
MODULE=openshift OPENSHIFT_IP="Openshift_IP_Address" OPENSHIFT_USERNAME=developer OPENSHIFT_PASSWD=developer mtf -l *.py

The Pull Request fixes issue #89

TomasTomecek · 2017-11-08T14:22:50Z

moduleframework/avocado_testers/openshift_avocado_test.py

+# INTERFACE CLASSES FOR SPECIFIC MODULE TESTS
+class OpenShiftAvocadoTest(AvocadoTest):
+    """
+    Class for writing tests specific just for DOCKER


s/DOCKER/OpenShift/

TomasTomecek · 2017-11-08T14:23:41Z

moduleframework/avocado_testers/openshift_avocado_test.py

+            self.cancel("OpenShift specific test")
+        super(OpenShiftAvocadoTest, self).setUp()
+
+    def checkLabel(self, key, value):


is this method up to date?

TomasTomecek · 2017-11-08T14:27:09Z

moduleframework/helpers/openshift_helper.py

+
+    def stop(self):
+        """
+        Stop the docker container


docstring not up to date

TomasTomecek · 2017-11-08T14:27:43Z

moduleframework/helpers/openshift_helper.py

+        """
+        start the OpenShift application
+
+        :param args: Do not use it directly (It is defined in config.yaml)


this method doesn't accept any arguments

TomasTomecek · 2017-11-08T14:29:07Z

moduleframework/helpers/openshift_helper.py

+            self.app_ip = service["items"][0]["spec"]["clusterIP"]
+            common.trans_dict['GUESTIPADDR'] = self.app_ip
+            common.print_info(common.trans_dict)
+            return True


docstring says that this method should return an ip address

TomasTomecek · 2017-11-08T14:29:53Z

moduleframework/helpers/openshift_helper.py

+            common.trans_dict['GUESTIPADDR'] = self.app_ip
+            common.print_info(common.trans_dict)
+            return True
+        except KeyError as e:


can also throw IndexError and TypeError (None[0])

TomasTomecek · 2017-11-08T14:30:51Z

moduleframework/helpers/openshift_helper.py

+        service = self._convert_string_to_json(oc_get_service.stdout)
+        try:
+            common.print_info(service["items"])
+            common.print_info(service["items"][0])


what if there are other services running?

TomasTomecek · 2017-11-08T14:31:43Z

moduleframework/helpers/openshift_helper.py

+        """
+        return json.loads(
+            self.runHost(
+                "docker inspect %s" %


openshift is not tied to docker

TomasTomecek · 2017-11-08T14:32:51Z

moduleframework/helpers/openshift_helper.py

+
+    def _openshift_login(self, oc_ip="127.0.0.1", oc_user='developer', oc_passwd='developer', env=False):
+        if env:
+            if 'OPENSHIFT_IP' in os.environ:


documentation would be lovely

Signed-off-by: Petr "Stone" Hracek <[email protected]>

TomasTomecek

docs still need more love

TomasTomecek · 2017-11-10T08:29:26Z

docs/user_guide/environment_setup.rst

+**OpenShift**
+
+ - Install OpenShift if not installed and if environment variable ``OPENSHIFT_LOCAL`` is specified.
+ - if ``OPENSHIFT_LOCAL`` variable is specified, the it starts an OpenShift by command ``oc cluster up`` or stops it by command ``oc cluster down``.


TomasTomecek · 2017-11-10T08:31:04Z

docs/user_guide/environment_variables.rst

+- **OPENSHIFT_LOCAL** enables installing ``origin`` and ``origin-clients`` on local machine
+- **OPENSHIFT_IP** uses this IP address for connecting to an OpenShift environment.
+- **OPENSHIFT_USER** uses this ``USER`` name for login to an OpenShift environment.
+- **OPENSHIFT_PWD** uses this ``PWD`` name for login to an OpenShift environment.


PWD in our world means "print working directory"

TomasTomecek · 2017-11-10T08:32:11Z

docs/user_guide/environment_variables.rst

@@ -25,6 +25,10 @@ Environment variables allow to overwrite some values of a module configuration f
 - **MTF_REMOTE_REPOS=yes** disables downloading of Koji packages and creating a local repo, and speeds up test execution.
 - **MTF_DISABLE_MODULE=yes** disables module handling to use nonmodular test mode (see `multihost tests`_ as an example).
 - **DOCKERFILE="<path_to_dockerfile"** overwrites the location of a Dockerfile.
+- **OPENSHIFT_LOCAL** enables installing ``origin`` and ``origin-clients`` on local machine


would be nice to follow the previous definitions: e.g. what is the sample value for this variable?

TomasTomecek · 2017-11-10T08:34:44Z

moduleframework/helpers/openshift_helper.py

+from moduleframework.mtfexceptions import ContainerExc, ConfigExc
+
+
+class OpenShiftHelper(CommonFunctions):


So how do I actually use this?

It is used from commandline sudo MODULE=openshift mtf which executes this class.

sweet! include it in docs please

Signed-off-by: Petr "Stone" Hracek <[email protected]>

jscotka

thanks for this PR, few comments inside code

jscotka · 2017-11-13T08:44:57Z

moduleframework/common.py

@@ -513,6 +513,8 @@ def getIPaddr(self):

        :return: str
        """
+        if 'openshift' == os.environ.get('MODULE'):


why this? in case you set self.ipaddr in Openshift helper it return proper value.

jscotka · 2017-11-13T08:46:15Z

moduleframework/environment_prepare/openshift_prepare.py

+
+    def __prepare_selinux(self):
+        # disable selinux by default if not turned off
+        if not os.environ.get('MTF_SKIP_DISABLING_SELINUX'):


why this? openshift is also crappy with selinux enabled (nspawn is crappy with selinux enabled)?

jscotka · 2017-11-13T08:49:03Z

moduleframework/environment_prepare/openshift_prepare.py

+
+        :return: None
+        """
+        if os.environ.get('OPENSHIFT_LOCAL'):


please move os.environ.get('OPENSHIFT_LOCAL') to own function in common module and then use it instead of this env variable check.

jscotka · 2017-11-13T08:51:35Z

moduleframework/helpers/openshift_helper.py

+from moduleframework.mtfexceptions import ContainerExc, ConfigExc
+
+
+class OpenShiftHelper(CommonFunctions):


doen't make bigger sense to have ContainerHelper as parent instead of CommonFunctions in case docker is close to openshitft. in case it does not make sense, then this is okay.

and also remember CONU project. If we expect similar code in CONU, woule be better to be closer to docker class, and then you can remove some functions what are already defined in ContainerHelper

jscotka · 2017-11-13T08:54:08Z

moduleframework/helpers/openshift_helper.py

+        self.app_ip = None
+        common.print_debug(self.icontainer, self.app_name)
+
+    def get_container_url(self):


as an example, this method is same as in ContainerHelper

jscotka · 2017-11-13T08:57:34Z

moduleframework/helpers/openshift_helper.py

+        """
+        if env:
+            if 'OPENSHIFT_IP' in os.environ:
+                oc_ip = os.environ.get('OPENSHIFT_IP')


please move all os.environ.get to separated function, then you don't need to do this code and do it simplier and cleaner like: oc_ip = common.get_os_ip() or oc_ip

jscotka · 2017-11-13T09:00:25Z

moduleframework/helpers/openshift_helper.py

+                                                                                   oc_user,
+                                                                                   oc_passwd),
+                                 verbose=common.is_not_silent())
+        common.print_debug(oc_output.stdout)


why you use this debugging? isn't better to use verbose=variable directly. it exactly does what you want
verbose=is_debug() print via avocado run command output to logger and it is there. in case DEBUG is not set, it is not printed out. (when you don't use it with avocado runner, log is not shown, but it can be enabled.)

jscotka · 2017-11-13T09:03:43Z

moduleframework/helpers/openshift_helper.py

+        try:
+            for svc in service["items"]:
+                if "clusterIP" in svc.get("spec"):
+                    common.trans_dict['GUESTIPADDR'] = svc.get("spec").get("clusterIP")


maybe this is proper place where to modify also self.ipaddr variable and then you can remove some special handling of ipadd in common.CommonFunciton for OS

and maybe question, why there is for cycle. means that there can be more ipadresses and and the last one is the best one?
or you meant code like:

for svc un service.get("items", []): ipaddr = svc.get("spec",{}).get("clusterIP") if ipaddr: common.trans_dict['GUESTIPADDR'] = ipaddr self.ipaddr = ipaddr return True return False

?

jscotka · 2017-11-13T09:14:44Z

moduleframework/helpers/openshift_helper.py

+        :param kwargs: dict
+        :return: avocado.process.run
+        """
+        return self.runHost("%s" % common.sanitize_cmd(command), **kwargs)


Are you sure that this does what you expect?
this run command on host.
run method of helper should do command on GUEST in this case on openshift instance in case of docker there is for example docker exec INSTANCE_ID command so in case of openshift I expect something similar like oc exec ...

it is connected with more missing special methods for openshift, like CopyFrom and CopyTo now are inherited from CommonFunctions, what leads to host to host implementation, I expect there also some special oc command, or do it via some ssh, or whatever how it is connected together

Probably my mistake were that I've defined these functions leads to localhost operations, instead of raising not defined exceptions, and implement these methods where it make sense like in RpmHelper, ContainerHelper etc...

TomasTomecek

The functionality seems like exactly what we need -- basic testing using oc new-app. The one comment I have here is that users should probably be able to construct the CLI themselves, the same as you do for docker run.

For implementation: it's really fragile -- sometimes you get json from the commands (good) and process it, sometimes you don't. You also ignore the fact that they may be other applications being deployed in the cluster -- or even other MTF tests being run in parallel.

TomasTomecek · 2017-11-13T08:53:23Z

moduleframework/helpers/openshift_helper.py

+                 False, application does not exist
+        """
+        oc_status = self.runHost("oc status", ignore_status=True)
+        if 'dc/%s' % self.app_name in oc_status.stdout:


why not doing oc get dc $APP_NAME?

TomasTomecek · 2017-11-13T08:54:54Z

moduleframework/helpers/openshift_helper.py

+            return True
+        oc_services = self.runHost("oc get services -o json")
+        json_svc = self._convert_string_to_json(oc_services.stdout)
+        if not json_svc["items"]:


so if I have ANY service deployed in my openshift cluster, this will return true

as @TomasTomecek mentioned, probably you need to add there some "service" specific calls. I don't know it but probably there will be something similar to docker container inspect CONTAINER_ID what will show state just for one service instead of list all services and try to get some data

TomasTomecek · 2017-11-13T08:57:22Z

moduleframework/helpers/openshift_helper.py

+        # dovecot     172.30.1.1:5000/myproject/dovecot     latest    15 minutes ago
+        # memcached   172.30.1.1:5000/myproject/memcached   latest    13 minutes ago
+
+        app_found = [x for x in oc_get.split('\n') if x.startswith(self.app_name)]


this is really fragile, why not using json as an output from oc get?

I will try to use it.

TomasTomecek · 2017-11-13T09:07:21Z

moduleframework/helpers/openshift_helper.py

+        It creates an application in OpenShift environment
+        """
+        # Switching to system user
+        oc_new_app = self.runHost("oc new-app %s --name=%s" % (self.container_name,


I think you should add a label here to mark the deployment being owned by mtf: oc new-app -l mtf_testing=true or something like that; and then operate on objects which have this label.

Edit: so we probably need two identifiers: one which mark the app as one deployed by MTF, and the other will be unique identifier for the test run.

@phracek @TomasTomecek yep, similar to docker container ... commands and then you will just use this tag. otherwise I expect that this iddentifier will be stored in OpenShiftHelper class like self.jmeno for ContainerHelper

TomasTomecek · 2017-11-13T09:10:56Z

moduleframework/helpers/openshift_helper.py

+        """
+        pod_initiated = False
+        for x in range(0, 20):
+            pod_state = self.runHost("oc get pods", ignore_status=True)


please use json and don't parse CLI output

TomasTomecek · 2017-11-13T09:11:59Z

moduleframework/helpers/openshift_helper.py

+        :param oc_user: an username under which we can login to OpenShift environment
+        :param oc_passwd: a password for specific username
+        :param env:
+        :return:


Comment should be: env: is used for specification OpenShift IP, user and password, otherwise defaults are used

TomasTomecek · 2017-11-13T09:12:16Z

moduleframework/helpers/openshift_helper.py

+        :param env:
+        :return:
+        """
+        if env:


I probably don't understand this logic

Does this make sense to you?
env: is used for specification OpenShift IP, user and password, otherwise defaults are used

no, I still don't understand the reason for that variablee, since the values are being picked up from environment

TomasTomecek · 2017-11-13T09:13:13Z

moduleframework/helpers/openshift_helper.py

+        """
+        super(OpenShiftHelper, self).tearDown()
+        if common.get_if_do_cleanup():
+            # TODO will be implemented later on. I have to find a usecase what to remove


I would probably remove everything which was put into openshift during this test

TomasTomecek · 2017-11-13T09:13:52Z

moduleframework/helpers/openshift_helper.py

+
+    def status(self):
+        """
+        get status of an OpenShift


doesn't seem to correspond with the actual code

TomasTomecek · 2017-11-13T09:14:17Z

moduleframework/helpers/openshift_helper.py

+
+        :return: bool
+        """
+        if self._app_exists():


you can easily return self._app_exists()

Signed-off-by: Petr "Stone" Hracek <[email protected]>

TomasTomecek

Looks a lot better! Can't wait to give this a shot!

I think that some docstrings are still misleading and would love to have them improved.

TomasTomecek · 2017-11-15T12:13:56Z

moduleframework/helpers/openshift_helper.py

+
+    def status(self):
+        """
+        get status of an application in OpenShift environment


I still think that this docstring doesn't correspond with the code: what does the "status" mean exactly? For me, status is usually enum, something like "running", "failed", "finished" and this method returns bool. It makes me confused.

This should be solved in whole MTF. status function always return bool value like e.g. here: https://github.com/fedora-modularity/meta-test-family/blob/devel/moduleframework/common.py#L552
https://github.com/fedora-modularity/meta-test-family/blob/devel/moduleframework/helpers/container_helper.py#L171

I have filed an issue for it here: #168

I will keep it now as it is.

TomasTomecek · 2017-11-15T12:15:00Z

moduleframework/helpers/openshift_helper.py

+
+    def stop(self):
+        """
+        Stops the OpenShift


another misleading docstring; I think this method actually removes the application from the cluster

TomasTomecek · 2017-11-15T12:15:27Z

moduleframework/helpers/openshift_helper.py

+        """
+        starts the OpenShift application
+
+        :param args: Do not use it directly (It is defined in config.yaml)


this function doesn't accept any args

TomasTomecek · 2017-11-15T12:17:19Z

moduleframework/helpers/openshift_helper.py

+            common.print_info(e.message)
+            return False
+
+    def getIPaddr(self):


pythonic way of doing setters and getters is like this: https://docs.python.org/3/library/functions.html#property

(not blocking this review, just a friendly advice)

I have filed an issue for it #167

TomasTomecek · 2017-11-15T12:18:27Z

moduleframework/helpers/openshift_helper.py

+        service = self._convert_string_to_json(oc_get_service.stdout)
+        try:
+            for svc in service:
+                if svc.get('metadata').get('labels').get('app') == self.app_name:


TomasTomecek · 2017-11-15T12:19:56Z

moduleframework/helpers/openshift_helper.py

+
+    def _get_ip_instance(self):
+        """
+        It gets and IP address of an application from of OpenShift POD.


How about: "This method verifies that we can obtain an IP address of the application deployed within OpenShift."

phracek · 2017-11-15T13:28:18Z

Hopefully, I have addressed all issues. The rest are filed as a new issues.

Signed-off-by: Petr "Stone" Hracek <[email protected]>

TomasTomecek

LGTM, nicely done! let's give this a go

TomasTomecek requested changes Nov 8, 2017

View reviewed changes

phracek added 3 commits November 9, 2017 12:43

Update setup.py

c93c5a8

testing containers in OpenShift

1e1eb6e

Signed-off-by: Petr "Stone" Hracek <[email protected]>

Add more docu stuff and some fixes.

79a6d8a

Signed-off-by: Petr "Stone" Hracek <[email protected]>

TomasTomecek requested changes Nov 10, 2017

View reviewed changes

Use command oc get and stdout instead of parsing json.

4472963

Signed-off-by: Petr "Stone" Hracek <[email protected]>

jscotka requested changes Nov 13, 2017

View reviewed changes

TomasTomecek requested changes Nov 13, 2017

View reviewed changes

phracek added 3 commits November 14, 2017 10:06

Fixes based on the PR comments.

1dca315

Signed-off-by: Petr "Stone" Hracek <[email protected]>

Better check if application exists

67d8ef4

Signed-off-by: Petr "Stone" Hracek <[email protected]>

Support run command.

6fa73d9

Signed-off-by: Petr "Stone" Hracek <[email protected]>

TomasTomecek requested changes Nov 15, 2017

View reviewed changes

phracek mentioned this pull request Nov 15, 2017

status function should return state and not boolean #168

Open

Updating docstring and adding pod functions

925017e

Signed-off-by: Petr "Stone" Hracek <[email protected]>

TomasTomecek approved these changes Nov 15, 2017

View reviewed changes

phracek merged commit 9b7210e into fedora-modularity:devel Nov 16, 2017

phracek mentioned this pull request Nov 21, 2017

[RFE] Support testing containers in OpenShift #89

Closed

6 tasks

		from moduleframework.mtfexceptions import ContainerExc, ConfigExc


		class OpenShiftHelper(CommonFunctions):

testing container in OpenShift #164

testing container in OpenShift #164

Conversation

phracek commented Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasTomecek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jscotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasTomecek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasTomecek Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasTomecek Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasTomecek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phracek Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phracek commented Nov 15, 2017

TomasTomecek left a comment

Choose a reason for hiding this comment

phracek commented Nov 8, 2017 •

edited

Loading

TomasTomecek Nov 13, 2017 •

edited

Loading

TomasTomecek Nov 13, 2017 •

edited

Loading

phracek Nov 15, 2017 •

edited

Loading