Use truncated base TaskRun name for lookup #565

abayer · 2019-02-28T20:06:03Z

Changes

Without this, a base TaskRun name longer than 57 (i.e., maxGeneratedNameLength) characters will not match the names of any existing TaskRuns, resulting in an infinite number of TaskRuns getting created.

Submitter Checklist

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

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

Release Notes

n/a

abayer · 2019-02-28T20:11:26Z

I probably should have created an issue for this too, but hey.

abayer · 2019-02-28T20:12:35Z

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go

@@ -241,7 +241,7 @@ func ResolveTaskRuns(getTaskRun GetTaskRun, state PipelineRunState) error {

 // getTaskRunName should return a uniquie name for a `TaskRun`.
 func getTaskRunName(taskRunsStatus map[string]v1alpha1.TaskRunStatus, prName string, pt *v1alpha1.PipelineTask) string {
-	base := fmt.Sprintf("%s-%s", prName, pt.Name)
+	base := names.SimpleNameGenerator.RestrictLengthWithSpaceForSuffix(fmt.Sprintf("%s-%s", prName, pt.Name))


I thought about just exposing maxGeneratedNameLength and doing fmt.Sprintf("%s-%s", prName, pt.name)[:names.MaxGeneratedNameLength], but opted to go this route for consistency. The name is kinda terrible though. =)

dwnusbaum · 2019-02-28T20:19:24Z

/lgtm

bobcatfish · 2019-02-28T21:38:06Z

resulting in an infinite number of TaskRuns getting created

bobcatfish

Okay @abayer this is what I think you are fixing, is my understanding right?

A PipelineRun kicks off, it creates a TaskRun for it's first Task, which is really long and called something like pretend-this-is-57-char-8asdf where 8asdf is our "random" postfix
The reconcile fires for the PipelineRun, and it looks for the runs that it thinks should exist.
It does this by calling getTaskRunName and generating a new TaskRun name, e.g. pretend-this-is-57-char-9t9t9
pretend-this-is-57-char-9t9t9 doesn't exist yet, so it makes a new TaskRun
And so on into infinity

So it seems to me that the real problem here is (3) - instead of generating a new name to look for, we should be using the name of the TaskRun that was already created - which we should have already in the status of the PipelineRun

I think there is a potential problem with the solution you have here: what if I try to run 2 PipelineRuns simultaneously with super long names, e.g. something like:

"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-1"
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-2"

If we only use the first 57 chars for lookup, we're not going to be able to tell the difference between any TaskRuns within the same PipelineRun let alone between the two.

A different idea: what if we change the logic at (3) to look in the Status of the PipelineRun and see if the PipelineTask already has a corresponding TaskRun, and use the name there?

bobcatfish · 2019-02-28T21:41:03Z

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution_test.go

-	names.TestingSeed()
-
+	// Not using names.TriggerSeed() here to ensure we would get a different taskrun name due to truncation if the base
+	// didn't match.


without TestingSeed I thought the generated name would vary and im not sure why it seems to be always pipelinerun-mytask-with-a-really-long-name-to-trigger-tru-9l9zj in this test?

Because the TaskRun already exists. =)

(basically the test wouldn't fail without the fix with the TestingSeed() call)

abayer · 2019-02-28T21:58:50Z

Aaaah, that be good logic. Lemme try.

abayer · 2019-02-28T22:08:07Z

Augh logic doesn't work as things are structured - getTaskRunName(...) is the only way we can map between the PipelineTask and its TaskRun. PipelineRun.Status.TaskRuns is a map of the result of getTaskRunName(...) and a TaskRunStatus, and TaskRunStatus doesn't have any info about the PipelineTask in it! Ow ow ow ow.

bobcatfish · 2019-02-28T22:21:47Z

Augh logic doesn't work as things are structured - getTaskRunName(...) is the only way we can map between the PipelineTask and its TaskRun. PipelineRun.Status.TaskRuns is a map of the result of getTaskRunName(...) and a TaskRunStatus, and TaskRunStatus doesn't have any info about the PipelineTask in it! Ow ow ow ow.

Ugh!!!! I guess this worked just fine back when we were statically generating the names 🤦‍♀️

Maybe we need to add some PipelineTask info into the Status?

abayer · 2019-02-28T22:33:23Z

Yeah, that's what I'm doing now. =)

abayer · 2019-02-28T23:48:53Z

...and done.

bobcatfish

Thanks for adding this so quickly @abayer 🎉 I think we're getting there! I think we might want to think a bit more about what we want the updates to the status section to look like, I added some ideas.

(Since this is an interface change I'd like to get a review from @shashwathi or @pivotal-nader-ziada or @vdemeester as well)

bobcatfish · 2019-02-28T23:55:19Z

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go

+		ptName := fmt.Sprintf("%s-%s", pipelineRun.Name, pt.Name)
+		if taskRunNames[ptName] == "" {
+			taskRunNames[ptName] = getTaskRunName(taskRunNames, pipelineRun.Status.TaskRuns, ptName)
+		}


what would you think about making a separate function that knows how to get TaskRunNames? we could:

call it in the reconciler before calling ResolvePipelineRun

Update pr.Status.TaskRunNames = taskRunNames before calling ResolvePipelineRun

Then by the time we're in here, we can assume pr.Status.TaskRunNames has the taskRunNames in it

(and we can have separate unit tests for the new function :D)

Sounds reasonable

bobcatfish · 2019-03-01T00:02:12Z

pkg/apis/pipeline/v1alpha1/pipelinerun_types.go

 	TaskRuns map[string]TaskRunStatus `json:"taskRuns,omitempty"`
+	// map of PipelineTask name keys to TaskRun name values
+	// +optional
+	TaskRunNames map[string]string `json:"taskRunNames,omitempty"`


im kinda tempted to suggest that we find a way to put the names of the PipelineTasks insto the existing taskRuns status if we can, v.s. having two separate fields for this info, what do you think?

With the current version (copying this from the PipelineRun example in tutorial.md, we'll end up with something like this:

status: ... taskRunNames: tutorial-pipeline-run-1-build-skaffold-web: build-skaffold-web tutorial-pipeline-run-1-deploy-web: deploy-web ... taskRuns: tutorial-pipeline-run-1-build-skaffold-web: conditions: - lastTransitionTime: 2018-12-11T20:31:41Z status: "True" type: Succeeded ...

But I think the interface might be a bit nicer if we put all the related info into status.taskRuns, e.g.:

status: ... taskRuns: tutorial-pipeline-run-1-build-skaffold-web: pipeline-task-name: build-skaffold-web <--- this would be new conditions: - lastTransitionTime: 2018-12-11T20:31:41Z status: "True" type: Succeeded ...

Or we could make it something like:

status: ... taskRuns: tutorial-pipeline-run-1-build-skaffold-web: pipeline-task-name: build-skaffold-web <--- this would be new status: <-- we could put the existing `TaskRunStatus` into a `status` section conditions: - lastTransitionTime: 2018-12-11T20:31:41Z status: "True" type: Succeeded ...

What do you think?

I didn't want to mess with TaskRunStatus if I could avoid it - just made things a bit messy.

Ooooh. That second option seems cleaner. I'm OK with that approach.

@abayer @bobcatfish this would mean the pipeline-task-name is only present when the TaskRun has been created from a PipelineRun right ?

Well, when the name has been created, which is basically how it works now.

No, I misread - you're right. Lemme see how this all works out. =)

bobcatfish · 2019-03-01T00:02:59Z

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution_test.go

 			}
-			_, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, tt.p.Spec.Tasks, providedResources)
+			_, _, err := ResolvePipelineRun(pr, getTask, getClusterTask, getResource, tt.p.Spec.Tasks, providedResources)


if we keep the names in the ResolvePipelineRun interface, we'd want to be asserting against them in at least some of our unit tests

(but it will be easier if we can move that logic into its own function that can be tested separately :D)

Oh, but we do! Twice in fact! =)

vdemeester

This seems ok for me (without TaskRunNames as suggested here). I wonder how much information related to TaskRuns status should be in the PipelineStatus (on top of my head, should the steps "status" be in there ?) – thinking out loud 🤔

vdemeester · 2019-03-01T11:04:40Z

pkg/apis/pipeline/v1alpha1/pipelinerun_types.go

 	TaskRuns map[string]TaskRunStatus `json:"taskRuns,omitempty"`
+	// map of PipelineTask name keys to TaskRun name values
+	// +optional
+	TaskRunNames map[string]string `json:"taskRunNames,omitempty"`


@abayer @bobcatfish this would mean the pipeline-task-name is only present when the TaskRun has been created from a PipelineRun right ?

abayer · 2019-03-01T13:35:53Z

@bobcatfish @vdemeester - fyi, once this approach has been approved, I'm going to squash this down to avoid three different implementations in the history. =)

nader-ziada · 2019-03-01T19:05:23Z

This approach agreed up makes a lot of sense. 🎉 👍

bobcatfish · 2019-03-01T19:14:31Z

I wonder how much information related to TaskRuns status should be in the PipelineStatus (on top of my head, should the steps "status" be in there ?) – thinking out loud

@vdemeester I could see going either way! I kind of think it should be all or nothing - all of the status info including steps, or none of it (and you have to go lookup the TaskRun to see it)

bobcatfish

Looking good! Just a minor bit of feedback that I think I'd have an easier time understanding the status output if we used the name of the PipelineTask directly vs. concatenating the Run name with the PipelineTask name.

bobcatfish · 2019-03-01T19:15:44Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

+			prtrs := pr.Status.TaskRuns[rprt.TaskRun.Name]
+			if prtrs == nil {
+				prtrs = &v1alpha1.PipelineRunTaskRunStatus{
+					PipelineTaskName: fmt.Sprintf("%s-%s", pr.Name, rprt.PipelineTask.Name),


how about making this just rprt.PipelineTask.Name?

Hrm, yeah, that would suffice.

bobcatfish · 2019-03-01T19:16:29Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

+				}
+			}
+			prtrs.Status = &rprt.TaskRun.Status
+			pr.Status.TaskRuns[rprt.TaskRun.Name] = prtrs


i think this assignment only needs to happen in the if prtrs == nil case, since if it's not nil we're already holding a pointer to the thing we're changing

bobcatfish · 2019-03-01T19:23:31Z

/meow space

knative-prow-robot · 2019-03-01T19:23:33Z

@bobcatfish:

In response to this:

/meow space

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.

…ames Without this, a base TaskRun name longer than 57 (i.e., `maxGeneratedNameLength`) characters will not match the names of any existing `TaskRun`s, resulting in an infinite number of `TaskRun`s getting created.

abayer · 2019-03-01T20:00:17Z

@bobcatfish Comments addressed, squashed, ready to go from my side. =)

knative-metrics-robot · 2019-03-01T20:01:30Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	81.1%	81.6%	0.5
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go	90.3%	90.2%	-0.1

vdemeester

/lgtm

knative-prow-robot · 2019-03-04T06:58:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, vdemeester

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:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Picking up tektoncd/pipeline#565

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Feb 28, 2019

knative-prow-robot requested review from dlorenc and shashwathi February 28, 2019 20:06

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 28, 2019

abayer commented Feb 28, 2019

View reviewed changes

knative-prow-robot assigned dwnusbaum Feb 28, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2019

bobcatfish self-assigned this Feb 28, 2019

bobcatfish reviewed Feb 28, 2019

View reviewed changes

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 28, 2019

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 28, 2019

abayer mentioned this pull request Feb 28, 2019

tektonify jx CI and add next gen pipeline 💥 jenkins-x/jx#3245

Merged

bobcatfish reviewed Mar 1, 2019

View reviewed changes

vdemeester reviewed Mar 1, 2019

View reviewed changes

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 1, 2019

bobcatfish reviewed Mar 1, 2019

View reviewed changes

abayer force-pushed the long-task-name-causes-infinite-taskruns branch from 2a14d3a to 4e0419d Compare March 1, 2019 19:59

vdemeester approved these changes Mar 4, 2019

View reviewed changes

knative-prow-robot assigned vdemeester Mar 4, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2019

knative-prow-robot merged commit 5456d20 into tektoncd:master Mar 4, 2019

abayer added a commit to abayer/knative-build-pipeline that referenced this pull request Mar 4, 2019

Fix handling of long task names in pods

f6a831f

Picking up tektoncd/pipeline#565

abayer mentioned this pull request Mar 4, 2019

Fix handling of long task names in pods jenkins-x-charts/tekton#22

Merged

mchmarny unassigned vdemeester and dwnusbaum Mar 7, 2019

Use truncated base TaskRun name for lookup #565

Use truncated base TaskRun name for lookup #565

Conversation

abayer commented Feb 28, 2019

Changes

Submitter Checklist

Release Notes

abayer commented Feb 28, 2019

Choose a reason for hiding this comment

dwnusbaum commented Feb 28, 2019

bobcatfish commented Feb 28, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abayer commented Feb 28, 2019

abayer commented Feb 28, 2019

bobcatfish commented Feb 28, 2019

abayer commented Feb 28, 2019

abayer commented Feb 28, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abayer commented Mar 1, 2019

nader-ziada commented Mar 1, 2019

bobcatfish commented Mar 1, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Mar 1, 2019

knative-prow-robot commented Mar 1, 2019

abayer commented Mar 1, 2019

knative-metrics-robot commented Mar 1, 2019

vdemeester left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Mar 4, 2019