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

Conversation

yuriolisa
Copy link
Contributor

Description:
Added updateStrategy field regarding the way existing DaemonSet pods will be replaced to new pods.

Link to tracking Issue:
Resolves: #2107
Testing:
Created unit tests and e2e tests to check if these fields have been properly populated.
Documentation:
https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/daemon-set-v1/#DaemonSetSpec

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
@yuriolisa yuriolisa requested a review from a team November 2, 2023 11:54
Signed-off-by: Yuri Sa <yurimsa@gmail.com>
@yuriolisa
Copy link
Contributor Author

@frzifus, WDYT to include updateStrategy for DaemonSet and Statefulset? So it will be like:
statefulsetUpdateStrategy and daemonsetUpdateStrategy, or if you have better suggestions, I'm all ears.

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

Added updateStrategy to DaemonSet mode

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
Signed-off-by: Yuri Sa <yurimsa@gmail.com>
Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

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

LGTM, just one tiny thing :)

@@ -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?

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
@yuriolisa yuriolisa requested a review from frzifus November 16, 2023 16:55
@jaronoff97 jaronoff97 merged commit 3b6cf2c into open-telemetry:main Nov 16, 2023
27 checks passed
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* Added updateStrategy to DaemonSet mode

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Added updateStrategy to DaemonSet mode

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Added updateStrategy to DaemonSet mode

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

Added updateStrategy to DaemonSet mode

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

* Added webhooks test for DaemonSet Mode

Signed-off-by: Yuri Sa <yurimsa@gmail.com>

---------

Signed-off-by: Yuri Sa <yurimsa@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support configuring updateStrategy in collector workloads
3 participants