diff --git a/.chloggen/fix-pdb-defaulting-logic.yaml b/.chloggen/fix-pdb-defaulting-logic.yaml
new file mode 100755
index 0000000000..ef94571620
--- /dev/null
+++ 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
index 8374615a88..92c91e8360 100644
--- 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
index bfc5c6e095..805861aea0 100644
--- 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
index 7aa341dade..cf31de5118 100644
--- 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
index 8d4740fd18..dd20af943d 100644
--- 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"`
diff --git a/controllers/builder_test.go b/controllers/builder_test.go
index f8959d99cb..2c5070fa84 100644
--- a/controllers/builder_test.go
+++ b/controllers/builder_test.go
@@ -26,6 +26,7 @@ import (
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
networkingv1 "k8s.io/api/networking/v1"
+ policyV1 "k8s.io/api/policy/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
@@ -222,6 +223,38 @@ service:
},
},
},
+ &policyV1.PodDisruptionBudget{
+ TypeMeta: metav1.TypeMeta{},
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "test-collector",
+ Namespace: "test",
+ Labels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-collector",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-collector",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ Annotations: map[string]string{},
+ },
+ Spec: policyV1.PodDisruptionBudgetSpec{
+ Selector: &v1.LabelSelector{
+ MatchLabels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-collector",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-collector",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ },
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ },
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-collector-" + goodConfigHash,
@@ -468,6 +501,38 @@ service:
},
},
},
+ &policyV1.PodDisruptionBudget{
+ TypeMeta: metav1.TypeMeta{},
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "test-collector",
+ Namespace: "test",
+ Labels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-collector",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-collector",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ Annotations: map[string]string{},
+ },
+ Spec: policyV1.PodDisruptionBudgetSpec{
+ Selector: &v1.LabelSelector{
+ MatchLabels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-collector",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-collector",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ },
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ },
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-collector-" + goodConfigHash,
@@ -750,6 +815,38 @@ service:
},
},
},
+ &policyV1.PodDisruptionBudget{
+ TypeMeta: metav1.TypeMeta{},
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "test-collector",
+ Namespace: "test",
+ Labels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-collector",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-collector",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ Annotations: map[string]string{},
+ },
+ Spec: policyV1.PodDisruptionBudgetSpec{
+ Selector: &v1.LabelSelector{
+ MatchLabels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-collector",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-collector",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ },
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ },
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-collector-" + goodConfigHash,
@@ -1163,8 +1260,9 @@ service:
Mode: "statefulset",
Config: goodConfig,
TargetAllocator: v1beta1.TargetAllocatorEmbedded{
- Enabled: true,
- FilterStrategy: "relabel-config",
+ Enabled: true,
+ FilterStrategy: "relabel-config",
+ AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyConsistentHashing,
PrometheusCR: v1beta1.TargetAllocatorPrometheusCR{
Enabled: true,
},
@@ -1276,6 +1374,38 @@ service:
PodManagementPolicy: "Parallel",
},
},
+ &policyV1.PodDisruptionBudget{
+ TypeMeta: metav1.TypeMeta{},
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "test-collector",
+ Namespace: "test",
+ Labels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-collector",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-collector",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ Annotations: map[string]string{},
+ },
+ Spec: policyV1.PodDisruptionBudgetSpec{
+ Selector: &v1.LabelSelector{
+ MatchLabels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-collector",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-collector",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ },
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ },
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-collector-" + goodConfigHash,
@@ -1530,6 +1660,39 @@ prometheus_cr:
Selector: taSelectorLabels,
},
},
+ &policyV1.PodDisruptionBudget{
+ TypeMeta: metav1.TypeMeta{},
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "test-targetallocator",
+ Namespace: "test",
+ Labels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-targetallocator",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-targetallocator",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ Annotations: map[string]string{
+ "opentelemetry-targetallocator-config/hash": "9d78d2ecfad18bad24dec7e9a825b4ce45657ecbb2e6b32845b585b7c15ea407",
+ },
+ },
+ Spec: policyV1.PodDisruptionBudgetSpec{
+ Selector: &v1.LabelSelector{
+ MatchLabels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-targetallocator",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-targetallocator",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ },
+ },
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ },
},
wantErr: false,
},
@@ -1553,7 +1716,8 @@ prometheus_cr:
PrometheusCR: v1beta1.TargetAllocatorPrometheusCR{
Enabled: true,
},
- FilterStrategy: "relabel-config",
+ AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyConsistentHashing,
+ FilterStrategy: "relabel-config",
Observability: v1beta1.ObservabilitySpec{
Metrics: v1beta1.MetricsConfigSpec{
EnableMetrics: true,
@@ -1667,6 +1831,38 @@ prometheus_cr:
PodManagementPolicy: "Parallel",
},
},
+ &policyV1.PodDisruptionBudget{
+ TypeMeta: metav1.TypeMeta{},
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "test-collector",
+ Namespace: "test",
+ Labels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-collector",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-collector",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ Annotations: map[string]string{},
+ },
+ Spec: policyV1.PodDisruptionBudgetSpec{
+ Selector: &v1.LabelSelector{
+ MatchLabels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-collector",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-collector",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ },
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ },
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-collector-" + goodConfigHash,
@@ -1921,6 +2117,39 @@ prometheus_cr:
Selector: taSelectorLabels,
},
},
+ &policyV1.PodDisruptionBudget{
+ TypeMeta: metav1.TypeMeta{},
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "test-targetallocator",
+ Namespace: "test",
+ Labels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-targetallocator",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-targetallocator",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ "app.kubernetes.io/version": "latest",
+ },
+ Annotations: map[string]string{
+ "opentelemetry-targetallocator-config/hash": "9d78d2ecfad18bad24dec7e9a825b4ce45657ecbb2e6b32845b585b7c15ea407",
+ },
+ },
+ Spec: policyV1.PodDisruptionBudgetSpec{
+ Selector: &v1.LabelSelector{
+ MatchLabels: map[string]string{
+ "app.kubernetes.io/component": "opentelemetry-targetallocator",
+ "app.kubernetes.io/instance": "test.test",
+ "app.kubernetes.io/managed-by": "opentelemetry-operator",
+ "app.kubernetes.io/name": "test-targetallocator",
+ "app.kubernetes.io/part-of": "opentelemetry",
+ },
+ },
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ },
&monitoringv1.ServiceMonitor{
ObjectMeta: metav1.ObjectMeta{
Name: "test-targetallocator",
diff --git a/controllers/reconcile_test.go b/controllers/reconcile_test.go
index 20e5e0822b..46b0d38837 100644
--- a/controllers/reconcile_test.go
+++ b/controllers/reconcile_test.go
@@ -416,8 +416,8 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
{
name: "policy v1 deployment collector",
args: args{
- params: testCollectorWithPDB(1, 0),
- updates: []v1alpha1.OpenTelemetryCollector{testCollectorWithPDB(0, 1)},
+ params: testCollectorWithModeAndReplicas("policytest", v1alpha1.ModeDeployment, 3),
+ updates: []v1alpha1.OpenTelemetryCollector{testCollectorWithPDB(1, 0)},
},
want: []want{
{
@@ -427,8 +427,8 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
actual := policyV1.PodDisruptionBudget{}
exists, pdbErr := populateObjectIfExists(t, &actual, namespacedObjectName(naming.HorizontalPodAutoscaler(params.Name), params.Namespace))
assert.NoError(t, pdbErr)
- assert.Equal(t, int32(1), actual.Spec.MinAvailable.IntVal)
- assert.Nil(t, actual.Spec.MaxUnavailable)
+ assert.Equal(t, int32(1), actual.Spec.MaxUnavailable.IntVal)
+ assert.Nil(t, actual.Spec.MinAvailable)
assert.True(t, exists)
},
},
@@ -442,8 +442,8 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
actual := policyV1.PodDisruptionBudget{}
exists, pdbErr := populateObjectIfExists(t, &actual, namespacedObjectName(naming.HorizontalPodAutoscaler(params.Name), params.Namespace))
assert.NoError(t, pdbErr)
- assert.Nil(t, actual.Spec.MinAvailable)
- assert.Equal(t, int32(1), actual.Spec.MaxUnavailable.IntVal)
+ assert.Nil(t, actual.Spec.MaxUnavailable)
+ assert.Equal(t, int32(1), actual.Spec.MinAvailable.IntVal)
assert.True(t, exists)
},
},
diff --git a/docs/api.md b/docs/api.md
index 7c756d98b8..103258832f 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -31031,7 +31031,7 @@ the generated pods.
object |
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.
|
false |
@@ -40815,7 +40815,7 @@ The operator.observability.prometheus feature gate must be enabled to use this f
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.
@@ -42094,7 +42094,8 @@ The default is relabel-config.
object |
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.
|
false |
@@ -44141,7 +44142,8 @@ The operator.observability.prometheus feature gate must be enabled to use this f
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.
diff --git a/internal/manifests/collector/poddisruptionbudget.go b/internal/manifests/collector/poddisruptionbudget.go
index 619fc7a110..093f2ba181 100644
--- a/internal/manifests/collector/poddisruptionbudget.go
+++ b/internal/manifests/collector/poddisruptionbudget.go
@@ -17,17 +17,28 @@ package collector
import (
policyV1 "k8s.io/api/policy/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "k8s.io/apimachinery/pkg/util/intstr"
+ "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
"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) (*policyV1.PodDisruptionBudget, error) {
- // 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")
- return nil, nil
+ pdbSpec := params.OtelCol.Spec.PodDisruptionBudget.DeepCopy()
+ // 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 pdbSpec == nil {
+ params.Log.Info("pdb field is unset in Spec, creating default")
+ pdbSpec = &v1beta1.PodDisruptionBudgetSpec{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ }
}
name := naming.Collector(params.OtelCol.Name)
@@ -47,8 +58,8 @@ func PodDisruptionBudget(params manifests.Params) (*policyV1.PodDisruptionBudget
return &policyV1.PodDisruptionBudget{
ObjectMeta: objectMeta,
Spec: policyV1.PodDisruptionBudgetSpec{
- MinAvailable: params.OtelCol.Spec.PodDisruptionBudget.MinAvailable,
- MaxUnavailable: params.OtelCol.Spec.PodDisruptionBudget.MaxUnavailable,
+ MinAvailable: pdbSpec.MinAvailable,
+ MaxUnavailable: pdbSpec.MaxUnavailable,
Selector: &metav1.LabelSelector{
MatchLabels: objectMeta.Labels,
},
diff --git a/internal/manifests/collector/poddisruptionbudget_test.go b/internal/manifests/collector/poddisruptionbudget_test.go
index 7bf709a6be..aa45e9f352 100644
--- a/internal/manifests/collector/poddisruptionbudget_test.go
+++ b/internal/manifests/collector/poddisruptionbudget_test.go
@@ -29,38 +29,84 @@ import (
)
func TestPDB(t *testing.T) {
- type test struct {
- name string
- MinAvailable *intstr.IntOrString
+ type expected struct {
MaxUnavailable *intstr.IntOrString
+ MinAvailable *intstr.IntOrString
+ }
+ type test struct {
+ name string
+ spec *v1beta1.PodDisruptionBudgetSpec
+ expected expected
}
tests := []test{
+ {
+ name: "defaults",
+ spec: nil,
+ expected: expected{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ },
{
name: "MinAvailable-int",
- MinAvailable: &intstr.IntOrString{
- Type: intstr.Int,
- IntVal: 1,
+ expected: expected{
+ MinAvailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ spec: &v1beta1.PodDisruptionBudgetSpec{
+ MinAvailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
},
},
{
name: "MinAvailable-string",
- MinAvailable: &intstr.IntOrString{
- Type: intstr.String,
- StrVal: "10%",
+ expected: expected{
+ MinAvailable: &intstr.IntOrString{
+ Type: intstr.String,
+ StrVal: "10%",
+ },
+ },
+ spec: &v1beta1.PodDisruptionBudgetSpec{
+ MinAvailable: &intstr.IntOrString{
+ Type: intstr.String,
+ StrVal: "10%",
+ },
},
},
{
name: "MaxUnavailable-int",
- MaxUnavailable: &intstr.IntOrString{
- Type: intstr.Int,
- IntVal: 1,
+ expected: expected{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ spec: &v1beta1.PodDisruptionBudgetSpec{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
},
},
{
name: "MaxUnavailable-string",
- MaxUnavailable: &intstr.IntOrString{
- Type: intstr.String,
- StrVal: "10%",
+ expected: expected{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.String,
+ StrVal: "10%",
+ },
+ },
+ spec: &v1beta1.PodDisruptionBudgetSpec{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.String,
+ StrVal: "10%",
+ },
},
},
}
@@ -76,10 +122,7 @@ func TestPDB(t *testing.T) {
for _, otelcol := range otelcols {
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
- otelcol.Spec.PodDisruptionBudget = &v1beta1.PodDisruptionBudgetSpec{
- MinAvailable: test.MinAvailable,
- MaxUnavailable: test.MaxUnavailable,
- }
+ otelcol.Spec.PodDisruptionBudget = test.spec.DeepCopy()
configuration := config.New()
pdb, err := PodDisruptionBudget(manifests.Params{
Log: logger,
@@ -91,8 +134,8 @@ func TestPDB(t *testing.T) {
// verify
assert.Equal(t, "my-instance-collector", pdb.Name)
assert.Equal(t, "my-instance-collector", pdb.Labels["app.kubernetes.io/name"])
- assert.Equal(t, test.MinAvailable, pdb.Spec.MinAvailable)
- assert.Equal(t, test.MaxUnavailable, pdb.Spec.MaxUnavailable)
+ assert.Equal(t, test.expected.MinAvailable, pdb.Spec.MinAvailable)
+ assert.Equal(t, test.expected.MaxUnavailable, pdb.Spec.MaxUnavailable)
})
}
}
diff --git a/internal/manifests/targetallocator/poddisruptionbudget.go b/internal/manifests/targetallocator/poddisruptionbudget.go
index 7e3b98e3da..51d385330e 100644
--- a/internal/manifests/targetallocator/poddisruptionbudget.go
+++ b/internal/manifests/targetallocator/poddisruptionbudget.go
@@ -19,6 +19,7 @@ import (
policyV1 "k8s.io/api/policy/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "k8s.io/apimachinery/pkg/util/intstr"
"github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils"
@@ -26,19 +27,30 @@ import (
)
func PodDisruptionBudget(params Params) (*policyV1.PodDisruptionBudget, error) {
- // defaulting webhook should set this if the strategy is compatible, but if unset then return nil.
- if params.TargetAllocator.Spec.PodDisruptionBudget == nil {
- params.Log.Info("pdb field is unset in Spec, skipping podDisruptionBudget creation")
- return nil, nil
- }
-
+ pdbSpec := params.TargetAllocator.Spec.PodDisruptionBudget.DeepCopy()
// 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 pdbSpec != nil && 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")
+ } else if pdbSpec == nil && params.TargetAllocator.Spec.AllocationStrategy == v1beta1.TargetAllocatorAllocationStrategyLeastWeighted {
+ params.Log.V(4).Info("current allocation strategy not compatible, skipping podDisruptionBudget creation")
+ return nil, nil
+ }
+ // 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 pdbSpec == nil {
+ pdbSpec = &v1beta1.PodDisruptionBudgetSpec{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ }
}
name := naming.TAPodDisruptionBudget(params.TargetAllocator.Name)
@@ -60,8 +72,8 @@ func PodDisruptionBudget(params Params) (*policyV1.PodDisruptionBudget, error) {
return &policyV1.PodDisruptionBudget{
ObjectMeta: objectMeta,
Spec: policyV1.PodDisruptionBudgetSpec{
- MinAvailable: params.TargetAllocator.Spec.PodDisruptionBudget.MinAvailable,
- MaxUnavailable: params.TargetAllocator.Spec.PodDisruptionBudget.MaxUnavailable,
+ MinAvailable: pdbSpec.MinAvailable,
+ MaxUnavailable: pdbSpec.MaxUnavailable,
Selector: &metav1.LabelSelector{
MatchLabels: manifestutils.TASelectorLabels(params.TargetAllocator, ComponentOpenTelemetryTargetAllocator),
},
diff --git a/internal/manifests/targetallocator/poddisruptionbudget_test.go b/internal/manifests/targetallocator/poddisruptionbudget_test.go
index 30e424981d..ec4845accc 100644
--- a/internal/manifests/targetallocator/poddisruptionbudget_test.go
+++ b/internal/manifests/targetallocator/poddisruptionbudget_test.go
@@ -27,39 +27,85 @@ import (
"github.com/open-telemetry/opentelemetry-operator/internal/config"
)
-type test struct {
- name string
- MinAvailable *intstr.IntOrString
+type expected struct {
MaxUnavailable *intstr.IntOrString
+ MinAvailable *intstr.IntOrString
+}
+type test struct {
+ name string
+ spec *v1beta1.PodDisruptionBudgetSpec
+ expected expected
}
var tests = []test{
+ {
+ name: "defaults",
+ spec: nil,
+ expected: expected{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ },
{
name: "MinAvailable-int",
- MinAvailable: &intstr.IntOrString{
- Type: intstr.Int,
- IntVal: 1,
+ expected: expected{
+ MinAvailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ spec: &v1beta1.PodDisruptionBudgetSpec{
+ MinAvailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
},
},
{
name: "MinAvailable-string",
- MinAvailable: &intstr.IntOrString{
- Type: intstr.String,
- StrVal: "10%",
+ expected: expected{
+ MinAvailable: &intstr.IntOrString{
+ Type: intstr.String,
+ StrVal: "10%",
+ },
+ },
+ spec: &v1beta1.PodDisruptionBudgetSpec{
+ MinAvailable: &intstr.IntOrString{
+ Type: intstr.String,
+ StrVal: "10%",
+ },
},
},
{
name: "MaxUnavailable-int",
- MaxUnavailable: &intstr.IntOrString{
- Type: intstr.Int,
- IntVal: 1,
+ expected: expected{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
+ },
+ spec: &v1beta1.PodDisruptionBudgetSpec{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.Int,
+ IntVal: 1,
+ },
},
},
{
name: "MaxUnavailable-string",
- MaxUnavailable: &intstr.IntOrString{
- Type: intstr.String,
- StrVal: "10%",
+ expected: expected{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.String,
+ StrVal: "10%",
+ },
+ },
+ spec: &v1beta1.PodDisruptionBudgetSpec{
+ MaxUnavailable: &intstr.IntOrString{
+ Type: intstr.String,
+ StrVal: "10%",
+ },
},
},
}
@@ -74,10 +120,7 @@ func TestPDBWithValidStrategy(t *testing.T) {
},
Spec: v1alpha1.TargetAllocatorSpec{
OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
- PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{
- MinAvailable: test.MinAvailable,
- MaxUnavailable: test.MaxUnavailable,
- },
+ PodDisruptionBudget: test.spec.DeepCopy(),
},
AllocationStrategy: strategy,
},
@@ -93,8 +136,8 @@ func TestPDBWithValidStrategy(t *testing.T) {
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)
+ assert.Equal(t, test.expected.MinAvailable, pdb.Spec.MinAvailable)
+ assert.Equal(t, test.expected.MaxUnavailable, pdb.Spec.MaxUnavailable)
})
}
}
@@ -109,10 +152,7 @@ func TestPDBWithNotValidStrategy(t *testing.T) {
},
Spec: v1alpha1.TargetAllocatorSpec{
OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
- PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{
- MinAvailable: test.MinAvailable,
- MaxUnavailable: test.MaxUnavailable,
- },
+ PodDisruptionBudget: test.spec.DeepCopy(),
},
AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyLeastWeighted,
},
@@ -124,8 +164,13 @@ func TestPDBWithNotValidStrategy(t *testing.T) {
TargetAllocator: targetAllocator,
})
- // verify
- assert.Error(t, err)
+ // verify that we error if the spec is set here
+ if test.spec.DeepCopy() != nil {
+ assert.Error(t, err)
+ } else {
+ // Should be no error if no one is attempting to set a PDB here
+ assert.NoError(t, err)
+ }
assert.Nil(t, pdb)
})
}
diff --git a/tests/e2e-pdb/pdb/00-assert.yaml b/tests/e2e-pdb/pdb/00-assert.yaml
index b882b74efa..89246c3c46 100644
--- a/tests/e2e-pdb/pdb/00-assert.yaml
+++ b/tests/e2e-pdb/pdb/00-assert.yaml
@@ -6,4 +6,4 @@ spec:
selector:
matchLabels:
app.kubernetes.io/component: opentelemetry-collector
- minAvailable: 1
+ maxUnavailable: 1
diff --git a/tests/e2e-pdb/pdb/00-install.yaml b/tests/e2e-pdb/pdb/00-install.yaml
index 19bd9bf74c..643f1dca4e 100644
--- a/tests/e2e-pdb/pdb/00-install.yaml
+++ b/tests/e2e-pdb/pdb/00-install.yaml
@@ -3,8 +3,6 @@ kind: OpenTelemetryCollector
metadata:
name: pdb
spec:
- podDisruptionBudget:
- minAvailable: 1
resources:
limits:
cpu: 500m
diff --git a/tests/e2e-pdb/pdb/01-assert.yaml b/tests/e2e-pdb/pdb/01-assert.yaml
new file mode 100644
index 0000000000..b882b74efa
--- /dev/null
+++ b/tests/e2e-pdb/pdb/01-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
diff --git a/tests/e2e-pdb/pdb/01-install.yaml b/tests/e2e-pdb/pdb/01-install.yaml
new file mode 100644
index 0000000000..19bd9bf74c
--- /dev/null
+++ b/tests/e2e-pdb/pdb/01-install.yaml
@@ -0,0 +1,31 @@
+apiVersion: opentelemetry.io/v1alpha1
+kind: OpenTelemetryCollector
+metadata:
+ 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:
+ debug:
+
+ service:
+ pipelines:
+ traces:
+ receivers: [otlp]
+ exporters: [debug]
\ No newline at end of file
diff --git a/tests/e2e-pdb/pdb/chainsaw-test.yaml b/tests/e2e-pdb/pdb/chainsaw-test.yaml
index d5e7025f54..4a4fd598e5 100755
--- a/tests/e2e-pdb/pdb/chainsaw-test.yaml
+++ b/tests/e2e-pdb/pdb/chainsaw-test.yaml
@@ -8,7 +8,13 @@ spec:
steps:
- name: step-00
try:
- - apply:
- file: 00-install.yaml
- - assert:
- file: 00-assert.yaml
+ - apply:
+ file: 00-install.yaml
+ - assert:
+ file: 00-assert.yaml
+ - name: step-01
+ try:
+ - apply:
+ file: 01-install.yaml
+ - assert:
+ file: 01-assert.yaml