Allow setting of PDB for per-node (#2932)

* create PDB automatically for per-node

* resolved

* fix ref

Author: jaronoff97

.chloggen/resolve-easy-miss.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: target-allocator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fixes a bug that didn't automatically create a PDB for a TA with per-node strategy
+
+# One or more tracking issues related to the change
+issues: [2900]
+
+# (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:

apis/v1beta1/collector_webhook.go

@@ -133,12 +133,12 @@ func (c CollectorWebhook) Default(_ context.Context, obj runtime.Object) error {
 	}
 
 	// if pdb isn't provided for target allocator and it's enabled
-	// using a valid strategy (consistent-hashing),
+	// 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 == TargetAllocatorAllocationStrategyConsistentHashing &&
+		otelcol.Spec.TargetAllocator.AllocationStrategy != TargetAllocatorAllocationStrategyLeastWeighted &&
 		otelcol.Spec.TargetAllocator.PodDisruptionBudget == nil {
 		otelcol.Spec.TargetAllocator.PodDisruptionBudget = &PodDisruptionBudgetSpec{
 			MaxUnavailable: &intstr.IntOrString{

apis/v1beta1/collector_webhook_test.go

@@ -360,6 +360,56 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Defined PDB for target allocator per-node",
+			otelcol: OpenTelemetryCollector{
+				Spec: OpenTelemetryCollectorSpec{
+					Mode: ModeDeployment,
+					TargetAllocator: TargetAllocatorEmbedded{
+						Enabled:            true,
+						AllocationStrategy: TargetAllocatorAllocationStrategyPerNode,
+						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,
+					OpenTelemetryCommonFields: OpenTelemetryCommonFields{
+						Replicas:        &one,
+						ManagementState: ManagementStateManaged,
+						PodDisruptionBudget: &PodDisruptionBudgetSpec{
+							MaxUnavailable: &intstr.IntOrString{
+								Type:   intstr.Int,
+								IntVal: 1,
+							},
+						},
+					},
+					UpgradeStrategy: UpgradeStrategyAutomatic,
+					TargetAllocator: TargetAllocatorEmbedded{
+						Enabled:            true,
+						Replicas:           &one,
+						AllocationStrategy: TargetAllocatorAllocationStrategyPerNode,
+						PodDisruptionBudget: &PodDisruptionBudgetSpec{
+							MinAvailable: &intstr.IntOrString{
+								Type:   intstr.String,
+								StrVal: "10%",
+							},
+						},
+					},
+				},
+			},
+		},
 		{
 			name: "Undefined PDB for target allocator and consistent-hashing strategy",
 			otelcol: OpenTelemetryCollector{

internal/manifests/targetallocator/poddisruptionbudget.go

@@ -36,7 +36,8 @@ func PodDisruptionBudget(params manifests.Params) (*policyV1.PodDisruptionBudget
 	// 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.TargetAllocator.Spec.AllocationStrategy != v1beta1.TargetAllocatorAllocationStrategyConsistentHashing {
+	if params.TargetAllocator.Spec.AllocationStrategy != v1beta1.TargetAllocatorAllocationStrategyConsistentHashing &&
+		params.TargetAllocator.Spec.AllocationStrategy != v1beta1.TargetAllocatorAllocationStrategyPerNode {
 		params.Log.V(4).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")
 	}

internal/manifests/targetallocator/poddisruptionbudget_test.go

@@ -15,6 +15,7 @@
 package targetallocator
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -66,35 +67,37 @@ var tests = []test{
 
 func TestPDBWithValidStrategy(t *testing.T) {
 	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			targetAllocator := v1alpha1.TargetAllocator{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "my-instance",
-				},
-				Spec: v1alpha1.TargetAllocatorSpec{
-					OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
-						PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{
-							MinAvailable:   test.MinAvailable,
-							MaxUnavailable: test.MaxUnavailable,
+		for _, strategy := range []v1beta1.TargetAllocatorAllocationStrategy{v1beta1.TargetAllocatorAllocationStrategyPerNode, v1beta1.TargetAllocatorAllocationStrategyConsistentHashing} {
+			t.Run(fmt.Sprintf("%s-%s", strategy, test.name), func(t *testing.T) {
+				targetAllocator := v1alpha1.TargetAllocator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "my-instance",
+					},
+					Spec: v1alpha1.TargetAllocatorSpec{
+						OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
+							PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{
+								MinAvailable:   test.MinAvailable,
+								MaxUnavailable: test.MaxUnavailable,
+							},
 						},
+						AllocationStrategy: strategy,
 					},
-					AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyConsistentHashing,
-				},
-			}
-			configuration := config.New()
-			pdb, err := PodDisruptionBudget(manifests.Params{
-				Log:             logger,
-				Config:          configuration,
-				TargetAllocator: targetAllocator,
-			})
+				}
+				configuration := config.New()
+				pdb, err := PodDisruptionBudget(manifests.Params{
+					Log:             logger,
+					Config:          configuration,
+					TargetAllocator: targetAllocator,
+				})
 
-			// verify
-			assert.NoError(t, err)
-			assert.Equal(t, "my-instance-targetallocator", pdb.Name)
-			assert.Equal(t, "my-instance-targetallocator", pdb.Labels["app.kubernetes.io/name"])
-			assert.Equal(t, test.MinAvailable, pdb.Spec.MinAvailable)
-			assert.Equal(t, test.MaxUnavailable, pdb.Spec.MaxUnavailable)
-		})
+				// verify
+				assert.NoError(t, err)
+				assert.Equal(t, "my-instance-targetallocator", pdb.Name)
+				assert.Equal(t, "my-instance-targetallocator", pdb.Labels["app.kubernetes.io/name"])
+				assert.Equal(t, test.MinAvailable, pdb.Spec.MinAvailable)
+				assert.Equal(t, test.MaxUnavailable, pdb.Spec.MaxUnavailable)
+			})
+		}
 	}
 }