Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support for PDBs on deployment and statefulset #2141

Merged
merged 8 commits into from
Oct 6, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .chloggen/add-pdb-support.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# 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: operator

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Add PDB support for OpenTelemetryCollector"

# One or more tracking issues related to the change
issues:
- 2136

# (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: |
This PR adds support for PodDisruptionBudgets when OpenTelemetryCollector is deployed
as `deployment` or `statefulset`.
1 change: 1 addition & 0 deletions .github/workflows/e2e.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ jobs:
- e2e-prometheuscr
- e2e-autoscale
- e2e-multi-instrumentation
- e2e-pdb

steps:
- name: Set up Go
Expand Down
5 changes: 5 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,11 @@ e2e-upgrade: undeploy
e2e-autoscale:
$(KUTTL) test --config kuttl-test-autoscale.yaml

# end-to-end-test for testing pdb support
.PHONY: e2e-pdb
e2e-pdb:
$(KUTTL) test --config kuttl-test-pdb.yaml

# end-to-end-test for testing OpenShift cases
.PHONY: e2e-openshift
e2e-openshift:
Expand Down
23 changes: 23 additions & 0 deletions apis/v1alpha1/opentelemetrycollector_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
v1 "k8s.io/api/core/v1"
networkingv1 "k8s.io/api/networking/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
)

// ManagementStateType defines the type for CR management states.
Expand Down Expand Up @@ -123,6 +124,11 @@ type OpenTelemetryCollectorSpec struct {
//
// +optional
Autoscaler *AutoscalerSpec `json:"autoscaler,omitempty"`
// PodDisruptionBudget specifies the pod disruption budget configuration to use
// for the OpenTelemetryCollector workload.
//
// +optional
PodDisruptionBudget *PodDisruptionBudgetSpec `json:"podDisruptionBudget,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note for other reviewers, this is a paired down version of the main PDB spec because we don't want to allow the configuration of the selector

// SecurityContext configures the container security context for
// the opentelemetry-collector container.
//
Expand Down Expand Up @@ -444,6 +450,23 @@ type AutoscalerSpec struct {
TargetMemoryUtilization *int32 `json:"targetMemoryUtilization,omitempty"`
}

// PodDisruptionBudgetSpec defines the OpenTelemetryCollector's pod disruption budget specification.
type PodDisruptionBudgetSpec struct {
// 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. So for example you can prevent all voluntary
// evictions by specifying "100%".
// +optional
MinAvailable *intstr.IntOrString `json:"minAvailable,omitempty"`
jaronoff97 marked this conversation as resolved.
Show resolved Hide resolved

// 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. For example, one can prevent all voluntary evictions
// by specifying 0. This is a mutually exclusive setting with "minAvailable".
// +optional
MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"`
}

// MetricsConfigSpec defines a metrics config.
type MetricsConfigSpec struct {
// EnableMetrics specifies if ServiceMonitor should be created for the OpenTelemetry Collector and Prometheus Exporters.
Expand Down
23 changes: 23 additions & 0 deletions apis/v1alpha1/opentelemetrycollector_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (

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"
logf "sigs.k8s.io/controller-runtime/pkg/log"
Expand Down Expand Up @@ -91,6 +92,20 @@ func (r *OpenTelemetryCollector) Default() {
r.Spec.Autoscaler.TargetCPUUtilization = &defaultCPUTarget
}
}

// if pod 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 r.Spec.PodDisruptionBudget == nil {
r.Spec.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
}
Expand Down Expand Up @@ -231,6 +246,14 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error {
}
}

// validate pod disruption budget

if r.Spec.PodDisruptionBudget != nil {
if r.Spec.PodDisruptionBudget.MaxUnavailable != nil && r.Spec.PodDisruptionBudget.MinAvailable != nil {
return fmt.Errorf("the OpenTelemetry Spec podDisruptionBudget configuration is incorrect, minAvailable and maxUnavailable are mutually exclusive")
}
}

if r.Spec.Ingress.Type == IngressTypeNginx && r.Spec.Mode == ModeSidecar {
return fmt.Errorf("the OpenTelemetry Spec Ingress configuiration is incorrect. Ingress can only be used in combination with the modes: %s, %s, %s",
ModeDeployment, ModeDaemonSet, ModeStatefulSet,
Expand Down
88 changes: 88 additions & 0 deletions apis/v1alpha1/opentelemetrycollector_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
)

func TestOTELColDefaultingWebhook(t *testing.T) {
Expand All @@ -48,6 +49,12 @@ func TestOTELColDefaultingWebhook(t *testing.T) {
Mode: ModeDeployment,
Replicas: &one,
UpgradeStrategy: UpgradeStrategyAutomatic,
PodDisruptionBudget: &PodDisruptionBudgetSpec{
MaxUnavailable: &intstr.IntOrString{
Type: intstr.Int,
IntVal: 1,
},
},
},
},
},
Expand All @@ -70,6 +77,12 @@ func TestOTELColDefaultingWebhook(t *testing.T) {
Mode: ModeSidecar,
Replicas: &five,
UpgradeStrategy: "adhoc",
PodDisruptionBudget: &PodDisruptionBudgetSpec{
MaxUnavailable: &intstr.IntOrString{
Type: intstr.Int,
IntVal: 1,
},
},
},
},
},
Expand Down Expand Up @@ -98,6 +111,12 @@ func TestOTELColDefaultingWebhook(t *testing.T) {
MaxReplicas: &five,
MinReplicas: &one,
},
PodDisruptionBudget: &PodDisruptionBudgetSpec{
MaxUnavailable: &intstr.IntOrString{
Type: intstr.Int,
IntVal: 1,
},
},
},
},
},
Expand Down Expand Up @@ -126,6 +145,12 @@ func TestOTELColDefaultingWebhook(t *testing.T) {
MinReplicas: &one,
},
MaxReplicas: &five,
PodDisruptionBudget: &PodDisruptionBudgetSpec{
MaxUnavailable: &intstr.IntOrString{
Type: intstr.Int,
IntVal: 1,
},
},
},
},
},
Expand Down Expand Up @@ -155,6 +180,44 @@ func TestOTELColDefaultingWebhook(t *testing.T) {
},
Replicas: &one,
UpgradeStrategy: UpgradeStrategyAutomatic,
PodDisruptionBudget: &PodDisruptionBudgetSpec{
MaxUnavailable: &intstr.IntOrString{
Type: intstr.Int,
IntVal: 1,
},
},
},
},
},
{
name: "Defined PDB",
otelcol: OpenTelemetryCollector{
Spec: OpenTelemetryCollectorSpec{
Mode: ModeDeployment,
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,
PodDisruptionBudget: &PodDisruptionBudgetSpec{
MinAvailable: &intstr.IntOrString{
Type: intstr.String,
StrVal: "10%",
},
},
},
},
},
Expand Down Expand Up @@ -237,6 +300,12 @@ func TestOTELColValidatingWebhook(t *testing.T) {
},
TargetCPUUtilization: &five,
},
PodDisruptionBudget: &PodDisruptionBudgetSpec{
MinAvailable: &intstr.IntOrString{
Type: intstr.Int,
IntVal: 1,
},
},
},
},
},
Expand Down Expand Up @@ -489,6 +558,25 @@ func TestOTELColValidatingWebhook(t *testing.T) {
},
expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, invalid pods target type",
},
{
name: "pdb minAvailable and maxUnavailable have been set together",
otelcol: OpenTelemetryCollector{
Spec: OpenTelemetryCollectorSpec{
MaxReplicas: &three,
PodDisruptionBudget: &PodDisruptionBudgetSpec{
MinAvailable: &intstr.IntOrString{
Type: intstr.Int,
IntVal: 1,
},
MaxUnavailable: &intstr.IntOrString{
Type: intstr.Int,
IntVal: 1,
},
},
},
},
expectedErr: "the OpenTelemetry Spec podDisruptionBudget configuration is incorrect, minAvailable and maxUnavailable are mutually exclusive",
},
{
name: "invalid deployment mode incompabible with ingress settings",
otelcol: OpenTelemetryCollector{
Expand Down
31 changes: 31 additions & 0 deletions apis/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ metadata:
categories: Logging & Tracing,Monitoring
certified: "false"
containerImage: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator
createdAt: "2023-09-20T15:05:15Z"
createdAt: "2023-09-21T11:33:59Z"
description: Provides the OpenTelemetry components, including the Collector
operators.operatorframework.io/builder: operator-sdk-v1.29.0
operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
Expand Down Expand Up @@ -237,6 +237,18 @@ spec:
- get
- patch
- update
- apiGroups:
- policy
resources:
- poddisruptionbudgets
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- route.openshift.io
resources:
Expand Down
21 changes: 21 additions & 0 deletions bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3719,6 +3719,27 @@ spec:
description: PodAnnotations is the set of annotations that will be
attached to Collector and Target Allocator pods.
type: object
podDisruptionBudget:
description: PodDisruptionBudget specifies the pod disruption budget
configuration to use for the OpenTelemetryCollector 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
podSecurityContext:
description: PodSecurityContext configures the pod security context
for the opentelemetry-collector pod, when running as a deployment,
Expand Down
Loading