skip "yum update" sanity check if not running on rhel #3757

juanvallejo · 2017-03-23T20:14:21Z

Related Trello card: https://trello.com/c/5pr2WcQH/43-diagnostics-additional-pre-flight-checks
Detect ansible host's OS using output of docker info and skip check if it is not being run on RHEL.

cc @brenton @sosiouxme @rhcarvalho

juanvallejo · 2017-03-23T20:36:50Z

roles/openshift_health_checker/openshift_checks/package_update.py

+        return {"changed": False, "msg": "Skipping check on unsupported OS: {os}".format(os=os)}
+
+    def get_operating_system(self, task_vars):
+        info = self.module_executor("docker_info", {}, task_vars)


@sosiouxme per your comment on the Trello card, since this is a pre-flight check, perhaps it would be better to use something like cat /proc/os-release on the ansible host?

We don't need to call any module. The operating system / package manager is readily available in task_vars, provided by http://docs.ansible.com/ansible/setup_module.html

$ ansible -i hosts -m setup master1 | grep yum "ansible_pkg_mgr": "yum",

All we need to do is implement is_active to look at task_vars['ansible_pkg_mgr'].

rhcarvalho · 2017-03-24T11:22:51Z

@juanvallejo note that this is not about "not running on RHEL", but about the package manager... e.g., CentOS also uses "yum" and is perfectly compatible with the existing check.

sosiouxme · 2017-04-03T13:08:16Z

roles/openshift_health_checker/openshift_checks/mixins.py

@@ -10,9 +10,13 @@ def is_active(cls, task_vars):
        return (
            # This mixin is meant to be used with subclasses of OpenShiftCheck.
            super(NotContainerizedMixin, cls).is_active(task_vars) and
-            not cls.is_containerized(task_vars)
+            not cls.is_containerized(task_vars) and cls.has_yum_pkg_mgr(task_vars)


Shouldn't this be in a separate mixin, like say:

class NeedsYumMixin(object): """Mixin for checks that are only active for hosts where yum package manager is present.""" @classmethod def is_active(cls, task_vars): # This mixin is meant to be used with subclasses of OpenShiftCheck. return super(NeedsYumMixin, cls).is_active(task_vars) and cls.has_yum_pkg_mgr(task_vars)

stevekuznetsov · 2017-04-08T11:55:41Z

roles/openshift_health_checker/openshift_checks/package_availability.py



-class PackageAvailability(NotContainerizedMixin, OpenShiftCheck):
+class PackageAvailability(NotContainerizedMixin, NeedsYumMixin, OpenShiftCheck):


On Fedora you can technically still use yum but instead of ignoring the check shouldn't you just use dnf?

This is about having the Python package yum available/installed. Not that we couldn't support DNF, just that we don't have code for it at the moment.

This PR makes the check be skipped instead of giving an spurious error.

rhcarvalho · 2017-04-09T15:26:55Z

roles/openshift_health_checker/openshift_checks/mixins.py

+        return (
+            # This mixin is meant to be used with subclasses of OpenShiftCheck.
+            super(NeedsYumMixin, cls).is_active(task_vars) and
+            not cls.is_containerized(task_vars) and cls.has_yum_pkg_mgr(task_vars)


@juanvallejo this mixin should do one thing -- it has nothing to do with 'containerized'.
We can remove this part of the condition here and remove the is_containerized method.

Note how we use the two mixins together to compose their effects:

class PackageAvailability(NotContainerizedMixin, NeedsYumMixin, OpenShiftCheck): ...

rhcarvalho · 2017-04-09T15:28:43Z

roles/openshift_health_checker/openshift_checks/mixins.py

+
+    @staticmethod
+    def has_yum_pkg_mgr(task_vars):
+        return task_vars["ansible_pkg_mgr"] == "yum"


We could simply inline this:

@classmethod def is_active(cls, task_vars): return super(NeedsYumMixin, cls).is_active(task_vars) and task_vars["ansible_pkg_mgr"] == "yum"

rhcarvalho · 2017-04-10T16:53:41Z

@juanvallejo this could be done similar to #3887.

rhcarvalho

Two nits, otherwise LGTM

rhcarvalho · 2017-04-11T14:21:01Z

roles/openshift_health_checker/test/package_availability_test.py

+    ('yum', False, True),
+    ('yum', True, False),
+    ('dnf', False, False),
+    ('dnf', False, False),


The two lines above are identical :-)

rhcarvalho · 2017-04-11T14:22:06Z

roles/openshift_health_checker/test/package_availability_test.py

+def test_is_active(pkg_mgr, is_containerized, is_active):
+    task_vars = dict(
+        ansible_pkg_mgr=pkg_mgr,
+        group_names=["masters", "nodes"],


This is irrelevant to determine is_active, right? If so, we can have one less line to read ;)

juanvallejo · 2017-04-11T14:34:16Z

@rhcarvalho thanks for the feedback, comments addressed

rhcarvalho · 2017-04-11T14:38:20Z

aos-ci-test

rhcarvalho · 2017-04-11T14:38:27Z

[merge]

openshift-bot · 2017-04-11T14:40:05Z

[test]ing while waiting on the merge queue

openshift-bot · 2017-04-11T14:47:55Z

Evaluated for openshift ansible test up to daebc9f

openshift-bot · 2017-04-11T15:54:31Z

error: aos-ci-jenkins/OS_3.6_containerized for daebc9f (logs)

openshift-bot · 2017-04-11T16:01:33Z

success: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" for daebc9f (logs)

openshift-bot · 2017-04-11T16:04:20Z

success: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" for daebc9f (logs)

openshift-bot · 2017-04-11T16:07:36Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible/11/) (Base Commit: 5fe1609)

openshift-bot · 2017-04-11T16:08:22Z

success: "aos-ci-jenkins/OS_3.6_NOT_containerized, aos-ci-jenkins/OS_3.6_NOT_containerized_e2e_tests" for daebc9f (logs)

rhcarvalho · 2017-04-12T10:14:20Z

Flake openshift/origin#13731.

[merge] again

openshift-bot · 2017-04-12T10:15:16Z

Evaluated for openshift ansible merge up to daebc9f

openshift-bot · 2017-04-12T10:15:25Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/207/) (Base Commit: 5ffbb64)

juanvallejo changed the title ~~skip "yum update" sanity check if not running on rhel~~ [WIP] skip "yum update" sanity check if not running on rhel Mar 23, 2017

juanvallejo commented Mar 23, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/skip-yum-update-check-on-fedora branch 2 times, most recently from 03a71f8 to 5e65384 Compare March 24, 2017 23:17

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2017

juanvallejo force-pushed the jvallejo/skip-yum-update-check-on-fedora branch from 5e65384 to 6e1e7c9 Compare March 31, 2017 14:06

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2017

sosiouxme reviewed Apr 3, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/skip-yum-update-check-on-fedora branch from 6e1e7c9 to 2ef0612 Compare April 6, 2017 21:41

stevekuznetsov reviewed Apr 8, 2017

View reviewed changes

rhcarvalho suggested changes Apr 9, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/skip-yum-update-check-on-fedora branch 2 times, most recently from 09b1c6e to 28af129 Compare April 10, 2017 19:10

juanvallejo changed the title ~~[WIP] skip "yum update" sanity check if not running on rhel~~ skip "yum update" sanity check if not running on rhel Apr 11, 2017

rhcarvalho reviewed Apr 11, 2017

View reviewed changes

skip PackageAvailability check if not yum

daebc9f

juanvallejo force-pushed the jvallejo/skip-yum-update-check-on-fedora branch from 28af129 to daebc9f Compare April 11, 2017 14:33

rhcarvalho approved these changes Apr 11, 2017

View reviewed changes

rhcarvalho mentioned this pull request Apr 12, 2017

e2e flake: [k8s.io] SchedulerPredicates [Serial] [It] validates that NodeSelector is respected if not matching [Conformance] openshift/origin#13731

Closed

openshift-bot merged commit 9ba1fde into openshift:master Apr 12, 2017

juanvallejo deleted the jvallejo/skip-yum-update-check-on-fedora branch April 12, 2017 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skip "yum update" sanity check if not running on rhel #3757

skip "yum update" sanity check if not running on rhel #3757

juanvallejo commented Mar 23, 2017 •

edited

Loading

juanvallejo Mar 23, 2017

rhcarvalho Mar 24, 2017

rhcarvalho Mar 24, 2017

rhcarvalho commented Mar 24, 2017

sosiouxme Apr 3, 2017

rhcarvalho Apr 3, 2017

stevekuznetsov Apr 8, 2017

rhcarvalho Apr 9, 2017

rhcarvalho Apr 9, 2017

rhcarvalho Apr 9, 2017

rhcarvalho commented Apr 10, 2017

rhcarvalho left a comment

rhcarvalho Apr 11, 2017

rhcarvalho Apr 11, 2017

juanvallejo commented Apr 11, 2017

rhcarvalho commented Apr 11, 2017

rhcarvalho commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

rhcarvalho commented Apr 12, 2017

openshift-bot commented Apr 12, 2017

openshift-bot commented Apr 12, 2017 •

edited

Loading



		class PackageAvailability(NotContainerizedMixin, OpenShiftCheck):
		class PackageAvailability(NotContainerizedMixin, NeedsYumMixin, OpenShiftCheck):

skip "yum update" sanity check if not running on rhel #3757

skip "yum update" sanity check if not running on rhel #3757

Conversation

juanvallejo commented Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhcarvalho commented Mar 24, 2017

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

rhcarvalho commented Apr 10, 2017

rhcarvalho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Apr 11, 2017

rhcarvalho commented Apr 11, 2017

rhcarvalho commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

openshift-bot commented Apr 11, 2017

rhcarvalho commented Apr 12, 2017

openshift-bot commented Apr 12, 2017

openshift-bot commented Apr 12, 2017 • edited Loading

juanvallejo commented Mar 23, 2017 •

edited

Loading

openshift-bot commented Apr 12, 2017 •

edited

Loading