Allow podSet count to be 0 #2268

mszadkow · 2024-05-23T15:01:58Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Allow for workload.spec.podSet.[*].count to be 0

Which issue(s) this PR fixes:

Fixes #2227

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Allow for `workload.spec.podSet.[*].count` to be 0

linux-foundation-easycla · 2024-05-23T15:02:04Z

The committers listed above are authorized under a signed CLA.

✅ login: mszadkow / name: Michał Szadkowski (4f9ab1d)

k8s-ci-robot · 2024-05-23T15:02:09Z

Welcome @mszadkow!

It looks like this is your first PR to kubernetes-sigs/kueue 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kueue has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-05-23T15:02:10Z

Hi @mszadkow. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

netlify · 2024-05-23T15:02:19Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`4f9ab1d`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6656f8b023d85b0008f3ff68

mszadkow · 2024-05-23T15:02:33Z

/assign @trasc

trasc · 2024-05-23T15:04:50Z

/ok-to-test
/test all

mszadkow · 2024-05-23T15:15:47Z

/check-cla

kannon92

Should we add a test for job or other workloads? I don’t know if others allow for 0 replicas but it may be worth checking this out as we are increasing coverage for this case.

WDYT @alculquicondor @tenzen-y?

alculquicondor · 2024-05-27T16:07:26Z

I think that only batch/job would be worth it.

Jobset is a bit trickier, because it can have a podset with zero and one without. I would like to have coverage for it.

test/e2e/singlecluster/jobset_test.go

alculquicondor

I think the file podset_reducer.go might be doing some divisions by zero.

We might need some unit tests in this area.

test/e2e/singlecluster/jobset_test.go

tenzen-y · 2024-05-28T09:21:10Z

I think that only batch/job would be worth it.

Jobset is a bit trickier, because it can have a podset with zero and one without. I would like to have coverage for it.

That makes sense.

test/integration/controller/jobs/jobset/jobset_controller_test.go

mimowo

/lgtm
/approve
/assign @tenzen-y @alculquicondor

k8s-ci-robot · 2024-05-29T09:59:04Z

LGTM label has been added.

Git tree hash: 2ee38303bb981831212704c06e291216036e7ed6

tenzen-y

I'm sure that you just copy and paste, but I really want to improve debuggability.
So, could you address my comments in followup PR since I don't want to block new release due to my comments.

/approve

tenzen-y · 2024-05-29T10:27:33Z

test/integration/controller/jobs/jobset/jobset_controller_test.go

+				gomega.Expect(k8sClient.Create(ctx, mixJobSet)).Should(gomega.Succeed())
+				gomega.Eventually(func() *bool {
+					createdJobSet1 := &jobsetapi.JobSet{}
+					gomega.Expect(k8sClient.Get(ctx, jobSetLookupKey, createdJobSet1)).Should(gomega.Succeed())
+					return createdJobSet1.Spec.Suspend
+				}, util.Timeout, util.Interval).Should(gomega.Equal(ptr.To(true)))


Suggested change

gomega.Expect(k8sClient.Create(ctx, mixJobSet)).Should(gomega.Succeed())

gomega.Eventually(func() *bool {

createdJobSet1 := &jobsetapi.JobSet{}

gomega.Expect(k8sClient.Get(ctx, jobSetLookupKey, createdJobSet1)).Should(gomega.Succeed())

return createdJobSet1.Spec.Suspend

}, util.Timeout, util.Interval).Should(gomega.Equal(ptr.To(true)))

gomega.Expect(k8sClient.Create(ctx, mixJobSet)).Should(gomega.Succeed())

gomega.Eventually(func(g gomega.Gomega) *bool {

createdJobSet1 := &jobsetapi.JobSet{}

g.Expect(k8sClient.Get(ctx, jobSetLookupKey, createdJobSet1)).Should(gomega.Succeed())

g.Expect(createdJobSet1.Spec.Suspend).ShouldNot(gomega.BeNil())

g.Expect(createdJobSet1.Spec.Suspend).Should(gomega.BeTrue())

}, util.Timeout, util.Interval).Should(gomega.Succeeded)

I would suggest passing the gomega instance via arg to increase debuggability.

tenzen-y · 2024-05-29T10:35:54Z

test/integration/controller/jobs/jobset/jobset_controller_test.go

+			err := k8sClient.Create(ctx, jobSet)
+			gomega.Expect(err).To(gomega.Succeed())


Suggested change

err := k8sClient.Create(ctx, jobSet)

gomega.Expect(err).To(gomega.Succeed())

k8sClient.Create(ctx, jobSet).To(gomega.Succeded())

did you mean:
gomega.Expect(k8sClient.Create(ctx, jobSet)).To(gomega.Succeed()) ?

Oops, that's right :)

tenzen-y · 2024-05-29T10:38:15Z

test/integration/controller/jobs/jobset/jobset_controller_test.go

+			gomega.Eventually(func() bool {
+				if err := k8sClient.Get(ctx, types.NamespacedName{Name: jobSetName, Namespace: ns.Name}, createdJobSet); err != nil {
+					return false
+				}
+				return ptr.Deref(createdJobSet.Spec.Suspend, false)
+			}, util.Timeout, util.Interval).Should(gomega.BeTrue())


Suggested change

gomega.Eventually(func() bool {

if err := k8sClient.Get(ctx, types.NamespacedName{Name: jobSetName, Namespace: ns.Name}, createdJobSet); err != nil {

return false

}

return ptr.Deref(createdJobSet.Spec.Suspend, false)

}, util.Timeout, util.Interval).Should(gomega.BeTrue())

gomega.Eventually(func(g gomega.Gomega) {

g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: jobSetName, Namespace: ns.Name}, createdJobSet)).Should(gomega.Succeeded())

g.Expect(ptr.Deref(createdJobSet.Spec.Suspend, false)).Should(gomega.BeTrue())

}, util.Timeout, util.Interval).Should(gomega.Succeeded())

tenzen-y · 2024-05-29T10:40:02Z

test/integration/controller/jobs/jobset/jobset_controller_test.go

+			gomega.Eventually(func() bool {
+				if err := k8sClient.Get(ctx, wlLookupKey, createdWorkload); err != nil {
+					return false
+				}
+				return createdWorkload.Spec.QueueName == jobSetQueueName
+			}, util.Timeout, util.Interval).Should(gomega.BeTrue())


Suggested change

gomega.Eventually(func() bool {

if err := k8sClient.Get(ctx, wlLookupKey, createdWorkload); err != nil {

return false

}

return createdWorkload.Spec.QueueName == jobSetQueueName

}, util.Timeout, util.Interval).Should(gomega.BeTrue())

gomega.Eventually(func(g gomega.Gomega) {

g.Expect(k8sClient.Get(ctx, wlLookupKey, createdWorkload)).Should(gomega.Succeeded)

g.Expect(createdWorkload.Spec.QueueName).Should(gomga.Equal(jobSetQueueName))

}, util.Timeout, util.Interval).Should(gomega.Succeeded())

tenzen-y · 2024-05-29T10:43:04Z

test/integration/controller/jobs/jobset/jobset_controller_test.go

+			gomega.Eventually(func() error {
+				wl := &kueue.Workload{}
+				key := types.NamespacedName{Name: secondWl.Name, Namespace: secondWl.Namespace}
+				return k8sClient.Get(ctx, key, wl)
+			}, util.Timeout, util.Interval).Should(testing.BeNotFoundError())
+			// check the original wl is still there
+			gomega.Eventually(func() error {
+				return k8sClient.Get(ctx, wlLookupKey, createdWorkload)
+			}, util.Timeout, util.Interval).Should(gomega.Succeed())


Suggested change

gomega.Eventually(func() error {

wl := &kueue.Workload{}

key := types.NamespacedName{Name: secondWl.Name, Namespace: secondWl.Namespace}

return k8sClient.Get(ctx, key, wl)

}, util.Timeout, util.Interval).Should(testing.BeNotFoundError())

// check the original wl is still there

gomega.Eventually(func() error {

return k8sClient.Get(ctx, wlLookupKey, createdWorkload)

}, util.Timeout, util.Interval).Should(gomega.Succeed())

key := types.NamespacedName{Name: secondWl.Name, Namespace: secondWl.Namespace}

gomega.Eventually(func(g gomega.Gomega) {

wl := &kueue.Workload{}

g.Expect(k8sClient.Get(ctx, key, wl)).Should(testing.BeNotFoundError())

// check the original wl is still there

g.Expect(k8sClient.Get(ctx, wlLookupKey, createdWorkload)).Should(gomega.Succeeded())

}, util.Timeout, util.Interval).Should(gomega.Succeeded)

tenzen-y · 2024-05-29T10:53:55Z

test/integration/controller/jobs/jobset/jobset_controller_test.go

+			gomega.Eventually(func() bool {
+				if err := k8sClient.Get(ctx, lookupKey, createdJobSet); err != nil {
+					return false
+				}
+				return createdJobSet.Spec.Suspend != nil && *createdJobSet.Spec.Suspend &&
+					len(jobSet.Spec.ReplicatedJobs[0].Template.Spec.Template.Spec.NodeSelector) == 0
+			}, util.Timeout, util.Interval).Should(gomega.BeTrue())


Suggested change

gomega.Eventually(func() bool {

if err := k8sClient.Get(ctx, lookupKey, createdJobSet); err != nil {

return false

}

return createdJobSet.Spec.Suspend != nil && *createdJobSet.Spec.Suspend &&

len(jobSet.Spec.ReplicatedJobs[0].Template.Spec.Template.Spec.NodeSelector) == 0

}, util.Timeout, util.Interval).Should(gomega.BeTrue())

gomega.Eventually(func(g gomega.Gomega) {

g.Expect(k8sClient.Get(ctx, lookupKey, createdJobSet)).Should(gomega.Succeeded())

g.Expect(createdJobSet.Spec.Suspend).ShouldNot(gomega.BeNil())

g.Expect(*createdJobSet.Spec.Suspend).Should(gomega.BeTrue())

g.Expect(jobSet.Spec.ReplicatedJobs[0].Template.Spec.Template.Spec.NodeSelector).Should(gomega.HaveLen(0))

}, util.Timeout, util.Interval).Should(gomega.Succeeded())

tenzen-y · 2024-05-29T10:54:42Z

test/integration/controller/jobs/jobset/jobset_controller_test.go

+			gomega.Eventually(func() bool {
+				ok, _ := testing.CheckLatestEvent(ctx, k8sClient, "DeletedWorkload", corev1.EventTypeNormal, fmt.Sprintf("Deleted not matching Workload: %v", wlLookupKey.String()))
+				return ok
+			}, util.Timeout, util.Interval).Should(gomega.BeTrue())


Suggested change

gomega.Eventually(func() bool {

ok, _ := testing.CheckLatestEvent(ctx, k8sClient, "DeletedWorkload", corev1.EventTypeNormal, fmt.Sprintf("Deleted not matching Workload: %v", wlLookupKey.String()))

return ok

}, util.Timeout, util.Interval).Should(gomega.BeTrue())

gomega.Eventually(func(g gomega.Gomega) {

ok, _ := testing.CheckLatestEvent(ctx, k8sClient, "DeletedWorkload", corev1.EventTypeNormal, fmt.Sprintf("Deleted not matching Workload: %v", wlLookupKey.String()))

g.Expect(ok).Should(gomega.BeTrue())

}, util.Timeout, util.Interval).Should(gomega.Succeeded())

tenzen-y · 2024-05-29T10:55:43Z

test/integration/controller/jobs/jobset/jobset_controller_test.go

+			gomega.Eventually(func() bool {
+				if err := k8sClient.Get(ctx, wlLookupKey, createdWorkload); err != nil {
+					return false
+				}
+				return createdWorkload.Spec.PodSets[0].Count == newParallelism
+			}, util.Timeout, util.Interval).Should(gomega.BeTrue())


Suggested change

gomega.Eventually(func() bool {

if err := k8sClient.Get(ctx, wlLookupKey, createdWorkload); err != nil {

return false

}

return createdWorkload.Spec.PodSets[0].Count == newParallelism

}, util.Timeout, util.Interval).Should(gomega.BeTrue())

gomega.Eventually(func(g gomega.Gomega) {

g.Expect(k8sClient.Get(ctx, wlLookupKey, createdWorkload).Should(gomega.Succeeded())

g.Expect(createdWorkload.Spec.PodSets[0].Count).Should(gomega.Equal(newParallelism))

}, util.Timeout, util.Interval).Should(gomega.Succeeded())

tenzen-y · 2024-05-29T10:57:08Z

test/integration/controller/jobs/jobset/jobset_controller_test.go

+			gomega.Eventually(func() bool {
+				if err := k8sClient.Get(ctx, lookupKey, createdJobSet); err != nil {
+					return false
+				}
+				return !*createdJobSet.Spec.Suspend
+			}, util.Timeout, util.Interval).Should(gomega.BeTrue())


Suggested change

gomega.Eventually(func() bool {

if err := k8sClient.Get(ctx, lookupKey, createdJobSet); err != nil {

return false

}

return !*createdJobSet.Spec.Suspend

}, util.Timeout, util.Interval).Should(gomega.BeTrue())

gomega.Eventually(func(g gomega.Gomega) {

g.Expect(k8sClient.Get(ctx, lookupKey, createdJobSet)).Should(gomega.Succeeded())

g.Expect(createdJobSet.Spec.Suspend).ShouldNot(gomega.BeNil())

g.Expect(*createdJobSet.Spec.Suspend).Should(gomega.BeFalse)

}, util.Timeout, util.Interval).Should(gomega.Succeeded())

tenzen-y · 2024-05-29T10:59:24Z

test/integration/controller/jobs/jobset/jobset_controller_test.go

+			gomega.Eventually(func() bool {
+				err := k8sClient.Get(ctx, wlLookupKey, createdWorkload)
+				if err != nil {
+					return false
+				}
+				return apimeta.IsStatusConditionTrue(createdWorkload.Status.Conditions, kueue.WorkloadFinished)
+			}, util.Timeout, util.Interval).Should(gomega.BeTrue())


Suggested change

gomega.Eventually(func() bool {

err := k8sClient.Get(ctx, wlLookupKey, createdWorkload)

if err != nil {

return false

}

return apimeta.IsStatusConditionTrue(createdWorkload.Status.Conditions, kueue.WorkloadFinished)

}, util.Timeout, util.Interval).Should(gomega.BeTrue())

util.ExpectWorkloadToFinish(...

k8s-ci-robot · 2024-05-29T11:00:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, mszadkow, tenzen-y, 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:

~~OWNERS~~ [tenzen-y]

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

mszadkow · 2024-05-29T11:18:14Z

I'm sure that you just copy and paste, but I really want to improve debuggability. So, could you address my comments in followup PR since I don't want to block new release due to my comments.

/approve

Sure, I will provide new PR.

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

k8s-ci-robot requested review from denkensk and mimowo May 23, 2024 15:02

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 23, 2024

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 23, 2024

k8s-ci-robot assigned trasc May 23, 2024

vladikkuzn approved these changes May 24, 2024

View reviewed changes

mszadkow force-pushed the fix/handle-jobset-replica-0-count branch from fe52608 to 667abab Compare May 24, 2024 14:02

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 24, 2024

kannon92 reviewed May 25, 2024

View reviewed changes

mszadkow force-pushed the fix/handle-jobset-replica-0-count branch from 667abab to 9e11d9e Compare May 27, 2024 06:30

mszadkow marked this pull request as ready for review May 27, 2024 09:11

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 27, 2024

k8s-ci-robot requested review from alculquicondor and kerthcet May 27, 2024 09:11

mszadkow force-pushed the fix/handle-jobset-replica-0-count branch from fab9c44 to d61f84d Compare May 27, 2024 12:41

alculquicondor reviewed May 27, 2024

View reviewed changes

test/e2e/singlecluster/jobset_test.go Outdated Show resolved Hide resolved

alculquicondor reviewed May 27, 2024

View reviewed changes

test/e2e/singlecluster/jobset_test.go Outdated Show resolved Hide resolved

mszadkow force-pushed the fix/handle-jobset-replica-0-count branch from d61f84d to 5c6603a Compare May 28, 2024 12:57

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 28, 2024

trasc reviewed May 28, 2024

View reviewed changes

mszadkow force-pushed the fix/handle-jobset-replica-0-count branch from 5c6603a to a2e9d90 Compare May 28, 2024 15:26

mimowo mentioned this pull request May 29, 2024

Release v0.7.0 #2305

Closed

26 tasks

Allow podSet count to be 0

4f9ab1d

mszadkow force-pushed the fix/handle-jobset-replica-0-count branch from a2e9d90 to 4f9ab1d Compare May 29, 2024 09:43

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 29, 2024

mimowo reviewed May 29, 2024

View reviewed changes

k8s-ci-robot assigned alculquicondor, tenzen-y and mimowo May 29, 2024

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

tenzen-y reviewed May 29, 2024

View reviewed changes

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

k8s-ci-robot merged commit eab4365 into kubernetes-sigs:main May 29, 2024
15 checks passed

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

mszadkow deleted the fix/handle-jobset-replica-0-count branch May 29, 2024 11:18

mszadkow mentioned this pull request May 29, 2024

improve debuggability in jobset controller tests #2315

Merged

mszadkow mentioned this pull request Jun 21, 2024

REQUEST: New membership for mszadkow kubernetes/org#5034

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow podSet count to be 0 #2268

Allow podSet count to be 0 #2268

mszadkow commented May 23, 2024

linux-foundation-easycla bot commented May 23, 2024 •

edited

Loading

k8s-ci-robot commented May 23, 2024

k8s-ci-robot commented May 23, 2024

netlify bot commented May 23, 2024 •

edited

Loading

mszadkow commented May 23, 2024

trasc commented May 23, 2024

mszadkow commented May 23, 2024

kannon92 left a comment

alculquicondor commented May 27, 2024

alculquicondor left a comment

tenzen-y commented May 28, 2024

mimowo left a comment

k8s-ci-robot commented May 29, 2024

tenzen-y left a comment

tenzen-y May 29, 2024

tenzen-y May 29, 2024

mszadkow May 29, 2024

tenzen-y May 29, 2024

tenzen-y May 29, 2024

tenzen-y May 29, 2024

tenzen-y May 29, 2024

tenzen-y May 29, 2024

tenzen-y May 29, 2024

tenzen-y May 29, 2024

tenzen-y May 29, 2024

tenzen-y May 29, 2024

k8s-ci-robot commented May 29, 2024

mszadkow commented May 29, 2024

		err := k8sClient.Create(ctx, jobSet)
		gomega.Expect(err).To(gomega.Succeed())

	err := k8sClient.Create(ctx, jobSet)
	gomega.Expect(err).To(gomega.Succeed())
	k8sClient.Create(ctx, jobSet).To(gomega.Succeded())

Allow podSet count to be 0 #2268

Allow podSet count to be 0 #2268

Conversation

mszadkow commented May 23, 2024

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?

linux-foundation-easycla bot commented May 23, 2024 • edited Loading

k8s-ci-robot commented May 23, 2024

k8s-ci-robot commented May 23, 2024

netlify bot commented May 23, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

mszadkow commented May 23, 2024

trasc commented May 23, 2024

mszadkow commented May 23, 2024

kannon92 left a comment

Choose a reason for hiding this comment

alculquicondor commented May 27, 2024

alculquicondor left a comment

Choose a reason for hiding this comment

tenzen-y commented May 28, 2024

mimowo left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 29, 2024

tenzen-y 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

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

k8s-ci-robot commented May 29, 2024

mszadkow commented May 29, 2024

linux-foundation-easycla bot commented May 23, 2024 •

edited

Loading

netlify bot commented May 23, 2024 •

edited

Loading