Make waitForPodsReady.timeout required field #7952

tenzen-y · 2025-11-27T12:32:57Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

As we discussed in #7767 (comment), we decided to make waitForPodsReady.timeout a required field.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

v1beta2: Make .waitForPodsReady.timeout required field in the Config API

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

netlify · 2025-11-27T12:33:03Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`1ecafc8`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/69285c5ab38227000857cbe1

k8s-ci-robot · 2025-11-27T12:33:04Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

mbobrovskyi · 2025-11-27T12:51:10Z

apis/config/v1beta1/configuration_conversion.go

 }

 func Convert_v1beta1_WaitForPodsReady_To_v1beta2_WaitForPodsReady(in *WaitForPodsReady, out *v1beta2.WaitForPodsReady, s conversionapi.Scope) error {
+	if in != nil && in.Enable && in.Timeout == nil {


Suggested change

if in != nil && in.Enable && in.Timeout == nil {

if in.Enable && in.Timeout == nil {

I'm wondering do we need to check for nil here. IIUC it shouldn't execute this function if we have nil, right?

I'm not sure. Do you know if it has never been called in case of in is nil?

yeah, checking in != nil feels safer

Yeah, we have this test case https://github.com/tenzen-y/kueue/blob/11c6f37a2bbf28eb0741709cca64b2c2b8b6fa3e/apis/config/v1beta1/configuration_conversion_test.go#L34-L37. If you remove this check nothing happens.

OK, let me check that in this branch.

I verified that in != nil can be removed.
@mimowo Do you think that we shold keep in != nil?

no, let's drop in that case

awesome, I dropped.

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

mimowo · 2025-11-27T13:24:44Z

/lgtm
Thank you 👍

k8s-ci-robot · 2025-11-27T13:24:52Z

LGTM label has been added.

Details

Git tree hash: 14ccbcdf387f6b9bcbd6b658cbd1f76a2f09f904

mbobrovskyi · 2025-11-27T13:33:21Z

apis/config/v1beta1/configuration_conversion.go

 }

 func Convert_v1beta1_WaitForPodsReady_To_v1beta2_WaitForPodsReady(in *WaitForPodsReady, out *v1beta2.WaitForPodsReady, s conversionapi.Scope) error {
+	if in.Enable && in.Timeout == nil {


Shouldn't we convert to pointer in Convert_v1beta2_WaitForPodsReady_To_v1beta1_WaitForPodsReady?

NVM I tested it manually. It is automatically convert it.

tenzen-y · 2025-11-27T13:36:45Z

apis/config/v1beta1/zz_generated.conversion.go

@mbobrovskyi I think this can automatically convert pointer <-> non-pointer.

Thank you for double check!

mimowo · 2025-11-27T13:39:49Z

https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kueue/7952/pull-kueue-test-e2e-customconfigs-main/1994031239329222656
looks like flake, but not sure, let's retry:
/test pull-kueue-test-e2e-customconfigs-main

mimowo · 2025-11-27T13:57:20Z

WaitForPodsReady with default Timeout and a tiny RecoveryTimeout [BeforeAll] should evict and requeue workload when pod failure causes recovery timeout
  [BeforeAll] /home/prow/go/src/sigs.k8s.io/kueue/test/e2e/customconfigs/waitforpodsready_test.go:217
  [It] /home/prow/go/src/sigs.k8s.io/kueue/test/e2e/customconfigs/waitforpodsready_test.go:288
  [FAILED] Timed out after 300.001s.
  The function passed to Eventually failed at /home/prow/go/src/sigs.k8s.io/kueue/test/util/e2e.go:239 with:
  Expected
      <string>: ReplicaSetUpdated
  to equal
      <string>: NewReplicaSetAvailable
  In [BeforeAll] at: /home/prow/go/src/sigs.k8s.io/kueue/test/util/e2e.go:530 @ 11/27/25 13:53:17.463

I can see that again already in the ongoing build, so likely not just a flake

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y · 2025-11-27T14:14:13Z

test/e2e/customconfigs/waitforpodsready_test.go


 		util.UpdateKueueConfiguration(ctx, k8sClient, defaultKueueCfg, kindClusterName, func(cfg *configapi.Configuration) {
 			cfg.WaitForPodsReady = &configapi.WaitForPodsReady{
+				Timeout:         metav1.Duration{Duration: 5 * time.Minute},


This is a root cause.
We added a validation to reject an empty timeout, but my previous commit does not specify this one.
So, the kueue-controller-manager failed to start itself due to validation.

tenzen-y · 2025-11-27T14:14:33Z

test/e2e/customconfigs/waitforpodsready_test.go


 		util.UpdateKueueConfiguration(ctx, k8sClient, defaultKueueCfg, kindClusterName, func(cfg *configapi.Configuration) {
 			cfg.WaitForPodsReady = &configapi.WaitForPodsReady{
+				Timeout:         metav1.Duration{Duration: 5 * time.Minute},


This is the same reason as the above.

mimowo

/lgtm

k8s-ci-robot · 2025-11-27T14:16:55Z

LGTM label has been added.

Details

Git tree hash: d1162f35f619c57138b97d2246b495931fdd341d

tenzen-y · 2025-11-28T09:16:18Z

/remove-kind cleanup
/kind api-change

Make waitForPodsReady.timeout required field

ec9c742

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Nov 27, 2025

k8s-ci-robot requested review from gabesaba and kannon92 November 27, 2025 12:33

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 27, 2025

Fix tests

11c6f37

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

mbobrovskyi reviewed Nov 27, 2025

View reviewed changes

Fix UTs

c5d6be3

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

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 Nov 27, 2025

drop the verification of in for conversions

e8296f2

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

k8s-ci-robot assigned mimowo Nov 27, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2025

mbobrovskyi reviewed Nov 27, 2025

View reviewed changes

tenzen-y commented Nov 27, 2025

View reviewed changes

Add a default timeout to customconfig test

1ecafc8

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2025

k8s-ci-robot requested a review from mimowo November 27, 2025 14:12

tenzen-y commented Nov 27, 2025

View reviewed changes

mimowo reviewed Nov 27, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2025

k8s-ci-robot merged commit f28acf1 into kubernetes-sigs:main Nov 27, 2025
29 checks passed

k8s-ci-robot added this to the v0.15 milestone Nov 27, 2025

tenzen-y deleted the make-required-field-wait-for-pods-ready branch November 27, 2025 15:19

k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Nov 28, 2025

	if in != nil && in.Enable && in.Timeout == nil {
	if in.Enable && in.Timeout == nil {

Make waitForPodsReady.timeout required field #7952

Make waitForPodsReady.timeout required field #7952

Conversation

tenzen-y commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

netlify bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Uh oh!

k8s-ci-robot commented Nov 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimowo commented Nov 27, 2025

Uh oh!

k8s-ci-robot commented Nov 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimowo commented Nov 27, 2025

Uh oh!

mimowo commented Nov 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimowo left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 27, 2025

Uh oh!

Uh oh!

tenzen-y commented Nov 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tenzen-y commented Nov 27, 2025 •

edited

Loading

netlify bot commented Nov 27, 2025 •

edited

Loading