fix: force strict mode for patch for safe concurrent writes

[mykysha]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Enforce strict mode in Patch operation to remove possible race conditions.

Which issue(s) this PR fixes:

Fixes #3899

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	7589f14
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/676eaf43f8bdaf00082f83b4
😎 Deploy Preview	https://deploy-preview-3912--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mykysha]

/test all

[mykysha]

/cc @mbobrovskyi

[mbobrovskyi]

/lgtm
Thanks!

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 8952d4f81acb656dc70e2e9ee998c643ab0a4228

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mbobrovskyi, mykysha
Once this PR has been reviewed and has the lgtm label, please assign tenzen-y for approval. For more information see the Kubernetes Code Review Process.

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

[mbobrovskyi]

cc: @troychiu

[tenzen-y]

AFAIK the nonstrict patch is the intended behavior.
Could you add test cases to prove the situation where the nonstrict patch removes non-owned finalizers (kueue.x-k8s. io/managed) from the Pod?

Additionally, what is the reason for enforcing the strict patch everywhere?

[mimowo]

AFAIK the nonstrict patch is the intended behavior.

I'm not sure about that actually, dropping finalizers successfully added by other controllers is not ideal.

Could you add test cases to prove the situation where the nonstrict patch removes non-owned finalizers (kueue.x-k8s. io/managed) from the Pod?

Race conditions like this might be tricky to test. How would you imagine the test? e2e test which repeats the operation multiple times, or rather a unit test?

Additionally, what is the reason for enforcing the strict patch everywhere?

RemoveFinalizer was the only use of non-strict mode, so it makes sense to drop the flag if no other place wants to use it.

However, I think there is some risk to use strict patches - it is the possible performance implication due to necessary re-tries. I don't expect it, because we anyway manage scheduling gates, and it wasn't a problem. However, for safety I would suggest to add a feature gate RemoveFinalizersWithStrictPatch which is beta and will graduate to GA in the future, but it will give us a safety mechanism for users who use Kueue at scale and the change could impact them. WDYT @tenzen-y ?