Fix live-updates for kubernetes Job getting stuck occasionally #3685

IrvingMg · 2024-11-28T17:31:13Z

What type of PR is this?

/kind bug
/kind flake

What this PR does / why we need it:

Adds a check when syncing jobs on multikueue to avoid trying to patch the spec before resuming the job.

Which issue(s) this PR fixes:

Fixes #3581

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a bug which occasionally prevented updates to the PodTemplate of the Job on the management cluster
when starting a Job (e.g. updating nodeSelectors), when using `MultiKueueBatchJobWithManagedBy` enabled.

netlify · 2024-11-28T17:31:30Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`05200ea`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67501d61592cff000815e0cf

mimowo · 2024-12-02T08:51:11Z

pkg/controller/jobs/job/job_multikueue_adapter.go

@@ -49,6 +50,12 @@ func (b *multikueueAdapter) SyncJob(ctx context.Context, localClient client.Clie
 		return err
 	}

+	log := ctrl.LoggerFrom(ctx)
+	if ptr.Deref(localJob.Spec.Suspend, false) { // Ensure the job is resumed before updating its status; otherwise, it will fail when patching the spec.


This changes the implementation - is this actually a bug? If so, we should add a release note describing the scenario that is fixed

The problem here is that remote job was unsuspended before local job. That's why after update status

kueue/pkg/controller/jobs/job/job_multikueue_adapter.go

Lines 67 to 72 in c3abf9c

if features.Enabled(features.MultiKueueBatchJobWithManagedBy) {

return clientutil.PatchStatus(ctx, localClient, &localJob, func() (bool, error) {

localJob.Status = remoteJob.Status

return true, nil

})

}

we can't update spec on jobframework

kueue/pkg/controller/jobframework/reconciler.go

Lines 478 to 480 in c3abf9c

err := r.startJob(ctx, job, object, wl)

if err != nil {

log.Error(err, "Unsuspending job")

.

Could you maybe explain a bit more what is the update that is failing trying to update in the spec?

Ah I think I know what you mean, k8s core blocks update of pod template if the job is running. And it determines that the job is running because status.startTime Is not bil

FYI: here and here are the relevant places in the core k8s which reject the spec update if the status is already updated (status.startTime != nil).

mimowo · 2024-12-02T08:52:38Z

Please describe what is the scenario when the test fails, could this impact end-to-end users of MultiKueue?

mbobrovskyi · 2024-12-02T15:14:31Z

/assign

mbobrovskyi

/cc @mszadkow

pkg/controller/jobs/job/job_multikueue_adapter.go

mbobrovskyi · 2024-12-03T08:40:21Z

pkg/controller/jobs/job/job_multikueue_adapter.go

@@ -49,6 +50,12 @@ func (b *multikueueAdapter) SyncJob(ctx context.Context, localClient client.Clie
 		return err
 	}

+	log := ctrl.LoggerFrom(ctx)
+	if ptr.Deref(localJob.Spec.Suspend, false) { // Ensure the job is resumed before updating its status; otherwise, it will fail when patching the spec.


It would be great to add a test case to verify skipping.

more tests! agree

This could be covered in integration test probably. right?

This bug seems hard to reproduce in a test case as it requires causing a failure in the reconciler of the local job while sync it with the remote one.

However, this bug was originally caught in multikueue e2e tests and after running them several times with this fix, the error didn't occur anymore.

This bug seems hard to reproduce in a test case as it requires causing a failure in the reconciler of the local job while sync it with the remote one.

I see, in that case I don't have a better test idea, maybe a unit test.

However, this bug was originally caught in multikueue e2e tests and after running them several times with this fix, the error didn't occur anymore.

Yes, this is covered, just flaky, so I'm ok to accept as is. Feel free to follow up with unit tests or extra tests when working on #3730

pkg/controller/jobs/job/job_multikueue_adapter.go

mimowo · 2024-12-03T09:08:21Z

Proposal:
/release-note-edit

Fix a bug which occasionally prevented updates to the PodTemplate of the Job on the management cluster
when starting a Job (e.g. updating nodeSelectors), when using `MultiKueueBatchJobWithManagedBy` enabled.

mimowo · 2024-12-03T09:10:44Z

IIUC this is not just a fix for flaky tests, but an important bugfix for the issue which could affect production systems.
I agree it would be great to also apply the same approach for other Job CRDs - they may or may not have similar
validation, but we also care about consistency. For other Jobs I'm ok with a follow up, if this makes this PR land before 0.9.2.

mbobrovskyi · 2024-12-03T11:15:50Z

@IrvingMg please update description with

Fixes #3581

IrvingMg · 2024-12-04T09:32:15Z

IIUC this is not just a fix for flaky tests, but an important bugfix for the issue which could affect production systems. I agree it would be great to also apply the same approach for other Job CRDs - they may or may not have similar validation, but we also care about consistency. For other Jobs I'm ok with a follow up, if this makes this PR land before 0.9.2.

Open #3730

mimowo · 2024-12-04T10:18:14Z

/lgtm
/approve
/retitle Fix live-updates for kubernetes Job getting stuck occasionally

k8s-ci-robot · 2024-12-04T10:18:21Z

LGTM label has been added.

Git tree hash: cc29b808ec1b919c9c640a1820f6ad354dfee0d6

k8s-ci-robot · 2024-12-04T10:18:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IrvingMg, mimowo

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~~ [mimowo]

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

mimowo · 2024-12-04T10:31:40Z

/cherry-pick release-0.9

k8s-infra-cherrypick-robot · 2024-12-04T10:32:18Z

@mimowo: new pull request created: #3731

In response to this:

/cherry-pick release-0.9

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-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. kind/flake Categorizes issue or PR as related to a flaky test. labels Nov 28, 2024

k8s-ci-robot requested review from mimowo and PBundyra November 28, 2024 17:31

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 28, 2024

IrvingMg changed the title ~~Fix flaky e2e test - Creating a multikueue admission check Should run a job on worker if admitted~~ Fix flaky - Creating a multikueue admission check Should run a job on worker if admitted Nov 28, 2024

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 2, 2024

mimowo reviewed Dec 2, 2024

View reviewed changes

IrvingMg marked this pull request as draft December 2, 2024 09:56

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

k8s-ci-robot assigned mbobrovskyi Dec 2, 2024

mbobrovskyi reviewed Dec 3, 2024

View reviewed changes

k8s-ci-robot requested a review from mszadkow December 3, 2024 08:40

mimowo reviewed Dec 3, 2024

View reviewed changes

pkg/controller/jobs/job/job_multikueue_adapter.go Outdated Show resolved Hide resolved

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 Dec 3, 2024

Fix bug to prevent updating PodTemplate on suspended jobs

05200ea

IrvingMg force-pushed the fix/flaky-creating-multikueue-admission-check branch from c3abf9c to 05200ea Compare December 4, 2024 09:14

IrvingMg mentioned this pull request Dec 4, 2024

[MultiKueue] Do not update Job status while the Job is suspended (for CRDs) #3730

Closed

IrvingMg marked this pull request as ready for review December 4, 2024 09:32

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 Dec 4, 2024

k8s-ci-robot requested a review from mbobrovskyi December 4, 2024 09:33

k8s-ci-robot changed the title ~~Fix flaky - Creating a multikueue admission check Should run a job on worker if admitted~~ Fix live-updates for kubernetes Job getting stuck occasionally Dec 4, 2024

k8s-ci-robot assigned mimowo Dec 4, 2024

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

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

k8s-ci-robot merged commit 8cc3157 into kubernetes-sigs:main Dec 4, 2024
16 checks passed

k8s-ci-robot added this to the v0.10 milestone Dec 4, 2024

mbobrovskyi deleted the fix/flaky-creating-multikueue-admission-check branch December 4, 2024 10:19

k8s-infra-cherrypick-robot mentioned this pull request Dec 4, 2024

[release-0.9] Fix live-updates for kubernetes Job getting stuck occasionally #3731

Merged

IrvingMg mentioned this pull request Jan 27, 2025

Prevent updating Job status on suspended jobs #4070

Merged

tenzen-y mentioned this pull request Feb 8, 2025

MultiKueue: Implement suspended Job status propagation UTs #4176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix live-updates for kubernetes Job getting stuck occasionally #3685

Fix live-updates for kubernetes Job getting stuck occasionally #3685

IrvingMg commented Nov 28, 2024 •

edited

Loading

netlify bot commented Nov 28, 2024 •

edited

Loading

mimowo Dec 2, 2024

mbobrovskyi Dec 2, 2024

mimowo Dec 2, 2024

mimowo Dec 2, 2024

mimowo Dec 3, 2024

mimowo commented Dec 2, 2024

mbobrovskyi commented Dec 2, 2024

mbobrovskyi left a comment

mbobrovskyi Dec 3, 2024

mszadkow Dec 4, 2024

mimowo Dec 4, 2024

IrvingMg Dec 4, 2024

mimowo Dec 4, 2024

mimowo commented Dec 3, 2024

mimowo commented Dec 3, 2024 •

edited

Loading

mbobrovskyi commented Dec 3, 2024

IrvingMg commented Dec 4, 2024

mimowo commented Dec 4, 2024

k8s-ci-robot commented Dec 4, 2024

k8s-ci-robot commented Dec 4, 2024

mimowo commented Dec 4, 2024

k8s-infra-cherrypick-robot commented Dec 4, 2024

	if features.Enabled(features.MultiKueueBatchJobWithManagedBy) {
	return clientutil.PatchStatus(ctx, localClient, &localJob, func() (bool, error) {
	localJob.Status = remoteJob.Status
	return true, nil
	})
	}

	err := r.startJob(ctx, job, object, wl)
	if err != nil {
	log.Error(err, "Unsuspending job")

Fix live-updates for kubernetes Job getting stuck occasionally #3685

Fix live-updates for kubernetes Job getting stuck occasionally #3685

Conversation

IrvingMg commented Nov 28, 2024 • edited 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?

netlify bot commented Nov 28, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

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

mimowo commented Dec 2, 2024

mbobrovskyi commented Dec 2, 2024

mbobrovskyi left a 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

mimowo commented Dec 3, 2024

mimowo commented Dec 3, 2024 • edited Loading

mbobrovskyi commented Dec 3, 2024

IrvingMg commented Dec 4, 2024

mimowo commented Dec 4, 2024

k8s-ci-robot commented Dec 4, 2024

k8s-ci-robot commented Dec 4, 2024

mimowo commented Dec 4, 2024

k8s-infra-cherrypick-robot commented Dec 4, 2024

IrvingMg commented Nov 28, 2024 •

edited

Loading

netlify bot commented Nov 28, 2024 •

edited

Loading

mimowo commented Dec 3, 2024 •

edited

Loading