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

Add custom init containers user can add #424

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

prafull01
Copy link
Collaborator

Fixes #422

@prafull01 prafull01 force-pushed the add-init-containers branch 2 times, most recently from ee7e336 to bcebac2 Compare October 10, 2024 12:25
@prafull01 prafull01 requested a review from udnay October 10, 2024 12:27
Copy link

@udnay udnay left a comment

Choose a reason for hiding this comment

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

This is looking really good. A couple questions

@@ -339,6 +347,9 @@ spec:
{{- else }}
emptyDir: {}
{{- end }}
{{ with .Values.statefulset.volumes }}
Copy link

Choose a reason for hiding this comment

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

Would it make more sense to have a different top-level value for volume mounts inside the statefulset? I can imagine that users might want to add a volume not used in an initContainer, wdyt? With the current setup a user could only ever define volumes that are also used in an init container.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added the test cases for all the scenarios as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay. Got it. You want to move the volumeMounts outside. Done.

Copy link

Choose a reason for hiding this comment

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

Do you think it's ok for all init containers to mount all extra dirs? I guess there is no harm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think it is ok. we don't want to over complicate things.

# - "sh"
# - "-c"
# - "curl -s -H \"Metadata:true\" --noproxy \"*\" \"http://169.254.169.254/metadata/instance?api-version=2021-02-01\" | jq '.' > /metadata/instance_metadata.json"
# volumeMounts:
Copy link

Choose a reason for hiding this comment

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

I would add a comment here that volume mounts are reused in the main crdb container again.

@@ -263,6 +266,11 @@ spec:
{{- end }}
protocol: TCP
volumeMounts:
{{ range .Values.statefulset.initContainers }}
Copy link

Choose a reason for hiding this comment

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

/nit but I would add this stanza to the bottom of the volumeMounts section just so we don't obscure the datadir and certs

@prafull01 prafull01 force-pushed the add-init-containers branch 2 times, most recently from f7e0e14 to 86822f7 Compare October 15, 2024 15:42
@prafull01 prafull01 requested a review from udnay October 15, 2024 15:44
@prafull01 prafull01 force-pushed the add-init-containers branch 3 times, most recently from 85ed46c to e5189f8 Compare October 16, 2024 10:38
@prafull01 prafull01 force-pushed the add-init-containers branch from e5189f8 to 423ee65 Compare October 18, 2024 01:14
Copy link

@udnay udnay left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -339,6 +347,9 @@ spec:
{{- else }}
emptyDir: {}
{{- end }}
{{ with .Values.statefulset.volumes }}
Copy link

Choose a reason for hiding this comment

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

Do you think it's ok for all init containers to mount all extra dirs? I guess there is no harm

@prafull01 prafull01 merged commit 2396411 into cockroachdb:master Oct 18, 2024
9 checks passed
@prafull01 prafull01 deleted the add-init-containers branch October 18, 2024 07:34
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.

Add the ability to define a custom init container to run in addition to the current one for certs
2 participants