[WaitForPodsReady] Make requeue base delay confiurable #2040

mimowo · 2024-04-23T10:40:07Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Part of #2009

Special notes for your reviewer:

I'm yet wondering about making the exponent and jitter configurable, wdyt?

Does this PR introduce a user-facing change?

Make the PodsReady base delay for requeuing configurable

k8s-ci-robot · 2024-04-23T10:40:09Z

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

netlify · 2024-04-23T10:40:25Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`86461d9`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6628b7277f782e00082026df

mimowo · 2024-04-23T10:40:33Z

/test all

mimowo · 2024-04-23T12:33:09Z

/assign @tenzen-y

mimowo · 2024-04-23T12:37:32Z

apis/config/v1beta1/configuration_types.go

+	//
+	// Defaults to 10.
+	// +optional
+	BaseDelaySeconds *int32 `json:"baseDelaySeconds,omitempty"`


I'm yet wondering about making the exponent and jitter configurable, wdyt @alculquicondor @tenzen-y?

I prefer to keep this simple, but @mimowo and/or @alculquicondor have requests from your customers or you have concrete use cases, I'm ok with implementing them.

Definitely not the jitter. But I'm also ok leaving the exponent out for now.

tenzen-y

Basically, lgtm
Also could you implement validations here:

kueue/pkg/config/validation.go

Line 60 in 92baacd

func validateWaitForPodsReady(c *configapi.Configuration) field.ErrorList {

mimowo · 2024-04-23T14:28:31Z

Basically, lgtm
Also could you implement validations here:

Added, thanks
/assign @alculquicondor

trasc · 2024-04-23T14:36:11Z

pkg/controller/core/core.go

-func SetupControllers(mgr ctrl.Manager, qManager *queue.Manager, cc *cache.Cache, cfg *configapi.Configuration, controllerOpts ...ControllerOption) (string, error) {
+func SetupControllers(mgr ctrl.Manager, qManager *queue.Manager, cc *cache.Cache, cfg *configapi.Configuration) (string, error) {


I personally like the Options pattern better.

Right, but the new parameter is not needed any longer since the configuration is not passed in cfg *configapi.Configuration. I guess we could replace cfg *configapi.Configuration with Options, but it would be a lot of changes, so I wouldn't like to mix this into this PR.

for now changing WithControllerRequeuingBaseDelaySeconds to something like WithControllerRequeuingBackoffBaseSeconds can work and we can do a clean-up to drop the cfg usage.

I see that, but the interim state of passing the params which are part of the cfg both ways would look weird. I'm happy to open a dedicated issue to do the refactoring at once.

If we agree we should go towards Options , why start with a step back?

It's not needed as a result of this PR implementation.

Again, going the path of keeping it requires more changes. I don't think refactoring of this function is a priority right now. So we should leave the code in a state that makes sense for the mid-term.

going the path of keeping it requires more changes

I'm not convinced, we used to have for now changing WithControllerRequeuingBaseDelaySeconds we can just replace it with WithControllerRequeuingBackoffBaseSeconds .

But then we have the same information flowing through cfg

Let me put the context of the reason why we added the controllerOpts.

We introduced the controllerOpts in #2025 instead of Config API to cherry-pick to the release v0.6 as an interim approach. Then, we try to remove the interim approach in this PR.

I agree with refactoring this function, but as @alculquicondor and @mimowo, I also think that the refactoring is beyond this PR responsibility.
So, I agree with leaving the refactoring in another PR.

alculquicondor · 2024-04-23T17:09:57Z

pkg/controller/core/core.go

-	}
-	for _, opt := range controllerOpts {
-		opt(&ctrlOpts)
-	}

 	if err := NewWorkloadReconciler(mgr.GetClient(), qManager, cc,
 		mgr.GetEventRecorderFor(constants.WorkloadControllerName),
 		WithWorkloadUpdateWatchers(qRec, cqRec),
 		WithPodsReadyTimeout(podsReadyTimeout(cfg)),


Maybe it would be worth something like:

WithPodsReadyTimeout(cfg.WaitForPodsReady)

So we don't need to keep adding options for every single field.

Good idea, I just pushed a commit along the lines, but I use:
WithWaitForPodsReady(waitForPodsReady(cfg.WaitForPodsReady)),
because the controller needs the entire config. Also I use the waitForPodsReady function to convert the underlying config into a structure easier to consume. In particular, I don't need the enable field.

tenzen-y · 2024-04-24T07:30:42Z

@mimowo Also, could you open another PR to update the document?
https://kueue.sigs.k8s.io/docs/tasks/manage/setup_sequential_admission/#enabling-waitforpodsready

mimowo · 2024-04-24T07:32:02Z

@mimowo Also, could you open another PR to update the document?
https://kueue.sigs.k8s.io/docs/tasks/manage/setup_sequential_admission/#enabling-waitforpodsready

Sure, I will follow up on this.

mimowo · 2024-04-24T07:47:13Z

/test pull-kueue-test-e2e-main-1-29
/test pull-kueue-test-scheduling-perf-main
/test pull-kueue-test-unit-main
/test pull-kueue-verify-main
not sure why the tests are failed, they don't even seem started

tenzen-y

Thank you!
/lgtm

k8s-ci-robot · 2024-04-24T08:02:50Z

LGTM label has been added.

Git tree hash: 27145d6920748a7c15ef5d1b55a9fd434e10e386

tenzen-y · 2024-04-24T08:03:00Z

/approve

k8s-ci-robot · 2024-04-24T08:03:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, tenzen-y

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

tenzen-y · 2024-04-24T08:20:46Z

/test pull-kueue-test-e2e-main-1-29 /test pull-kueue-test-scheduling-perf-main /test pull-kueue-test-unit-main /test pull-kueue-verify-main not sure why the tests are failed, they don't even seem started

/test pull-kueue-verify-main

mimowo · 2024-04-24T08:34:39Z

/test pull-kueue-build-image-main

k8s-ci-robot requested review from denkensk and kerthcet April 23, 2024 10:40

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 23, 2024

mimowo force-pushed the make-requeue-configurable branch from d2d1bc7 to 2f83e04 Compare April 23, 2024 12:04

mimowo marked this pull request as ready for review April 23, 2024 12:09

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 Apr 23, 2024

k8s-ci-robot requested review from alculquicondor and trasc April 23, 2024 12:09

k8s-ci-robot assigned tenzen-y Apr 23, 2024

mimowo commented Apr 23, 2024

View reviewed changes

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 Apr 23, 2024

mimowo changed the title ~~[WaitForPodsReady] Make requeue base delay confiurable~~ WIP: [WaitForPodsReady] Make requeue base delay confiurable Apr 23, 2024

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 Apr 23, 2024

Make requeuing backoff base configurable

1643b2d

mimowo force-pushed the make-requeue-configurable branch from e739b55 to 1643b2d Compare April 23, 2024 13:28

tenzen-y reviewed Apr 23, 2024

View reviewed changes

mimowo changed the title ~~WIP: [WaitForPodsReady] Make requeue base delay confiurable~~ [WaitForPodsReady] Make requeue base delay confiurable Apr 23, 2024

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 Apr 23, 2024

Add a validation test

4dcabe6

k8s-ci-robot assigned alculquicondor Apr 23, 2024

trasc reviewed Apr 23, 2024

View reviewed changes

alculquicondor reviewed Apr 23, 2024

View reviewed changes

tenzen-y mentioned this pull request Apr 24, 2024

Tighten asserts for PodsReady requeueAt by using a fake clock and jitter=0 #2044

Merged

Pass config for waitForPodsReady

86461d9

tenzen-y reviewed Apr 24, 2024

View reviewed changes

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

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

k8s-ci-robot merged commit 9b25d99 into kubernetes-sigs:main Apr 24, 2024
15 checks passed

k8s-ci-robot added this to the v0.7 milestone Apr 24, 2024

This was referenced May 17, 2024

[release-0.6] WaitForPodsReady: Load the backoffLimitCount regardless of the requeueingStrategy.timestamp #2223

Closed

[release-0.6] WaitForPodsReady: Load the backoffLimitCount regardless of the requeueingStrategy.timestamp #2224

Merged

mimowo deleted the make-requeue-configurable branch May 29, 2024 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WaitForPodsReady] Make requeue base delay confiurable #2040

[WaitForPodsReady] Make requeue base delay confiurable #2040

mimowo commented Apr 23, 2024 •

edited

Loading

k8s-ci-robot commented Apr 23, 2024

netlify bot commented Apr 23, 2024 •

edited

Loading

mimowo commented Apr 23, 2024

mimowo commented Apr 23, 2024

mimowo Apr 23, 2024 •

edited

Loading

tenzen-y Apr 23, 2024

alculquicondor Apr 23, 2024

tenzen-y left a comment

mimowo commented Apr 23, 2024

trasc Apr 23, 2024

mimowo Apr 23, 2024

trasc Apr 23, 2024

mimowo Apr 23, 2024 •

edited

Loading

trasc Apr 23, 2024

trasc Apr 23, 2024

alculquicondor Apr 23, 2024

trasc Apr 23, 2024

alculquicondor Apr 23, 2024

tenzen-y Apr 24, 2024

alculquicondor Apr 23, 2024

tenzen-y Apr 24, 2024

mimowo Apr 24, 2024

tenzen-y commented Apr 24, 2024

mimowo commented Apr 24, 2024

mimowo commented Apr 24, 2024

tenzen-y left a comment

k8s-ci-robot commented Apr 24, 2024

tenzen-y commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

tenzen-y commented Apr 24, 2024

mimowo commented Apr 24, 2024

		func SetupControllers(mgr ctrl.Manager, qManager queue.Manager, cc cache.Cache, cfg *configapi.Configuration, controllerOpts ...ControllerOption) (string, error) {
		func SetupControllers(mgr ctrl.Manager, qManager queue.Manager, cc cache.Cache, cfg *configapi.Configuration) (string, error) {

[WaitForPodsReady] Make requeue base delay confiurable #2040

[WaitForPodsReady] Make requeue base delay confiurable #2040

Conversation

mimowo commented Apr 23, 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?

k8s-ci-robot commented Apr 23, 2024

netlify bot commented Apr 23, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

mimowo commented Apr 23, 2024

mimowo commented Apr 23, 2024

mimowo Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

mimowo commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo Apr 23, 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

tenzen-y commented Apr 24, 2024

mimowo commented Apr 24, 2024

mimowo commented Apr 24, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 24, 2024

tenzen-y commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

tenzen-y commented Apr 24, 2024

mimowo commented Apr 24, 2024

mimowo commented Apr 23, 2024 •

edited

Loading

netlify bot commented Apr 23, 2024 •

edited

Loading

mimowo Apr 23, 2024 •

edited

Loading

mimowo Apr 23, 2024 •

edited

Loading