-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Drop the tracker. #4111
Drop the tracker. #4111
Conversation
The following is the coverage report on the affected files.
|
Tekton's use of the `tracker` is redundant with the informer events it already has configured, and the tracker lease the pipeline run controller is using is probably too small to actually be effective for long-running pipelines. The tracker is built around the premise that resyncs happen to "refresh" things, and so to be safe the tracker lease should be a multiple of the resync period. The pipeline controller is currently hardcoding `30m`, but the default resync period is `10h`, so anything taking longer than `30m` via the tracker will actually drop events. Fortunately, I believe that both usages of the tracker are wholly redundant with informer events on owned resources anyhow, so this change should just remove unneeded complexity. For additional background, the purpose of the tracked isn't to track owned resources, but rather to track resources referenced via `corev1.ObjectReference` (or equivalent). We use this in Knative to track changes to Addressable resources that some resource might be delivering events to (e.g. maybe the address changed, or it is no longer Ready).
Rebased this on #4110 which just landed. |
The following is the coverage report on the affected files.
|
/test check-pr-has-kind-label |
1 similar comment
/test check-pr-has-kind-label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me 😉
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Going back all the way to #757, this is the first reference of the tracker that I see, but it was in a different place in the code, and it does not seem to be applicable anymore - today a Pipeline is not marked as done until all associated TaskRun have finished execution in some way.
/lgtm
Changes
Tekton's use of the
tracker
is redundant with the informer events it already has configured, and the tracker lease the pipeline run controller is using is probably too small to actually be effective for long-running pipelines.The tracker is built around the premise that resyncs happen to "refresh" things, and so to be safe the tracker lease should be a multiple of the resync period. The pipeline controller is currently hardcoding
30m
, but the default resync period is10h
, so anything taking longer than30m
via the tracker will actually drop events. The controller also seems to track aTaskRun
with the name of thePipelineRun
, which also feels wrong... 🤔Fortunately, I believe that both usages of the tracker are wholly redundant with informer events on owned resources anyhow, so this change should just remove unneeded complexity.
For additional background, the purpose of the tracked isn't to track owned resources, but rather to track resources referenced via
corev1.ObjectReference
(or equivalent). We use this in Knative to track changes to Addressable resources that some resource might be delivering events to (e.g. maybe the address changed, or it is no longer Ready)./kind cleanup
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes
cc @dlorenc @imjasonh @vdemeester @afrittoli