Fix bug where Init:Error failed pods displayed "Pod initializing" #2676

rhamilto · 2018-01-17T16:53:47Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1512473

jwforres · 2018-01-17T17:06:38Z

app/scripts/filters/resources.js


-        if (containerReason) {
+        // only show containerReason if pod.status.phase is not 'Failed'
+        if (reason !== 'Failed' && containerReason) {


@spadgett can you think of a case where this isn't what we would want?

I feel like there might be specific reasons you'd want to see even for failed pods. Evicted? Deadline exceeded?

This logic was modeled after the CLI, so it would be good to stay consistent. I'm curious what the CLI shows for these pods.

NAME READY STATUS RESTARTS AGE dancer-ex-1-build 0/1 Init:Error 0 1h

@jwforres Do we need to iterate over initContainerStatuses here too?

I believe this is the current CLI logic if we want to be consistent:

https://github.com/spadgett/origin/blob/5b71e8d8e441a0d39f9228a4bce4c7a6a13be08b/vendor/k8s.io/kubernetes/pkg/printers/internalversion/printers.go#L523-L615

yeah i think that is probably the correct fix for this

Looks like CLI has special initializing logic.

rhamilto · 2018-01-17T21:49:02Z

Take 2. I didn't add all the initializing logic since it seems we only care about errors. Thoughts?

spadgett · 2018-01-19T15:33:08Z

/hold
/assign

I'm not sure the logic is right. I'll follow up with a review, but hold for now.

spadgett · 2018-01-19T23:21:10Z

app/scripts/filters/resources.js

+      var initReason;
+
+      angular.forEach(pod.status.initContainerStatuses, function(initContainerStatus) {
+        var initContainerReason = _.get(initContainerStatus, 'state.terminated.reason');


It looks like there is a state.terminated.reason even for successful init containters. We probably need to check exitCode, too.

initContainerStatuses: - containerID: >- docker://695625642cd6a776c8fe86a0ee530e0acbe04cfbb7187552c55b068f60df1333 image: 'docker.io/openshift/origin-sti-builder:v3.9.0-alpha.3' imageID: >- docker-pullable://docker.io/openshift/origin-sti-builder@sha256:1120189696907b0451dcd081b39406be02c14b988cc7fc655af39002a0953ced lastState: {} name: git-clone ready: true restartCount: 0 state: terminated: containerID: >- docker://695625642cd6a776c8fe86a0ee530e0acbe04cfbb7187552c55b068f60df1333 exitCode: 0 finishedAt: '2018-01-19T23:18:35Z' reason: Completed startedAt: '2018-01-19T23:18:33Z'

If there are multiple init containers and only the first init container has it, it looks like initContainerReason will be unset since we don't break the loop early.

rhamilto · 2018-01-22T20:37:55Z

Take 3. No idea if this is right, but I gave it a go.

spadgett · 2018-01-23T14:33:51Z

app/scripts/filters/resources.js

-          return;
+        if (initContainerState.terminated && initContainerState.terminated.exitCode === 0) {
+          // initialization is complete
+          return true;


return true will short-circuit here and skip the remaining init containers. Since this container was successful, it's not what we want. We want to short0circuit only when we find one that is not complete or failed.

I think you want just return;

spadgett · 2018-01-23T14:36:22Z

app/scripts/filters/resources.js

-        var containerReason = _.get(containerStatus, 'state.waiting.reason') || _.get(containerStatus, 'state.terminated.reason'),
-            signal,
-            exitCode;
+      _.forEach(pod.status.initContainerStatuses, function(initContainerStatus) {


nit: We try to consistently use _.each (alias to _.forEach) so we don't have a mix of one and the other. They're the same, but if you don't know that, it might be confusing to see both in the code.

spadgett · 2018-01-23T14:37:02Z

app/scripts/filters/resources.js

+
+        // Print detailed container reasons if available. Only the last will be
+        // displayed if multiple containers have this detail.
+        _.forEach(pod.status.containerStatuses, function(containerStatus) {


nit: _.each

rhamilto · 2018-01-23T18:41:46Z

Take 3 comments address.

spadgett · 2018-01-26T00:15:53Z

app/scripts/filters/resources.js

+      if (!initializing) {
+        reason = pod.status.reason || pod.status.phase;
+
+        // Print detailed container reasons if available. Only the last will be


Update comment to say only first will be displayed if multiple containers have a reason. The comment no longer matches the code.

spadgett · 2018-01-26T00:16:25Z

app/scripts/filters/resources.js

+
+          exitCode = _.get(containerStatus, 'state.terminated.exitCode');
+          if (exitCode) {
+            reason = "Exit Code: " + exitCode;


We should return true after this line, too, so we stop looking at other containers like the other checks above. Otherwise it'll keep iterating.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1512473

rhamilto · 2018-01-26T14:16:04Z

Round 4. Thank you for your patience, @spadgett.

spadgett

/lgtm

spadgett · 2018-01-30T14:07:19Z

Sorry didn't realize this still had the hold label

/hold cancel

openshift-merge-robot · 2018-01-30T14:07:43Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2018-01-30T14:29:03Z

Automatic merge from submit-queue.

rhamilto requested a review from jwforres January 17, 2018 16:53

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 17, 2018

jwforres reviewed Jan 17, 2018

View reviewed changes

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 17, 2018

openshift-ci-robot assigned spadgett Jan 19, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 19, 2018

spadgett requested changes Jan 19, 2018

View reviewed changes

spadgett requested changes Jan 23, 2018

View reviewed changes

spadgett reviewed Jan 26, 2018

View reviewed changes

Fix bug where Init:Error failed pods displayed "Pod initializing"

9bffdf3

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1512473

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2018

spadgett approved these changes Jan 26, 2018

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2018

openshift-merge-robot merged commit ed20b3a into openshift:master Jan 30, 2018

rhamilto deleted the bz-1512473 branch January 30, 2018 15:36

spadgett mentioned this pull request Jun 22, 2018

PodInitializing is not generally an error state, messaging is confusing openshift/console#107

Closed

Fix bug where Init:Error failed pods displayed "Pod initializing" #2676

Fix bug where Init:Error failed pods displayed "Pod initializing" #2676

Uh oh!

Conversation

rhamilto commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhamilto commented Jan 17, 2018

Uh oh!

spadgett commented Jan 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhamilto commented Jan 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhamilto commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhamilto commented Jan 26, 2018

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

spadgett commented Jan 30, 2018

Uh oh!

openshift-merge-robot commented Jan 30, 2018

Uh oh!

openshift-merge-robot commented Jan 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rhamilto commented Jan 17, 2018 •

edited

Loading