Use kmeta.ChildName for child resources #4361

afrittoli · 2021-11-04T19:47:31Z

Changes

When a reconciler generates a child resource, it risks creating a
duplicate resource because of stale informer cache. This may happen
for instance when two reconciliations of the same resource happen
almost at the same time.
To avoid this risk, children resources should use a name that is uniquely
associated to that of the parent, so that any attempt of recreation would
fail. Such failure, when it happens, is considered as a transient error,
and it does not cause the TaskRun or the PipelineRun to fail.

We use kmeta.ChildName to generate the names. This applies to Pods
generated by TaskRuns and TaskRuns and Runs generated by PipelineRuns.
ChildName combines a base name with a suffix, and if the resulting name is
too long it shortens it by truncating it and appending an hash of the
original name: https://pkg.go.dev/github.com/knative/pkg/kmeta#ChildName

Resources that are not k8s resources, such as steps or scripts, are not
affected by this change.

Fixes #4358

Signed-off-by: Andrea Frittoli [email protected]

/kind bug

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Name of `Pods` and  `TaskRuns` created by Tekton controllers are now generated with [`kmeta.ChildName`](https://pkg.go.dev/github.com/knative/pkg/kmeta#ChildName):

> ChildName generates a name for the resource based upon the parent resource and suffix. If the concatenated name is longer than K8s permits the name is hashed and truncated to permit construction of the resource, but still keeps it unique. If the suffix itself is longer than 31 characters, then the whole string will be hashed and `parent|hash|suffix` will be returned, where parent and suffix will be trimmed to fit (prefix of parent at most of length 31, and prefix of suffix at most length 30).

The name of the `Pods` owned by a `TaskRun`  is univocally associated to the owning resource. If a `TaskRun` resource is deleted and created with the same name, the child `Pod` will be created with the same name as before.
The base format of the name is `<taskrun-name>-pod`. The name may vary as described above if it becomes too long.
In case of retries, starting from the first retry, the base format of the name is `<taskrun-name>-retry<N>` where `N` is the retry number.

The name of the `TaskRuns` and `Runs` owned by a `PipelineRun`  are univocally associated to the owning resource. If a `PipelineRun` resource is deleted and created with the same name, the child `TaskRuns` will be created with the same name as before.
The base format of the name is `<pipelinerun-name>-<pipelinetask-name>`. The name may vary as described above if it becomes too long.

tekton-robot · 2021-11-04T19:50:51Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.0%	91.8%	-1.2

imjasonh · 2021-11-04T20:02:59Z

This looks great! What's WIP about it? (aside from unit tests 😈 )

afrittoli · 2021-11-04T20:55:43Z

This looks great! What's WIP about it? (aside from unit tests 😈 )

Only that I need to fix the tests first :P

afrittoli · 2021-11-05T12:36:33Z

There may be something to check about retries:

        status:
          completionTime: "2021-11-05T12:16:21Z"
          conditions:
          - lastTransitionTime: "2021-11-05T12:16:21Z"
            message: 'failed to create task run pod "retry-pipeline-retry-me": pods "retry-pipeline-retry-me-pod"
              already exists. Maybe invalid TaskSpec'
            reason: CouldntGetTask
            status: "False"
            type: Succeeded

Also I need to make sure that failures like this are considered transient and do not cause taskruns or pipelineruns to fail.

tekton-robot · 2021-11-05T14:57:31Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/pod.go	88.5%	88.8%	0.3

afrittoli · 2021-11-05T14:59:28Z

There may be something to check about retries:

        status:
          completionTime: "2021-11-05T12:16:21Z"
          conditions:
          - lastTransitionTime: "2021-11-05T12:16:21Z"
            message: 'failed to create task run pod "retry-pipeline-retry-me": pods "retry-pipeline-retry-me-pod"
              already exists. Maybe invalid TaskSpec'
            reason: CouldntGetTask
            status: "False"
            type: Succeeded

Also I need to make sure that failures like this are considered transient and do not cause taskruns or pipelineruns to fail.

For retries, the PipelineRun controller saves the original TaskRun status in the list of retries, and it clears the TaskRun status, so that the TaskRun controllers sees a TaskRun that needs to be reconciled, and triggers a new Pod creation. The Pod name must include the number of retry, at least starting from the first retry on.

afrittoli · 2021-11-05T15:30:57Z

/test pull-tekton-pipeline-alpha-integration-tests

afrittoli · 2021-11-05T15:31:49Z

I think the failures on pull-tekton-pipeline-alpha-integration-tests may be flakes.
/cc @imjasonh @mattmoor @vdemeester

pkg/pod/pod.go

tekton-robot · 2021-11-05T15:36:05Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/pod.go	88.5%	88.8%	0.3

tekton-robot · 2021-11-05T15:44:42Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/pod.go	88.5%	88.8%	0.3

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

afrittoli · 2021-11-05T18:13:36Z

/test pull-tekton-pipeline-alpha-integration-tests

tekton-robot · 2021-11-08T14:08:58Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/pod.go	88.5%	88.8%	0.3

ghost · 2021-11-08T14:11:44Z

/lgtm

ghost · 2021-11-08T14:45:51Z

The run-steps-as-non-root Example failed with:

        >>> Container step-show-user-1001:
        Signal 23 (URG) caught by ps (3.3.16).
        ps:ps/display.c:66: please report this bug

It looks like this has been recently fixed in ps by ignoring the signal so probably nothing for us to do here. Re-running.

/test pull-tekton-pipeline-integration-tests

jerop

thanks @afrittoli 🎉

added a comment documenting the change to Runs as well

jerop · 2021-11-09T17:26:38Z

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

@@ -572,8 +573,7 @@ func getRunName(runsStatus map[string]*v1beta1.PipelineRunRunStatus, ptName, prN
 			return k
 		}
 	}
-
-	return names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("%s-%s", prName, ptName))
+	return kmeta.ChildName(prName, fmt.Sprintf("-%s", ptName))


looks like you've made the same change to Runs as well, please documentation for that like you've done for TaskRuns and Pods

adding that detail to the commit message and pr description would be great too!

Nice catch, thank you. Fixed.

pritidesai · 2021-11-10T19:27:59Z

Thanks @afrittoli for this PR 🙏 I think this also fixes (both were closed but I just reopened them since the issues are still seen with recent releases) :

#3126
#3557

pritidesai · 2021-11-10T19:39:12Z

I might be missing something but can we please add some tests around the release note if they don't exist already:

> ChildName generates a name for the resource based upon the parent resource and suffix. If the concatenated name is longer than K8s permits the name is hashed and truncated to permit construction of the resource, but still keeps it unique. If the suffix itself is longer than 31 characters, then the whole string will be hashed and `parent|hash|suffix` will be returned, where parent and suffix will be trimmed to fit (prefix of parent at most of length 31, and prefix of suffix at most length 30).

Documenting some samples would be ideal for the folks creating pipelineRuns 🙏 .

tekton-robot · 2021-11-16T21:11:35Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/pod.go	88.5%	88.8%	0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.0%	93.8%	0.8

afrittoli · 2021-11-16T21:12:08Z

I might be missing something but can we please add some tests around the release note if they don't exist already:

> ChildName generates a name for the resource based upon the parent resource and suffix. If the concatenated name is longer than K8s permits the name is hashed and truncated to permit construction of the resource, but still keeps it unique. If the suffix itself is longer than 31 characters, then the whole string will be hashed and `parent|hash|suffix` will be returned, where parent and suffix will be trimmed to fit (prefix of parent at most of length 31, and prefix of suffix at most length 30).

Thank you - the logic of the ChildName function is tested on knative side, which is why I did not add tests.
I added some tests for that now so that if the logic is changed on the library side we will notice it and have a chance to update docs along with the tests accordingly.

Documenting some samples would be ideal for the folks creating pipelineRuns 🙏 .

Good idea, done!

jerop

examples_test.go:144: exit status 1 Output: Error from server (InternalError): error when creating "STDIN": Internal error occurred: failed calling webhook "webhook.pipeline.tekton.dev": Post "https://tekton-pipelines-webhook.tekton-pipelines.svc:443/defaulting?timeout=10s": context deadline exceeded

/test pull-tekton-pipeline-alpha-integration-tests

docs/pipelineruns.md

pritidesai

thank you @afrittoli

a couple of minor nits but otherwise looks great, thank you for fixing this 🙏

tekton-robot · 2021-11-16T22:31:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pritidesai

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~~ [pritidesai]

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

When a reconciler generates a child resource, it risks creating a duplicate resource because of stale informer cache. This may happen for instance when two reconciliations of the same resource happen almost at the same time. To avoid this risk, children resources should use a name that is uniquely associated to that of the parent, so that any attempt of recreation would fail. Such failure, when it happens, is considered as a transient error, and it does not cause the TaskRun or the PipelineRun to fail. We use kmeta.ChildName to generate the names. This applies to Pods generated by TaskRuns and TaskRuns and Runs generated by PipelineRuns. ChildName combines a base name with a suffix, and if the resulting name is too long it shortens it by truncating it and appending an hash of the original name: https://pkg.go.dev/github.com/knative/pkg/kmeta#ChildName Resources that are not k8s resources, such as steps or scripts, are not affected by this change. Fixes tektoncd#4358 Signed-off-by: Andrea Frittoli <[email protected]>

tekton-robot · 2021-11-17T12:00:20Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/pod.go	88.5%	88.8%	0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.0%	93.8%	0.8

vdemeester

/lgtm

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Nov 4, 2021

tekton-robot requested review from dlorenc and imjasonh November 4, 2021 19:47

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 4, 2021

afrittoli force-pushed the issues/4358 branch from eaee88d to 9de77ec Compare November 5, 2021 12:01

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 5, 2021

afrittoli force-pushed the issues/4358 branch 2 times, most recently from 1ae95c5 to 15732f2 Compare November 5, 2021 14:55

afrittoli changed the title ~~WIP Use kmeta.ChildName for child resources~~ Use kmeta.ChildName for child resources Nov 5, 2021

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2021

tekton-robot requested review from mattmoor and vdemeester November 5, 2021 15:31

afrittoli force-pushed the issues/4358 branch from 15732f2 to 1d1a365 Compare November 5, 2021 15:33

afrittoli commented Nov 5, 2021

View reviewed changes

pkg/pod/pod.go Show resolved Hide resolved

afrittoli force-pushed the issues/4358 branch from 1d1a365 to fa56d38 Compare November 5, 2021 15:41

mattmoor reviewed Nov 5, 2021

View reviewed changes

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go Show resolved Hide resolved

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2021

afrittoli added this to the Pipelines v0.30 milestone Nov 8, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2021

jerop reviewed Nov 9, 2021

View reviewed changes

This was referenced Nov 10, 2021

Executing a Pipeline with a single Task results in multiple TaskRuns #3126

Closed

Tekton is duplicating task runs that make up a pipeline run #3557

Closed

afrittoli force-pushed the issues/4358 branch from d0b946a to 645cf5a Compare November 16, 2021 21:08

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2021

jerop reviewed Nov 16, 2021

View reviewed changes

pritidesai reviewed Nov 16, 2021

View reviewed changes

docs/pipelineruns.md Outdated Show resolved Hide resolved

pritidesai reviewed Nov 16, 2021

View reviewed changes

docs/pipelineruns.md Show resolved Hide resolved

pritidesai approved these changes Nov 16, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2021

afrittoli force-pushed the issues/4358 branch from 645cf5a to 82a43dc Compare November 17, 2021 11:57

vdemeester reviewed Nov 17, 2021

View reviewed changes

tekton-robot assigned vdemeester Nov 17, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021

tekton-robot merged commit 277dd9e into tektoncd:main Nov 17, 2021

ghost mentioned this pull request Nov 17, 2021

manual delete TaskRun, its ownerReference PipelineRun remains Running #4066

Closed

pritidesai mentioned this pull request Mar 11, 2022

Add CloudEvents for Run definitions #4659

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use kmeta.ChildName for child resources #4361

Use kmeta.ChildName for child resources #4361

afrittoli commented Nov 4, 2021 •

edited

Loading

tekton-robot commented Nov 4, 2021

imjasonh commented Nov 4, 2021

afrittoli commented Nov 4, 2021

afrittoli commented Nov 5, 2021

tekton-robot commented Nov 5, 2021

afrittoli commented Nov 5, 2021

afrittoli commented Nov 5, 2021

afrittoli commented Nov 5, 2021

tekton-robot commented Nov 5, 2021

tekton-robot commented Nov 5, 2021

afrittoli commented Nov 5, 2021

tekton-robot commented Nov 8, 2021

ghost commented Nov 8, 2021

ghost commented Nov 8, 2021

jerop left a comment

jerop Nov 9, 2021

afrittoli Nov 16, 2021

pritidesai commented Nov 10, 2021

pritidesai commented Nov 10, 2021

tekton-robot commented Nov 16, 2021

afrittoli commented Nov 16, 2021

jerop left a comment

pritidesai left a comment

tekton-robot commented Nov 16, 2021

tekton-robot commented Nov 17, 2021

vdemeester left a comment

Use kmeta.ChildName for child resources #4361

Use kmeta.ChildName for child resources #4361

Conversation

afrittoli commented Nov 4, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Nov 4, 2021

imjasonh commented Nov 4, 2021

afrittoli commented Nov 4, 2021

afrittoli commented Nov 5, 2021

tekton-robot commented Nov 5, 2021

afrittoli commented Nov 5, 2021

afrittoli commented Nov 5, 2021

afrittoli commented Nov 5, 2021

tekton-robot commented Nov 5, 2021

tekton-robot commented Nov 5, 2021

afrittoli commented Nov 5, 2021

tekton-robot commented Nov 8, 2021

ghost commented Nov 8, 2021

ghost commented Nov 8, 2021

jerop left a comment

Choose a reason for hiding this comment

jerop Nov 9, 2021

Choose a reason for hiding this comment

afrittoli Nov 16, 2021

Choose a reason for hiding this comment

pritidesai commented Nov 10, 2021

pritidesai commented Nov 10, 2021

tekton-robot commented Nov 16, 2021

afrittoli commented Nov 16, 2021

jerop left a comment

Choose a reason for hiding this comment

pritidesai left a comment

Choose a reason for hiding this comment

tekton-robot commented Nov 16, 2021

tekton-robot commented Nov 17, 2021

vdemeester left a comment

Choose a reason for hiding this comment

afrittoli commented Nov 4, 2021 •

edited

Loading