Default to minAvailable: 1 in Servings PodDisruptionBudgets (#2985)

* Default to minAvailable: 1 in Servings PodDisruptionBudgetsi

* Add PDBs to extension tests

Author: ReToCode

openshift-knative-operator/pkg/serving/extension.go

@@ -141,6 +141,9 @@ func (e *extension) Reconcile(ctx context.Context, comp base.KComponent) error {
 		}
 	}
 
+	// Default to `1` as PodDisruptionBudget if not specified.
+	defaultToOneAsPodDisruptionBudget(ks)
+
 	// Apply an Ingress config with Kourier enabled if nothing else is defined.
 	defaultToKourier(ks)
 	common.ConfigureIfUnset(&ks.Spec.CommonSpec, "network", "ingress.class", defaultIngressClass(ks))

openshift-knative-operator/pkg/serving/extension_test.go

@@ -15,18 +15,20 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 	routev1 "github.com/openshift/api/route/v1"
 	corev1 "k8s.io/api/core/v1"
+	policyv1 "k8s.io/api/policy/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/version"
 	fakediscovery "k8s.io/client-go/discovery/fake"
+	"k8s.io/utils/ptr"
 	"knative.dev/operator/pkg/apis/operator/base"
 	operatorv1beta1 "knative.dev/operator/pkg/apis/operator/v1beta1"
 	operator "knative.dev/operator/pkg/reconciler/common"
 	"knative.dev/pkg/apis"
 	kubeclient "knative.dev/pkg/client/injection/kube/client"
 	kubefake "knative.dev/pkg/client/injection/kube/client/fake"
-	"knative.dev/pkg/ptr"
 )
 
 var (
@@ -81,13 +83,13 @@ func TestReconcile(t *testing.T) {
 			Spec: operatorv1beta1.KnativeServingSpec{
 				CommonSpec: base.CommonSpec{
 					HighAvailability: &base.HighAvailability{
-						Replicas: ptr.Int32(3),
+						Replicas: ptr.To(int32(3)),
 					},
 				},
 			},
 		},
 		expected: ks(func(ks *operatorv1beta1.KnativeServing) {
-			ks.Spec.HighAvailability.Replicas = ptr.Int32(3)
+			ks.Spec.HighAvailability.Replicas = ptr.To(int32(3))
 		}),
 	}, {
 		name: "different certificate settings",
@@ -504,7 +506,20 @@ func ks(mods ...func(*operatorv1beta1.KnativeServing)) *operatorv1beta1.KnativeS
 		Spec: operatorv1beta1.KnativeServingSpec{
 			CommonSpec: base.CommonSpec{
 				HighAvailability: &base.HighAvailability{
-					Replicas: ptr.Int32(2),
+					Replicas: ptr.To(int32(2)),
+				},
+				PodDisruptionBudgetOverride: []base.PodDisruptionBudgetOverride{
+					{
+						Name: "activator-pdb",
+						PodDisruptionBudgetSpec: policyv1.PodDisruptionBudgetSpec{
+							MinAvailable: ptr.To(intstr.IntOrString{IntVal: 1, Type: intstr.Int}),
+						},
+					}, {
+						Name: "webhook-pdb",
+						PodDisruptionBudgetSpec: policyv1.PodDisruptionBudgetSpec{
+							MinAvailable: ptr.To(intstr.IntOrString{IntVal: 1, Type: intstr.Int}),
+						},
+					},
 				},
 				Config: base.ConfigMapData{
 					"deployment": map[string]string{

openshift-knative-operator/pkg/serving/poddisruptionbudget.go

@@ -0,0 +1,42 @@
+package serving
+
+import (
+	policyv1 "k8s.io/api/policy/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/utils/ptr"
+	"knative.dev/operator/pkg/apis/operator/base"
+	operatorv1beta1 "knative.dev/operator/pkg/apis/operator/v1beta1"
+)
+
+var servingPDBNames = []string{"activator-pdb", "webhook-pdb"}
+
+// Upstream has a PodDisruptionBudgetOverride with minAvailable: 80% which does not work with
+// HighAvailability of two Pods. We need to override this to minAvailable: 1 if the user did not specify
+// another value.
+func defaultToOneAsPodDisruptionBudget(ks *operatorv1beta1.KnativeServing) {
+	overrides := ks.GetSpec().GetPodDisruptionBudgetOverride()
+
+	if overrides == nil {
+		overrides = []base.PodDisruptionBudgetOverride{}
+	}
+
+	for _, pdbName := range servingPDBNames {
+		if !hasOverride(overrides, pdbName) {
+			ks.Spec.PodDisruptionBudgetOverride = append(ks.Spec.PodDisruptionBudgetOverride, base.PodDisruptionBudgetOverride{
+				Name: pdbName,
+				PodDisruptionBudgetSpec: policyv1.PodDisruptionBudgetSpec{
+					MinAvailable: ptr.To(intstr.IntOrString{Type: intstr.Int, IntVal: 1}),
+				},
+			})
+		}
+	}
+}
+
+func hasOverride(overrides []base.PodDisruptionBudgetOverride, pdbName string) bool {
+	for _, override := range overrides {
+		if override.Name == pdbName {
+			return true
+		}
+	}
+	return false
+}

openshift-knative-operator/pkg/serving/poddisruptionbudget_test.go

@@ -0,0 +1,81 @@
+package serving
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	policyv1 "k8s.io/api/policy/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/utils/ptr"
+	"knative.dev/operator/pkg/apis/operator/base"
+	operatorv1beta1 "knative.dev/operator/pkg/apis/operator/v1beta1"
+)
+
+func TestDefaultToOneAsPodDisruptionBudget(t *testing.T) {
+	tests := []struct {
+		name     string
+		ks       *operatorv1beta1.KnativeServing
+		expected []base.PodDisruptionBudgetOverride
+	}{
+		{
+			name: "no overrides",
+			ks: &operatorv1beta1.KnativeServing{
+				Spec: operatorv1beta1.KnativeServingSpec{},
+			},
+			expected: []base.PodDisruptionBudgetOverride{
+				{
+					Name: "activator-pdb",
+					PodDisruptionBudgetSpec: policyv1.PodDisruptionBudgetSpec{
+						MinAvailable: ptr.To(intstr.IntOrString{Type: intstr.Int, IntVal: 1}),
+					},
+				},
+				{
+					Name: "webhook-pdb",
+					PodDisruptionBudgetSpec: policyv1.PodDisruptionBudgetSpec{
+						MinAvailable: ptr.To(intstr.IntOrString{Type: intstr.Int, IntVal: 1}),
+					},
+				},
+			},
+		},
+		{
+			name: "with existing overrides",
+			ks: &operatorv1beta1.KnativeServing{
+				Spec: operatorv1beta1.KnativeServingSpec{
+					CommonSpec: base.CommonSpec{
+						PodDisruptionBudgetOverride: []base.PodDisruptionBudgetOverride{
+							{
+								Name: "activator-pdb",
+								PodDisruptionBudgetSpec: policyv1.PodDisruptionBudgetSpec{
+									MinAvailable: ptr.To(intstr.IntOrString{Type: intstr.Int, IntVal: 2}),
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: []base.PodDisruptionBudgetOverride{
+				{
+					Name: "activator-pdb",
+					PodDisruptionBudgetSpec: policyv1.PodDisruptionBudgetSpec{
+						MinAvailable: ptr.To(intstr.IntOrString{Type: intstr.Int, IntVal: 2}),
+					},
+				},
+				{
+					Name: "webhook-pdb",
+					PodDisruptionBudgetSpec: policyv1.PodDisruptionBudgetSpec{
+						MinAvailable: ptr.To(intstr.IntOrString{Type: intstr.Int, IntVal: 1}),
+					},
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			defaultToOneAsPodDisruptionBudget(tt.ks)
+			if diff := cmp.Diff(tt.expected, tt.ks.Spec.PodDisruptionBudgetOverride); diff != "" {
+				t.Errorf("PodDisruptionBudgetOverride mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}