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.

Sign up for GitHub

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

Jump to bottom

feat: add pvc-cleaner to the chart #2796

Merged
merged 3 commits into from
Feb 21, 2023
Merged

feat: add pvc-cleaner to the chart #2796

merged 3 commits into from
Feb 21, 2023

Conversation

aboguszewski-sumo
Copy link
Contributor

@aboguszewski-sumo aboguszewski-sumo commented Jan 16, 2023 •
edited
Loading

This PR adds pvc-cleaner cronjobs to the helm chart. It's based on what @sumo-drosiek created on this branch.

Things done:

  • added additional config to values.yaml
  • added helm templates for cronjobs and service account
  • created helm template test
  • manually tested that it works
  • add docs (readme for values.yaml, description in best-practices.md)
  • add integration test

Resolves #2602

@aboguszewski-sumo aboguszewski-sumo force-pushed the ab-pvc-cleaner branch 7 times, most recently from 66ced85 to d5b3aa7 Compare January 19, 2023 10:59
aboguszewski-sumo
aboguszewski-sumo commented Jan 19, 2023
View reviewed changes
deploy/helm/sumologic/templates/pvc-cleaner/serviceaccount.yaml Outdated
Comment on lines 17 to 69
---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: {{ template "sumologic.metadata.name.pvc-cleaner.roles.serviceaccount" . }}
rules:
- apiGroups:
- ""
resources:
- persistentvolumeclaims
verbs:
- get
- list
- delete
- apiGroups:
- "autoscaling"
resources:
- horizontalpodautoscalers
verbs:
- get
- list
- apiGroups:
- ""
resources:
- pods
verbs:
- get
- list

---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: {{ template "sumologic.metadata.name.pvc-cleaner.roles.serviceaccount" . }}
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: Role
name: {{ template "sumologic.metadata.name.pvc-cleaner.roles.serviceaccount" . }}
subjects:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was based on internal setup. Strangely enough, this wasn't necessary when tested manually, but integration tests didn't work without this role.

@aboguszewski-sumo aboguszewski-sumo force-pushed the ab-pvc-cleaner branch 2 times, most recently from 5f6009f to c0926d9 Compare January 19, 2023 11:09
@aboguszewski-sumo
Copy link
Contributor Author

This is ready to review now, apart from one thing:
API for CronJobs can't be determined now. k8s versions earlier than v1.21 use batch/v1beta1, which is deprecated from v1.21 and removed in v1.25. Versions from v1.21 should use batch/v1 instead. My suggestion is to use something similar to what bitnami does: https://github.com/bitnami/charts/blob/cce758c7135d488c8b3eebde7be8211d6b9a2a8a/bitnami/common/templates/_capabilities.tpl#L73-L94

@aboguszewski-sumo aboguszewski-sumo marked this pull request as ready for review January 19, 2023 11:13
@aboguszewski-sumo aboguszewski-sumo requested a review from a team as a code owner January 19, 2023 11:13
@swiatekm
Copy link

swiatekm commented Jan 19, 2023 •
edited
Loading

This is ready to review now, apart from one thing: API for CronJobs can't be determined now. k8s versions earlier than v1.21 use batch/v1beta1, which is deprecated from v1.21 and removed in v1.25. Versions from v1.21 should use batch/v1 instead. My suggestion is to use something similar to what bitnami does: https://github.com/bitnami/charts/blob/cce758c7135d488c8b3eebde7be8211d6b9a2a8a/bitnami/common/templates/_capabilities.tpl#L73-L94

Yeah, we already do the same thing for PDBs: https://github.com/SumoLogic/sumologic-kubernetes-collection/blob/main/deploy/helm/sumologic/templates/_api_versions_helpers.tpl

@aboguszewski-sumo
Copy link
Contributor Author

Integration tests are failing because of insufficient CPU, I'll try to merge the new test into one of the older ones.

@aboguszewski-sumo aboguszewski-sumo force-pushed the ab-pvc-cleaner branch 3 times, most recently from 03bbd30 to aa344fa Compare January 19, 2023 14:24
@aboguszewski-sumo
Copy link
Contributor Author

Integration tests work after lowering the cpu requests in values.yaml for tests.

@aboguszewski-sumo aboguszewski-sumo added the enhancement New feature or request label Jan 19, 2023
swiatekm
swiatekm reviewed Jan 20, 2023
View reviewed changes
deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
tests/helm/pvc-cleaner/config.sh Outdated Show resolved Hide resolved
@perk-sumo perk-sumo added this to the v3.1 milestone Jan 22, 2023
@aboguszewski-sumo
Copy link
Contributor Author

I applied changes after review.

swiatekm
swiatekm approved these changes Feb 20, 2023
View reviewed changes
Copy link

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

LGTM

@aboguszewski-sumo aboguszewski-sumo merged commit 90370ff into main Feb 21, 2023
@aboguszewski-sumo aboguszewski-sumo deleted the ab-pvc-cleaner branch February 21, 2023 09:24
@perk-sumo perk-sumo modified the milestones: v3.2, v3.3 Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swiatekm swiatekm swiatekm approved these changes

@github-actions github-actions[bot] github-actions[bot] approved these changes

@sumo-drosiek sumo-drosiek Awaiting requested review from sumo-drosiek

@perk-sumo perk-sumo Awaiting requested review from perk-sumo

Assignees
No one assigned
Labels
documentation documentation enhancement New feature or request integration-tests
Projects
None yet
Milestone
v3.3
Development

Successfully merging this pull request may close these issues.

Provide information on using the pvc cleaner script
4 participants
@aboguszewski-sumo @swiatekm @sumo-drosiek @perk-sumo