Skip to content
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

Update TaskRun status even if PipelineRun is done #757

Merged
merged 1 commit into from
Apr 15, 2019

Conversation

dicarlo2
Copy link
Contributor

Changes

Due to the DAG execution model, it's possible for the PipelineRun to error while other TaskRuns are still executing. This change resolves a bug where the status for the other TaskRuns would not propagate to the PipelineRun if it has failed.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide
for more details.

Release Notes

Fix propagation of concurrent TaskRun status to failed PipelineRuns

Due to the DAG execution model, it's possible for the PipelineRun to
error while other TaskRuns are still executing. This change resolves a
bug where the status for the other TaskRuns would not propagate to the
PipelineRun if it has failed.
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 13, 2019
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 13, 2019
@abayer
Copy link
Contributor

abayer commented Apr 13, 2019

/ok-to-test
/lgtm

@tekton-robot tekton-robot added lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 13, 2019
@dlorenc
Copy link
Contributor

dlorenc commented Apr 15, 2019

/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dicarlo2, dlorenc

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2019
@tekton-robot tekton-robot merged commit 2a9c7f8 into tektoncd:master Apr 15, 2019
Copy link
Collaborator

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @dicarlo2 !!! i just left a couple comments b/c i was late to the party :)

@@ -176,22 +176,25 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {

if pr.IsDone() {
c.timeoutHandler.Release(pr)
c.Recorder.Event(pr, corev1.EventTypeNormal, eventReasonSucceeded, "PipelineRun completed successfully.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, looks like in this change this line got removed, so we no longer emit an event indicating successful completion? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, will submit a followup pr to fix that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 thanks!!

@@ -396,6 +399,23 @@ func updateTaskRunsStatus(pr *v1alpha1.PipelineRun, pipelineState []*resources.R
}
}

func (c *Reconciler) updateTaskRunsStatusDirectly(pr *v1alpha1.PipelineRun) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note for the future (I'm pretty late to the party so not something we need to change now!) is that we generally try to avoid adding functionality at the reconciler level if we can: it's much easier to write (and read) test cases that don't require the entire reconciler to be setup (and the code tends to be simpler too)

@@ -479,10 +502,15 @@ func TestReconcileOnCompletedPipelineRun(t *testing.T) {
c.Reconciler.Reconcile(context.Background(), "foo/test-pipeline-run-completed")

// make sure there is no failed events
validateEvents(t, fr)
validateNoEvents(t, fr)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is b/c we don't have the success event anymore? (just not clear to me if this was an intentional change or not!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, good catch - I was wondering how it was passing before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually, now that I think about it - I think I removed that event because it was emitted for both pipeline success and failure. Does that make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dicarlo2 can you explain a bit more? I think we'd want to have an event in both cases - i.e. one event in each case, just to try to be very clear:

  • Failure: event indicating failure
  • Success: event indication success

(maybe that is already happening?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, to be clear, I removed it because we seemed to be emitting it in error, or at least it didn't make sense because regardless of success/failure we were always emitting a success event. Looking at it again, I think that would be the wrong place to emit an event anyways since we would essentially emit an event every time the controller resynchronizes and runs reconciliation on a completed/stale PipelineRun.

Also, it appears we're already emitting a success/failure event here:

before := pr.Status.GetCondition(apis.ConditionSucceeded)
c.timeoutHandler.StatusLock(pr)
after := resources.GetPipelineConditionStatus(pr.Name, pipelineState, c.Logger, pr.Status.StartTime, pr.Spec.Timeout)
pr.Status.SetCondition(after)
c.timeoutHandler.StatusUnlock(pr)
reconciler.EmitEvent(c.Recorder, before, after, pr)

(and emitting it appropriately, i.e. it should be emitted only once on final success/error status change)

rupalibehera pushed a commit to rupalibehera/pipeline that referenced this pull request Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants