Enable Server-Side Apply for finalizers management#1322
Conversation
|
@enarha: The label(s) DetailsIn response to this:
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. |
tekton-robot
added
the
release-note
tekton-robot
added
the
size/S
|
/kind misc |
aThorp96
left a comment
aThorp96
left a comment
There was a problem hiding this comment.
/approve
Before merging, are there any tradeoffs to using serversideapply here? I remember the discussion that led to exploring serversideapply, but I don't remember if there were any drawbacks discussed
| SkipStatusUpdates: true, | ||
| ConfigStore: configStore, | ||
| FinalizerName: "results.tekton.dev/pipelinerun", | ||
| UseServerSideApplyForFinalizers: true, |
There was a problem hiding this comment.
Should this be a feature flag or is it safe/stable enough to hardcode as enabled?
There was a problem hiding this comment.
I contributed that functionality to Knative, but it's an opt-in feature, so not sure really how widely it's used. IMO that's too low level stuff and not really related to Tekton or the Results functionality, to expose to users to configure and only makes the configuration even more complex. With that said, on the project level we can take more cautious approach and only enable it to a single component before enabling it to all. The real benefit though will come when all components/controllers acting on the same objects enable it.
The only risk IMO is the transition from merge patch to SSA for the in-flight objects. I was sure with SSA this works (SSA will update the object unless there is a clash of ownership - another SSA manager updated the same field), but I'm testing this scenario now and it looks like it does not. I'm looking into that now, thus put the PR on hold.
There was a problem hiding this comment.
@aThorp96 to add just one more thing, we already use SSA for managing the annotations. That was done almost a year ago. Now we switch the finalizers management. So in terms of "technology", we already use SSA for some time now.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aThorp96 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/hold |
tekton-robot
added
the
do-not-merge/hold
enarha
force-pushed
the
switch-finalizers-ssa
branch
from
65bd2bd to
ac346fe
Compare
tekton-robot
added
size/L
Leverages functionality added here: knative/pkg#3275 Options documented here: https://github.com/knative/pkg/blob/main/controller/options.go#L31-L40 Added migration cleanup process, for inflight objects caught between the old version (using merge patch) and the new version (using SSA patch). It could be removed once all supported versions using merge patch become EOL. That code add minimal overhead and no extra API calls. Assisted-by: Claude Code
enarha
force-pushed
the
switch-finalizers-ssa
branch
from
ac346fe to
80f391d
Compare
3 participants
Leverages functionality added here:
knative/pkg#3275
Options documented here:
https://github.com/knative/pkg/blob/main/controller/options.go#L31-L40
Added migration cleanup process, for inflight objects caught between the old version (using merge patch) and the new version (using SSA patch). It could be removed once all supported versions using merge patch become EOL. That code add minimal overhead and no extra API calls.
Assisted-by: Claude Code
Changes
/kind enhancement
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you review them:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes