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 ?

[trasc]

/uncc

[tenzen-y]

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 ?

Yes, performance was my primary concern due to requeue objects to reconcilers.
So, guarding the strict patch by the RemoveFinalizersWithStrictPatch would be worth it.
Additionally, I would propose adding the kueue performance testings metrics to the feature graduation criteria. For example, we want to restrict the graduation like If the performance metrics are not decreased by 5% or something else, we can graduate it to GA.

@mimowo WDYT?

[mimowo]

For example, we want to restrict the graduation like If the performance metrics are not decreased by 5% or something else, we can graduate it to GA.

Sounds reasonable, I'm just wondering how much insight we get by artificial testing, but the scale requirement wouldn't be clear to me. Some users will use it at a large scale, and not sure how small scale testing will be representative. I suppose we would need to have at least 10000 pods. WDYT @tenzen-y @mykysha ?

Alternative (or along with that) we could enable it by default and wait 3 releases with graduation for the feedback from users.

[mykysha]

Both approaches sound good to me. I believe the user-feedback approach would be more informative, however making some performance metering alongside definitely wouldn't hurt either

[mimowo]

I think we will not need a KEP process for that, but let me check if @tenzen-y is ok with the plan:

• introduce the feature-gate in Beta, and add TODO with the graduation criteria to stable with a link to the issue "Performance testing for the impact of RemoveFinalizersWithStrictPatch". In the issue propose to test it manually.
• also add a comment to graduate in 0.13 or later to await for the user feedback while the gate is enabled

[tenzen-y]

Some users will use it at a large scale, and not sure how small scale testing will be representative. I suppose we would need to have at least 10000 pods. WDYT @tenzen-y @mykysha ?

That makes sense. We should verify the performance issue in the large env to represent issue obviously.
So, if it's challenging to simulate the situation by our performance testings, I guess that we can simulate it by other tools like KWOK manually.

I think we will not need a KEP process for that, but let me check if @tenzen-y is ok with the plan:

introduce the feature-gate in Beta, and add TODO with the graduation criteria to stable with a link to the issue "Performance testing for the impact of RemoveFinalizersWithStrictPatch". In the issue propose to test it manually.
also add a comment to graduate in 0.13 or later to await for the user feedback while the gate is enabled

I'm ok without KEP. But at least, could we summarize graduation criteria and background in the dedicated issue?

[mimowo]

So, if it's challenging to simulate the situation by our performance testings, I guess that we can simulate it by other tools like KWOK manually.

Right, I think it might be tricky for automated testing, and will consume our build time. So, I'm leaning to just a manual experiment, either in a real cluster k8s or kwok. I think the scale of 10000 pods would be enough, they don't need to be running at the same time. Please also confirm how many of the errors we got in that case.

[mimowo]

But at least, could we summarize graduation criteria and background in the dedicated issue?

SGTM, We can add TODO(# issue number) comment. @mykysha please open the issue and update the PR accordingly.

[k8s-ci-robot]

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