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.

Sign up for GitHub

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

Define a common function to exit ReconcileKind #2805

Merged
merged 1 commit into from
Jun 19, 2020
Merged

Define a common function to exit ReconcileKind #2805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 43 additions & 47 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,8 @@ var (
// resource with the current status of the resource.
func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1beta1.PipelineRun) pkgreconciler.Event {
logger := logging.FromContext(ctx)
recorder := controller.GetEventRecorder(ctx)
// Snapshot original for the label/annotation check below.
original := pr.DeepCopy()
// Read the initial condition
before := pr.Status.GetCondition(apis.ConditionSucceeded)

if !pr.HasStarted() {
pr.Status.InitializeConditions()
Expand All @@ -142,80 +141,77 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1beta1.PipelineRun)
}
// start goroutine to track pipelinerun timeout only startTime is not set
go c.timeoutHandler.WaitPipelineRun(pr, pr.Status.StartTime)
} else {
pr.Status.InitializeConditions()
}

// In case of reconcile errors, we store the error in a multierror, attempt
// to update, and return the original error combined with any update error
var merr *multierror.Error

switch {
case pr.IsDone():
if pr.IsDone() {
// We may be reading a version of the object that was stored at an older version
// and may not have had all of the assumed default specified.
pr.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx))

c.updatePipelineResults(ctx, pr)
if err := artifacts.CleanupArtifactStorage(pr, c.KubeClientSet, logger); err != nil {
logger.Errorf("Failed to delete PVC for PipelineRun %s: %v", pr.Name, err)
return err
return c.finishReconcileUpdateEmitEvents(ctx, pr, before, err)
}
if err := c.cleanupAffinityAssistants(pr); err != nil {
logger.Errorf("Failed to delete StatefulSet for PipelineRun %s: %v", pr.Name, err)
return err
return c.finishReconcileUpdateEmitEvents(ctx, pr, before, err)
}
c.timeoutHandler.Release(pr)
if err := c.updateTaskRunsStatusDirectly(pr); err != nil {
logger.Errorf("Failed to update TaskRun status for PipelineRun %s: %v", pr.Name, err)
return err
return c.finishReconcileUpdateEmitEvents(ctx, pr, before, err)
}
go func(metrics *Recorder) {
err := metrics.DurationAndCount(pr)
if err != nil {
logger.Warnf("Failed to log the metrics : %v", err)
}
}(c.metrics)
case pr.IsCancelled():
return c.finishReconcileUpdateEmitEvents(ctx, pr, before, nil)
}

if pr.IsCancelled() {
// If the pipelinerun is cancelled, cancel tasks and update status
before := pr.Status.GetCondition(apis.ConditionSucceeded)
merr = multierror.Append(merr, cancelPipelineRun(logger, pr, c.PipelineClientSet))
after := pr.Status.GetCondition(apis.ConditionSucceeded)
events.Emit(recorder, before, after, pr)
default:
if err := c.tracker.Track(pr.GetTaskRunRef(), pr); err != nil {
logger.Errorf("Failed to create tracker for TaskRuns for PipelineRun %s: %v", pr.Name, err)
recorder.Event(pr, corev1.EventTypeWarning, v1beta1.PipelineRunReasonFailed.String(), "Failed to create tracker for TaskRuns for PipelineRun")
return err
}
err := cancelPipelineRun(logger, pr, c.PipelineClientSet)
return c.finishReconcileUpdateEmitEvents(ctx, pr, before, err)
}

// Make sure that the PipelineRun status is in sync with the actual TaskRuns
err := c.updatePipelineRunStatusFromInformer(ctx, pr)
if err != nil {
// This should not fail. Return the error so we can re-try later.
logger.Errorf("Error while syncing the pipelinerun status: %v", err.Error())
return err
}
if err := c.tracker.Track(pr.GetTaskRunRef(), pr); err != nil {
logger.Errorf("Failed to create tracker for TaskRuns for PipelineRun %s: %v", pr.Name, err)
return c.finishReconcileUpdateEmitEvents(ctx, pr, before, err)
}

// Reconcile this copy of the pipelinerun and then write back any status or label
// updates regardless of whether the reconciliation errored out.
if err = c.reconcile(ctx, pr); err != nil {
logger.Errorf("Reconcile error: %v", err.Error())
merr = multierror.Append(merr, err)
}
// Make sure that the PipelineRun status is in sync with the actual TaskRuns
err := c.updatePipelineRunStatusFromInformer(ctx, pr)
if err != nil {
// This should not fail. Return the error so we can re-try later.
logger.Errorf("Error while syncing the pipelinerun status: %v", err.Error())
return c.finishReconcileUpdateEmitEvents(ctx, pr, before, err)
}

Copy link
Member

@pritidesai pritidesai Jun 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

thanks @afrittoli, I am still trying to understand these changes, compared to taskRun, we are missing before here before reconciling ... this before condition is used while updating emit events, would it cause any conflicting events? 🤔

	// Store the condition before reconcile
	before := tr.Status.GetCondition(apis.ConditionSucceeded)

	// Reconcile this copy of the task run and then write back any status
	// updates regardless of whether the reconciliation errored out.
	if err = c.reconcile(ctx, tr, taskSpec, rtr); err != nil {
		logger.Errorf("Reconcile error: %v", err.Error())
	}
	// Emit events (only when ConditionSucceeded was changed)
	return c.finishReconcileUpdateEmitEvents(ctx, tr, before, err)

please feel free to ignore if its not relevant here.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is also a before here, but it's earlier in the reconciler code. The reason being is that the code for sending the start even is not yet in for pipelineruns. Once I add that, I will need to move L133 further down in the reconcile code, similar to what taskrun does.

// If we need to update the labels or annotations, we need to call Update with these
// changes explicitly.
if !reflect.DeepEqual(original.ObjectMeta.Labels, pr.ObjectMeta.Labels) || !reflect.DeepEqual(original.ObjectMeta.Annotations, pr.ObjectMeta.Annotations) {
if _, err := c.updateLabelsAndAnnotations(pr); err != nil {
logger.Warn("Failed to update PipelineRun labels/annotations", zap.Error(err))
recorder.Event(pr, corev1.EventTypeWarning, "Error", "PipelineRun failed to update labels/annotations")
return multierror.Append(merr, err)
}
// Reconcile this copy of the pipelinerun and then write back any status or label
// updates regardless of whether the reconciliation errored out.
if err = c.reconcile(ctx, pr); err != nil {
logger.Errorf("Reconcile error: %v", err.Error())
}

return c.finishReconcileUpdateEmitEvents(ctx, pr, before, err)
}

func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, pr *v1beta1.PipelineRun, beforeCondition *apis.Condition, previousError error) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i really like having one function to handle this!

recorder := controller.GetEventRecorder(ctx)
logger := logging.FromContext(ctx)

afterCondition := pr.Status.GetCondition(apis.ConditionSucceeded)
events.Emit(recorder, beforeCondition, afterCondition, pr)
_, err := c.updateLabelsAndAnnotations(pr)
if err != nil {
Copy link
Member

@pritidesai pritidesai Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

no issues here with these changes but this same check is missing from the taskrun finishReconcileUpdateEmitEvents:

	_, err := c.updateLabelsAndAnnotations(tr)
	events.EmitError(recorder, err, tr)

taskrun emits error event without checking if err is nil, we can create a separate PR for adding that check.

Copy link
Member

Choose a reason for hiding this comment

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

Just read the comments from the taskrun PR, if EmitError has check for nil, should we drop the check from here like taskrun reconciler? Or do we still want to keep it?

Copy link
Member

Choose a reason for hiding this comment

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

if we drop nil check from here, no need for the logger as well ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the check here because @bobcatfish mentioned before that she didn't like the check to be in EmitError only, so I was planning on changing that in the task run controller too (but in a different PR)

Copy link
Member

Choose a reason for hiding this comment

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

thanks @afrittoli

logger.Warn("Failed to update PipelineRun labels/annotations", zap.Error(err))
events.EmitError(recorder, err, pr)
}

return merr.ErrorOrNil()
return multierror.Append(previousError, err).ErrorOrNil()
}

func (c *Reconciler) updatePipelineResults(ctx context.Context, pr *v1beta1.PipelineRun) {
Expand Down
6 changes: 3 additions & 3 deletions pkg/reconciler/taskrun/taskrun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ var _ taskrunreconciler.Interface = (*Reconciler)(nil)
func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkgreconciler.Event {
logger := logging.FromContext(ctx)
recorder := controller.GetEventRecorder(ctx)
// Read the initial condition
before := tr.Status.GetCondition(apis.ConditionSucceeded)
Copy link
Collaborator

Choose a reason for hiding this comment

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

good call, no reason to be fetching that so many times if we can do it once :D


// If the TaskRun is just starting, this will also set the starttime,
// from which the timeout will immediately begin counting down.
Expand Down Expand Up @@ -157,7 +159,6 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkg

// If the TaskRun is cancelled, kill resources and update status
if tr.IsCancelled() {
before := tr.Status.GetCondition(apis.ConditionSucceeded)
message := fmt.Sprintf("TaskRun %q was cancelled", tr.Name)
err := c.failTaskRun(ctx, tr, v1beta1.TaskRunReasonCancelled, message)
return c.finishReconcileUpdateEmitEvents(ctx, tr, before, err)
Expand All @@ -166,7 +167,6 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkg
// Check if the TaskRun has timed out; if it is, this will set its status
// accordingly.
if tr.HasTimedOut() {
before := tr.Status.GetCondition(apis.ConditionSucceeded)
message := fmt.Sprintf("TaskRun %q failed to finish within %q", tr.Name, tr.GetTimeout())
err := c.failTaskRun(ctx, tr, v1beta1.TaskRunReasonTimedOut, message)
return c.finishReconcileUpdateEmitEvents(ctx, tr, before, err)
Expand All @@ -184,7 +184,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkg
}

// Store the condition before reconcile
before := tr.Status.GetCondition(apis.ConditionSucceeded)
before = tr.Status.GetCondition(apis.ConditionSucceeded)

// Reconcile this copy of the task run and then write back any status
// updates regardless of whether the reconciliation errored out.
Expand Down