add check for scheduled pods #4728
Conversation
juanvallejo
force-pushed
the
jvallejo/handle-unscheduled-logging-stack-pods
branch
2 times, most recently
from
d5cba42
to
b42d7d1
Compare
|
aos-ci-test |
| def not_running_pods(self, pods): | ||
| """Returns: list of pods not in a ready and running state | ||
| or an OpenShiftCheckException if no pods have been scheduled.""" | ||
| self.ensure_scheduled_pods(pods) |
There was a problem hiding this comment.
So after thinking about this for a bit, while I like the idea of informing users about specific problems with their pods, this just does not feel like the right place to do it. And I think there could be legitimate reasons why a pod might not be working but not actually impacting anything because there's already a replacement, so I wouldn't want the check to fail on any particular pod having problems, just if there are the wrong number of pods actually in service.
WDYT?
There was a problem hiding this comment.
And I think there could be legitimate reasons why a pod might not be working but not actually impacting anything because there's already a replacement, so I wouldn't want the check to fail on any particular pod having problems
I think that is a valid point. Will update this change to only fail if no pods or > 1 pods are found to be Ready
| def not_running_pods(self, pods): | ||
| """Returns: list of pods not in a ready and running state | ||
| or an OpenShiftCheckException if no pods have been scheduled.""" | ||
| self.ensure_scheduled_pods(pods) | ||
| return [ | ||
| pod for pod in pods | ||
| if any( |
There was a problem hiding this comment.
We could just stick to the goal of filtering out pods that aren't actually contributing, and fix the check breakage like so:
if not pod.get('status', {}).get('containerStatuses') or any(
... and later ...
for condition in pod['status'].get('conditions', [])
|
More than 1 pods can be valid too... I vote for just the minimal change
here.
…On Tue, Jul 11, 2017 at 1:36 PM, Juan Vallejo ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In roles/openshift_health_checker/openshift_checks/logging/logging.py
<#4728 (comment)>
:
> @@ -49,9 +49,10 @@ def get_pods_for_component(self, execute_module, namespace, logging_component, t
return pods['items'], None
- @staticmethod
- def not_running_pods(pods):
- """Returns: list of pods not in a ready and running state"""
+ def not_running_pods(self, pods):
+ """Returns: list of pods not in a ready and running state
+ or an OpenShiftCheckException if no pods have been scheduled."""
+ self.ensure_scheduled_pods(pods)
@sosiouxme <https://github.com/sosiouxme>
And I think there could be legitimate reasons why a pod might not be
working but not actually impacting anything because there's already a
replacement, so I wouldn't want the check to fail on any particular pod
having problems
I think that is a valid point. Will update this change to only fail if no
pods or > 1 pods are found to be Ready
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4728 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABz-rNDYT2xq804lQm4PEQqbe7AxX05ks5sM7K4gaJpZM4OTUIM>
.
|
|
@sosiouxme thanks, updated check based on #4728 (comment) |
| @@ -51,15 +51,16 @@ def get_pods_for_component(self, execute_module, namespace, logging_component, t | |||
|
|
|||
| @staticmethod | |||
| def not_running_pods(pods): | |||
| """Returns: list of pods not in a ready and running state""" | |||
| """Returns: list of pods not in a ready and running state | |||
| or an OpenShiftCheckException if no pods have been scheduled.""" | |||
There was a problem hiding this comment.
doesn't throw an exception now :)
There was a problem hiding this comment.
otherwise LGTM... can you squash commits?
There was a problem hiding this comment.
doesn't throw an exception now :)
:/ saw this as I started to make changes and knew I would somehow forget to edit it out before pushing
juanvallejo
force-pushed
the
jvallejo/handle-unscheduled-logging-stack-pods
branch
from
1c99a90
to
0192385
Compare
|
@sosiouxme thanks for the feedback. commits squashed |
|
aos-ci-test |
|
merged in #4737 |
Adds a check that ensures pods have been scheduled (and that containerStatuses exist) before attempting to retrieve pod container statuses.
Addresses BZ: 1468760
cc @sosiouxme @rhcarvalho