Plain Pod gets deleted once admitted via ProvisioningRequest (DWS) #2239

vladikkuzn · 2024-05-20T16:03:17Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Ignores pods' tolerations during equality check for admitted workloads

Which issue(s) this PR fixes:

Fixes #2213

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Pod Integration: Prevent Pod from being deleted when admitted via ProvisioningRequest that has pod updates on tolerations

k8s-ci-robot · 2024-05-20T16:03:19Z

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

vladikkuzn · 2024-05-20T16:03:26Z

/assign

netlify · 2024-05-20T16:03:36Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	`39571d0`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/664b74474d08780008c94fb8
😎 Deploy Preview	https://deploy-preview-2239--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.

vladikkuzn · 2024-05-20T16:03:36Z

/cc @trasc @alculquicondor

trasc · 2024-05-20T16:10:41Z

/assign

trasc · 2024-05-20T16:11:18Z

/test all

trasc

/lgtm

k8s-ci-robot · 2024-05-21T05:20:35Z

LGTM label has been added.

Git tree hash: 5e8cbb0f5bedafeb681fe1d3b14d0f5068185dee

mimowo · 2024-05-21T08:10:22Z

pkg/util/equality/podset.go

-func comparePodTemplate(a, b *corev1.PodSpec) bool {
-	if !equality.Semantic.DeepEqual(a.Tolerations, b.Tolerations) {
+func comparePodTemplate(a, b *corev1.PodSpec, ignoreTolerations bool) bool {
+	if !ignoreTolerations && !equality.Semantic.DeepEqual(a.Tolerations, b.Tolerations) {


Currently this PR only ignores the equality check on tolerations, while the comment indicates we should also relax the validation for nodeSelectors. How confident we are tolerations are enough?
IIUC the autoscaling.gke.io/provisioning-request nodeSelector can be added as well based on the annotation.

The node selectors are not part of the equivalency checks.

Got it, thanks for clarifying. Do we have this covered in a unit / integration test?

It's not ignored, because it doesn't participate in comparison as of now

I would just like to have a test to make sure this is the case, to prevent future regressions, since this is an important scenario.

Anyway we can probably defer this to a follow up manual testing

mimowo · 2024-05-21T09:44:08Z

pkg/controller/jobframework/reconciler.go

@@ -745,17 +745,17 @@ func equivalentToWorkload(ctx context.Context, c client.Client, job GenericJob,
 	jobPodSets := clearMinCountsIfFeatureDisabled(job.PodSets())

 	if runningPodSets := expectedRunningPodSets(ctx, c, wl); runningPodSets != nil {
-		if equality.ComparePodSetSlices(jobPodSets, runningPodSets) {
+		if equality.ComparePodSetSlices(jobPodSets, runningPodSets, workload.IsAdmitted(wl)) {


This ignoring of tolerations is done independently of the framework, but there is no issue with jobs, just with pods. I'm wondering if we should make the fix more specific. WDYT @alculquicondor ?

On one hand the relaxation of the check isn't necessary for Jobs.

On the other it seems that scoping the fix would be more involving because we would need to pass this information, the options I see:

add a new param to FindMatchingWorkloads

add a new interface like CustomEquivalenceConfigJob with a function like ignoreTolerations() bool

None of these is appealing, especially since we will not need the custom options when the proper fix is implemented.

I'm ok with this for now. But can we use the condition status of QuotaReserved, instead of Admitted?

I don't think it makes any difference, the toleration are only changing after the pods are unsuspended.

sg, thanks for the reminder.

mimowo · 2024-05-21T10:42:58Z

pkg/controller/jobs/job/job_controller_test.go

 			wantJob: *baseJobWrapper.Clone().Toleration(corev1.Toleration{
-				Key:      "tolerationkey1",
+				Key:      "tolerationkey2",


Please make sure we have an analogous test (either by adding or finding an existing) which demonstrate that node selectors are not part of the equality check.

alculquicondor · 2024-05-21T21:44:50Z

pkg/controller/jobframework/reconciler.go

@@ -745,17 +745,17 @@ func equivalentToWorkload(ctx context.Context, c client.Client, job GenericJob,
 	jobPodSets := clearMinCountsIfFeatureDisabled(job.PodSets())

 	if runningPodSets := expectedRunningPodSets(ctx, c, wl); runningPodSets != nil {
-		if equality.ComparePodSetSlices(jobPodSets, runningPodSets) {
+		if equality.ComparePodSetSlices(jobPodSets, runningPodSets, workload.IsAdmitted(wl)) {


I'm ok with this for now. But can we use the condition status of QuotaReserved, instead of Admitted?

alculquicondor · 2024-05-21T21:48:55Z

pkg/util/equality/podset_test.go

 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			got := ComparePodSetSlices(tc.a, tc.b)
+			got := ComparePodSetSlices(tc.a, tc.b, false)


needs a unit test for true too

alculquicondor · 2024-05-21T21:49:23Z

/approve
/hold for additional test

k8s-ci-robot · 2024-05-21T21:49:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, vladikkuzn

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:

~~pkg/controller/OWNERS~~ [alculquicondor]
~~pkg/util/OWNERS~~ [alculquicondor]

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

trasc · 2024-05-22T05:53:18Z

/test pull-kueue-test-integration-main

alculquicondor · 2024-05-22T18:01:29Z

we can leave the additional test for a follow up.

/hold cancel

alculquicondor · 2024-05-22T18:01:50Z

/cherry-pick release-0.6

k8s-infra-cherrypick-robot · 2024-05-22T18:01:52Z

@alculquicondor: once the present PR merges, I will cherry-pick it on top of release-0.6 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.6

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.

k8s-infra-cherrypick-robot · 2024-05-22T18:23:35Z

@alculquicondor: #2239 failed to apply on top of branch "release-0.6":

Applying: Plain Pod gets deleted once admitted via ProvisioningRequest (DWS)
Using index info to reconstruct a base tree...
M	pkg/controller/jobframework/reconciler.go
M	pkg/controller/jobs/job/job_controller_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/controller/jobs/job/job_controller_test.go
CONFLICT (content): Merge conflict in pkg/controller/jobs/job/job_controller_test.go
Auto-merging pkg/controller/jobframework/reconciler.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Plain Pod gets deleted once admitted via ProvisioningRequest (DWS)
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.6

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.

* Test for ignoreTolerations

* Test for ignoreTolerations * Add node selector to test to make sure it's not a part of equality check

* Add separate unit test for node selectors

* Rename wantEqual -> wantEquivalent

* Follow-up of #2239 * Test for ignoreTolerations * Add node selector to test to make sure it's not a part of equality check * Follow-up of #2239 * Add separate unit test for node selectors * Follow-up of #2239 * Rename wantEqual -> wantEquivalent

alculquicondor · 2024-05-27T21:40:10Z

/release-note-edit

Prevent Pod from being deleted when admitted via ProvisioningRequest that has pod updates on tolerations

alculquicondor · 2024-05-30T14:30:16Z

/release-note-edit

Pod Integration: Prevent Pod from being deleted when admitted via ProvisioningRequest that has pod updates on tolerations

Plain Pod gets deleted once admitted via ProvisioningRequest (DWS)

39571d0

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 20, 2024

k8s-ci-robot requested review from alculquicondor and mimowo May 20, 2024 16:03

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 20, 2024

k8s-ci-robot assigned vladikkuzn May 20, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 20, 2024

k8s-ci-robot requested a review from trasc May 20, 2024 16:03

k8s-ci-robot assigned trasc May 20, 2024

trasc reviewed May 21, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2024

vladikkuzn marked this pull request as ready for review May 21, 2024 07:03

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2024

k8s-ci-robot requested a review from kerthcet May 21, 2024 07:03

mimowo mentioned this pull request May 21, 2024

[kueuectl] Flaky test "Should stop a ClusterQueue Stop a ClusterQueue and drain workloads" #2245

Closed

mimowo reviewed May 21, 2024

View reviewed changes

alculquicondor reviewed May 21, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 22, 2024

k8s-ci-robot merged commit 9c867c9 into kubernetes-sigs:main May 22, 2024
15 checks passed

k8s-ci-robot added this to the v0.7 milestone May 22, 2024

vladikkuzn added a commit to epam/kubernetes-kueue that referenced this pull request May 22, 2024

Follow-up of kubernetes-sigs#2239

3d8424c

* Test for ignoreTolerations

vladikkuzn mentioned this pull request May 22, 2024

Follow-up of #2239 #2261

Merged

vladikkuzn deleted the Plain-Pod-gets-deleted branch May 22, 2024 18:27

vladikkuzn mentioned this pull request May 22, 2024

Automated cherry pick of #2239: Plain Pod gets deleted once admitted via ProvisioningRequest #2262

Merged

vladikkuzn added a commit to epam/kubernetes-kueue that referenced this pull request May 22, 2024

Follow-up of kubernetes-sigs#2239

ab92647

* Test for ignoreTolerations

vladikkuzn added a commit to epam/kubernetes-kueue that referenced this pull request May 22, 2024

Follow-up of kubernetes-sigs#2239

02d3f9b

* Test for ignoreTolerations * Add node selector to test to make sure it's not a part of equality check

vladikkuzn added a commit to epam/kubernetes-kueue that referenced this pull request May 23, 2024

Follow-up of kubernetes-sigs#2239

8e4e8ee

* Test for ignoreTolerations * Add node selector to test to make sure it's not a part of equality check

vladikkuzn added a commit to epam/kubernetes-kueue that referenced this pull request May 23, 2024

Follow-up of kubernetes-sigs#2239

aca2622

* Test for ignoreTolerations * Add node selector to test to make sure it's not a part of equality check

vladikkuzn added a commit to epam/kubernetes-kueue that referenced this pull request May 23, 2024

Follow-up of kubernetes-sigs#2239

1a15838

* Add separate unit test for node selectors

vladikkuzn added a commit to epam/kubernetes-kueue that referenced this pull request May 23, 2024

Follow-up of kubernetes-sigs#2239

bdf88db

* Add separate unit test for node selectors

vladikkuzn added a commit to epam/kubernetes-kueue that referenced this pull request May 23, 2024

Follow-up of kubernetes-sigs#2239

c6e27c1

* Rename wantEqual -> wantEquivalent

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plain Pod gets deleted once admitted via ProvisioningRequest (DWS) #2239

Plain Pod gets deleted once admitted via ProvisioningRequest (DWS) #2239

vladikkuzn commented May 20, 2024 •

edited by k8s-ci-robot

Loading

k8s-ci-robot commented May 20, 2024

vladikkuzn commented May 20, 2024

netlify bot commented May 20, 2024 •

edited

Loading

vladikkuzn commented May 20, 2024

trasc commented May 20, 2024

trasc commented May 20, 2024

trasc left a comment

k8s-ci-robot commented May 21, 2024

mimowo May 21, 2024 •

edited

Loading

trasc May 21, 2024

mimowo May 21, 2024 •

edited

Loading

vladikkuzn May 21, 2024

mimowo May 21, 2024

mimowo May 21, 2024

mimowo May 21, 2024

mimowo May 21, 2024

alculquicondor May 21, 2024

trasc May 22, 2024

alculquicondor May 22, 2024

mimowo May 21, 2024

alculquicondor May 21, 2024

alculquicondor May 21, 2024

alculquicondor commented May 21, 2024

k8s-ci-robot commented May 21, 2024

trasc commented May 22, 2024

alculquicondor commented May 22, 2024

alculquicondor commented May 22, 2024

k8s-infra-cherrypick-robot commented May 22, 2024

k8s-infra-cherrypick-robot commented May 22, 2024

alculquicondor commented May 27, 2024

alculquicondor commented May 30, 2024

Plain Pod gets deleted once admitted via ProvisioningRequest (DWS) #2239

Plain Pod gets deleted once admitted via ProvisioningRequest (DWS) #2239

Conversation

vladikkuzn commented May 20, 2024 • edited by k8s-ci-robot Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented May 20, 2024

vladikkuzn commented May 20, 2024

netlify bot commented May 20, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue ready!

vladikkuzn commented May 20, 2024

trasc commented May 20, 2024

trasc commented May 20, 2024

trasc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 21, 2024

mimowo May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented May 21, 2024

k8s-ci-robot commented May 21, 2024

trasc commented May 22, 2024

alculquicondor commented May 22, 2024

alculquicondor commented May 22, 2024

k8s-infra-cherrypick-robot commented May 22, 2024

k8s-infra-cherrypick-robot commented May 22, 2024

alculquicondor commented May 27, 2024

alculquicondor commented May 30, 2024

vladikkuzn commented May 20, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented May 20, 2024 •

edited

Loading

mimowo May 21, 2024 •

edited

Loading

mimowo May 21, 2024 •

edited

Loading