-
Notifications
You must be signed in to change notification settings - Fork 563
feat: enhance worker deployments with additional packages and CA certificates #1944
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
base: develop
Are you sure you want to change the base?
feat: enhance worker deployments with additional packages and CA certificates #1944
Conversation
charts/sentry/templates/sentry/worker/deployment-sentry-worker.yaml
Outdated
Show resolved
Hide resolved
charts/sentry/templates/sentry/worker/deployment-sentry-worker.yaml
Outdated
Show resolved
Hide resolved
|
check pod with default values: |
|
@honghainguyen777 @jiriks74 @Poil @bhataprameya @Jorgagu @harmjanblok @PaulFarver @niknozhenko |
|
Hi, Since you're messing around with worker arguments in this PR, would it be possible to add
for Sentry consumers alongside Tweaking these helps to reduce lag quite a lot. |
|
I'm thinking of adding |
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.
I've not checked the integrations, but I have some comments on template problems.
| - | | ||
| {{- if .Values.sentry.web.installAdditionalPackages }} | ||
| pip install {{ range .Values.sentry.web.installAdditionalPackages }}{{ . }} {{ end }} | ||
| {{- end }} | ||
| {{- if .Values.sentry.web.caCertificatesSecret }} | ||
| mkdir -p /usr/local/share/ca-certificates/ | ||
| for c in $(ls -1 /usr/local/share/ca-certificates/); do | ||
| cat /usr/local/share/ca-certificates/$c >> $(python3 -m certifi) && echo >> $(python3 -m certifi) | ||
| done | ||
| update-ca-certificates | ||
| {{- end }} | ||
| sentry run web |
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.
If I understand correctly about /bin/bash -c, it accepts exactly one argument. All the arguments after the first one will be ignored (well not really ignored but these args will be assigned to $0, $1, ...).
# Example
command: ["/bin/bash", "-c"]
args:
- |
sentry run web
- "--workers"
- "4"
$0 = --workers and $1 = 4, but sentry run web (or the whole first string) doesn’t use $0 or $1, so those args don’t get passed to it.
To make it work, you have to have:
command: ["/bin/bash", "-c"]
args:
- |
{{- if .Values.sentry.web.installAdditionalPackages }}
pip install {{ range .Values.sentry.web.installAdditionalPackages }}{{ . }} {{ end }}
{{- end }}
{{- if .Values.sentry.web.caCertificatesSecret }}
mkdir -p /usr/local/share/ca-certificates/
for c in $(ls -1 /usr/local/share/ca-certificates/); do
cat /usr/local/share/ca-certificates/$c >> $(python3 -m certifi) && echo >> $(python3 -m certifi)
done
update-ca-certificates
{{- end }}
sentry run web {{- if .Values.sentry.web.workers }} --workers {{ .Values.sentry.web.workers }}{{- end }}{{- if .Values.sentry.web.logLevel }} --loglevel {{ .Values.sentry.web.logLevel }}{{- end }}{{- if .Values.sentry.web.logFormat }} --logformat {{ .Values.sentry.web.logFormat }}{{- end }}
You may want to add set -euo pipefail so the container fails fast on errors.
| - | | ||
| {{- if .Values.sentry.workerEvents.installAdditionalPackages }} | ||
| pip install {{ range .Values.sentry.workerEvents.installAdditionalPackages }}{{ . }} {{ end }} | ||
| {{- end }} | ||
| {{- if .Values.sentry.workerEvents.caCertificatesSecret }} | ||
| mkdir -p /usr/local/share/ca-certificates/ | ||
| for c in $(ls -1 /usr/local/share/ca-certificates/); do | ||
| cat /usr/local/share/ca-certificates/$c >> $(python3 -m certifi) && echo >> $(python3 -m certifi) | ||
| done | ||
| update-ca-certificates | ||
| {{- end }} | ||
| sentry run worker |
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.
See the first comment
| - | | ||
| {{- if .Values.sentry.workerTransactions.installAdditionalPackages }} | ||
| pip install {{ range .Values.sentry.workerTransactions.installAdditionalPackages }}{{ . }} {{ end }} | ||
| {{- end }} | ||
| {{- if .Values.sentry.workerTransactions.caCertificatesSecret }} | ||
| mkdir -p /usr/local/share/ca-certificates/ | ||
| for c in $(ls -1 /usr/local/share/ca-certificates/); do | ||
| cat /usr/local/share/ca-certificates/$c >> $(python3 -m certifi) && echo >> $(python3 -m certifi) | ||
| done | ||
| update-ca-certificates | ||
| {{- end }} | ||
| sentry run worker |
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.
See the first comment
| - | | ||
| {{- if .Values.sentry.worker.installAdditionalPackages }} | ||
| pip install {{ range .Values.sentry.worker.installAdditionalPackages }}{{ . }} {{ end }} | ||
| {{- end }} | ||
| {{- if .Values.sentry.worker.caCertificatesSecret }} | ||
| mkdir -p /usr/local/share/ca-certificates/ | ||
| for c in $(ls -1 /usr/local/share/ca-certificates/); do | ||
| cat /usr/local/share/ca-certificates/$c >> $(python3 -m certifi) && echo >> $(python3 -m certifi) | ||
| done | ||
| update-ca-certificates | ||
| {{- end }} | ||
| sentry run worker |
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.
See the first comment
|
For the custom CA certificates, can we save the certs in K8S Secret resources and mount them to the containers using |
|
I'm thinking whether it wouldn't be better to have the CA under And I find it better to ass the CA everywhere as Eg. Snuba can connect to Redis using SSL and both Snuba and Sentry can connect to Kafka using SSL. I don't think that having the option for Sentry and having to rebuild the Snuba image would be better than having it everywhere and potentially not using it in some deployments. |
This pull request introduces several enhancements to the Sentry worker deployments:
Additional Package Installation:
installAdditionalPackagesflag in thevalues.yamlfile.django-multidb-routersentry-nodestore-elasticsentry-s3-nodestore(from a specific release)CA Certificates Management:
/usr/local/share/ca-certificates/.update-ca-certificates.caCertificatesSecretfield in thevalues.yamlfile.Changes
commandandargssections in the worker deployment templates to include a bash script that handles the installation of additional packages and CA certificates.Check pip packages
Check certificates
check certificate by openssl
output: