Skip to content

v1beta2: Delete .enable field from WaitForPodsReady API in config #7628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
k8s-ci-robot merged 1 commit into from
Nov 14, 2025
Merged

v1beta2: Delete .enable field from WaitForPodsReady API in config #7628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions apis/config/v1beta1/configuration_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ func Convert_v1beta1_Configuration_To_v1beta2_Configuration(in *Configuration, o
if in.FairSharing == nil || !in.FairSharing.Enable {
out.FairSharing = nil
}
if in.WaitForPodsReady == nil || !in.WaitForPodsReady.Enable {
out.WaitForPodsReady = nil
}
return nil
}

Expand All @@ -59,3 +62,12 @@ func Convert_v1beta2_FairSharing_To_v1beta1_FairSharing(in *v1beta2.FairSharing,
out.Enable = true
return autoConvert_v1beta2_FairSharing_To_v1beta1_FairSharing(in, out, s)
}

func Convert_v1beta1_WaitForPodsReady_To_v1beta2_WaitForPodsReady(in *WaitForPodsReady, out *v1beta2.WaitForPodsReady, s conversionapi.Scope) error {
return autoConvert_v1beta1_WaitForPodsReady_To_v1beta2_WaitForPodsReady(in, out, s)
}

func Convert_v1beta2_WaitForPodsReady_To_v1beta1_WaitForPodsReady(in *v1beta2.WaitForPodsReady, out *WaitForPodsReady, s conversionapi.Scope) error {
out.Enable = true
return autoConvert_v1beta2_WaitForPodsReady_To_v1beta1_WaitForPodsReady(in, out, s)
}
37 changes: 37 additions & 0 deletions apis/config/v1beta1/configuration_conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,26 @@ func TestConfigurationQueueConvertTo(t *testing.T) {
FairSharing: nil,
},
},
"with WaitForPodsReady": {
input: &Configuration{
WaitForPodsReady: &WaitForPodsReady{
Enable: true,
},
},
expected: &v1beta2.Configuration{
WaitForPodsReady: &v1beta2.WaitForPodsReady{},
},
},
"with WaitForPodsReady disabled": {
input: &Configuration{
WaitForPodsReady: &WaitForPodsReady{
Enable: false,
},
},
expected: &v1beta2.Configuration{
WaitForPodsReady: nil,
},
},
}

for name, tc := range testCases {
Expand Down Expand Up @@ -94,6 +114,16 @@ func TestConfigurationQueueConvertFrom(t *testing.T) {
},
},
},
"with WaitForPodsReady": {
input: &v1beta2.Configuration{
WaitForPodsReady: &v1beta2.WaitForPodsReady{},
},
expected: &Configuration{
WaitForPodsReady: &WaitForPodsReady{
Enable: true,
},
},
},
}

for name, tc := range testCases {
Expand Down Expand Up @@ -124,6 +154,13 @@ func TestConfigurationQueueConversion_RoundTrip(t *testing.T) {
},
},
},
"with WaitForPodsReady": {
v1beta1Obj: &Configuration{
WaitForPodsReady: &WaitForPodsReady{
Enable: true,
},
},
},
}

for name, tc := range testCases {
Expand Down
53 changes: 29 additions & 24 deletions apis/config/v1beta1/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 2 additions & 4 deletions apis/config/v1beta2/configuration_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,6 @@ type ControllerConfigurationSpec struct {
// WaitForPodsReady defines configuration for the Wait For Pods Ready feature,
// which is used to ensure that all Pods are ready within the specified time.
type WaitForPodsReady struct {
Comment on lines 215 to 217
Copy link
Member

@tenzen-y tenzen-y Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If users want to enable w/ default parameters, how to specify that?
@mimowo @mbobrovskyi Do you assume the following one?

waitForPodsReady: {}

I am slightly histatin this way since It looks weird.

Copy link
Contributor Author

@mbobrovskyi mbobrovskyi Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, to specify default values you need to set:

waitForPodsReady: {}

For me, it’s also not obvious. I think using the enable field is clearer. And I would propose to revert this and also #7583 PRs.

But final decision for you @tenzen-y and @mimowo.

Copy link
Member

@tenzen-y tenzen-y Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so too, let me mention this in issue.

// Enable indicates whether to enable wait for pods ready feature.
// Defaults to false.
Enable bool `json:"enable,omitempty"`

// Timeout defines the time for an admitted workload to reach the
// PodsReady=true condition. When the timeout is exceeded, the workload
// evicted and requeued in the same cluster queue.
Expand All @@ -229,6 +225,8 @@ type WaitForPodsReady struct {
// BlockAdmission when true, cluster queue will block admissions for all
// subsequent jobs until the jobs reach the PodsReady=true condition.
// This setting is only honored when `Enable` is set to true.
// Defaults to true.
// +optional
BlockAdmission *bool `json:"blockAdmission,omitempty"`

// RequeuingStrategy defines the strategy for requeuing a Workload.
Expand Down
5 changes: 2 additions & 3 deletions apis/config/v1beta2/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,9 @@ func SetDefaults_Configuration(cfg *Configuration) {
cfg.ClientConnection.QPS = cmp.Or(cfg.ClientConnection.QPS, ptr.To(DefaultClientConnectionQPS))
cfg.ClientConnection.Burst = cmp.Or(cfg.ClientConnection.Burst, ptr.To(DefaultClientConnectionBurst))

cfg.WaitForPodsReady = cmp.Or(cfg.WaitForPodsReady, &WaitForPodsReady{Enable: false})
if cfg.WaitForPodsReady.Enable {
if cfg.WaitForPodsReady != nil {
cfg.WaitForPodsReady.Timeout = cmp.Or(cfg.WaitForPodsReady.Timeout, &metav1.Duration{Duration: defaultPodsReadyTimeout})
cfg.WaitForPodsReady.BlockAdmission = cmp.Or(cfg.WaitForPodsReady.BlockAdmission, &cfg.WaitForPodsReady.Enable)
cfg.WaitForPodsReady.BlockAdmission = cmp.Or(cfg.WaitForPodsReady.BlockAdmission, ptr.To(true))
Copy link
Contributor

@mimowo mimowo Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think we should change the default to "false" as part of v1beta2. This is much more commonly used setting.

Let me open: #7656

cfg.WaitForPodsReady.RequeuingStrategy = cmp.Or(cfg.WaitForPodsReady.RequeuingStrategy, &RequeuingStrategy{})
cfg.WaitForPodsReady.RequeuingStrategy.Timestamp = cmp.Or(cfg.WaitForPodsReady.RequeuingStrategy.Timestamp, ptr.To(EvictionTimestamp))
cfg.WaitForPodsReady.RequeuingStrategy.BackoffBaseSeconds = cmp.Or(cfg.WaitForPodsReady.RequeuingStrategy.BackoffBaseSeconds, ptr.To[int32](DefaultRequeuingBackoffBaseSeconds))
Expand Down
46 changes: 1 addition & 45 deletions apis/config/v1beta2/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
Integrations: defaultIntegrations,
MultiKueue: defaultMultiKueue,
ManagedJobsNamespaceSelector: defaultManagedJobsNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"defaulting ControllerManager": {
Expand Down Expand Up @@ -161,7 +160,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
Integrations: defaultIntegrations,
MultiKueue: defaultMultiKueue,
ManagedJobsNamespaceSelector: defaultManagedJobsNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"should not default ControllerManager": {
Expand Down Expand Up @@ -220,7 +218,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
Integrations: defaultIntegrations,
MultiKueue: defaultMultiKueue,
ManagedJobsNamespaceSelector: defaultManagedJobsNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"should not set LeaderElectionID": {
Expand Down Expand Up @@ -263,7 +260,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
Integrations: defaultIntegrations,
MultiKueue: defaultMultiKueue,
ManagedJobsNamespaceSelector: defaultManagedJobsNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"defaulting InternalCertManagement": {
Expand All @@ -282,7 +278,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
Integrations: overwriteNamespaceIntegrations,
MultiKueue: defaultMultiKueue,
ManagedJobsNamespaceSelector: overwriteNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"should not default InternalCertManagement": {
Expand All @@ -302,7 +297,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
Integrations: overwriteNamespaceIntegrations,
MultiKueue: defaultMultiKueue,
ManagedJobsNamespaceSelector: overwriteNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"should not default values in custom ClientConnection": {
Expand All @@ -329,7 +323,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
Integrations: overwriteNamespaceIntegrations,
MultiKueue: defaultMultiKueue,
ManagedJobsNamespaceSelector: overwriteNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"should default empty custom ClientConnection": {
Expand All @@ -350,21 +343,17 @@ func TestSetDefaults_Configuration(t *testing.T) {
Integrations: overwriteNamespaceIntegrations,
MultiKueue: defaultMultiKueue,
ManagedJobsNamespaceSelector: overwriteNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"defaulting waitForPodsReady values": {
original: &Configuration{
WaitForPodsReady: &WaitForPodsReady{
Enable: true,
},
WaitForPodsReady: &WaitForPodsReady{},
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
},
want: &Configuration{
WaitForPodsReady: &WaitForPodsReady{
Enable: true,
BlockAdmission: ptr.To(true),
Timeout: &podsReadyTimeout,
RecoveryTimeout: nil,
Expand All @@ -385,34 +374,9 @@ func TestSetDefaults_Configuration(t *testing.T) {
ManagedJobsNamespaceSelector: defaultManagedJobsNamespaceSelector,
},
},
"set waitForPodsReady.blockAdmission to false, and waitForPodsReady.recoveryTimeout to nil when enable is false": {
original: &Configuration{
WaitForPodsReady: &WaitForPodsReady{
Enable: false,
},
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
},
want: &Configuration{
WaitForPodsReady: &WaitForPodsReady{
Enable: false,
},
Namespace: ptr.To(DefaultNamespace),
ControllerManager: defaultCtrlManagerConfigurationSpec,
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
MultiKueue: defaultMultiKueue,
ManagedJobsNamespaceSelector: defaultManagedJobsNamespaceSelector,
},
},
"respecting provided waitForPodsReady values": {
original: &Configuration{
WaitForPodsReady: &WaitForPodsReady{
Enable: true,
Timeout: &podsReadyTimeoutOverwrite,
RequeuingStrategy: &RequeuingStrategy{
Timestamp: ptr.To(CreationTimestamp),
Expand All @@ -427,7 +391,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
},
want: &Configuration{
WaitForPodsReady: &WaitForPodsReady{
Enable: true,
BlockAdmission: ptr.To(true),
Timeout: &podsReadyTimeoutOverwrite,
RecoveryTimeout: &metav1.Duration{Duration: time.Minute},
Expand Down Expand Up @@ -469,7 +432,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
},
MultiKueue: defaultMultiKueue,
ManagedJobsNamespaceSelector: defaultManagedJobsNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"multiKueue": {
Expand Down Expand Up @@ -499,7 +461,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
DispatcherName: ptr.To(MultiKueueDispatcherModeIncremental),
},
ManagedJobsNamespaceSelector: defaultManagedJobsNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"multiKueue origin is an empty value": {
Expand Down Expand Up @@ -529,7 +490,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
DispatcherName: defaultMultiKueue.DispatcherName,
},
ManagedJobsNamespaceSelector: defaultManagedJobsNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"multiKueue GCInterval 0": {
Expand Down Expand Up @@ -557,7 +517,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
DispatcherName: defaultMultiKueue.DispatcherName,
},
ManagedJobsNamespaceSelector: defaultManagedJobsNamespaceSelector,
WaitForPodsReady: &WaitForPodsReady{},
},
},
"add default fair sharing configuration when enabled": {
Expand All @@ -580,7 +539,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
FairSharing: &FairSharing{
PreemptionStrategies: []PreemptionStrategy{LessThanOrEqualToFinalShare, LessThanInitialShare},
},
WaitForPodsReady: &WaitForPodsReady{},
},
},
"set object retention policy for workloads": {
Expand Down Expand Up @@ -611,7 +569,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
AfterDeactivatedByKueue: &metav1.Duration{Duration: 30 * time.Minute},
},
},
WaitForPodsReady: &WaitForPodsReady{},
},
},
"resources.transformations strategy": {
Expand Down Expand Up @@ -644,7 +601,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
{Input: corev1.ResourceEphemeralStorage, Strategy: ptr.To(DefaultResourceTransformationStrategy)},
},
},
WaitForPodsReady: &WaitForPodsReady{},
},
},
}
Expand Down
Loading