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

Added updateStrategy to DaemonSet mode #2305

Merged
merged 5 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
16 changes: 16 additions & 0 deletions .chloggen/add-updatestrategy.yaml
Original file line number Diff line number Diff line change
@@ -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: operator

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Added updateStrategy for DaemonSet mode.

# One or more tracking issues related to the change
issues: [2107]

# (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:
5 changes: 5 additions & 0 deletions apis/v1alpha1/collector_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,11 @@ func (c CollectorWebhook) validate(r *OpenTelemetryCollector) (admission.Warning
}
}

// validate updateStrategy for DaemonSet
if r.Spec.Mode != ModeDaemonSet && len(r.Spec.UpdateStrategy.Type) > 0 {
return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'updateStrategy'", r.Spec.Mode)
}

return warnings, nil
}

Expand Down
17 changes: 17 additions & 0 deletions apis/v1alpha1/collector_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"github.com/go-logr/logr"
"github.com/stretchr/testify/assert"
appsv1 "k8s.io/api/apps/v1"
autoscalingv2 "k8s.io/api/autoscaling/v2"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
Expand Down Expand Up @@ -766,6 +767,22 @@ func TestOTELColValidatingWebhook(t *testing.T) {
},
expectedErr: "a valid Ingress hostname has to be defined for subdomain ruleType",
},
{
name: "invalid updateStrategy for Deployment mode",
otelcol: OpenTelemetryCollector{
Spec: OpenTelemetryCollectorSpec{
Mode: ModeDeployment,
UpdateStrategy: appsv1.DaemonSetUpdateStrategy{
Type: "RollingUpdate",
RollingUpdate: &appsv1.RollingUpdateDaemonSet{
MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)},
MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)},
},
},
},
},
expectedErr: "the OpenTelemetry Collector mode is set to deployment, which does not support the attribute 'updateStrategy'",
},
}

for _, test := range tests {
Expand Down
6 changes: 6 additions & 0 deletions apis/v1alpha1/opentelemetrycollector_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package v1alpha1

import (
appsv1 "k8s.io/api/apps/v1"
autoscalingv2 "k8s.io/api/autoscaling/v2"
v1 "k8s.io/api/core/v1"
networkingv1 "k8s.io/api/networking/v1"
Expand Down Expand Up @@ -275,6 +276,11 @@ type OpenTelemetryCollectorSpec struct {
// object, which shall be mounted into the Collector Pods.
// Each ConfigMap will be added to the Collector's Deployments as a volume named `configmap-<configmap-name>`.
ConfigMaps []ConfigMapsSpec `json:"configmaps,omitempty"`
// UpdateStrategy represents the strategy the operator will take replacing existing DaemonSet pods with new pods
// https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/daemon-set-v1/#DaemonSetSpec
// This is only applicable to Daemonset mode.
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to validate this in the validation webhook?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I'll add a validation to enable this field only for DaemonSet mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added the validation you mentioned. Could you please have a look?

// +optional
UpdateStrategy appsv1.DaemonSetUpdateStrategy `json:"updateStrategy,omitempty"`
}

// OpenTelemetryTargetAllocator defines the configurations for the Prometheus target allocator.
Expand Down
1 change: 1 addition & 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.

30 changes: 30 additions & 0 deletions bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5320,6 +5320,36 @@ spec:
- whenUnsatisfiable
type: object
type: array
updateStrategy:
description: UpdateStrategy represents the strategy the operator will
take replacing existing DaemonSet pods with new pods https://kubernetes.
properties:
rollingUpdate:
description: 'Rolling update config params. Present only if type
= "RollingUpdate". --- TODO: Update this to follow our convention
for oneOf, whatever we decide it to be. Same as Deployment `strategy.'
properties:
maxSurge:
anyOf:
- type: integer
- type: string
description: The maximum number of nodes with an existing
available DaemonSet pod that can have an updated DaemonSet
pod during during an update.
x-kubernetes-int-or-string: true
maxUnavailable:
anyOf:
- type: integer
- type: string
description: The maximum number of DaemonSet pods that can
be unavailable during the update.
x-kubernetes-int-or-string: true
type: object
type:
description: Type of daemon set update. Can be "RollingUpdate"
or "OnDelete". Default is RollingUpdate.
type: string
type: object
upgradeStrategy:
description: UpgradeStrategy represents how the operator will handle
upgrades to the CR when a newer version of the operator is deployed
Expand Down
30 changes: 30 additions & 0 deletions config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5317,6 +5317,36 @@ spec:
- whenUnsatisfiable
type: object
type: array
updateStrategy:
description: UpdateStrategy represents the strategy the operator will
take replacing existing DaemonSet pods with new pods https://kubernetes.
properties:
rollingUpdate:
description: 'Rolling update config params. Present only if type
= "RollingUpdate". --- TODO: Update this to follow our convention
for oneOf, whatever we decide it to be. Same as Deployment `strategy.'
properties:
maxSurge:
anyOf:
- type: integer
- type: string
description: The maximum number of nodes with an existing
available DaemonSet pod that can have an updated DaemonSet
pod during during an update.
x-kubernetes-int-or-string: true
maxUnavailable:
anyOf:
- type: integer
- type: string
description: The maximum number of DaemonSet pods that can
be unavailable during the update.
x-kubernetes-int-or-string: true
type: object
type:
description: Type of daemon set update. Can be "RollingUpdate"
or "OnDelete". Default is RollingUpdate.
type: string
type: object
upgradeStrategy:
description: UpgradeStrategy represents how the operator will handle
upgrades to the CR when a newer version of the operator is deployed
Expand Down
75 changes: 75 additions & 0 deletions docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -9677,6 +9677,13 @@ OpenTelemetryCollectorSpec defines the desired state of OpenTelemetryCollector.
TopologySpreadConstraints embedded kubernetes pod configuration option, controls how pods are spread across your cluster among failure-domains such as regions, zones, nodes, and other user-defined top<br/>
</td>
<td>false</td>
</tr><tr>
<td><b><a href="#opentelemetrycollectorspecupdatestrategy">updateStrategy</a></b></td>
<td>object</td>
<td>
UpdateStrategy represents the strategy the operator will take replacing existing DaemonSet pods with new pods https://kubernetes.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>upgradeStrategy</b></td>
<td>enum</td>
Expand Down Expand Up @@ -20194,6 +20201,74 @@ A label selector requirement is a selector that contains values, a key, and an o
</table>


### OpenTelemetryCollector.spec.updateStrategy
<sup><sup>[↩ Parent](#opentelemetrycollectorspec)</sup></sup>



UpdateStrategy represents the strategy the operator will take replacing existing DaemonSet pods with new pods https://kubernetes.

<table>
<thead>
<tr>
<th>Name</th>
<th>Type</th>
<th>Description</th>
<th>Required</th>
</tr>
</thead>
<tbody><tr>
<td><b><a href="#opentelemetrycollectorspecupdatestrategyrollingupdate">rollingUpdate</a></b></td>
<td>object</td>
<td>
Rolling update config params. Present only if type = "RollingUpdate". --- TODO: Update this to follow our convention for oneOf, whatever we decide it to be. Same as Deployment `strategy.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>type</b></td>
<td>string</td>
<td>
Type of daemon set update. Can be "RollingUpdate" or "OnDelete". Default is RollingUpdate.<br/>
</td>
<td>false</td>
</tr></tbody>
</table>


### OpenTelemetryCollector.spec.updateStrategy.rollingUpdate
<sup><sup>[↩ Parent](#opentelemetrycollectorspecupdatestrategy)</sup></sup>



Rolling update config params. Present only if type = "RollingUpdate". --- TODO: Update this to follow our convention for oneOf, whatever we decide it to be. Same as Deployment `strategy.

<table>
<thead>
<tr>
<th>Name</th>
<th>Type</th>
<th>Description</th>
<th>Required</th>
</tr>
</thead>
<tbody><tr>
<td><b>maxSurge</b></td>
<td>int or string</td>
<td>
The maximum number of nodes with an existing available DaemonSet pod that can have an updated DaemonSet pod during during an update.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>maxUnavailable</b></td>
<td>int or string</td>
<td>
The maximum number of DaemonSet pods that can be unavailable during the update.<br/>
</td>
<td>false</td>
</tr></tbody>
</table>


### OpenTelemetryCollector.spec.volumeClaimTemplates[index]
<sup><sup>[↩ Parent](#opentelemetrycollectorspec)</sup></sup>

Expand Down
1 change: 1 addition & 0 deletions internal/manifests/collector/daemonset.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ func DaemonSet(params manifests.Params) *appsv1.DaemonSet {
Affinity: params.OtelCol.Spec.Affinity,
},
},
UpdateStrategy: params.OtelCol.Spec.UpdateStrategy,
},
}
}
70 changes: 70 additions & 0 deletions internal/manifests/collector/daemonset_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ import (
"testing"

"github.com/stretchr/testify/assert"
appsv1 "k8s.io/api/apps/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"

"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
"github.com/open-telemetry/opentelemetry-operator/internal/config"
Expand Down Expand Up @@ -419,3 +421,71 @@ func TestDaemonSetAdditionalContainer(t *testing.T) {
assert.Len(t, d.Spec.Template.Spec.Containers, 2)
assert.Equal(t, v1.Container{Name: "test"}, d.Spec.Template.Spec.Containers[0])
}

func TestDaemonSetDefaultUpdateStrategy(t *testing.T) {
// prepare
otelcol := v1alpha1.OpenTelemetryCollector{
ObjectMeta: metav1.ObjectMeta{
Name: "my-instance",
Namespace: "my-namespace",
},
Spec: v1alpha1.OpenTelemetryCollectorSpec{
UpdateStrategy: appsv1.DaemonSetUpdateStrategy{
Type: "RollingUpdate",
RollingUpdate: &appsv1.RollingUpdateDaemonSet{
MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)},
MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)},
},
},
},
}
cfg := config.New()

params := manifests.Params{
Config: cfg,
OtelCol: otelcol,
Log: logger,
}

// test
d := DaemonSet(params)
assert.Equal(t, "my-instance-collector", d.Name)
assert.Equal(t, "my-instance-collector", d.Labels["app.kubernetes.io/name"])
assert.Equal(t, appsv1.DaemonSetUpdateStrategyType("RollingUpdate"), d.Spec.UpdateStrategy.Type)
assert.Equal(t, &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, d.Spec.UpdateStrategy.RollingUpdate.MaxSurge)
assert.Equal(t, &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, d.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable)
}

func TestDaemonSetOnDeleteUpdateStrategy(t *testing.T) {
// prepare
otelcol := v1alpha1.OpenTelemetryCollector{
ObjectMeta: metav1.ObjectMeta{
Name: "my-instance",
Namespace: "my-namespace",
},
Spec: v1alpha1.OpenTelemetryCollectorSpec{
UpdateStrategy: appsv1.DaemonSetUpdateStrategy{
Type: "OnDelete",
RollingUpdate: &appsv1.RollingUpdateDaemonSet{
MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)},
MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)},
},
},
},
}
cfg := config.New()

params := manifests.Params{
Config: cfg,
OtelCol: otelcol,
Log: logger,
}

// test
d := DaemonSet(params)
assert.Equal(t, "my-instance-collector", d.Name)
assert.Equal(t, "my-instance-collector", d.Labels["app.kubernetes.io/name"])
assert.Equal(t, appsv1.DaemonSetUpdateStrategyType("OnDelete"), d.Spec.UpdateStrategy.Type)
assert.Equal(t, &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, d.Spec.UpdateStrategy.RollingUpdate.MaxSurge)
assert.Equal(t, &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, d.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable)
}
14 changes: 14 additions & 0 deletions tests/e2e/smoke-daemonset/00-assert.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
apiVersion: apps/v1
kind: DaemonSet
metadata:
name: daemonset-test-collector
spec:
updateStrategy:
type: RollingUpdate
rollingUpdate:
maxSurge: 0
maxUnavailable: 1
status:
numberAvailable: 1
numberMisscheduled: 0
numberReady: 1
24 changes: 24 additions & 0 deletions tests/e2e/smoke-daemonset/00-install.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
metadata:
name: daemonset-test
spec:
mode: daemonset
updateStrategy:
type: RollingUpdate
rollingUpdate:
maxUnavailable: 1
config: |
receivers:
jaeger:
protocols:
grpc:
processors:
exporters:
debug:
service:
pipelines:
traces:
receivers: [jaeger]
processors: []
exporters: [debug]
Loading