feat: Add pdb support for target allocator (#2411)

* feat: Add pdb support for target allocator Signed-off-by: Jorge Turrado <[email protected]> * fix e2e tests Signed-off-by: Jorge Turrado <[email protected]> * fix e2e tests Signed-off-by: Jorge Turrado <[email protected]> * update bundle Signed-off-by: Jorge Turrado <[email protected]> * Address feedback Signed-off-by: Jorge Turrado <[email protected]> * Change log level Signed-off-by: Jorge Turrado <[email protected]> --------- Signed-off-by: Jorge Turrado <[email protected]>
open-telemetry · Dec 18, 2023 · d7d0166 · d7d0166
1 parent c8b2970
commit d7d0166
Show file tree

Hide file tree

Showing 20 changed files with 572 additions and 215 deletions.
diff --git a/.chloggen/target-allocator-pdb.yaml b/.chloggen/target-allocator-pdb.yaml
@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+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: 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
+
+# One or more tracking issues related to the change
+issues: [2261]
+
+# (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/v1alpha1/collector_webhook.go b/apis/v1alpha1/collector_webhook.go
@@ -126,7 +126,7 @@ func (c CollectorWebhook) defaulter(r *OpenTelemetryCollector) error {
 		}
 	}
 
-	// if pod isn't provided, we set MaxUnavailable 1,
+	// 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
@@ -139,6 +139,22 @@ func (c CollectorWebhook) defaulter(r *OpenTelemetryCollector) error {
 		}
 	}
 
+	// 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.AllocationStrategy == OpenTelemetryTargetAllocatorAllocationStrategyConsistentHashing &&
+		r.Spec.TargetAllocator.PodDisruptionBudget == nil {
+		r.Spec.TargetAllocator.PodDisruptionBudget = &PodDisruptionBudgetSpec{
+			MaxUnavailable: &intstr.IntOrString{
+				Type:   intstr.Int,
+				IntVal: 1,
+			},
+		}
+	}
+
 	if r.Spec.Ingress.Type == IngressTypeRoute && r.Spec.Ingress.Route.Termination == "" {
 		r.Spec.Ingress.Route.Termination = TLSRouteTerminationTypeEdge
 	}

diff --git a/apis/v1alpha1/collector_webhook_test.go b/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 {

diff --git a/apis/v1alpha1/opentelemetrycollector_types.go b/apis/v1alpha1/opentelemetrycollector_types.go
@@ -340,6 +340,11 @@ type OpenTelemetryTargetAllocator struct {
 	// consumed in the config file for the TargetAllocator.
 	// +optional
 	Env []v1.EnvVar `json:"env,omitempty"`
+	// PodDisruptionBudget specifies the pod disruption budget configuration to use
+	// for the target allocator workload.
+	//
+	// +optional
+	PodDisruptionBudget *PodDisruptionBudgetSpec `json:"podDisruptionBudget,omitempty"`
 }
 
 type OpenTelemetryTargetAllocatorPrometheusCR struct {

diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go
diff --git a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml
@@ -5160,6 +5160,27 @@ spec:
                     description: NodeSelector to schedule OpenTelemetry TargetAllocator
                       pods.
                     type: object
+                  podDisruptionBudget:
+                    description: PodDisruptionBudget specifies the pod disruption
+                      budget configuration to use for the target allocator workload.
+                    properties:
+                      maxUnavailable:
+                        anyOf:
+                        - type: integer
+                        - type: string
+                        description: An eviction is allowed if at most "maxUnavailable"
+                          pods selected by "selector" are unavailable after the eviction,
+                          i.e. even in absence of the evicted pod.
+                        x-kubernetes-int-or-string: true
+                      minAvailable:
+                        anyOf:
+                        - type: integer
+                        - type: string
+                        description: An eviction is allowed if at least "minAvailable"
+                          pods selected by "selector" will still be available after
+                          the eviction, i.e. even in the absence of the evicted pod.
+                        x-kubernetes-int-or-string: true
+                    type: object
                   prometheusCR:
                     description: PrometheusCR defines the configuration for the retrieval
                       of PrometheusOperator CRDs ( servicemonitor.monitoring.coreos.com/v1

diff --git a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml
@@ -5157,6 +5157,27 @@ spec:
                     description: NodeSelector to schedule OpenTelemetry TargetAllocator
                       pods.
                     type: object
+                  podDisruptionBudget:
+                    description: PodDisruptionBudget specifies the pod disruption
+                      budget configuration to use for the target allocator workload.
+                    properties:
+                      maxUnavailable:
+                        anyOf:
+                        - type: integer
+                        - type: string
+                        description: An eviction is allowed if at most "maxUnavailable"
+                          pods selected by "selector" are unavailable after the eviction,
+                          i.e. even in absence of the evicted pod.
+                        x-kubernetes-int-or-string: true
+                      minAvailable:
+                        anyOf:
+                        - type: integer
+                        - type: string
+                        description: An eviction is allowed if at least "minAvailable"
+                          pods selected by "selector" will still be available after
+                          the eviction, i.e. even in the absence of the evicted pod.
+                        x-kubernetes-int-or-string: true
+                    type: object
                   prometheusCR:
                     description: PrometheusCR defines the configuration for the retrieval
                       of PrometheusOperator CRDs ( servicemonitor.monitoring.coreos.com/v1

diff --git a/docs/api.md b/docs/api.md
@@ -18067,6 +18067,13 @@ TargetAllocator indicates a value which determines whether to spawn a target all
           NodeSelector to schedule OpenTelemetry TargetAllocator pods.<br/>
         </td>
         <td>false</td>
+      </tr><tr>
+        <td><b><a href="#opentelemetrycollectorspectargetallocatorpoddisruptionbudget">podDisruptionBudget</a></b></td>
+        <td>object</td>
+        <td>
+          PodDisruptionBudget specifies the pod disruption budget configuration to use for the target allocator workload.<br/>
+        </td>
+        <td>false</td>
       </tr><tr>
         <td><b><a href="#opentelemetrycollectorspectargetallocatorprometheuscr">prometheusCR</a></b></td>
         <td>object</td>
@@ -19670,6 +19677,40 @@ Selects a key of a secret in the pod's namespace
 </table>
 
 
+### OpenTelemetryCollector.spec.targetAllocator.podDisruptionBudget
+<sup><sup>[↩ Parent](#opentelemetrycollectorspectargetallocator)</sup></sup>
+
+
+
+PodDisruptionBudget specifies the pod disruption budget configuration to use for the target allocator workload.
+
+<table>
+    <thead>
+        <tr>
+            <th>Name</th>
+            <th>Type</th>
+            <th>Description</th>
+            <th>Required</th>
+        </tr>
+    </thead>
+    <tbody><tr>
+        <td><b>maxUnavailable</b></td>
+        <td>int or string</td>
+        <td>
+          An eviction is allowed if at most "maxUnavailable" pods selected by "selector" are unavailable after the eviction, i.e. even in absence of the evicted pod.<br/>
+        </td>
+        <td>false</td>
+      </tr><tr>
+        <td><b>minAvailable</b></td>
+        <td>int or string</td>
+        <td>
+          An eviction is allowed if at least "minAvailable" pods selected by "selector" will still be available after the eviction, i.e. even in the absence of the evicted pod.<br/>
+        </td>
+        <td>false</td>
+      </tr></tbody>
+</table>
+
+
 ### OpenTelemetryCollector.spec.targetAllocator.prometheusCR
 <sup><sup>[↩ Parent](#opentelemetrycollectorspectargetallocator)</sup></sup>
 

diff --git a/internal/manifests/collector/poddisruptionbudget.go b/internal/manifests/collector/poddisruptionbudget.go
@@ -17,14 +17,13 @@ package collector
 import (
 	policyV1 "k8s.io/api/policy/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils"
 	"github.com/open-telemetry/opentelemetry-operator/internal/naming"
 )
 
-func PodDisruptionBudget(params manifests.Params) client.Object {
+func PodDisruptionBudget(params manifests.Params) *policyV1.PodDisruptionBudget {
 	// defaulting webhook should always set this, but if unset then return nil.
 	if params.OtelCol.Spec.PodDisruptionBudget == nil {
 		params.Log.Info("pdb field is unset in Spec, skipping podDisruptionBudget creation")

diff --git a/internal/manifests/collector/poddisruptionbudget_test.go b/internal/manifests/collector/poddisruptionbudget_test.go
@@ -18,7 +18,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	policyV1 "k8s.io/api/policy/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 
@@ -81,13 +80,12 @@ func TestPDB(t *testing.T) {
 					MaxUnavailable: test.MaxUnavailable,
 				}
 				configuration := config.New()
-				raw := PodDisruptionBudget(manifests.Params{
+				pdb := PodDisruptionBudget(manifests.Params{
 					Log:     logger,
 					Config:  configuration,
 					OtelCol: otelcol,
 				})
 
-				pdb := raw.(*policyV1.PodDisruptionBudget)
 				// verify
 				assert.Equal(t, "my-instance-collector", pdb.Name)
 				assert.Equal(t, "my-instance-collector", pdb.Labels["app.kubernetes.io/name"])