Skip to content

Commit

Permalink
Cleanup: Use CustomRun instead of RunObject
Browse files Browse the repository at this point in the history 
v1alpha1 Runs are no longer supported by the PipelineRun reconciler.
The PipelineRun reconciler currently operates on RunObjects (an interface
which is implemented by v1alpha1.Run and v1beta1.CustomRun).
However, this code is misleading, since retry behavior differs between v1alpha1.Run
and v1beta1.CustomRun, but the controller only supports the behavior of v1beta1.CustomRuns.
This commit replaces usages of RunObject with CustomRun, and renames references to runs/runObjects
to customRuns for clarity. No functional changes.
  • Loading branch information
@lbernick @tekton-robot
lbernick authored and tekton-robot committed Jun 8, 2023
1 parent ce253bb commit ee3c22c
Show file tree
Hide file tree
Showing 10 changed files with 401 additions and 418 deletions.
77 changes: 33 additions & 44 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,16 +333,11 @@ func (c *Reconciler) resolvePipelineState(
}
fn := tresources.GetTaskFunc(ctx, c.KubeClientSet, c.PipelineClientSet, c.resolutionRequester, pr, task.TaskRef, trName, pr.Namespace, pr.Spec.ServiceAccountName, vp)

getRunObjectFunc := func(name string) (v1beta1.RunObject, error) {
getCustomRunFunc := func(name string) (*v1beta1.CustomRun, error) {
r, err := c.customRunLister.CustomRuns(pr.Namespace).Get(name)
if err != nil {
return nil, err
}
// If we just return c.customRunLister.CustomRuns(...).Get(...) and there is no run, we end up returning
// a v1beta1.RunObject that won't == nil, so do an explicit check.
if r == nil {
return nil, nil
}
return r, nil
}

Expand All @@ -352,7 +347,7 @@ func (c *Reconciler) resolvePipelineState(
func(name string) (*v1beta1.TaskRun, error) {
return c.taskRunLister.TaskRuns(pr.Namespace).Get(name)
},
getRunObjectFunc,
getCustomRunFunc,
task,
)
if err != nil {
Expand Down Expand Up @@ -778,10 +773,10 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip
}()

if rpt.IsCustomTask() {
rpt.RunObjects, err = c.createRunObjects(ctx, rpt, pr)
rpt.CustomRuns, err = c.createCustomRuns(ctx, rpt, pr)
if err != nil {
recorder.Eventf(pr, corev1.EventTypeWarning, "RunsCreationFailed", "Failed to create Runs %q: %v", rpt.RunObjectNames, err)
err = fmt.Errorf("error creating Runs called %s for PipelineTask %s from PipelineRun %s: %w", rpt.RunObjectNames, rpt.PipelineTask.Name, pr.Name, err)
recorder.Eventf(pr, corev1.EventTypeWarning, "RunsCreationFailed", "Failed to create CustomRuns %q: %v", rpt.CustomRunNames, err)
err = fmt.Errorf("error creating CustomRuns called %s for PipelineTask %s from PipelineRun %s: %w", rpt.CustomRunNames, rpt.PipelineTask.Name, pr.Name, err)
return err
}
} else {
Expand Down Expand Up @@ -887,32 +882,32 @@ func (c *Reconciler) createTaskRun(ctx context.Context, taskRunName string, para
return c.PipelineClientSet.TektonV1beta1().TaskRuns(pr.Namespace).Create(ctx, tr, metav1.CreateOptions{})
}

func (c *Reconciler) createRunObjects(ctx context.Context, rpt *resources.ResolvedPipelineTask, pr *v1beta1.PipelineRun) ([]v1beta1.RunObject, error) {
var runObjects []v1beta1.RunObject
ctx, span := c.tracerProvider.Tracer(TracerName).Start(ctx, "createRunObjects")
func (c *Reconciler) createCustomRuns(ctx context.Context, rpt *resources.ResolvedPipelineTask, pr *v1beta1.PipelineRun) ([]*v1beta1.CustomRun, error) {
var customRuns []*v1beta1.CustomRun
ctx, span := c.tracerProvider.Tracer(TracerName).Start(ctx, "createCustomRuns")
defer span.End()
var matrixCombinations []v1beta1.Params

if rpt.PipelineTask.IsMatrixed() {
matrixCombinations = rpt.PipelineTask.Matrix.FanOut()
}

for i, runObjectName := range rpt.RunObjectNames {
for i, customRunName := range rpt.CustomRunNames {
var params v1beta1.Params
if len(matrixCombinations) > i {
params = matrixCombinations[i]
}
runObject, err := c.createRunObject(ctx, runObjectName, params, rpt, pr)
customRun, err := c.createCustomRun(ctx, customRunName, params, rpt, pr)
if err != nil {
return nil, err
}
runObjects = append(runObjects, runObject)
customRuns = append(customRuns, customRun)
}
return runObjects, nil
return customRuns, nil
}

func (c *Reconciler) createRunObject(ctx context.Context, runName string, params v1beta1.Params, rpt *resources.ResolvedPipelineTask, pr *v1beta1.PipelineRun) (v1beta1.RunObject, error) {
ctx, span := c.tracerProvider.Tracer(TracerName).Start(ctx, "createRunObject")
func (c *Reconciler) createCustomRun(ctx context.Context, runName string, params v1beta1.Params, rpt *resources.ResolvedPipelineTask, pr *v1beta1.PipelineRun) (*v1beta1.CustomRun, error) {
ctx, span := c.tracerProvider.Tracer(TracerName).Start(ctx, "createCustomRun")
defer span.End()
logger := logging.FromContext(ctx)
rpt.PipelineTask = resources.ApplyPipelineTaskContexts(rpt.PipelineTask)
Expand Down Expand Up @@ -1269,22 +1264,17 @@ func (c *Reconciler) updatePipelineRunStatusFromInformer(ctx context.Context, pr
logger.Errorf("could not list TaskRuns %#v", err)
return err
}
var runObjects []v1beta1.RunObject

customRuns, err := c.customRunLister.CustomRuns(pr.Namespace).List(k8slabels.SelectorFromSet(pipelineRunLabels))
if err != nil {
logger.Errorf("could not list CustomRuns %#v", err)
return err
}
for _, cr := range customRuns {
runObjects = append(runObjects, cr)
}

return updatePipelineRunStatusFromChildObjects(ctx, logger, pr, taskRuns, runObjects)
return updatePipelineRunStatusFromChildObjects(ctx, logger, pr, taskRuns, customRuns)
}

func updatePipelineRunStatusFromChildObjects(ctx context.Context, logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, taskRuns []*v1beta1.TaskRun, runObjects []v1beta1.RunObject) error {
updatePipelineRunStatusFromChildRefs(logger, pr, taskRuns, runObjects)
func updatePipelineRunStatusFromChildObjects(ctx context.Context, logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, taskRuns []*v1beta1.TaskRun, customRuns []*v1beta1.CustomRun) error {
updatePipelineRunStatusFromChildRefs(logger, pr, taskRuns, customRuns)

return validateChildObjectsInPipelineRunStatus(ctx, pr.Status)
}
Expand Down Expand Up @@ -1321,38 +1311,37 @@ func filterTaskRunsForPipelineRunStatus(logger *zap.SugaredLogger, pr *v1beta1.P
return ownedTaskRuns
}

// filterRunsForPipelineRunStatus filters the given slice of run objects, returning information only those owned by the given PipelineRun.
func filterRunsForPipelineRunStatus(logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, runObjects []v1beta1.RunObject) ([]string, []string, []schema.GroupVersionKind, []*v1beta1.CustomRunStatus) {
// filterCustomRunsForPipelineRunStatus filters the given slice of customRuns, returning information only those owned by the given PipelineRun.
func filterCustomRunsForPipelineRunStatus(logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, customRuns []*v1beta1.CustomRun) ([]string, []string, []schema.GroupVersionKind, []*v1beta1.CustomRunStatus) {
var names []string
var taskLabels []string
var gvks []schema.GroupVersionKind
var statuses []*v1beta1.CustomRunStatus

// Loop over all the run objects associated to Tasks
for _, runObj := range runObjects {
// Only process run objects that are owned by this PipelineRun.
// This skips Runs that are indirectly created by the PipelineRun (e.g. by custom tasks).
if len(runObj.GetObjectMeta().GetOwnerReferences()) < 1 || runObj.GetObjectMeta().GetOwnerReferences()[0].UID != pr.ObjectMeta.UID {
logger.Debugf("Found a %s %s that is not owned by this PipelineRun", runObj.GetObjectKind().GroupVersionKind().Kind, runObj.GetObjectMeta().GetName())
// Loop over all the customRuns associated to Tasks
for _, cr := range customRuns {
// Only process customRuns that are owned by this PipelineRun.
// This skips customRuns that are indirectly created by the PipelineRun (e.g. by custom tasks).
if len(cr.GetObjectMeta().GetOwnerReferences()) < 1 || cr.GetObjectMeta().GetOwnerReferences()[0].UID != pr.ObjectMeta.UID {
logger.Debugf("Found a %s %s that is not owned by this PipelineRun", cr.GetObjectKind().GroupVersionKind().Kind, cr.GetObjectMeta().GetName())
continue
}

names = append(names, runObj.GetObjectMeta().GetName())
taskLabels = append(taskLabels, runObj.GetObjectMeta().GetLabels()[pipeline.PipelineTaskLabelKey])
names = append(names, cr.GetObjectMeta().GetName())
taskLabels = append(taskLabels, cr.GetObjectMeta().GetLabels()[pipeline.PipelineTaskLabelKey])

r := runObj.(*v1beta1.CustomRun)
statuses = append(statuses, &r.Status)
// We can't just get the gvk from the run's TypeMeta because that isn't populated for resources created through the fake client.
statuses = append(statuses, &cr.Status)
// We can't just get the gvk from the customRun's TypeMeta because that isn't populated for resources created through the fake client.
gvks = append(gvks, v1beta1.SchemeGroupVersion.WithKind(customRun))
}

return names, taskLabels, gvks, statuses
}

func updatePipelineRunStatusFromChildRefs(logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, trs []*v1beta1.TaskRun, runObjects []v1beta1.RunObject) {
// If no TaskRun or RunObject was found, nothing to be done. We never remove child references from the status.
func updatePipelineRunStatusFromChildRefs(logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, trs []*v1beta1.TaskRun, customRuns []*v1beta1.CustomRun) {
// If no TaskRun or CustomRun was found, nothing to be done. We never remove child references from the status.
// We do still return an empty map of TaskRun/Run names keyed by PipelineTask name for later functions.
if len(trs) == 0 && len(runObjects) == 0 {
if len(trs) == 0 && len(customRuns) == 0 {
return
}

Expand Down Expand Up @@ -1388,7 +1377,7 @@ func updatePipelineRunStatusFromChildRefs(logger *zap.SugaredLogger, pr *v1beta1
}

// Get the names, their task label values, and their group/version/kind info for all CustomRuns or Runs associated with the PipelineRun
names, taskLabels, gvks, _ := filterRunsForPipelineRunStatus(logger, pr, runObjects)
names, taskLabels, gvks, _ := filterCustomRunsForPipelineRunStatus(logger, pr, customRuns)

// Loop over that data and populate the child references
for idx := range names {
Expand Down
40 changes: 20 additions & 20 deletions pkg/reconciler/pipelinerun/pipelinerun_updatestatus_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,9 +299,9 @@ func TestUpdatePipelineRunStatusFromChildRefs(t *testing.T) {
},
}

allTaskRuns, taskRunsFromAnotherPR, taskRunsWithNoOwner, _, runsFromAnotherPR, runsWithNoOwner := getTestTaskRunsAndRuns(t)
allTaskRuns, taskRunsFromAnotherPR, taskRunsWithNoOwner, _, runsFromAnotherPR, runsWithNoOwner := getTestTaskRunsAndCustomRuns(t)

singleRun := []v1beta1.RunObject{parse.MustParseCustomRun(t, `
singleRun := []*v1beta1.CustomRun{parse.MustParseCustomRun(t, `
metadata:
labels:
tekton.dev/pipelineTask: task-6
Expand All @@ -314,20 +314,20 @@ metadata:
prName string
prStatus v1beta1.PipelineRunStatus
trs []*v1beta1.TaskRun
runs []v1beta1.RunObject
customRuns []*v1beta1.CustomRun
expectedPrStatus v1beta1.PipelineRunStatus
}{
{
prName: "no-status-no-taskruns-or-runs",
prStatus: v1beta1.PipelineRunStatus{},
trs: nil,
runs: nil,
customRuns: nil,
expectedPrStatus: v1beta1.PipelineRunStatus{},
}, {
prName: "status-no-taskruns-or-runs",
prStatus: prStatusWithNoTaskRuns,
trs: nil,
runs: nil,
customRuns: nil,
expectedPrStatus: prStatusWithNoTaskRuns,
}, {
prName: "status-nil-taskruns",
Expand All @@ -344,7 +344,7 @@ metadata:
}, {
prName: "status-nil-runs",
prStatus: prStatusWithEmptyChildRefs,
runs: singleRun,
customRuns: singleRun,
expectedPrStatus: prStatusRecoveredSimpleWithRun,
}, {
prName: "status-missing-taskruns",
Expand All @@ -363,7 +363,7 @@ metadata:
}, {
prName: "status-missing-runs",
prStatus: prStatusMissingRun,
runs: singleRun,
customRuns: singleRun,
expectedPrStatus: prStatusWithNoTaskRuns,
}, {
prName: "status-matching-taskruns-pr",
Expand All @@ -374,19 +374,19 @@ metadata:
prName: "orphaned-taskruns-pr",
prStatus: prStatusWithOrphans,
trs: allTaskRuns,
runs: singleRun,
customRuns: singleRun,
expectedPrStatus: prStatusRecovered,
}, {
prName: "tr-and-run-from-another-pr",
prStatus: prStatusWithEmptyChildRefs,
trs: taskRunsFromAnotherPR,
runs: runsFromAnotherPR,
customRuns: runsFromAnotherPR,
expectedPrStatus: prStatusWithEmptyChildRefs,
}, {
prName: "tr-and-run-with-no-owner",
prStatus: prStatusWithEmptyChildRefs,
trs: taskRunsWithNoOwner,
runs: runsWithNoOwner,
customRuns: runsWithNoOwner,
expectedPrStatus: prStatusWithEmptyChildRefs,
}, {
prName: "matrixed-taskruns-pr",
Expand All @@ -409,7 +409,7 @@ metadata:
- uid: 11111111-1111-1111-1111-111111111111
`),
},
runs: nil,
customRuns: nil,
expectedPrStatus: v1beta1.PipelineRunStatus{
Status: prRunningStatus,
PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{
Expand Down Expand Up @@ -441,7 +441,7 @@ pipelineTaskName: task
Status: tc.prStatus,
}

updatePipelineRunStatusFromChildRefs(logger, pr, tc.trs, tc.runs)
updatePipelineRunStatusFromChildRefs(logger, pr, tc.trs, tc.customRuns)

actualPrStatus := pr.Status

Expand Down Expand Up @@ -501,9 +501,9 @@ func TestUpdatePipelineRunStatusFromChildObjects(t *testing.T) {
}
}

allTaskRuns, _, _, _, _, _ := getTestTaskRunsAndRuns(t)
allTaskRuns, _, _, _, _, _ := getTestTaskRunsAndCustomRuns(t)

singleCustomRun := []v1beta1.RunObject{parse.MustParseCustomRun(t, `
singleCustomRun := []*v1beta1.CustomRun{parse.MustParseCustomRun(t, `
metadata:
labels:
tekton.dev/pipelineTask: task-6
Expand All @@ -516,7 +516,7 @@ metadata:
prName string
prStatus func() v1beta1.PipelineRunStatus
trs []*v1beta1.TaskRun
runs []v1beta1.RunObject
runs []*v1beta1.CustomRun
expectedStatusTRs map[string]*v1beta1.PipelineRunTaskRunStatus
expectedStatusRuns map[string]*v1beta1.PipelineRunRunStatus
expectedStatusCRs []v1beta1.ChildStatusReference
Expand Down Expand Up @@ -698,7 +698,7 @@ func prStatusFromInputs(status duckv1.Status, taskRuns map[string]*v1beta1.Pipel
return prs
}

func getTestTaskRunsAndRuns(t *testing.T) ([]*v1beta1.TaskRun, []*v1beta1.TaskRun, []*v1beta1.TaskRun, []v1beta1.RunObject, []v1beta1.RunObject, []v1beta1.RunObject) {
func getTestTaskRunsAndCustomRuns(t *testing.T) ([]*v1beta1.TaskRun, []*v1beta1.TaskRun, []*v1beta1.TaskRun, []*v1beta1.CustomRun, []*v1beta1.CustomRun, []*v1beta1.CustomRun) {
t.Helper()
allTaskRuns := []*v1beta1.TaskRun{
parse.MustParseV1beta1TaskRun(t, `
Expand Down Expand Up @@ -735,7 +735,7 @@ metadata:
name: pr-task-1-xxyyy
`)}

allRuns := []v1beta1.RunObject{
allCustomRuns := []*v1beta1.CustomRun{
parse.MustParseCustomRun(t, `
metadata:
labels:
Expand All @@ -762,7 +762,7 @@ metadata:
`),
}

runsFromAnotherPR := []v1beta1.RunObject{parse.MustParseCustomRun(t, `
customRunsFromAnotherPR := []*v1beta1.CustomRun{parse.MustParseCustomRun(t, `
metadata:
labels:
tekton.dev/pipelineTask: run-1
Expand All @@ -771,14 +771,14 @@ metadata:
- uid: 22222222-2222-2222-2222-222222222222
`)}

runsWithNoOwner := []v1beta1.RunObject{parse.MustParseCustomRun(t, `
customRunsWithNoOwner := []*v1beta1.CustomRun{parse.MustParseCustomRun(t, `
metadata:
labels:
tekton.dev/pipelineTask: run-1
name: pr-run-1-xxyyy
`)}

return allTaskRuns, taskRunsFromAnotherPR, taskRunsWithNoOwner, allRuns, runsFromAnotherPR, runsWithNoOwner
return allTaskRuns, taskRunsFromAnotherPR, taskRunsWithNoOwner, allCustomRuns, customRunsFromAnotherPR, customRunsWithNoOwner
}

func mustParsePipelineRunTaskRunStatus(t *testing.T, yamlStr string) *v1beta1.PipelineRunTaskRunStatus {
Expand Down
Loading

0 comments on commit ee3c22c

Please sign in to comment.