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

Fix/persistent queue #668

Closed
wants to merge 8 commits into from

Conversation

wojtekzyla
Copy link
Contributor

Things included in this PR:

  • enable persistent queues configuration in the helm chart
  • while creating ACLs in the initContainer in the daemonset, change the default user and group of the otel image to 999
  • add post delete hook which deletes ACLs set for directories used by agent, gateway and clusterReceiver while those pods run as non-root users

@wojtekzyla wojtekzyla requested review from a team as code owners February 16, 2023 14:01
# NOTE: The File Storage extension will persist state to the node's local file system.
# While using the persistent queue it is advised to increase memory limit for agent (agent.resources.limits.memory)
# to 1Gi.
persistentQueueEnabled:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make more sense to nest this in sendingQueue since that's what uses it?

@@ -11,6 +11,11 @@ extensions:
directory: {{ .Values.logsCollection.checkpointPath }}
{{- end }}

{{- if or .Values.splunkPlatform.persistentQueueEnabled.logs .Values.splunkPlatform.persistentQueueEnabled.metrics }}
Copy link
Contributor

Choose a reason for hiding this comment

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

you defined the "splunk-otel-collector.persistentQueueEnabled" template for this purpose

@@ -614,6 +619,9 @@ service:
{{- if and (eq (include "splunk-otel-collector.logsEnabled" .) "true") (eq .Values.logsEngine "otel") }}
- file_storage
{{- end }}
{{- if or .Values.splunkPlatform.persistentQueueEnabled.logs .Values.splunkPlatform.persistentQueueEnabled.metrics }}
Copy link
Contributor

@rmfitzpatrick rmfitzpatrick Feb 23, 2023

Choose a reason for hiding this comment

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

same template note (there are other cases throughout)**

@@ -135,27 +135,39 @@ spec:
imagePullPolicy: {{ .Values.image.initPatchLogDirs.pullPolicy }}
command: ['sh', '-c', '
mkdir -p {{ .Values.logsCollection.checkpointPath }};
chown -Rv {{ $agent.securityContext.runAsUser | default 20000 }}:{{ $agent.securityContext.runAsGroup | default 20000 }} {{ .Values.logsCollection.checkpointPath }};
chown -Rv {{ $agent.securityContext.runAsUser | default 999 }}:{{ $agent.securityContext.runAsGroup | default 999 }} {{ .Values.logsCollection.checkpointPath }};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good change but could be problematic if users have adopted the 20000 id. Can you please add a breaking change note in the changelog?

chmod -v g+rwxs {{ .Values.logsCollection.checkpointPath }};
{{ if .Values.logsCollection.containers.enabled -}}
if [ -d "/var/lib/docker/containers" ];
then
setfacl -n -Rm d:m::rx,m::rx,d:g:{{ $agent.securityContext.runAsGroup | default 20000 }}:rx,g:{{ $agent.securityContext.runAsGroup | default 20000 }}:rx /var/lib/docker/containers;
setfacl -n -Rm d:m::rx,m::rx,d:g:{{ $agent.securityContext.runAsGroup | default 999 }}:rx,g:{{ $agent.securityContext.runAsGroup | default 999 }}:rx /var/lib/docker/containers;
Copy link
Contributor

@dmitryax dmitryax Feb 23, 2023

Choose a reason for hiding this comment

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

@wojtekzyla can you please make a separate PR for these changes? They seem unrelated and should be captured in a changelog separately. And please link an issue in that PR

@dmitryax
Copy link
Contributor

dmitryax commented Mar 8, 2023

Please rebase

@wojtekzyla wojtekzyla closed this Apr 24, 2023
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.

3 participants