feat: Add pdb support for target allocator

.chloggen/target-allocator-pdb.yaml

@@ -2,7 +2,7 @@
 change_type: enhancement
 
 # The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action)
-component: operator
+component: target allocator
 
 # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
 note: PDB support for target allocator

apis/v1alpha1/collector_webhook.go

@@ -139,11 +139,14 @@ func (c CollectorWebhook) defaulter(r *OpenTelemetryCollector) error {
 		}
 	}
 
-	// if pdb isn't provided for target allocator and it's enabled,
+	// if pdb isn't provided for target allocator and it's enabled
+	// using a valid strategy (consistent-hashing),
 	// we set MaxUnavailable 1, which will work even if there is
 	// just one replica, not blocking node drains but preventing
 	// out-of-the-box from disruption generated by them with replicas > 1
-	if r.Spec.TargetAllocator.Enabled && r.Spec.TargetAllocator.PodDisruptionBudget == nil {
+	if r.Spec.TargetAllocator.Enabled &&
+		r.Spec.TargetAllocator.AllocationStrategy == OpenTelemetryTargetAllocatorAllocationStrategyConsistentHashing &&
+		r.Spec.TargetAllocator.PodDisruptionBudget == nil {
 		r.Spec.TargetAllocator.PodDisruptionBudget = &PodDisruptionBudgetSpec{
 			MaxUnavailable: &intstr.IntOrString{
 				Type:   intstr.Int,

apis/v1alpha1/collector_webhook_test.go

@@ -242,7 +242,7 @@ func TestOTELColDefaultingWebhook(t *testing.T) {
 			},
 		},
 		{
-			name: "Defined PDB",
+			name: "Defined PDB for collector",
 			otelcol: OpenTelemetryCollector{
 				Spec: OpenTelemetryCollectorSpec{
 					Mode: ModeDeployment,
@@ -274,6 +274,133 @@ func TestOTELColDefaultingWebhook(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Defined PDB for target allocator",
+			otelcol: OpenTelemetryCollector{
+				Spec: OpenTelemetryCollectorSpec{
+					Mode: ModeDeployment,
+					TargetAllocator: OpenTelemetryTargetAllocator{
+						Enabled:            true,
+						AllocationStrategy: OpenTelemetryTargetAllocatorAllocationStrategyConsistentHashing,
+						PodDisruptionBudget: &PodDisruptionBudgetSpec{
+							MinAvailable: &intstr.IntOrString{
+								Type:   intstr.String,
+								StrVal: "10%",
+							},
+						},
+					},
+				},
+			},
+			expected: OpenTelemetryCollector{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						"app.kubernetes.io/managed-by": "opentelemetry-operator",
+					},
+				},
+				Spec: OpenTelemetryCollectorSpec{
+					Mode:            ModeDeployment,
+					Replicas:        &one,
+					UpgradeStrategy: UpgradeStrategyAutomatic,
+					ManagementState: ManagementStateManaged,
+					PodDisruptionBudget: &PodDisruptionBudgetSpec{
+						MaxUnavailable: &intstr.IntOrString{
+							Type:   intstr.Int,
+							IntVal: 1,
+						},
+					},
+					TargetAllocator: OpenTelemetryTargetAllocator{
+						Enabled:            true,
+						Replicas:           &one,
+						AllocationStrategy: OpenTelemetryTargetAllocatorAllocationStrategyConsistentHashing,
+						PodDisruptionBudget: &PodDisruptionBudgetSpec{
+							MinAvailable: &intstr.IntOrString{
+								Type:   intstr.String,
+								StrVal: "10%",
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "Undefined PDB for target allocator and consistent-hashing strategy",
+			otelcol: OpenTelemetryCollector{
+				Spec: OpenTelemetryCollectorSpec{
+					Mode: ModeDeployment,
+					TargetAllocator: OpenTelemetryTargetAllocator{
+						Enabled:            true,
+						Replicas:           &one,
+						AllocationStrategy: OpenTelemetryTargetAllocatorAllocationStrategyConsistentHashing,
+					},
+				},
+			},
+			expected: OpenTelemetryCollector{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						"app.kubernetes.io/managed-by": "opentelemetry-operator",
+					},
+				},
+				Spec: OpenTelemetryCollectorSpec{
+					Mode:            ModeDeployment,
+					Replicas:        &one,
+					UpgradeStrategy: UpgradeStrategyAutomatic,
+					ManagementState: ManagementStateManaged,
+					PodDisruptionBudget: &PodDisruptionBudgetSpec{
+						MaxUnavailable: &intstr.IntOrString{
+							Type:   intstr.Int,
+							IntVal: 1,
+						},
+					},
+					TargetAllocator: OpenTelemetryTargetAllocator{
+						Enabled:            true,
+						Replicas:           &one,
+						AllocationStrategy: OpenTelemetryTargetAllocatorAllocationStrategyConsistentHashing,
+						PodDisruptionBudget: &PodDisruptionBudgetSpec{
+							MaxUnavailable: &intstr.IntOrString{
+								Type:   intstr.Int,
+								IntVal: 1,
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "Undefined PDB for target allocator and not consistent-hashing strategy",
+			otelcol: OpenTelemetryCollector{
+				Spec: OpenTelemetryCollectorSpec{
+					Mode: ModeDeployment,
+					TargetAllocator: OpenTelemetryTargetAllocator{
+						Enabled:            true,
+						AllocationStrategy: OpenTelemetryTargetAllocatorAllocationStrategyLeastWeighted,
+					},
+				},
+			},
+			expected: OpenTelemetryCollector{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						"app.kubernetes.io/managed-by": "opentelemetry-operator",
+					},
+				},
+				Spec: OpenTelemetryCollectorSpec{
+					Mode:            ModeDeployment,
+					Replicas:        &one,
+					UpgradeStrategy: UpgradeStrategyAutomatic,
+					ManagementState: ManagementStateManaged,
+					PodDisruptionBudget: &PodDisruptionBudgetSpec{
+						MaxUnavailable: &intstr.IntOrString{
+							Type:   intstr.Int,
+							IntVal: 1,
+						},
+					},
+					TargetAllocator: OpenTelemetryTargetAllocator{
+						Enabled:            true,
+						Replicas:           &one,
+						AllocationStrategy: OpenTelemetryTargetAllocatorAllocationStrategyLeastWeighted,
+					},
+				},
+			},
+		},
 	}
 
 	for _, test := range tests {

internal/manifests/targetallocator/poddisruptionbudget.go

@@ -15,6 +15,7 @@
 package targetallocator
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
@@ -26,17 +27,20 @@ import (
 )
 
 func PodDisruptionBudget(params manifests.Params) (*policyV1.PodDisruptionBudget, error) {
-	if params.OtelCol.Spec.TargetAllocator.AllocationStrategy != v1alpha1.OpenTelemetryTargetAllocatorAllocationStrategyConsistentHashing {
-		params.Log.Info("current allocation strategy not compatible, skipping podDisruptionBudget creation")
-		return nil, nil
-	}
-
-	// defaulting webhook should always set this, but if unset then return nil.
+	// defaulting webhook should set this if the strategy is compatible, but if unset then return nil.
 	if params.OtelCol.Spec.TargetAllocator.PodDisruptionBudget == nil {
 		params.Log.Info("pdb field is unset in Spec, skipping podDisruptionBudget creation")
 		return nil, nil
 	}
 
+	// defaulter doesn't set PodDisruptionBudget if the strategy isn't valid,
+	// if PodDisruptionBudget != nil and stategy isn't correct, users have set
+	// it wrongly
+	if params.OtelCol.Spec.TargetAllocator.AllocationStrategy != v1alpha1.OpenTelemetryTargetAllocatorAllocationStrategyConsistentHashing {
+		params.Log.Info("current allocation strategy not compatible, skipping podDisruptionBudget creation")
+		return nil, fmt.Errorf("target allocator pdb has been configured but the allocation strategy isn't not compatible")
+	}
+
 	name := naming.TAPodDisruptionBudget(params.OtelCol.Name)
 	version := strings.Split(params.OtelCol.Spec.Image, ":")
 	labels := Labels(params.OtelCol, name)

internal/manifests/targetallocator/poddisruptionbudget_test.go

@@ -122,8 +122,31 @@ func TestPDBWithNotValidStrategy(t *testing.T) {
 			})
 
 			// verify
-			assert.NoError(t, err)
+			assert.Error(t, err)
 			assert.Nil(t, pdb)
 		})
 	}
 }
+
+func TestNoPDB(t *testing.T) {
+	otelcol := v1alpha1.OpenTelemetryCollector{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "my-instance",
+		},
+		Spec: v1alpha1.OpenTelemetryCollectorSpec{
+			TargetAllocator: v1alpha1.OpenTelemetryTargetAllocator{
+				AllocationStrategy: v1alpha1.OpenTelemetryTargetAllocatorAllocationStrategyLeastWeighted,
+			},
+		},
+	}
+	configuration := config.New()
+	pdb, err := PodDisruptionBudget(manifests.Params{
+		Log:     logger,
+		Config:  configuration,
+		OtelCol: otelcol,
+	})
+
+	// verify
+	assert.NoError(t, err)
+	assert.Nil(t, pdb)
+}

tests/e2e-pdb/pdb/00-assert.yaml

@@ -0,0 +1,9 @@
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+  name: pdb-collector
+spec:
+  selector:
+    matchLabels:
+      app.kubernetes.io/component: opentelemetry-collector
+  minAvailable: 1

tests/e2e-pdb/pdb/00-install.yaml

@@ -1,4 +1,32 @@
-apiVersion: v1
-kind: Namespace
+apiVersion: opentelemetry.io/v1alpha1
+kind: OpenTelemetryCollector
 metadata:
-  name: create-pdbs
+  name: pdb
+spec:
+  podDisruptionBudget:
+    minAvailable: 1
+  resources:
+    limits:
+      cpu: 500m
+      memory: 128Mi
+    requests:
+      cpu: 5m
+      memory: 64Mi
+
+  config: |
+    receivers:
+      otlp:
+        protocols:
+          grpc:
+          http:
+    processors:
+
+    exporters:
+      logging:
+
+    service:
+      pipelines:
+        traces:
+          receivers: [otlp]
+          processors: []
+          exporters: [logging]