Add support for Server-Side Apply finalizer management by enarha · Pull Request #3275 · knative/pkg

enarha · 2025-10-18T20:55:12Z

This improves performance and load on the k8s API by decreasing the number of re-tries, especially on objects managed by multiple controllers. The feature is opt-in and the current use of merge patch is still the default behavior.
We introduce three controller options:

UseServerSideApplyForFinalizers used to enable that functionality.
FinalizerFieldManager to specify finalizer field manager, required when SSA is enabled.
ForceApplyFinalizers to set the force option on the apply patch (default disabled).

Changes

🎁 Add support for Server-Side Apply finalizer management
This improves performance and load on the k8s API by decreasing the number of re-tries, especially on objects managed by multiple controllers. The feature is opt-in and the current use of merge patch is still the default behavior.
We introduce three controller options:
UseServerSideApplyForFinalizers used to enable that functionality.
FinalizerFieldManager to specify finalizer field manager, required when SSA is enabled.
ForceApplyFinalizers to set the force option on the apply patch (default disabled).

/kind api-change

Fixes #2128

Release Note

Added support Server-Side Apply finalizer management. The feature is opt-in and the current use of merge patch is still the default behavior. Here is an example of controller options which enable and configure that functionality:

controller.Options{
    FinalizerName: "my-finalizer",
    UseServerSideApplyForFinalizers: true,
    FinalizerFieldManager: "my-finalizer-manager"
}

`UseServerSideApplyForFinalizers` enables the use Server-Side Apply for the finalizer management. If enabled, `FinalizerFieldManager` must also be provided. Use unique finalizer name and finalizer field manager to avoid conflicts. If you need to force the finalizer update, you can also add the `ForceApplyFinalizers` option and set it to `true`. That's usually not a good idea and different controllers should use different finalizer name and field manager.

IMPORTANT: 
If your controller is managing other fields using Server-Side Apply as part of its reconciliation loop (e.g. annotations), make sure you are using different filed manager for each (e.g. "my-controller/finalizers", "my-controller/annotations") to avoid accidental removal.
When switching from Merge Patch strategy to Server-Side Apply, objects caught in flight (existing on the cluster during transition) will need extra attention. Finalizer set using Merge Patch can't be removed using Server-Side Apply because filed manager (used with Server-Side Apply) does not own that field. In that case the finalizer must be removed manually.

Docs

linux-foundation-easycla · 2025-10-18T20:55:21Z

The committers listed above are authorized under a signed CLA.

✅ login: enarha / name: Emil Natan (0e36a17)

knative-prow · 2025-10-18T20:55:22Z

Welcome @enarha! It looks like this is your first PR to knative/pkg 🎉

knative-prow · 2025-10-18T20:55:22Z

Hi @enarha. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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-sigs/prow repository.

cardil · 2025-10-21T08:21:58Z

/assign @dprotaso

dsimansk · 2025-10-21T08:28:33Z

/ok-to-test

codecov · 2025-10-21T08:34:38Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.58%. Comparing base (aadc6f6) to head (0e36a17).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3275      +/-   ##
==========================================
- Coverage   74.93%   74.58%   -0.36%     
==========================================
  Files         188      188              
  Lines       10263     8184    -2079     
==========================================
- Hits         7691     6104    -1587     
+ Misses       2332     1840     -492     
  Partials      240      240

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dprotaso · 2025-10-21T13:09:57Z

/hold

pkg release-1.20 branch was cut today. Let's revisit this after the 1.20 release next week

enarha · 2025-10-29T11:56:55Z

Can this PR get some attention please. I'll be happy to elaborate if you have any questions. Thank you.

nader-ziada

added some comments in the updateFinalizersFilteredServerSideApply func which is repeated in a bunch of places as far as I can see

nader-ziada · 2025-11-03T16:16:59Z

+		updated, err := patcher.Patch(ctx, resource.Name, types.ApplyPatchType, patch, patchOpts)
+		if err != nil {
+			r.Recorder.Eventf(resource, corev1.EventTypeWarning, "FinalizerUpdateFailed",
+				"Failed to remove finalizer for %q via server-side apply: %v", resource.Name, err)


Should use %w for error wrapping

Here we do not return the error, just print it, so the simpler %v makes more sense and it is also consistent with the original code (merge patch). I updated the case where we return the error to use %w which allows unwrapping. If you still think I should update this one, I'll do.

nader-ziada · 2025-11-03T16:17:14Z

+		// This effectively removes our finalizer while preserving others.
+		gvks, _, err := scheme.Scheme.ObjectKinds(resource)
+		if err != nil || len(gvks) == 0 {
+			return resource, fmt.Errorf("failed to determine GVK for resource: %v", err)


Should use %w for error wrapping

nader-ziada · 2025-11-03T16:17:51Z

+			return resource, nil
+		}
+		// Apply configuration with only our finalizer to add it.
+		gvks, _, err := scheme.Scheme.ObjectKinds(resource)


This GVK lookup logic is duplicated below in the else condition

Indeed it has. It could be called before the if clause, but then it should be called on each reconciliation cycle. Instead we call it just twice in the lifetime of the object, once to set it and once to remove it, while the reconciliation loop is called 10-20 or even more times, at least on the controllers I work on. That's why I think less calls is worth the duplication, but I'll be happy to change it you think otherwise.

Nevermind, I moved it after the else block.

nader-ziada · 2025-11-03T16:18:01Z

+		// Apply configuration with only our finalizer to add it.
+		gvks, _, err := scheme.Scheme.ObjectKinds(resource)
+		if err != nil || len(gvks) == 0 {
+			return resource, fmt.Errorf("failed to determine GVK for resource: %v", err)


Should use %w for error wrapping

Agree, that makes sense as we return error.

nader-ziada · 2025-11-03T16:19:01Z

+			},
+		}
+
+		patch, err := json.Marshal(applyConfig)


This patch operation logic is duplicated below as well

Indeed. It could be moved after the else clause with the only penalty of having less accurate log message "failed to add/remove finalizer" and "Added/removed finalizer" compared to "failed to update finalizer" and "updated finalizer". Personally I prefer the former, but I'll update so it's also inline with the merge patch method.

Updated. Much cleaner now.

nader-ziada · 2025-11-03T16:21:23Z

+
+		patchOpts := metav1.PatchOptions{
+			FieldManager: r.finalizerFieldManager,
+			Force:        &r.forceApplyFinalizers,


Is force necessary here?

By default it's not set as the value is set to false. We expose the ForceApplyFinalizers controller option though, so the controller implementation can set that to true. Force apply is often discussed in the context of server-side apply, so I decided to provide that flexibility to the controller implementation.

dprotaso · 2025-11-03T17:40:17Z

/hold cancel

dprotaso · 2025-11-03T17:56:33Z

Testing this downstream with - knative/serving#16217

dprotaso · 2025-11-03T21:18:50Z

Downstream test ran successfully. I'm good to merge once Nader's feedback is addressed.

This improves performance and load on the k8s API by decreasing the number of re-tries, especially on objects managed by multiple controllers. The feature is opt-in and the current use of merge patch is still the default behavior. We introduce three controller options: - UseServerSideApplyForFinalizers used to enable that functionality. - FinalizerFieldManager to specify finalizer field manager, required when SSA is enabled. - ForceApplyFinalizers to set the force option on the apply patch (default disabled).

nader-ziada · 2025-11-04T13:41:47Z

/lgtm

nader-ziada · 2025-11-04T13:42:35Z

thanks for the updates @enarha

@dprotaso good from my side, you will need to approve it if you are good

dprotaso · 2025-11-04T14:13:11Z

/approve

knative-prow · 2025-11-04T14:13:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, enarha

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dprotaso]

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

Leverages functionality added here: knative/pkg#3275 Options documented here: https://github.com/knative/pkg/blob/main/controller/options.go#L31-L40

knative-prow Bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 18, 2025

knative-prow Bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2025

knative-prow Bot requested review from Leo6Leo and skonto October 18, 2025 20:55

knative-prow Bot assigned dprotaso Oct 21, 2025

knative-prow Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 21, 2025

enarha force-pushed the support-ssa-finalizers branch from 5dc1f6b to 363a01d Compare October 21, 2025 08:58

knative-prow Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2025

nader-ziada reviewed Nov 3, 2025

View reviewed changes

knative-prow Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2025

dprotaso mentioned this pull request Nov 3, 2025

[wip] test ssa finalizer knative/serving#16217

Closed

enarha force-pushed the support-ssa-finalizers branch from 363a01d to 0e36a17 Compare November 4, 2025 12:57

knative-prow Bot assigned nader-ziada Nov 4, 2025

knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2025

knative-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2025

knative-prow Bot merged commit 04fdd0b into knative:main Nov 4, 2025
36 checks passed

enarha mentioned this pull request Dec 26, 2025

feat(metrics): Migrate from OpenCensus to OpenTelemetry tektoncd/pipeline#9043

Merged

8 tasks

enarha mentioned this pull request Apr 16, 2026

Enable Server-Side Apply for finalizers management tektoncd/chains#1635

Merged

6 tasks

Conversation

enarha commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

linux-foundation-easycla Bot commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow Bot commented Oct 18, 2025

Uh oh!

knative-prow Bot commented Oct 18, 2025

Uh oh!

cardil commented Oct 21, 2025

Uh oh!

dsimansk commented Oct 21, 2025

Uh oh!

codecov Bot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dprotaso commented Oct 21, 2025

Uh oh!

enarha commented Oct 29, 2025

Uh oh!

nader-ziada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Nov 3, 2025

Uh oh!

dprotaso commented Nov 3, 2025

Uh oh!

dprotaso commented Nov 3, 2025

Uh oh!

nader-ziada commented Nov 4, 2025

Uh oh!

nader-ziada commented Nov 4, 2025

Uh oh!

dprotaso commented Nov 4, 2025

Uh oh!

knative-prow Bot commented Nov 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

enarha commented Oct 18, 2025 •

edited

Loading

linux-foundation-easycla Bot commented Oct 18, 2025 •

edited

Loading

codecov Bot commented Oct 21, 2025 •

edited

Loading