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

feat: Add labels to setup Pods for easy logs collection #3817

Merged
merged 8 commits into from
Aug 7, 2024

Conversation

harshshahsumo
Copy link
Contributor

@harshshahsumo harshshahsumo commented Jul 30, 2024

OSC-832 Add labels to setup Pods for easy logs collection

@harshshahsumo harshshahsumo changed the title [OSC-832] Add labels to setup Pods for easy logs collection Add labels to setup Pods for easy logs collection Jul 30, 2024
@harshshahsumo harshshahsumo changed the title Add labels to setup Pods for easy logs collection feat: Add labels to setup Pods for easy logs collection Jul 31, 2024
@harshshahsumo harshshahsumo marked this pull request as ready for review July 31, 2024 20:30
@harshshahsumo harshshahsumo requested a review from a team as a code owner July 31, 2024 20:30
@@ -155,7 +155,11 @@ machineconfiguration.openshift.io/role: worker
machineconfiguration.openshift.io/role: master
{{- end -}}

{{/*
{{- define "sumologic.labels.otelcol.events" -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm this is in a common file, should this be in a new file instead?

Comment on lines 31 to 32
{{- if .Values.sumologic.podLabels }}
{{- include "sumologic.labels.otelcol.events" . | nindent 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- if .Values.sumologic.podLabels }}
{{- include "sumologic.labels.otelcol.events" . | nindent 8 }}
{{- include "sumologic.labels.otelcol.events" . | nindent 8 }}
{{- if .Values.sumologic.podLabels }}

this include should be outside of if and also events shouldn't be in setup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

➜  sumologic-kubernetes-collection git:(osc832_AddLabelsToSetupPods) helm template deploy/helm/sumologic --set sumologic.accessId=dummy --set sumologic.accessKey=dummy
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /Users/drosiek/.kube/config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /Users/drosiek/.kube/config
Error: YAML parse error on sumologic/templates/setup/job.yaml: error converting YAML to JSON: yaml: line 14: did not find expected key

Use --debug flag to render out invalid YAML

https://github.com/SumoLogic/sumologic-kubernetes-collection/actions/runs/10187187667/job/28180686924?pr=3817

Error: templates/setup/job.yaml: unable to parse YAML: error converting YAML to JSON: yaml: line 18: did not find expected key

There is a problem with generating templates/setup/job.yaml

You can generate the file locally with the following command:
helm template deploy/helm/sumologic --set sumologic.accessId=dummy --set sumologic.accessKey=dummy -s templates/setup/job.yaml --debug

➜  sumologic-kubernetes-collection git:(osc832_AddLabelsToSetupPods) helm template deploy/helm/sumologic --set sumologic.accessId=dummy --set sumologic.accessKey=dummy -s templates/setup/job.yaml --debug
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /Users/drosiek/.kube/config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /Users/drosiek/.kube/config
install.go:222: [debug] Original chart version: ""
install.go:239: [debug] CHART PATH: /Users/drosiek/projects/sumologic-kubernetes-collection/deploy/helm/sumologic

---
# Source: sumologic/templates/setup/job.yaml




apiVersion: batch/v1
kind: Job
metadata:
  name: release-name-sumologic-setup
  namespace: sumologic
  annotations:
    helm.sh/hook: pre-install,pre-upgrade
    helm.sh/hook-weight: "3"
    helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded
  labels:
    app: release-name-sumologic
    chart: "sumologic-4.9.0"
    release: "release-name"
    heritage: "Helm"
        sumologic.com/app: otelcol-events
spec:
  ttlSecondsAfterFinished: 120
  template:
    metadata:
      annotations:
      labels:
    spec:
      restartPolicy: OnFailure
      serviceAccountName: release-name-sumologic-setup
      nodeSelector:
        kubernetes.io/os: linux
      volumes:
      - name: setup
        configMap:
          name: release-name-sumologic-setup
          defaultMode: 0777
      - name: custom
        configMap:
          name: release-name-sumologic-setup-custom
          defaultMode: 0777
      containers:
      - name: setup
        image: public.ecr.aws/sumologic/kubernetes-setup:3.15.1
        imagePullPolicy: IfNotPresent
        command: ["/etc/terraform/setup.sh"]
        resources:
          limits:
            cpu: 2000m
            memory: 256Mi
          requests:
            cpu: 200m
            memory: 64Mi
        volumeMounts:
        - name: setup
          mountPath: /etc/terraform
        - name: custom
          mountPath: /customer-scripts
        envFrom:
        - secretRef:
            name: release-name-sumologic-setup
        env:
        - name: NAMESPACE
          valueFrom:
            fieldRef:
              fieldPath: metadata.namespace
        - name: SUMOLOGIC_BASE_URL
          value: 
        - name: SUMOLOGIC_COLLECTOR_NAME
          value: kubernetes
        - name: SUMOLOGIC_SECRET_NAME
          value: "sumologic"
        - name: CHART_VERSION
          value: "4.9.0"
        - name: SUMOLOGIC_MONITORS_ENABLED
          value: "true"
        - name: SUMOLOGIC_MONITORS_STATUS
          value: "enabled"
        - name: SUMOLOGIC_DASHBOARDS_ENABLED
          value: "true"
        
        - name: NO_PROXY
          value: kubernetes.default.svc
      securityContext:
        runAsUser: 1000
Error: YAML parse error on sumologic/templates/setup/job.yaml: error converting YAML to JSON: yaml: line 14: did not find expected key
helm.go:84: [debug] error converting YAML to JSON: yaml: line 14: did not find expected key
YAML parse error on sumologic/templates/setup/job.yaml
helm.sh/helm/v3/pkg/releaseutil.(*manifestFile).sort
	helm.sh/helm/v3/pkg/releaseutil/manifest_sorter.go:146
helm.sh/helm/v3/pkg/releaseutil.SortManifests
	helm.sh/helm/v3/pkg/releaseutil/manifest_sorter.go:106
helm.sh/helm/v3/pkg/action.(*Configuration).renderResources
	helm.sh/helm/v3/pkg/action/action.go:168
helm.sh/helm/v3/pkg/action.(*Install).RunWithContext
	helm.sh/helm/v3/pkg/action/install.go:312
main.runInstall
	helm.sh/helm/v3/cmd/helm/install.go:314
main.newTemplateCmd.func2
	helm.sh/helm/v3/cmd/helm/template.go:95
github.com/spf13/cobra.(*Command).execute
	github.com/spf13/[email protected]/command.go:983
github.com/spf13/cobra.(*Command).ExecuteC
	github.com/spf13/[email protected]/command.go:1115
github.com/spf13/cobra.(*Command).Execute
	github.com/spf13/[email protected]/command.go:1039
main.main
	helm.sh/helm/v3/cmd/helm/helm.go:83
runtime.main
	runtime/proc.go:271
runtime.goexit
	runtime/asm_arm64.s:1222

And then, we can see that the indentation around 18th line is broken:

labels:
    app: release-name-sumologic
    chart: "sumologic-4.9.0"
    release: "release-name"
    heritage: "Helm"
        sumologic.com/app: otelcol-events

it should be

labels:
    app: release-name-sumologic
    chart: "sumologic-4.9.0"
    release: "release-name"
    heritage: "Helm"
    sumologic.com/app: otelcol-events

Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

Please do not add sumologic.com/app: otelcol-events label to setup pods. Setup is not events and it's not otelcol

Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

LGTM, addressing @chan-tim-sumo comment, We don't have separate file for setup right now. If we feel, we need it, we can move setup related templates to new file in another PR

@harshshahsumo harshshahsumo merged commit 17df7b4 into main Aug 7, 2024
55 checks passed
@harshshahsumo harshshahsumo deleted the osc832_AddLabelsToSetupPods branch August 7, 2024 18:20
jagan2221 added a commit that referenced this pull request Sep 13, 2024
* feat: Add labels to setup Pods for easy logs collection (#3817)

* Add labels to setup Pods for easy logs collection

* Addressing comments

* remove unwanted line

* Addressing reviews and fixing bugs

* Fixing name and indent

* Fix test

* Removing the extra space

* Changed events to setup

* feat(metrics): Define allowlist for histogram metrics (#3821)

* feat(metrics): Define allowlist for histogram metrics

* Adding template and integration tests

* Removed KOPs 1.30, not supported yet (#3823)

* docs(histogram-metrics): Updates to the doc around allowing histogram metrics (#3824)

* chore: Upgrade OpenTelemetry Collector to 0.104.0-sumo-1 (#3829)

* chore: update OpenTelemetry Collector to 0.104.0-sumo-1

* Remove deprecated compress_encoding to use compression

* Change the bind address for the otel collector and the health check ext to the pod IP

* Regenerate test files after changes to bind IP address configuration across all pods

* Add otel collector labels to metrics collection

* fix(metrics): Correct the name of k8s hpa metrics (part of kube-state-metrics) (#3832)

* chore: Prepare release 4.10.0 (#3834)

* feat: Add support for the Korea region of the Sumologic deployment (#3835)

---------

Co-authored-by: Harsh Shah <[email protected]>
Co-authored-by: Raj Nishtala <[email protected]>
Co-authored-by: Tim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants