New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1912558: Show pending status for taskRuns that haven't started #7701
Bug 1912558: Show pending status for taskRuns that haven't started #7701
Conversation
@rottencandy: This pull request references Bugzilla bug 1912558, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/kind bug |
/cc @jerolimov |
@@ -61,5 +61,11 @@ export const pipelineResourceTypeFilter = (filters, pipelineResource): boolean = | |||
|
|||
export const taskRunFilterReducer = (taskRun): string => { | |||
const status = pipelineRunStatus(taskRun); | |||
if (status === 'Running') { | |||
const condition = taskRun.status?.conditions?.find((c) => c.type === 'Succeeded'); | |||
if (condition.reason === 'ExceededNodeResources') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If taskRun.status could be empty (see optional check one line above), and the conditions could not include a type === 'Successeded', you must check here again if the condition is defined.
if (condition.reason === 'ExceededNodeResources') { | |
if (condition?.reason === 'ExceededNodeResources') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also more reasons why a TaskRun is pending. Maybe you can update the pipelineRunStatus
instead. I now found the source code of the tkn
cli which converts the "Succeeded" condition into a "human readable text".
Since the tkn cli do this for PipelineRun and TaskRun in a similar way, I think we could do this also for PipelineRuns. Otherwise I liked that you tried to ensure with this local change to not break any PipelineRun related check.
See https://github.com/tektoncd/cli/blob/master/pkg/formatted/k8s.go#L55-L82
Wdyt? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I Initially avoided modifying the PipelineRun check because I wasn't sure if PipelineRun and TaskRun could have the same reasons. Updated now to handle all the cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should align this with the tkn cli and add more cases then just the one reported case (ExceededResourceQuota).
de3ebd5
to
65d9b45
Compare
@@ -5,7 +5,7 @@ import { referenceForModel } from '@console/internal/module/k8s'; | |||
import { TaskRunKind, getModelReferenceFromTaskKind } from '../../../utils/pipeline-augment'; | |||
import { TaskRunModel, PipelineModel } from '../../../models'; | |||
import { tableColumnClasses } from './taskruns-table'; | |||
import { pipelineRunFilterReducer as taskRunFilterReducer } from '../../../utils/pipeline-filter-reducer'; | |||
import { taskRunFilterReducer } from '../../../utils/pipeline-filter-reducer'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😳 👍 Good catch
case 'PipelineRunStopping': | ||
case 'TaskRunStopping': | ||
return 'Failed'; | ||
case 'ContainerConfigError': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In go code this error reason is named
case 'ContainerConfigError': | |
case 'CreateContainerConfigError': |
const isCancelled = conditions.find((c) => | ||
['PipelineRunCancelled', 'TaskRunCancelled'].some((cancel) => cancel === c.reason), | ||
['PipelineRunCancelled', 'TaskRunCancelled', 'Cancelled'].some((cancel) => cancel === c.reason), | ||
); | ||
if (isCancelled) { | ||
return 'Cancelled'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. I know its saver to keep the existing code. But what do you think if we move this (similar to the go code) also in the switch below?
@@ -2,22 +2,39 @@ import * as _ from 'lodash'; | |||
|
|||
export const pipelineRunStatus = (pipelineRun): string => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an documentation link here? Maybe like this:
export const pipelineRunStatus = (pipelineRun): string => { | |
// Converts the PipelineRun (and TaskRun) condition status into a human readable string. | |
// See also tkn cli implementation at https://github.com/tektoncd/cli/blob/release-v0.15.0/pkg/formatted/k8s.go#L54-L83 | |
export const pipelineRunStatus = (pipelineRun): string => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably could have used /** ... */
for an official JSDoc of the method... but that's fine.
65d9b45
to
69837d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine, thanks for the cleanup 👍
Tested it with canceled, running, pending and successfully PipelineRuns and TaskRuns.
/lgtm
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, jerolimov, rottencandy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@rottencandy: All pull requests linked via external trackers have merged: Bugzilla bug 1912558 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Fixes:
https://issues.redhat.com/browse/ODC-5222
Analysis / Root cause:
TaskRun list and detail page shows that a TaskRun is "Running" if it has not yet started with the reason "ExceededNodeResources".
Solution Description:
Show "Pending" status for TaskRuns that have the reason "ExceededNodeResources" in the Succeeded condition.
Screen shots / Gifs for design review:
Test setup:
Browser conformance: