Repository: kubernetes-sigs/kueue

Issue:
v1beta2: change default for waitForPodsReady.blockAdmission to false
<!-- Please only use this template for submitting clean up requests -->

**What would you like to be cleaned**:

I would like to change the default for blockAdmission to false

Before we change the logic it would be nice to send a preparatory PR to documenation which already suggests using `blockAdmission: false` in most places.

Part of https://github.com/kubernetes-sigs/kueue/issues/7113

**Why is this needed**:

blockAdmission=true is used more rarely because:
- it blocks admission during 2-phase scheduling, so it is not useable with AdmissionChecks essentially, which is surprising to users 
- it is very slow to schedule workloads sequentially completly
- blockAdmission=true was introduced to prevent deadlocks which now can be avoided better with TopologyAwareScheduling

Discussion:
@mimowo: cc @tenzen-y @mbobrovskyi @kannon92: It sounds like you really actually want to deprecate blockAdmission: true?@mimowo: > It sounds like you really actually want to deprecate blockAdmission: true?

Good question, I didn't think about it. I think for that we need additional feedback from the community. 

However, all installations I've seen so far use `blockAdmission: false`.@kannon92: > blockAdmission=true was introduced to prevent deadlocks which now can be avoided better with TopologyAwareScheduling

This is saying that our recommendation would be to use TAS anyway?@mimowo: I think so, I think TAS is pretty good these days, and solves the important issue of fragmentation, essentially providing gang scheduling.

Sure some users don't know the datacenter topology, but then they can just use `kubernetes.io/hostname` as a single level.@mimowo: /assign @mbobrovskyi 
ptal@kannon92: > Before we change the logic it would be nice to send a preparatory PR to documenation which already suggests using blockAdmission: false in most places.

https://github.com/kubernetes-sigs/kueue/pull/7676

We do set blockAdmission: true in WaitForPodsReady documentation but from what I can tell that is intential for our example.

So I left that one alone.@tenzen-y: I'm ok with changing the default value to `true`. But, I think that the `blockAdmission` functionality is still valuable.
So, keeping both of TAS and blockAdmission sounds reasonable.


Solution patch:
diff --git a/apis/config/v1beta1/configuration_conversion.go b/apis/config/v1beta1/configuration_conversion.go
index d23470cf4..2482ac8e8 100644
--- a/apis/config/v1beta1/configuration_conversion.go
+++ b/apis/config/v1beta1/configuration_conversion.go
@@ -18,6 +18,7 @@ package v1beta1
 
 import (
 	conversionapi "k8s.io/apimachinery/pkg/conversion"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/conversion"
 
 	"sigs.k8s.io/kueue/apis/config/v1beta2"
@@ -64,10 +65,22 @@ func Convert_v1beta2_FairSharing_To_v1beta1_FairSharing(in *v1beta2.FairSharing,
 }
 
 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)
+	if err := autoConvert_v1beta1_WaitForPodsReady_To_v1beta2_WaitForPodsReady(in, out, s); err != nil {
+		return err
+	}
+	if in.BlockAdmission == nil {
+		out.BlockAdmission = ptr.To(true)
+	}
+	return nil
 }
 
 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)
+	if err := autoConvert_v1beta2_WaitForPodsReady_To_v1beta1_WaitForPodsReady(in, out, s); err != nil {
+		return err
+	}
+	if in.BlockAdmission == nil {
+		out.BlockAdmission = ptr.To(false)
+	}
+	return nil
 }
diff --git a/apis/config/v1beta2/configuration_types.go b/apis/config/v1beta2/configuration_types.go
index 28370b1e5..8c18113cc 100644
--- a/apis/config/v1beta2/configuration_types.go
+++ b/apis/config/v1beta2/configuration_types.go
@@ -222,10 +222,9 @@ type WaitForPodsReady struct {
 	// +optional
 	Timeout *metav1.Duration `json:"timeout,omitempty"`
 
-	// BlockAdmission when true, cluster queue will block admissions for all
+	// BlockAdmission when true, the 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.
+	// Defaults to false.
 	// +optional
 	BlockAdmission *bool `json:"blockAdmission,omitempty"`
 
@@ -238,8 +237,7 @@ type WaitForPodsReady struct {
 	// Such a transition may happen when a Pod failed and the replacement Pod
 	// is awaited to be scheduled.
 	// After exceeding the timeout the corresponding job gets suspended again
-	// and requeued after the backoff delay. The timeout is enforced only if waitForPodsReady.enable=true.
-	// If not set, there is no timeout.
+	// and requeued after the backoff delay.
 	// +optional
 	RecoveryTimeout *metav1.Duration `json:"recoveryTimeout,omitempty"`
 }
diff --git a/apis/config/v1beta2/defaults.go b/apis/config/v1beta2/defaults.go
index ca6dadb09..60a221ba5 100644
--- a/apis/config/v1beta2/defaults.go
+++ b/apis/config/v1beta2/defaults.go
@@ -94,7 +94,7 @@ func SetDefaults_Configuration(cfg *Configuration) {
 
 	if cfg.WaitForPodsReady != nil {
 		cfg.WaitForPodsReady.Timeout = cmp.Or(cfg.WaitForPodsReady.Timeout, &metav1.Duration{Duration: defaultPodsReadyTimeout})
-		cfg.WaitForPodsReady.BlockAdmission = cmp.Or(cfg.WaitForPodsReady.BlockAdmission, ptr.To(true))
+		cfg.WaitForPodsReady.BlockAdmission = cmp.Or(cfg.WaitForPodsReady.BlockAdmission, ptr.To(false))
 		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))
diff --git a/site/content/en/docs/reference/kueue-config.v1beta2.md b/site/content/en/docs/reference/kueue-config.v1beta2.md
index 667301d81..40391b9ad 100644
--- a/site/content/en/docs/reference/kueue-config.v1beta2.md
+++ b/site/content/en/docs/reference/kueue-config.v1beta2.md
@@ -842,10 +842,9 @@ Defaults to 5min.</p>
 <code>bool</code>
 </td>
 <td>
-   <p>BlockAdmission when true, cluster queue will block admissions for all
+   <p>BlockAdmission when true, the cluster queue will block admissions for all
 subsequent jobs until the jobs reach the PodsReady=true condition.
-This setting is only honored when <code>Enable</code> is set to true.
-Defaults to true.</p>
+Defaults to false.</p>
 </td>
 </tr>
 <tr><td><code>requeuingStrategy</code><br/>
@@ -864,8 +863,7 @@ last transition to the PodsReady=false condition after a Workload is Admitted an
 Such a transition may happen when a Pod failed and the replacement Pod
 is awaited to be scheduled.
 After exceeding the timeout the corresponding job gets suspended again
-and requeued after the backoff delay. The timeout is enforced only if waitForPodsReady.enable=true.
-If not set, there is no timeout.</p>
+and requeued after the backoff delay.</p>
 </td>
 </tr>
 </tbody>



Test patch:
diff --git a/apis/config/v1beta1/configuration_conversion_test.go b/apis/config/v1beta1/configuration_conversion_test.go
index 61a199299..47da3a65c 100644
--- a/apis/config/v1beta1/configuration_conversion_test.go
+++ b/apis/config/v1beta1/configuration_conversion_test.go
@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"k8s.io/utils/ptr"
 
 	"sigs.k8s.io/kueue/apis/config/v1beta2"
 )
@@ -64,7 +65,9 @@ func TestConfigurationQueueConvertTo(t *testing.T) {
 				},
 			},
 			expected: &v1beta2.Configuration{
-				WaitForPodsReady: &v1beta2.WaitForPodsReady{},
+				WaitForPodsReady: &v1beta2.WaitForPodsReady{
+					BlockAdmission: ptr.To(true),
+				},
 			},
 		},
 		"with WaitForPodsReady disabled": {
@@ -120,7 +123,8 @@ func TestConfigurationQueueConvertFrom(t *testing.T) {
 			},
 			expected: &Configuration{
 				WaitForPodsReady: &WaitForPodsReady{
-					Enable: true,
+					Enable:         true,
+					BlockAdmission: ptr.To(false),
 				},
 			},
 		},
@@ -154,13 +158,6 @@ func TestConfigurationQueueConversion_RoundTrip(t *testing.T) {
 				},
 			},
 		},
-		"with WaitForPodsReady": {
-			v1beta1Obj: &Configuration{
-				WaitForPodsReady: &WaitForPodsReady{
-					Enable: true,
-				},
-			},
-		},
 	}
 
 	for name, tc := range testCases {
diff --git a/apis/config/v1beta2/defaults_test.go b/apis/config/v1beta2/defaults_test.go
index 1083ec1eb..71fd254f9 100644
--- a/apis/config/v1beta2/defaults_test.go
+++ b/apis/config/v1beta2/defaults_test.go
@@ -354,7 +354,7 @@ func TestSetDefaults_Configuration(t *testing.T) {
 			},
 			want: &Configuration{
 				WaitForPodsReady: &WaitForPodsReady{
-					BlockAdmission:  ptr.To(true),
+					BlockAdmission:  ptr.To(false),
 					Timeout:         &podsReadyTimeout,
 					RecoveryTimeout: nil,
 					RequeuingStrategy: &RequeuingStrategy{
@@ -391,7 +391,7 @@ func TestSetDefaults_Configuration(t *testing.T) {
 			},
 			want: &Configuration{
 				WaitForPodsReady: &WaitForPodsReady{
-					BlockAdmission:  ptr.To(true),
+					BlockAdmission:  ptr.To(false),
 					Timeout:         &podsReadyTimeoutOverwrite,
 					RecoveryTimeout: &metav1.Duration{Duration: time.Minute},
 					RequeuingStrategy: &RequeuingStrategy{
diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
index 75adf8ffd..499871fcd 100644
--- a/pkg/config/config_test.go
+++ b/pkg/config/config_test.go
@@ -202,7 +202,7 @@ apiVersion: config.kueue.x-k8s.io/v1beta2
 kind: Configuration
 waitForPodsReady:
   timeout: 50s
-  blockAdmission: false
+  blockAdmission: true
   recoveryTimeout: 3m
   requeuingStrategy:
     timestamp: Creation
@@ -619,7 +619,7 @@ objectRetentionPolicies:
 				ManageJobsWithoutQueueName: false,
 				InternalCertManagement:     enableDefaultInternalCertManagement,
 				WaitForPodsReady: &configapi.WaitForPodsReady{
-					BlockAdmission:  ptr.To(false),
+					BlockAdmission:  ptr.To(true),
 					Timeout:         &metav1.Duration{Duration: 50 * time.Second},
 					RecoveryTimeout: &metav1.Duration{Duration: 3 * time.Minute},
 					RequeuingStrategy: &configapi.RequeuingStrategy{