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

Run an initContainer on an enabled flag #2166

Closed
wants to merge 4 commits into from

Conversation

jaronoff97
Copy link
Contributor

@jaronoff97 jaronoff97 commented Sep 27, 2023

closes #1771

This PR adds a new field to the collector CRD which will automatically add an init container that runs the collector's validate command. This will ensure that no pods roll out with potentially invalid config.

@jaronoff97 jaronoff97 requested a review from a team September 27, 2023 17:38
@jaronoff97
Copy link
Contributor Author

hmm, after some testing i realize that this will only be helpful for a deployment where it's one up one down, for daemonset and statefulset this won't do much more than the current functionality as you have to delete the existing pod first. Should I instead attempt my initial approach where I run a job before applying the deployment? cc @iblancasa @pavolloffay

@swiatekm
Copy link
Contributor

Can you explain in more detail what this approach accomplishes vs just letting the main container crash? We get a failure in the init container instead of the main container, but in terms of a rollout the effect is the same, no?

@@ -97,6 +97,10 @@ type OpenTelemetryCollectorSpec struct {
// +kubebuilder:validation:Required
// +kubebuilder:default:=managed
ManagementState ManagementStateType `json:"managementState,omitempty"`
// RunValidation enables the operator to automatically run the collector's validate command
// as an initContainer to prevent the rollout of potentially bad configuration.
Copy link
Member

Choose a reason for hiding this comment

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

Shall we document what is the default?

metadata:
name: simplest-collector
status:
unavailableReplicas: 1
Copy link
Member

Choose a reason for hiding this comment

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

how does this differ to when validation is disabled?

@@ -0,0 +1,25 @@
# Install a bad config
Copy link
Member

Choose a reason for hiding this comment

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

Could we as well document what is invalid in this config?

@@ -0,0 +1,34 @@
apiVersion: apps/v1
Copy link
Member

Choose a reason for hiding this comment

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

could we add as well check for the init container?

@pavolloffay
Copy link
Member

Should I instead attempt my initial approach where I run a job before applying the deployment?

We could explore this path, but I am not sure if it is feasible to block the validating webhook for so long.

@iblancasa
Copy link
Contributor

Should I instead attempt my initial approach where I run a job before applying the deployment? cc @iblancasa @pavolloffay

Interesting. Do we know about other operators doing something similar?

@iblancasa
Copy link
Contributor

Can you explain in more detail what this approach accomplishes vs just letting the main container crash? We get a failure in the init container instead of the main container, but in terms of a rollout the effect is the same, no?

I think the idea is to get the error at some point and make it part of the events of the pod.

@frzifus
Copy link
Member

frzifus commented Sep 28, 2023

@jaronoff97 Wasnt the initial idea we discussed last time starting such a pod from the webhook? That way, a invalid config would never be applied?

@jaronoff97
Copy link
Contributor Author

Yeah that's what I'm going to try out now, reworking the PR currently :) initially I'm going to test that the idea works from the reconcile loop and then I can run it from the webhook.

@jaronoff97
Copy link
Contributor Author

going to close this and re-open with an approach that uses a webhook.

@jaronoff97 jaronoff97 closed this Oct 3, 2023
@jaronoff97 jaronoff97 deleted the init-container branch October 3, 2023 14:55
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.

Run the collector's validate command prior to rollout
5 participants