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 additionalLabels in collection-fluent-bit #1599

Merged
merged 1 commit into from
May 27, 2021

Conversation

voidlily
Copy link
Contributor

In #690 the prometheus additionalLabels were updated to use sumologic.com/app instead of app. The entry for collection-fluent-bit was missed, causing the helm chart to fail when using the built in prometheus stack due to duplicate labels.

This change should fix that so the built in prometheus stack works with collection-fluent-bit.

Description

Fill in your description here.


Testing performed
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

In SumoLogic#690 the prometheus additionalLabels were updated to use `sumologic.com/app` instead of `app`. The entry for `collection-fluent-bit` was missed, causing the helm chart to fail when using the built in prometheus stack due to duplicate labels.

This change should fix that so the built in prometheus stack works with collection-fluent-bit.
@voidlily
Copy link
Contributor Author

Have not tested this myself, we currently aren't using the prometheus side of the collector. I noticed this bug when I was upgrading my helm chart from 1.0 to 2.0, missed the breaking change for disabling the prometheus stack, and ran into errors parsing the yaml in the helm chart due to duplicate keys.

@sumo-drosiek
Copy link
Contributor

Hi @voidlily
Thank you for your PR!

This looks fine, but need to get more information. I would rather prefer to add new label instead of replacing current, unless I understand what exactly happens

causing the helm chart to fail when using the built in prometheus stack due to duplicate labels

Could you elaborate what does it exactly mean? What do you mean by built in prometheus stack? Are you operating in openshift?

I tested installation in both cases (before and after change) in our vagrant environment and didn't get any mentioned error.

@gempesaw
Copy link

gempesaw commented May 25, 2021

@sumo-drosiek 👋🏽 when I tried to install the helm chart with terraform like resource "helm_resource" "" { }, I saw an error message like

  line 82: mapping key "app" already defined at line 77 for the following resource:

and then the lines in question are the metadata.labels of this ServiceMonitor thing: (arrows added manually for emphasis)

- apiVersion: monitoring.coreos.com/v1
  kind: ServiceMonitor
  metadata:
    name: collection-fluent-bit
    namespace: sumologic
    labels:
      app: kube-prometheus-stack-prometheus # <--
      chart: kube-prometheus-stack-12.3.0
      release: "sumologic"
      heritage: "Helm"
      app: collection-fluent-bit            # <--
  spec:
    endpoints:
    - path: /api/v1/metrics/prometheus
      port: http
    namespaceSelector:
      matchNames:
      - $(NAMESPACE)
    selector:
      matchLabels:
        app.kubernetes.io/name: fluent-bit
        sumologic.com/scrape: "true"

I'm not actually 100% sure which thing in my stack is doing the yaml validation there.. (terraform? helm provider? kustomize? k8s? 🤷🏽 )

@sumo-drosiek
Copy link
Contributor

@gempesaw Thanks for the comment. It's clear for me now 👍

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.

None yet

3 participants