Fix pdb defaulting logic (#3200)

* Fix PDB defaulting logic * oop * chlog * fix unit * fix tests * Update docs
open-telemetry · Aug 7, 2024 · 0551975 · 0551975
1 parent 92652a5
commit 0551975
Show file tree

Hide file tree

Showing 17 changed files with 489 additions and 175 deletions.
diff --git a/.chloggen/fix-pdb-defaulting-logic.yaml b/.chloggen/fix-pdb-defaulting-logic.yaml
@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
+component: collector
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fixes a bug where the operator would default the PDB in the wrong place.
+
+# One or more tracking issues related to the change
+issues: [3198]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:
diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go
@@ -22,7 +22,6 @@ import (
 	"github.com/go-logr/logr"
 	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -92,35 +91,6 @@ func (c CollectorWebhook) Default(_ context.Context, obj runtime.Object) error {
 		}
 	}
 
-	// if pdb isn't provided, 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 otelcol.Spec.PodDisruptionBudget == nil {
-		otelcol.Spec.PodDisruptionBudget = &PodDisruptionBudgetSpec{
-			MaxUnavailable: &intstr.IntOrString{
-				Type:   intstr.Int,
-				IntVal: 1,
-			},
-		}
-	}
-
-	// if pdb isn't provided for target allocator and it's enabled
-	// using a valid strategy (consistent-hashing, per-node),
-	// 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 otelcol.Spec.TargetAllocator.Enabled &&
-		otelcol.Spec.TargetAllocator.AllocationStrategy != TargetAllocatorAllocationStrategyLeastWeighted &&
-		otelcol.Spec.TargetAllocator.PodDisruptionBudget == nil {
-		otelcol.Spec.TargetAllocator.PodDisruptionBudget = &PodDisruptionBudgetSpec{
-			MaxUnavailable: &intstr.IntOrString{
-				Type:   intstr.Int,
-				IntVal: 1,
-			},
-		}
-	}
-
 	if otelcol.Spec.Ingress.Type == IngressTypeRoute && otelcol.Spec.Ingress.Route.Termination == "" {
 		otelcol.Spec.Ingress.Route.Termination = TLSRouteTerminationTypeEdge
 	}

diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go
@@ -120,12 +120,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 						Args:            map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"},
 						ManagementState: ManagementStateManaged,
 						Replicas:        &one,
-						PodDisruptionBudget: &PodDisruptionBudgetSpec{
-							MaxUnavailable: &intstr.IntOrString{
-								Type:   intstr.Int,
-								IntVal: 1,
-							},
-						},
 					},
 					Mode:            ModeDeployment,
 					UpgradeStrategy: UpgradeStrategyAutomatic,
@@ -156,12 +150,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 						Args:            map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"},
 						Replicas:        &five,
 						ManagementState: ManagementStateManaged,
-						PodDisruptionBudget: &PodDisruptionBudgetSpec{
-							MaxUnavailable: &intstr.IntOrString{
-								Type:   intstr.Int,
-								IntVal: 1,
-							},
-						},
 					},
 				},
 			},
@@ -191,12 +179,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 						Args:            map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"},
 						Replicas:        &five,
 						ManagementState: ManagementStateUnmanaged,
-						PodDisruptionBudget: &PodDisruptionBudgetSpec{
-							MaxUnavailable: &intstr.IntOrString{
-								Type:   intstr.Int,
-								IntVal: 1,
-							},
-						},
 					},
 				},
 			},
@@ -224,12 +206,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 						Args:            map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"},
 						Replicas:        &one,
 						ManagementState: ManagementStateManaged,
-						PodDisruptionBudget: &PodDisruptionBudgetSpec{
-							MaxUnavailable: &intstr.IntOrString{
-								Type:   intstr.Int,
-								IntVal: 1,
-							},
-						},
 					},
 					Autoscaler: &AutoscalerSpec{
 						TargetCPUUtilization: &defaultCPUTarget,
@@ -261,12 +237,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 						Args:            map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"},
 						ManagementState: ManagementStateManaged,
 						Replicas:        &one,
-						PodDisruptionBudget: &PodDisruptionBudgetSpec{
-							MaxUnavailable: &intstr.IntOrString{
-								Type:   intstr.Int,
-								IntVal: 1,
-							},
-						},
 					},
 					Ingress: Ingress{
 						Type: IngressTypeRoute,
@@ -345,12 +315,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 						Args:            map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"},
 						Replicas:        &one,
 						ManagementState: ManagementStateManaged,
-						PodDisruptionBudget: &PodDisruptionBudgetSpec{
-							MaxUnavailable: &intstr.IntOrString{
-								Type:   intstr.Int,
-								IntVal: 1,
-							},
-						},
 					},
 					UpgradeStrategy: UpgradeStrategyAutomatic,
 					TargetAllocator: TargetAllocatorEmbedded{
@@ -396,12 +360,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 						Args:            map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"},
 						Replicas:        &one,
 						ManagementState: ManagementStateManaged,
-						PodDisruptionBudget: &PodDisruptionBudgetSpec{
-							MaxUnavailable: &intstr.IntOrString{
-								Type:   intstr.Int,
-								IntVal: 1,
-							},
-						},
 					},
 					UpgradeStrategy: UpgradeStrategyAutomatic,
 					TargetAllocator: TargetAllocatorEmbedded{
@@ -442,24 +400,12 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 						Args:            map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"},
 						Replicas:        &one,
 						ManagementState: ManagementStateManaged,
-						PodDisruptionBudget: &PodDisruptionBudgetSpec{
-							MaxUnavailable: &intstr.IntOrString{
-								Type:   intstr.Int,
-								IntVal: 1,
-							},
-						},
 					},
 					UpgradeStrategy: UpgradeStrategyAutomatic,
 					TargetAllocator: TargetAllocatorEmbedded{
 						Enabled:            true,
 						Replicas:           &one,
 						AllocationStrategy: TargetAllocatorAllocationStrategyConsistentHashing,
-						PodDisruptionBudget: &PodDisruptionBudgetSpec{
-							MaxUnavailable: &intstr.IntOrString{
-								Type:   intstr.Int,
-								IntVal: 1,
-							},
-						},
 					},
 				},
 			},
@@ -487,12 +433,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 						Args:            map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"},
 						Replicas:        &one,
 						ManagementState: ManagementStateManaged,
-						PodDisruptionBudget: &PodDisruptionBudgetSpec{
-							MaxUnavailable: &intstr.IntOrString{
-								Type:   intstr.Int,
-								IntVal: 1,
-							},
-						},
 					},
 					UpgradeStrategy: UpgradeStrategyAutomatic,
 					TargetAllocator: TargetAllocatorEmbedded{

diff --git a/apis/v1beta1/common.go b/apis/v1beta1/common.go
@@ -116,7 +116,8 @@ type OpenTelemetryCommonFields struct {
 	// +optional
 	Replicas *int32 `json:"replicas,omitempty"`
 	// PodDisruptionBudget specifies the pod disruption budget configuration to use
-	// for the generated workload.
+	// for the generated workload. By default, a PDB with a MaxUnavailable of one is set.
+	//
 	// +optional
 	PodDisruptionBudget *PodDisruptionBudgetSpec `json:"podDisruptionBudget,omitempty"`
 	// SecurityContext configures the container security context for

diff --git a/apis/v1beta1/opentelemetrycollector_types.go b/apis/v1beta1/opentelemetrycollector_types.go
@@ -216,7 +216,8 @@ type TargetAllocatorEmbedded struct {
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Observability"
 	Observability ObservabilitySpec `json:"observability,omitempty"`
 	// PodDisruptionBudget specifies the pod disruption budget configuration to use
-	// for the target allocator workload.
+	// for the target allocator workload. By default, a PDB with a MaxUnavailable of one is set for a valid
+	// allocation strategy.
 	//
 	// +optional
 	PodDisruptionBudget *PodDisruptionBudgetSpec `json:"podDisruptionBudget,omitempty"`