Add PodsReady condition if WaitForPodsReady is enabled #460

mimowo · 2022-12-05T09:59:41Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This is the first part of implementing the solution designed in: #433.
Admission of queued workloads will be blocked until the currently starting workload has the PodsReady condition (when WaitForPodsReady is configured at the Kueue level).

Which issue(s) this PR fixes:

Part of: #349

Special notes for your reviewer:

the current description of the WaitForPodsReady field is not fully implemented - there is going to be a follow up
PR to implement blocking of admission

k8s-ci-robot · 2022-12-05T09:59:42Z

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

Makefile

apis/kueue/v1alpha2/workload_types.go

main.go

apis/config/v1alpha2/configuration_types.go

pkg/controller/workload/job/job_controller.go

pkg/controller/workload/job/job_controller_test.go

alculquicondor · 2022-12-06T20:30:17Z

pkg/controller/workload/job/job_controller_test.go

+
+	for name, tc := range testcases {
+		t.Run(name, func(t *testing.T) {
+			got := jobPodsReady(tc.job)


We already have all the coverage in integration.

Not everything, for example in integration tests we only test parallelism=completions. Further, the integration tests are much slower, so I would rather decrease the number of scenarios in integration tests than deleting unit tests.

test/integration/controller/job/job_controller_test.go

pkg/controller/workload/job/job_controller.go

alculquicondor · 2022-12-12T18:11:22Z

/lgtm
/label tide/merge-method-squash

ahg-g · 2022-12-12T18:12:54Z

/hold

I would like to take a look

ahg-g · 2022-12-12T18:21:14Z

apis/config/v1alpha2/configuration_types.go

@@ -45,6 +45,18 @@ type Configuration struct {

 	// InternalCertManagement is configuration for internalCertManagement
 	InternalCertManagement *InternalCertManagement `json:"internalCertManagement,omitempty"`
+
+	// WaitForPodsReady is configuration for waitForPodsReady


not useful docs :)

apis/config/v1alpha2/configuration_types.go

ahg-g · 2022-12-12T20:53:53Z

pkg/controller/workload/job/job_controller.go

+	// handle a job when waitForPodsReady is enabled
+	if r.waitForPodsReady {
+		log.V(5).Info("Handling a job when waitForPodsReady is enabled")
+		condition := generatePodsReadyCondition(&job)


Don't we need to update the condition when workload.admitted is set to nil? Or we want to wait until the job is actually suspended and we get the job update event?

I don't see a practical scenario when it matters. Maybe if the request to set admission=nil, but the request to set the suspend=true fails (and is not executed as the job gets quickly re-admitted), but then it seems to make sense to react to the change of the suspend field.

The only scenario where it could matter is when the workload is immediately admitted again and we didn't have time to update the PodsReady condition to false.

But that could still happen in any case, because we can't update the status and spec at the same time. Unless we can in a mutating webhook?

We can't reset the condition in a mutating webhook either. To be on the safer side, maybe we can set the condition to false as soon as we see admission=nil.

I have updated the code now to check for workload admission to be on the safer side.

ahg-g · 2022-12-12T23:13:35Z

test/integration/controller/job/job_controller_test.go

@@ -265,6 +268,217 @@ var _ = ginkgo.Describe("Job controller for workloads with no queue set", func()
 	})
 })

+var _ = ginkgo.Describe("Job controller when waitForPodsReady enabled", func() {


Can we add a test to verify that the condition gets updated for a workload that initially has spec.Admission set and then unset (to simulate preemption).

Modified the tests, but I reuse the previously added test cases (with suspended=true), but in these cases the job was never unsuspended. Now I unconditionally admit the workload at the beginning, but if a test specifies "suspended=true" then Admission is unset, the job is suspended, and then we verify the condition.

sg, we can have a test in the scheduler once preemption is implemented.

alculquicondor · 2022-12-13T16:32:53Z

pkg/workload/workload.go

+	return InConditionWithStatus(w, condition, metav1.ConditionTrue)
+}
+
+func InConditionWithStatus(w *kueue.Workload, condition string, status metav1.ConditionStatus) bool {


Don't add this function.

As a follow up: remove InCondition

Cleaned up unnecessary changes, will create the follow up PR to remove the function

alculquicondor · 2022-12-13T18:23:31Z

/lgtm

Leaving the hold to @ahg-g

ahg-g · 2022-12-13T19:59:04Z

/hold cancel
/lgtm
/approve

k8s-ci-robot · 2022-12-13T19:59:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, alculquicondor, 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:

~~OWNERS~~ [ahg-g,alculquicondor]

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

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/feature Categorizes issue or PR as related to a new feature. labels Dec 5, 2022

k8s-ci-robot requested review from alculquicondor and ArangoGutierrez December 5, 2022 09:59

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 5, 2022

mimowo changed the title ~~Add PodsReady condition if WaitForPodsReady is enabled~~ WIP: Add PodsReady condition if WaitForPodsReady is enabled Dec 5, 2022

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 5, 2022

mimowo force-pushed the pods-ready-condition branch from acce86b to 1f53a9a Compare December 5, 2022 11:16

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 5, 2022

mimowo force-pushed the pods-ready-condition branch 6 times, most recently from 2f78a66 to 195d055 Compare December 6, 2022 12:23

mimowo changed the title ~~WIP: Add PodsReady condition if WaitForPodsReady is enabled~~ Add PodsReady condition if WaitForPodsReady is enabled Dec 6, 2022

mimowo marked this pull request as ready for review December 6, 2022 12:40

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 6, 2022

k8s-ci-robot requested a review from ahg-g December 6, 2022 12:40

mimowo force-pushed the pods-ready-condition branch 5 times, most recently from f2f11c4 to d09fce4 Compare December 6, 2022 13:14

alculquicondor reviewed Dec 6, 2022

View reviewed changes

mimowo force-pushed the pods-ready-condition branch 3 times, most recently from 1854f1d to c18da68 Compare December 8, 2022 14:29

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

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 12, 2022

k8s-ci-robot assigned alculquicondor Dec 12, 2022

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

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

ahg-g reviewed Dec 12, 2022

View reviewed changes

Add PodsReady condition if WaitForPodsReady is enabled

af1117f

mimowo force-pushed the pods-ready-condition branch from f1f77b3 to af1117f Compare December 13, 2022 08:00

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 13, 2022

mimowo force-pushed the pods-ready-condition branch 2 times, most recently from bd4a29a to 2b19996 Compare December 13, 2022 08:17

Remarks

cfe1f31

mimowo force-pushed the pods-ready-condition branch 2 times, most recently from 543d096 to 843b00d Compare December 13, 2022 09:57

Improve integration tests

b6f096e

mimowo force-pushed the pods-ready-condition branch from 843b00d to b6f096e Compare December 13, 2022 10:08

Set PodsReady=False as soon as wl.Spec.Admission=False

617fde2

alculquicondor reviewed Dec 13, 2022

View reviewed changes

Revert unnecessary changes

23693cc

mimowo mentioned this pull request Dec 13, 2022

Remove unnecessary function in kueue #474

Merged

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

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

k8s-ci-robot assigned ahg-g Dec 13, 2022

k8s-ci-robot merged commit 0c18192 into kubernetes-sigs:main Dec 13, 2022

mimowo deleted the pods-ready-condition branch March 18, 2023 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PodsReady condition if WaitForPodsReady is enabled #460

Add PodsReady condition if WaitForPodsReady is enabled #460

mimowo commented Dec 5, 2022 •

edited

Loading

k8s-ci-robot commented Dec 5, 2022

alculquicondor Dec 6, 2022

mimowo Dec 8, 2022

alculquicondor commented Dec 12, 2022

ahg-g commented Dec 12, 2022

ahg-g Dec 12, 2022

mimowo Dec 13, 2022 •

edited

Loading

ahg-g Dec 12, 2022

mimowo Dec 13, 2022

alculquicondor Dec 13, 2022

alculquicondor Dec 13, 2022

mimowo Dec 13, 2022

ahg-g Dec 12, 2022

mimowo Dec 13, 2022 •

edited

Loading

ahg-g Dec 13, 2022

alculquicondor Dec 13, 2022

mimowo Dec 13, 2022

mimowo Dec 13, 2022

alculquicondor commented Dec 13, 2022

ahg-g commented Dec 13, 2022

k8s-ci-robot commented Dec 13, 2022

Add PodsReady condition if WaitForPodsReady is enabled #460

Add PodsReady condition if WaitForPodsReady is enabled #460

Conversation

mimowo commented Dec 5, 2022 • 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:

k8s-ci-robot commented Dec 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Dec 12, 2022

ahg-g commented Dec 12, 2022

Choose a reason for hiding this comment

mimowo Dec 13, 2022 • 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

mimowo Dec 13, 2022 • 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

alculquicondor commented Dec 13, 2022

ahg-g commented Dec 13, 2022

k8s-ci-robot commented Dec 13, 2022

mimowo commented Dec 5, 2022 •

edited

Loading

mimowo Dec 13, 2022 •

edited

Loading

mimowo Dec 13, 2022 •

edited

Loading