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 labels to fluentd sts and setup job pods #819

Merged
merged 12 commits into from
Aug 6, 2020

Conversation

vsinghal13
Copy link
Contributor

@vsinghal13 vsinghal13 commented Aug 4, 2020

Description
  • Users will be able to add labels to all fluentd pods and setup job.
  • There is a common property to apply to all fluentd pods together, as well as the ability to add to each individually.
  • expose the ability to add labels on fluent-bit and Prometheus

Configs:

  • sumologic.podLabels -> add labels to fluentd sts, setup job, otelcol deployment
  • fluentd.podLabels -> add labels to all fluentd sts.
  • fluentd.logs.statefulset.podLabels -> add labels only to logs fluentd sts.
  • fluentd.metrics.statefulset.podLabels -> add labels only to metrics fluentd sts.
  • fluentd.events.statefulsetpodLabels -> add labels only to events fluentd sts.
  • otelcol.deployment.podLabels -> add labels only to otelcol deployment.
  • sumologic.setup.job.podLabels -> add labels only to setup job.
Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

@vsinghal13 vsinghal13 requested review from frankreno, samjsong and sumo-drosiek and removed request for frankreno and samjsong August 4, 2020 21:11
Copy link
Contributor

@frankreno frankreno left a comment

Choose a reason for hiding this comment

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

one comment, then LGTM

@@ -9,6 +9,9 @@ metadata:
labels:
app: {{ template "sumologic.labels.app.setup.job" . }}
{{- include "sumologic.labels.common" . | nindent 4 }}
{{- if .Values.sumologic.setup.job.podLabels }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add the common set here as well i think

@sumo-drosiek
Copy link
Contributor

We should add this to the otelcol as well 😅

@vsinghal13 vsinghal13 marked this pull request as draft August 5, 2020 15:49
@vsinghal13 vsinghal13 marked this pull request as ready for review August 5, 2020 17:03
@frankreno frankreno linked an issue Aug 5, 2020 that may be closed by this pull request
@frankreno
Copy link
Contributor

Good call on OTELCOL, we should add them there too as it will be needed.

Copy link
Contributor

@samjsong samjsong left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit

@vsinghal13 vsinghal13 merged commit f1f0905 into master Aug 6, 2020
@vsinghal13 vsinghal13 deleted the vsinghal-add-custom-labels branch August 6, 2020 17:00
@sumo-drosiek
Copy link
Contributor

Customers should be aware that changing the labels for existing installation need to be done with --force flag

cc: @frankreno

@perk-sumo
Copy link
Contributor

perk-sumo commented Sep 22, 2020

@sumo-drosiek I'm trying to reproduce the error with labels and/or annotations and can't do it, I don't need to provide the --force to change them.

@sumo-drosiek
Copy link
Contributor

sumo-drosiek commented Sep 22, 2020

@perk-sumo even if you change value of the label during upgrade?

@perk-sumo
Copy link
Contributor

Yes, I've tested this with latest 1.2.3 and see no issue with fresh install nor upgrade.

@sumo-drosiek
Copy link
Contributor

@perk-sumo, confirming 👍

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.

Ability to inject additional labels to fluentD statefulsets
5 participants