openshift_checks: refactor check results #4913

sosiouxme · 2017-07-27T19:24:20Z

Introduced the 'changed' property for checks that can make changes to track whether they did or not. Rather than the check's own logic having to track this and include it in the result hash, just set the property and have the action plugin insert it in the result hash after running (even if there is an exception).

Cleared out a lot of crufty "changed: false" hash entries.

Refactored logging checks:

Turned failure messages into exceptions.
Have tests look at exceptions raised instead of text intended for users.
Turned logging_namespace property into a method.
Got rid of _exec_oc and just use logging.exec_oc.

rhcarvalho

I like it, thanks @sosiouxme! I focused on some parts, glanced at others, ignored others. Please comment if there's some part you'd want better review.

rhcarvalho · 2017-07-28T08:50:23Z

roles/openshift_health_checker/action_plugins/openshift_health_check.py

@@ -69,13 +69,15 @@ def run(self, tmp=None, task_vars=None):
                        msg=str(e),
                    )

+            if check.changed:
+                r["changed"] = True


This is okay. Have you considered setting r['changed'] = check.changed unconditionally?

Yes, but I thought it wouldn't hurt to allow the check to continue to return it in the result hash if desired for some reason. But thinking about it, why complicate what is really quite simple... "one way to do it" is probably best here.

Actually, I remembered why I did it this way. I don't want to clutter up every check result in -vvv output with "changed" when it won't matter for most. I think I'll leave it as-is.

Right. We can omit from checks, but always set it in the task/action plugin level at most once.

Now whether to always include it on the task/action plugin level will depend on how Ansible interpret it when it is missing (default True or default False) ;-)

Pretty sure Ansible defaults to missing == False

For this code, it seemed simplest to always set it at the task level.

rhcarvalho · 2017-07-28T08:52:54Z

roles/openshift_health_checker/openshift_checks/__init__.py

+    def __init__(self, name, msg=None):
+        # msg is for the message the user will see when this is raised.
+        # name is for test code to identify the error without looking at msg text.
+        if msg is None:  # for parameter backward compatibility


I think if we swap the order of the arguments we can avoid this little hack?

Of course, but I like to have the concise identifier first and then the blah blah blah when I'm looking at the code raising an exception. Selfish of me? And for once I thought it would be nice not to force a total transformation to the new method signature.

rhcarvalho · 2017-07-28T08:55:58Z

roles/openshift_health_checker/openshift_checks/__init__.py

@@ -34,6 +42,8 @@ def __init__(self, execute_module=None, task_vars=None, tmp=None):
        self._execute_module = execute_module
        self.task_vars = task_vars or {}
        self.tmp = tmp
+        # set True when the check makes a change to the host so it can be reported to the user:


s/set True/set to True/ ?

s/so it can be reported to the user// ? "reporting to the user" is kind of optional, we don't need to make promises in this comment, but I'll leave it up to you, I'm fine either way.

set to True 👍

"report to user" right that can be worded better. All we're doing is marking the task as having made a change so that the sum total of "changed" tasks can be reported at the end of the run. As if anyone cares (but there's always someone). That's all that's "reported" to the user unless they're looking at -vvv output.

# set to True when the check changes the host so the total "changed" count is accurate

rhcarvalho · 2017-07-28T08:56:45Z

roles/openshift_health_checker/openshift_checks/logging/curator.py

-            return {"failed": True, "changed": False, "msg": msg}
-
+        curator_pods = self.get_pods_for_component("curator")
+        self.check_curator(curator_pods)
        # TODO(lmeyer): run it all again for the ops cluster


Is this still relevant?

Sadly yes... these checks do nothing to examine the ops cluster. The main cluster is far more likely to fall over and affect users though so the focus is right. These little reminders are where they are because ideally we would literally just run the same high-level methods against a different set of pods/services/etc to do ops cluster checks.

rhcarvalho · 2017-07-28T09:00:06Z

roles/openshift_health_checker/openshift_checks/logging/logging.py

+    pass
+
+
+class LoggingErrorList(OpenShiftCheckException):


Perhaps this doesn't need to have "Logging" in the name? It could as well sit next to "OpenShiftCheckException" and be an alternative to it? class OpenShiftCheckExceptionList(OpenShiftCheckException)?

I wasn't sure if it would be useful anywhere else. I'm not sure any other checks have a need to compile a list of failures instead of just quitting on the first. But again, thinking about it... it will probably come up. I guess I'll generalize it.

That name is too long though 🤔

sosiouxme · 2017-07-29T14:59:05Z

@rhcarvalho you saw the bits I thought you'd find interesting. Thanks for the feedback.

I might run the "changed" commit in a different PR; the rest of this is more tedious than I expected.

rhcarvalho

LGTM, let's run the tests

rhcarvalho · 2017-07-31T13:19:53Z

aos-ci-test

rhcarvalho · 2017-07-31T13:20:19Z

@sosiouxme if this is good to go please remove the [WIP] from the title ;)

openshift-bot · 2017-07-31T14:28:22Z

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

openshift-bot · 2017-07-31T14:37:16Z

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

sosiouxme · 2017-07-31T14:56:29Z

It can be good to go. There are a lot of other checks to get the same treatment but there's no need to hold up these changes for that.

Introduced the 'changed' property for checks that can make changes to track whether they did or not. Rather than the check's own logic having to track this and include it in the result hash, just set the property and have the action plugin insert it in the result hash after running (even if there is an exception). Cleared out a lot of crufty "changed: false" hash entries.

Turn failure messages into exceptions that tests can look for without depending on text meant for humans. Turn logging_namespace property into a method. Get rid of _exec_oc and just use logging.exec_oc.

sosiouxme · 2017-08-07T13:01:59Z

aos-ci-test

openshift-bot · 2017-08-07T14:15:39Z

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

openshift-bot · 2017-08-07T14:18:29Z

success: "aos-ci-jenkins/OS_3.6_containerized, aos-ci-jenkins/OS_3.6_containerized_e2e_tests" for 06a6fb9 (logs)

sosiouxme · 2017-08-07T15:25:41Z

[merge]

sosiouxme · 2017-08-07T19:05:39Z

https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/800/ looks like flakes openshift/origin#14829 and openshift/origin#14898
re[merge]

sosiouxme · 2017-08-08T11:51:11Z

https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/803/ indicates flakes openshift/origin#8571 and openshift/origin#10162

[merge] again

openshift-bot · 2017-08-08T11:55:22Z

Evaluated for openshift ansible merge up to 06a6fb9

openshift-bot · 2017-08-08T14:07:32Z

continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/811/) (Base Commit: 0569c50) (PR Branch Commit: 06a6fb9)

rhcarvalho · 2017-08-08T14:40:43Z

Flake openshift/origin#13977

rhcarvalho · 2017-08-08T14:41:52Z

@sosiouxme okay for us to manually merge this after the recent changes that merged in?

sosiouxme · 2017-08-08T16:23:50Z

@rhcarvalho yes that would be nice... enough test failures already.

rhcarvalho · 2017-08-08T16:53:37Z

Going to merge manually as per https://github.com/openshift/openshift-ansible/blob/master/docs/pull_requests.md#manual-merges

sosiouxme mentioned this pull request Jul 27, 2017

add fluentd logging driver config check #4592

Merged

1 task

sosiouxme requested a review from rhcarvalho July 28, 2017 03:34

rhcarvalho reviewed Jul 28, 2017

View reviewed changes

rhcarvalho approved these changes Jul 31, 2017

View reviewed changes

sosiouxme changed the title ~~[WIP] openshift_checks: refactor check results~~ openshift_checks: refactor check results Jul 31, 2017

sosiouxme requested a review from juanvallejo July 31, 2017 14:58

sosiouxme mentioned this pull request Aug 1, 2017

openshift_checks: refactor find_ansible_mount #4944

Merged

sosiouxme added 2 commits August 2, 2017 14:06

openshift_checks: refactor logging checks

06a6fb9

Turn failure messages into exceptions that tests can look for without depending on text meant for humans. Turn logging_namespace property into a method. Get rid of _exec_oc and just use logging.exec_oc.

rhcarvalho merged commit 7121e06 into openshift:master Aug 8, 2017

openshift_checks: refactor check results #4913

openshift_checks: refactor check results #4913

Conversation

sosiouxme commented Jul 27, 2017 • edited Loading

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

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

sosiouxme Jul 28, 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

sosiouxme commented Jul 29, 2017

rhcarvalho left a comment

Choose a reason for hiding this comment

rhcarvalho commented Jul 31, 2017

rhcarvalho commented Jul 31, 2017

openshift-bot commented Jul 31, 2017

openshift-bot commented Jul 31, 2017

sosiouxme commented Jul 31, 2017

sosiouxme commented Aug 7, 2017

openshift-bot commented Aug 7, 2017

openshift-bot commented Aug 7, 2017

sosiouxme commented Aug 7, 2017

sosiouxme commented Aug 7, 2017 • edited Loading

sosiouxme commented Aug 8, 2017 • edited Loading

openshift-bot commented Aug 8, 2017

openshift-bot commented Aug 8, 2017

rhcarvalho commented Aug 8, 2017

rhcarvalho commented Aug 8, 2017

sosiouxme commented Aug 8, 2017

rhcarvalho commented Aug 8, 2017

sosiouxme commented Jul 27, 2017 •

edited

Loading

sosiouxme Jul 28, 2017 •

edited

Loading

sosiouxme commented Aug 7, 2017 •

edited

Loading

sosiouxme commented Aug 8, 2017 •

edited

Loading