-
Notifications
You must be signed in to change notification settings - Fork 459
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
Conversation
Signed-off-by: Yuri Sa <yurimsa@gmail.com>
Signed-off-by: Yuri Sa <yurimsa@gmail.com>
@frzifus, WDYT to include |
Signed-off-by: Yuri Sa <yurimsa@gmail.com> Added updateStrategy to DaemonSet mode Signed-off-by: Yuri Sa <yurimsa@gmail.com>
8d1165b
to
6ff4cd2
Compare
Signed-off-by: Yuri Sa <yurimsa@gmail.com>
There was a problem hiding this 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
* 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>
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