Remove Updating labels and annotations… #6127

vdemeester · 2023-02-08T15:28:18Z

Changes

Labels and Annotations on PipelineRun and TaskRun won't be mutated by the controller anymore. Usually, a controller doesn't modify the metadata of the object it reconcile.

This effectively remove the propagation of annotations and labels from Task to TaskRun, and Pipeline to PipelineRun.

This is an "attempt" to fix #5146 but it is mainly opened to start some discussion around it. Mainly the following :

One feature that we "want" is that labels and annotations from a Task and a TaskRun are both inherited by the Pod. Same goes for PipelineRun. Prior to this change, we get away with it by merging it at the TaskRun or PipelineRun level — which cause issues in the past (and still might), like #5297.
Even if this PR propose to remove this "feature", we still want the same behavior (aka a Pod gets its labels from both the Task and the TaskRun). Because we are only "caching" the spec of the Task in TaskRun.status (and same goes for PipelineRun), we would have to query the metadata for the Task each time. Using informers, it wouldn't cost that much, but we would run the same possible race as we did before and that's the reason we use TaskRun.status and PipelineRun.status as source of truth for the spec in all reconciler loop for a given object.

So the question is the following : Where should we store the Task (and Pipeline for PipelineRun) labels and annotations in a TaskRun object ? Because we have already release v1, we can only do "additive" changes, so, most likely, the only way would be to have a taskMetadata field in the status in addition to taskSpec.
But I am curious if anyone has any other ideas 👼🏼.

/cc @imjasonh @afrittoli @abayer @lbernick @jerop @chuangw6

Signed-off-by: Vincent Demeester vdemeest@redhat.com

Submitter Checklist

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

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

imjasonh

Love it!

/lgtm

imjasonh · 2023-02-08T15:35:01Z

pkg/reconciler/pipelinerun/pipelinerun.go

-
-		// Propagate annotations from Pipeline to PipelineRun. PipelineRun annotations take precedences over Pipeline.
-		pr.ObjectMeta.Annotations = kmap.Union(meta.Annotations, pr.ObjectMeta.Annotations)
+		// // Propagate labels from Pipeline to PipelineRun. PipelineRun labels take precedences over Pipeline.


These can just be dropped?

From there yes, overall no as written in the PR description.

Even if this PR propose to remove this "feature", we still want the same behavior (aka a Pod gets its labels from both the Task and the TaskRun). Because we are only "caching" the spec of the Task in TaskRun.status (and same goes for PipelineRun), we would have to query the metadata for the Task each time. Using informers, it wouldn't cost that much, but we would run the same possible race as we did before and that's the reason we use TaskRun.status and PipelineRun.status as source of truth for the spec in all reconciler loop for a given object.

So the question is the following : Where should we store the Task (and Pipeline for PipelineRun) labels and annotations in a TaskRun object ? Because we have already release v1, we can only do "additive" changes, so, most likely, the only way would be to have a taskMetadata field in the status in addition to taskSpec.
But I am curious if anyone has any other ideas 👼🏼.

lbernick · 2023-02-08T21:55:23Z

I'm curious why we can't separate out the labels/annotations propagation from the fix to #5146? i.e. if we just remove the call to update, and modify the labels of the TR/PR object passed to ReconcileKind, will knative update the object with the new metadata? Or does it only handle status updates?

Also want to better understand the race condition you're referring to. My understanding is that if a Task's labels change, we will re-reconcile the TaskRun, and update the TaskRun with the Task's new labels (and likewise for PipelineRuns). The issue described in #5297 was the PipelineRun controller replacing labels, but we've updated it to merge labels. Can you give an example scenario where the race condition you're describing occurs?

vdemeester · 2023-02-09T15:05:19Z

I'm curious why we can't separate out the labels/annotations propagation from the fix to #5146? i.e. if we just remove the call to update, and modify the labels of the TR/PR object passed to ReconcileKind, will knative update the object with the new metadata? Or does it only handle status updates?

@lbernick As you "hinted" in one of your question, this is because it only update the status 👼🏼. It doesn't update anything else (metadata or spec). The take on updating only the status is that, users are supposed to interact with metadata and spec and controllers with status (separation of concern, …). If it did, that PR would be green already (and the code I'm deleting here wouldn't have been required in the first place).

Also want to better understand the race condition you're referring to. My understanding is that if a Task's labels change, we will re-reconcile the TaskRun, and update the TaskRun with the Task's new labels (and likewise for PipelineRuns). The issue described in #5297 was the PipelineRun controller replacing labels, but we've updated it to merge labels. Can you give an example scenario where the race condition you're describing occurs?

If a Task label's change, it changes, nothing get's "reconciled" whatsoever. #5297 was not really "removing" things but overriding the content of the labels we "got" at the begining of the reconciler loop, even if, in the meantime, the PipelineRun itself was updated. But this is not the race I am talking about here.
The race I am talking about is if we choose "to query the metadata for the Task each time". If we decide to do that, we would be in the same situation as prior to use status.taskrunSpec as a source of truth. At time T, the Task would have labels "a, b and c", we start something and then we are in a new reconciler loop (new event), the labels could have been changed on the Task (now only "a and b"), and this could be seen as a race. It's less of a problem on metadata than on the spec, but still it could lead to confusing behaviour (if something acts on labels, …).

lbernick · 2023-02-10T13:02:07Z

@lbernick As you "hinted" in one of your question, this is because it only update the status 👼🏼. It doesn't update anything else (metadata or spec). The take on updating only the status is that, users are supposed to interact with metadata and spec and controllers with status (separation of concern, …). If it did, that PR would be green already (and the code I'm deleting here wouldn't have been required in the first place).

Makes sense! I understand why you wouldn't want a controller to update the object's spec but why would it be bad for the controller to update labels? @imjasonh is there a recommended alternative pattern to use?

If a Task label's change, it changes, nothing get's "reconciled" whatsoever.

I'm not sure why I said this 🤦‍♀️ Curious if label changes in general trigger re-reconciles though?

#5297 was not really "removing" things but overriding the content of the labels we "got" at the begining of the reconciler loop, even if, in the meantime, the PipelineRun itself was updated. But this is not the race I am talking about here. The race I am talking about is if we choose "to query the metadata for the Task each time". If we decide to do that, we would be in the same situation as prior to use status.taskrunSpec as a source of truth. At time T, the Task would have labels "a, b and c", we start something and then we are in a new reconciler loop (new event), the labels could have been changed on the Task (now only "a and b"), and this could be seen as a race. It's less of a problem on metadata than on the spec, but still it could lead to confusing behaviour (if something acts on labels, …).

Makes sense, thanks for the explanation! I'm not sure off the top of my head of any solutions other than adding status.taskMetadata or something along those lines but can think a bit more

vdemeester · 2023-02-10T15:08:02Z

I'm not sure why I said this Curious if label changes in general trigger re-reconciles though?

It would on the object(s) that are watched, so TaskRun or PipelineRun. There is no reconciler watching Task or Pipeline.

tekton-robot · 2023-05-22T23:23:25Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

tekton-robot · 2023-06-21T23:34:25Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

vdemeester · 2023-07-03T12:03:39Z

/remove-lifecycle rotten
Still need to rebase and rework a bit that one.. 😅

tekton-robot · 2023-08-07T14:56:25Z

New changes are detected. LGTM label has been removed.

tekton-robot · 2023-08-07T14:56:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from imjasonh after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

vdemeester · 2023-08-07T14:58:38Z

Updated this, aside from some tests to be rewritten, it should be ready for review.
This removes the extra update we do during reconcilation, which means we rely solely on knative/pkg for updating the object (see #5146) and removes one possible "update" call during the loop.

/kind enhancement

tekton-robot · 2023-08-07T14:58:40Z

@vdemeester: The label(s) kind/enhancement cannot be applied, because the repository doesn't have them.

In response to this:

Updated this, aside from some tests to be rewritten, it should be ready for review.
This removes the extra update we do during reconcilation, which means we rely solely on knative/pkg for updating the object (see #5146) and removes one possible "update" call during the loop.

/kind enhancement

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.

vdemeester · 2023-08-07T14:59:08Z

/kind bug

Labels and Annotations on PipelineRun and TaskRun won't be mutated by the controller anymore. Usually, a controller doesn't modify the metadata of the object it reconcile. This effectively remove the propagation of annotations and labels from Task to TaskRun, and Pipeline to PipelineRun. Instead, it stores the Task or Pipeline metadata in the corresponding status field (`Status.PipelineMeta` for PipelineRun, and `Status.TaskMeta` for TaskRun). Merging the two is done at the "lower" object creation (aka when creating the `TaskRun` from `PipelineRun` or when creating the `Pod` from `TaskRun`). Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

tekton-robot · 2023-08-07T16:06:12Z

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	92.5%	91.6%	-0.9
pkg/reconciler/taskrun/taskrun.go	85.3%	85.6%	0.3

tekton-robot · 2023-08-07T16:09:27Z

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

File	Old Coverage	New Coverage	Delta
pkg/pod/pod.go	92.5%	91.6%	-0.9
pkg/reconciler/taskrun/taskrun.go	85.3%	85.6%	0.3

tekton-robot · 2023-08-07T17:32:01Z

@vdemeester: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-tekton-pipeline-unit-tests	`c9831eb`	link	true	`/test tekton-pipeline-unit-tests`
pull-tekton-pipeline-build-tests	`c9831eb`	link	true	`/test pull-tekton-pipeline-build-tests`
pull-tekton-pipeline-beta-integration-tests	`c9831eb`	link	true	`/test pull-tekton-pipeline-beta-integration-tests`
pull-tekton-pipeline-integration-tests	`c9831eb`	link	true	`/test pull-tekton-pipeline-integration-tests`
pull-tekton-pipeline-alpha-integration-tests	`c9831eb`	link	true	`/test pull-tekton-pipeline-alpha-integration-tests`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

tekton-robot · 2023-10-03T15:35:51Z

@vdemeester: PR needs rebase.

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.

jimmyjones2 · 2024-05-22T19:23:35Z

Label propagation also causes issues with ArgoCD e.g
argoproj/argo-cd#5792 (comment)
argoproj/argo-cd#10982

tekton-robot requested a review from abayer February 8, 2023 15:28

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 8, 2023

tekton-robot requested review from afrittoli and chuangw6 February 8, 2023 15:28

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Feb 8, 2023

tekton-robot requested review from imjasonh, jerop and lbernick February 8, 2023 15:28

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 8, 2023

imjasonh reviewed Feb 8, 2023

View reviewed changes

tekton-robot assigned imjasonh Feb 8, 2023

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

vdemeester mentioned this pull request Feb 14, 2023

Skip last-applied-configuration annotation when merging them. #6161

Merged

7 tasks

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2023

tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 22, 2023

vdemeester mentioned this pull request May 23, 2023

Ignore tekton reserved annotations #6441

Merged

7 tasks

tekton-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 21, 2023

tekton-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jul 3, 2023

vdemeester added this to the Pipeline v1 milestone Jul 3, 2023

vdemeester force-pushed the 5146-remove-label-and-annotation-merging branch from 6852977 to 996f60f Compare August 7, 2023 14:56

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2023

tekton-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 Aug 7, 2023

vdemeester changed the title ~~wip: Remove Updating labels and annotations…~~ Remove Updating labels and annotations… Aug 7, 2023

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 Aug 7, 2023

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 7, 2023

vdemeester force-pushed the 5146-remove-label-and-annotation-merging branch from 996f60f to 9b4285b Compare August 7, 2023 15:04

vdemeester force-pushed the 5146-remove-label-and-annotation-merging branch from 9b4285b to c9831eb Compare August 7, 2023 15:59

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 7, 2023

jerop added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2023

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2023

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2023

vdemeester mentioned this pull request Nov 2, 2023

Pipelines is incorrectly removing Tekton Chains annotations #7291

Open

lcarva mentioned this pull request Dec 1, 2023

"rerun" of pipelineRun in OpenShift, doesn't clear annotations tektoncd/chains#1004

Open

afrittoli modified the milestones: Pipeline v1.0.0, Pipelines v0.59, Pipeline v0.60 Apr 30, 2024

imjasonh closed this May 23, 2024

vdemeester deleted the 5146-remove-label-and-annotation-merging branch July 3, 2024 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Updating labels and annotations… #6127

Remove Updating labels and annotations… #6127

vdemeester commented Feb 8, 2023 •

edited

Loading

imjasonh left a comment

imjasonh Feb 8, 2023

vdemeester Feb 8, 2023

lbernick commented Feb 8, 2023

vdemeester commented Feb 9, 2023 •

edited

Loading

lbernick commented Feb 10, 2023

vdemeester commented Feb 10, 2023

tekton-robot commented May 22, 2023

tekton-robot commented Jun 21, 2023

vdemeester commented Jul 3, 2023

tekton-robot commented Aug 7, 2023

tekton-robot commented Aug 7, 2023

vdemeester commented Aug 7, 2023

tekton-robot commented Aug 7, 2023

vdemeester commented Aug 7, 2023

tekton-robot commented Aug 7, 2023

tekton-robot commented Aug 7, 2023

tekton-robot commented Aug 7, 2023

tekton-robot commented Oct 3, 2023

jimmyjones2 commented May 22, 2024

Remove Updating labels and annotations… #6127

Remove Updating labels and annotations… #6127

Conversation

vdemeester commented Feb 8, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

imjasonh left a comment

Choose a reason for hiding this comment

imjasonh Feb 8, 2023

Choose a reason for hiding this comment

vdemeester Feb 8, 2023

Choose a reason for hiding this comment

lbernick commented Feb 8, 2023

vdemeester commented Feb 9, 2023 • edited Loading

lbernick commented Feb 10, 2023

vdemeester commented Feb 10, 2023

tekton-robot commented May 22, 2023

tekton-robot commented Jun 21, 2023

vdemeester commented Jul 3, 2023

tekton-robot commented Aug 7, 2023

tekton-robot commented Aug 7, 2023

vdemeester commented Aug 7, 2023

tekton-robot commented Aug 7, 2023

vdemeester commented Aug 7, 2023

tekton-robot commented Aug 7, 2023

tekton-robot commented Aug 7, 2023

tekton-robot commented Aug 7, 2023

tekton-robot commented Oct 3, 2023

jimmyjones2 commented May 22, 2024

vdemeester commented Feb 8, 2023 •

edited

Loading

vdemeester commented Feb 9, 2023 •

edited

Loading