-
Notifications
You must be signed in to change notification settings - Fork 184
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 pvcCleaner for events #3634
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| feat: add pvcCleaner for events |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
71 changes: 71 additions & 0 deletions
71
deploy/helm/sumologic/templates/pvc-cleaner/cron-job-events.yaml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| {{- if eq .Values.pvcCleaner.events.enabled true }} | ||
| apiVersion: batch/v1 | ||
| kind: CronJob | ||
| metadata: | ||
| name: {{ template "sumologic.metadata.name.pvcCleaner.events" . }} | ||
| namespace: {{ template "sumologic.namespace" . }} | ||
| labels: | ||
| app: {{ template "sumologic.labels.app.pvcCleaner.events" . }} | ||
| {{- include "sumologic.labels.common" . | nindent 4 }} | ||
| spec: | ||
| schedule: {{ .Values.pvcCleaner.job.schedule | quote }} | ||
| jobTemplate: | ||
| spec: | ||
| template: | ||
| metadata: | ||
| name: {{ template "sumologic.metadata.name.pvcCleaner.events" . }} | ||
| labels: | ||
| app: {{ template "sumologic.labels.app.pvcCleaner.events" . }} | ||
| {{- include "sumologic.labels.common" . | nindent 12 }} | ||
| {{- with .Values.sumologic.podLabels }} | ||
| {{ toYaml . | indent 12 }} | ||
| {{- end }} | ||
| {{- with .Values.pvcCleaner.job.podLabels }} | ||
| {{ toYaml . | indent 12 }} | ||
| {{- end }} | ||
| annotations: | ||
| {{- with .Values.sumologic.podAnnotations }} | ||
| {{ toYaml . | indent 12 }} | ||
| {{- end }} | ||
| {{- with .Values.pvcCleaner.job.podAnnotations }} | ||
| {{ toYaml . | indent 12 }} | ||
| {{- end }} | ||
| spec: | ||
| nodeSelector: | ||
| {{- if not (empty (include "pvcCleaner.job.nodeSelector" .)) }} | ||
| {{ include "pvcCleaner.job.nodeSelector" . | indent 12 }} | ||
| {{- end }} | ||
| {{- if not (empty (include "pvcCleaner.job.tolerations" .)) }} | ||
| tolerations: | ||
| {{ include "pvcCleaner.job.tolerations" . | indent 12 }} | ||
| {{- end }} | ||
| {{- if not (empty (include "pvcCleaner.job.affinity" .)) }} | ||
| affinity: | ||
| {{ include "pvcCleaner.job.affinity" . | indent 12 }} | ||
| {{- end }} | ||
| {{- with .Values.pvcCleaner.job.securityContext }} | ||
| securityContext: | ||
| {{ toYaml . | indent 12 }} | ||
| {{- end }} | ||
| containers: | ||
| - name: pvc-cleaner | ||
| image: {{ .Values.pvcCleaner.job.image.repository }}:{{ .Values.pvcCleaner.job.image.tag }} | ||
| command: | ||
| - "bash" | ||
| - "/pvc-cleaner/pvc-cleaner.sh" | ||
| - "{{ template "sumologic.namespace" . }}" | ||
| - "app={{ template "sumologic.labels.app.events.statefulset" . }}" | ||
| imagePullPolicy: {{ .Values.pvcCleaner.job.image.pullPolicy }} | ||
| resources: | ||
| {{- toYaml .Values.pvcCleaner.job.resources | nindent 14 }} | ||
| volumeMounts: | ||
| - name: pvc-cleaner | ||
| mountPath: /pvc-cleaner | ||
| volumes: | ||
| - configMap: | ||
| defaultMode: 420 | ||
| name: {{ template "sumologic.metadata.name.pvcCleaner.configmap" . }} | ||
| name: pvc-cleaner | ||
| restartPolicy: Never | ||
| serviceAccountName: {{ template "sumologic.metadata.name.pvcCleaner.roles.serviceaccount" . }} | ||
| {{- end }} | ||
2 changes: 1 addition & 1 deletion
2
deploy/helm/sumologic/templates/pvc-cleaner/serviceaccount.yaml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2363,6 +2363,8 @@ pvcCleaner: | |
| enabled: false | ||
| logs: | ||
| enabled: false | ||
| events: | ||
| enabled: false | ||
|
|
||
| job: | ||
| image: | ||
|
|
||
79 changes: 79 additions & 0 deletions
79
tests/helm/testdata/goldenfile/pvc-cleaner/full_config_events.input.yaml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| sumologic: | ||
| podLabels: | ||
| someSumo: label | ||
| podAnnotations: | ||
| someSumo: annotation | ||
| nodeSelector: | ||
| notMy: node | ||
| tolerations: | ||
| - key: null | ||
| operator: NotExists | ||
| effect: "TestFail" | ||
| affinity: | ||
| nodeAffinity: | ||
| requiredSomethingDuringSomethingElse: | ||
| nodeSelectorTerms: | ||
| - matchExpressions: | ||
| - key: definitely_not | ||
| operator: In | ||
| values: | ||
| - a-correct-affinity | ||
|
|
||
| pvcCleaner: | ||
| events: | ||
| enabled: true | ||
| job: | ||
| image: | ||
| repository: private.ecr.aws/sumologic/kubernetes-tools | ||
| tag: x.y.z | ||
| pullPolicy: Always | ||
| pullSecrets: | ||
| - name: myRegistryKeySecretName | ||
| resources: | ||
| limits: | ||
| memory: 1025Mi | ||
| cpu: 31m | ||
| requests: | ||
| memory: 63Mi | ||
| cpu: 12m | ||
| nodeSelector: | ||
| my: node | ||
| # clean up kubernetes.io/os selector | ||
| kubernetes.io/os: null | ||
| ## Add custom labels only to setup job pod | ||
|
|
||
| ## Node tolerations for server scheduling to nodes with taints | ||
| ## Ref: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/ | ||
| ## | ||
| tolerations: | ||
| - key: null | ||
| operator: Exists | ||
| effect: "NoSchedule" | ||
|
|
||
| ## Affinity and anti-affinity | ||
| ## Ref: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#affinity-and-anti-affinity | ||
| ## | ||
| affinity: | ||
| podAntiAffinity: | ||
| requiredDuringSchedulingIgnoredDuringExecution: | ||
| - labelSelector: | ||
| matchExpressions: | ||
| - key: app | ||
| operator: In | ||
| values: | ||
| - RELEASE-NAME-sumologic-otelcol-logs | ||
| - RELEASE-NAME-sumologic-otelcol-metrics | ||
| - RELEASE-NAME-sumologic-otelcol-events | ||
| - key: app | ||
| operator: In | ||
| values: | ||
| - prometheus-operator-prometheus | ||
| topologyKey: "kubernetes.io/hostname" | ||
|
|
||
| podLabels: | ||
| some: label | ||
| ## Add custom annotations only to setup job pod | ||
| podAnnotations: | ||
| some: annotation | ||
|
|
||
| schedule: "*/2 * * * *" |
81 changes: 81 additions & 0 deletions
81
tests/helm/testdata/goldenfile/pvc-cleaner/full_config_events.output.yaml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| --- | ||
| # Source: sumologic/templates/pvc-cleaner/cron-job-events.yaml | ||
| apiVersion: batch/v1 | ||
| kind: CronJob | ||
| metadata: | ||
| name: RELEASE-NAME-sumologic-pvc-cleaner-events | ||
| namespace: sumologic | ||
| labels: | ||
| app: pvc-cleaner-events | ||
| chart: "sumologic-%CURRENT_CHART_VERSION%" | ||
| release: "RELEASE-NAME" | ||
| heritage: "Helm" | ||
| spec: | ||
| schedule: "*/2 * * * *" | ||
| jobTemplate: | ||
| spec: | ||
| template: | ||
| metadata: | ||
| name: RELEASE-NAME-sumologic-pvc-cleaner-events | ||
| labels: | ||
| app: pvc-cleaner-events | ||
| chart: "sumologic-%CURRENT_CHART_VERSION%" | ||
| release: "RELEASE-NAME" | ||
| heritage: "Helm" | ||
| someSumo: label | ||
| some: label | ||
| annotations: | ||
| someSumo: annotation | ||
| some: annotation | ||
| spec: | ||
| nodeSelector: | ||
| kubernetes.io/os: null | ||
| my: node | ||
| tolerations: | ||
| - effect: NoSchedule | ||
| key: null | ||
| operator: Exists | ||
| affinity: | ||
| podAntiAffinity: | ||
| requiredDuringSchedulingIgnoredDuringExecution: | ||
| - labelSelector: | ||
| matchExpressions: | ||
| - key: app | ||
| operator: In | ||
| values: | ||
| - RELEASE-NAME-sumologic-otelcol-logs | ||
| - RELEASE-NAME-sumologic-otelcol-metrics | ||
| - RELEASE-NAME-sumologic-otelcol-events | ||
| - key: app | ||
| operator: In | ||
| values: | ||
| - prometheus-operator-prometheus | ||
| topologyKey: kubernetes.io/hostname | ||
| securityContext: | ||
| runAsUser: 1000 | ||
| containers: | ||
| - name: pvc-cleaner | ||
| image: private.ecr.aws/sumologic/kubernetes-tools:x.y.z | ||
| command: | ||
| - "bash" | ||
| - "/pvc-cleaner/pvc-cleaner.sh" | ||
| - "sumologic" | ||
| - "app=RELEASE-NAME-sumologic-otelcol-events" | ||
| imagePullPolicy: Always | ||
| resources: | ||
| limits: | ||
| cpu: 31m | ||
| memory: 1025Mi | ||
| requests: | ||
| cpu: 12m | ||
| memory: 63Mi | ||
| volumeMounts: | ||
| - name: pvc-cleaner | ||
| mountPath: /pvc-cleaner | ||
| volumes: | ||
| - configMap: | ||
| defaultMode: 420 | ||
| name: RELEASE-NAME-sumologic-pvc-cleaner | ||
| name: pvc-cleaner | ||
| restartPolicy: Never | ||
| serviceAccountName: RELEASE-NAME-sumologic-pvc-cleaner |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 don't like adding a whole separate CronJob manifest just to change this one parameter. Can we use a tpl function instead?
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.
Actually events differs more, as they do not have hpa
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.
Worth mentioning that we already have separate manifests for metrics and logs: https://github.com/SumoLogic/sumologic-kubernetes-collection/tree/main/deploy/helm/sumologic/templates/pvc-cleaner
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 know, and I think 3 copies is enough to start making them DRY. It'd be best if we could have just one, with different parameters depending on config, but I'm not sure how realistic that is.
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 can create issue for that. Right now I'm on customer support, so I would like to have it done rather quickly with low effort
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 that happens, the Pod will fail to start in the new AZ, because the PVC name is the same, so the previous PVC would need to be manually deleted anyway.
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.
This is what pvcCleaner is for. To remove this pvc, which cannot be attached to the pod
\
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.
pvcCleaner was originally for cleaning up unused PVCs after metadata downscaling. It was definitely not for fixing AZ-related volume mounting problems. And it doesn't run frequently enough by default to be able to do this effectively anyway.
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.
Actually I disagree, the original problem was AZ problem AFAIR, but it was result of non-cleaning unused PVCs. I think we can discuss it internally 😅
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.
Outcome after tests is that this PR doesn't resolve the issue. Closed in favor of #3639