Fix workload would be created and deleted indefinitely #597

kerthcet · 2023-02-27T10:42:46Z

In #317, we will default workload resource requests to limits if requests not specified. But this will introduce a bug as in job controller reconciling, job will be judged as not equal to workload, so workload will be deleted and re-create indefinitely

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #590

Special notes for your reviewer:

netlify · 2023-02-27T10:42:51Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`89940d5`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6409ccb7f114f10008e91f61

alculquicondor · 2023-02-27T16:37:29Z

What's missing for it to be ready for review?

kerthcet · 2023-02-28T02:04:32Z

What's missing for it to be ready for review?

Integration tests.

kerthcet · 2023-03-02T09:44:37Z

cc @alculquicondor

kerthcet · 2023-03-02T10:59:28Z

pkg/util/api/api.go

+// E.g. job may have resources limits configured but no requests,
+// this is not allowed in workload but we can't update the job directly,
+// so we need a function like this to set the defaults.
+func SetContainersDefaults(containers []v1.Container) []v1.Container {


If we support LimitRange later, we can also add here. If the LimitRange changes, workload will be non-equal to job.

alculquicondor · 2023-03-02T14:09:38Z

/assign

alculquicondor · 2023-03-02T18:33:40Z

pkg/util/api/api.go

+// SetContainersDefaults will fill up the container defaults.
+// Note that this is used to bridge the gap between job and workload,
+// E.g. job may have resources limits configured but no requests,
+// this is not allowed in workload but we can't update the job directly,


This sounds confusing.

It's not that it's not allowed. The purpose of the Workload object is to represent the requests that the Pod containers will ultimately have.

Updated. One more thing, if LimitRange is introduced, workload with no requests should be rejected, wdyt?

Didn't we vote against that already? #485

alculquicondor · 2023-03-02T18:35:21Z

pkg/util/api/api.go

+// so we need a function like this to set the defaults.
+func SetContainersDefaults(containers []v1.Container) []v1.Container {
+	// This should only happen in tests.
+	// We didn't return nil here for spec.Containers should not be nil,


but it also shouldn't be an empty list. I don't think returning nil makes a difference.

As mentioned, this only happens in test, If we return nil here, integration tests will fail because it shouldn't be nil.

If we return empty slice here, unit tests will fail like

kueue/apis/kueue/webhooks/workload_webhook_test.go

Lines 43 to 50 in 271e66a

"add default podSet name": {

wl: kueue.Workload{

Spec: kueue.WorkloadSpec{

PodSets: []kueue.PodSet{

{},

},

},

},

I think I should polish the comments.

pkg/util/api/api.go

pkg/util/api/api_test.go

pkg/controller/workload/job/job_controller.go

alculquicondor · 2023-03-02T18:40:02Z

pkg/util/api/api.go

@@ -16,6 +16,10 @@ limitations under the License.

 package api


I'm not convinced about putting this function here. But we can revisit once we have the jobs library completed, which will probably use it.

Deal. Didn't find where to place this.

alculquicondor · 2023-03-03T14:56:19Z

/lgtm
/approve
/label tide/merge-method-squash

k8s-ci-robot · 2023-03-03T14:56:27Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [alculquicondor]

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

mimowo · 2023-03-03T15:49:52Z

@kerthcet would you like to follow up with an analogous fix for the MPIJob?

alculquicondor · 2023-03-03T15:53:52Z

Instead, can we focus on the library?

mimowo · 2023-03-03T15:58:13Z

Instead, can we focus on the library?

Yes, I was just thinking it makes sense to get rid of the issue as it is now fresh in our minds, and the fix should be just a couple of lines, but I agree with the priorities.

kerthcet · 2023-03-09T06:29:01Z

Let me rebase this one until #616 got merged.

In kubernetes-sigs#317, we will default workload resource requests to limits if requests not specified. But this will introduce a bug for in job controller reconciling, job will be judged as not equal to workload, so workload will be deleted and re-create indefinitely Signed-off-by: Kante Yin <[email protected]>

k8s-ci-robot · 2023-03-09T12:10:32Z

New changes are detected. LGTM label has been removed.

kerthcet · 2023-03-09T12:11:30Z

Rebased.

alculquicondor · 2023-03-09T16:34:33Z

@kerthcet wdyt about #600 (comment)?
I wonder if we should hold for this PR given that it would change significantly.

k8s-ci-robot · 2023-03-15T03:11:12Z

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/test-infra repository.

k8s-ci-robot · 2023-03-15T18:25:19Z

@kerthcet: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kueue-test-e2e-main-1-25	`89940d5`	link	true	`/test pull-kueue-test-e2e-main-1-25`
pull-kueue-test-e2e-main-1-26	`89940d5`	link	true	`/test pull-kueue-test-e2e-main-1-26`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

kerthcet · 2023-03-16T02:04:10Z

/close

superseded by #600

k8s-ci-robot · 2023-03-16T02:04:15Z

@kerthcet: Closed this PR.

In response to this:

/close

superseded by #600

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/test-infra repository.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. labels Feb 27, 2023

k8s-ci-robot requested review from ArangoGutierrez and denkensk February 27, 2023 10:42

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 27, 2023

kerthcet mentioned this pull request Feb 27, 2023

Kueue deletes workloads for jobs with limits, but no requests #590

Closed

kerthcet force-pushed the fix/job-with-limits-only branch from dde0aa4 to 4ffbfca Compare March 2, 2023 08:21

kerthcet changed the title ~~[WIP] Fix workload would be created and deleted indefinitely~~ Fix workload would be created and deleted indefinitely Mar 2, 2023

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 Mar 2, 2023

kerthcet force-pushed the fix/job-with-limits-only branch 2 times, most recently from d8d0493 to 778c860 Compare March 2, 2023 09:19

kerthcet commented Mar 2, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor Mar 2, 2023

alculquicondor reviewed Mar 2, 2023

View reviewed changes

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 3, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2023

kerthcet force-pushed the fix/job-with-limits-only branch from 2194280 to 89940d5 Compare March 9, 2023 12:10

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2023

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2023

mimowo mentioned this pull request Mar 9, 2023

Resync the workload resource values upon LimitRange changes #611

Closed

3 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2023

k8s-ci-robot closed this Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix workload would be created and deleted indefinitely #597

Fix workload would be created and deleted indefinitely #597

kerthcet commented Feb 27, 2023

netlify bot commented Feb 27, 2023 •

edited

Loading

alculquicondor commented Feb 27, 2023

kerthcet commented Feb 28, 2023

kerthcet commented Mar 2, 2023

kerthcet Mar 2, 2023

alculquicondor commented Mar 2, 2023

alculquicondor Mar 2, 2023

kerthcet Mar 3, 2023

alculquicondor Mar 3, 2023

alculquicondor Mar 2, 2023

kerthcet Mar 3, 2023

alculquicondor Mar 2, 2023

kerthcet Mar 3, 2023

alculquicondor commented Mar 3, 2023

k8s-ci-robot commented Mar 3, 2023

mimowo commented Mar 3, 2023

alculquicondor commented Mar 3, 2023

mimowo commented Mar 3, 2023

kerthcet commented Mar 9, 2023

k8s-ci-robot commented Mar 9, 2023

kerthcet commented Mar 9, 2023

alculquicondor commented Mar 9, 2023

k8s-ci-robot commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

kerthcet commented Mar 16, 2023

k8s-ci-robot commented Mar 16, 2023

	"add default podSet name": {
	wl: kueue.Workload{
	Spec: kueue.WorkloadSpec{
	PodSets: []kueue.PodSet{
	{},
	},
	},
	},

		@@ -16,6 +16,10 @@ limitations under the License.

		package api

Fix workload would be created and deleted indefinitely #597

Fix workload would be created and deleted indefinitely #597

Conversation

kerthcet commented Feb 27, 2023

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:

netlify bot commented Feb 27, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

alculquicondor commented Feb 27, 2023

kerthcet commented Feb 28, 2023

kerthcet commented Mar 2, 2023

Choose a reason for hiding this comment

alculquicondor commented Mar 2, 2023

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 Mar 3, 2023

k8s-ci-robot commented Mar 3, 2023

mimowo commented Mar 3, 2023

alculquicondor commented Mar 3, 2023

mimowo commented Mar 3, 2023

kerthcet commented Mar 9, 2023

k8s-ci-robot commented Mar 9, 2023

kerthcet commented Mar 9, 2023

alculquicondor commented Mar 9, 2023

k8s-ci-robot commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

kerthcet commented Mar 16, 2023

k8s-ci-robot commented Mar 16, 2023

netlify bot commented Feb 27, 2023 •

edited

Loading