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: use kubectl image for pvc cleaner #2973

Merged
merged 7 commits into from
Apr 11, 2023
Merged

Conversation

sumo-drosiek
Copy link
Contributor

@sumo-drosiek sumo-drosiek commented Apr 6, 2023

The motivation is to use the minimal image required to provide the functionality and I propose to use bitnami/kubectl

Checklist

  • Changelog updated or skip changelog label added
  • Documentation updated
  • Template tests added for new features
  • Integration tests added or modified for major features

Dominik Rosiek added 2 commits April 6, 2023 18:51
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek sumo-drosiek changed the title Drosiek pvc cleaner feat: stop using sumologic kubernetes tools for pvc cleaner Apr 6, 2023
Dominik Rosiek added 3 commits April 7, 2023 09:14
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@github-actions github-actions bot added the documentation documentation label Apr 7, 2023
Dominik Rosiek added 2 commits April 7, 2023 10:17
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek sumo-drosiek marked this pull request as ready for review April 7, 2023 09:25
@sumo-drosiek sumo-drosiek requested a review from a team as a code owner April 7, 2023 09:25
@sumo-drosiek sumo-drosiek changed the title feat: stop using sumologic kubernetes tools for pvc cleaner feat: usie kubectl image for pvc cleaner Apr 7, 2023
@sumo-drosiek sumo-drosiek changed the title feat: usie kubectl image for pvc cleaner feat: use kubectl image for pvc cleaner Apr 7, 2023
Comment on lines -576 to +577
| `pvcCleaner.job.image.repository` | Image repository for pvcCleaner docker containers. | `public.ecr.aws/sumologic/kubernetes-tools` |
| `pvcCleaner.job.image.tag` | Image tag for pvcCleaner docker containers. | `2.15.0` |
| `pvcCleaner.job.image.repository` | Image repository for pvcCleaner docker containers. | `public.ecr.aws/sumologic/kubectl` |
| `pvcCleaner.job.image.tag` | Image tag for pvcCleaner docker containers. | `1.26.3` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, that sounds like a breaking change (although I doubt that many customers are using it now), do we have any procedures for that?

Copy link
Contributor Author

@sumo-drosiek sumo-drosiek Apr 7, 2023

Choose a reason for hiding this comment

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

It is breaking change, as we change mount property of the pod. I don't think we have any procedure for that. We can add warning message for helm chart before 3.5.0 with ask for customers to remove the current jobs

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, but I guess that it still should be discussed once @perk-sumo is back.

Choose a reason for hiding this comment

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

I don't completely understand why this is a breaking change. Because we add a new volume? I suppose someone could build their own version of the tools image and put something there, but that seems farfetched. What kind of configuration would this break?

Copy link
Contributor

Choose a reason for hiding this comment

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

If someone changed the image tag, for example.
I doubt someone is using this with overridden image tag or image name, but still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we add a new volume?

volumeMount is immutable in k8s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double checked and pod spec in cronjob doesn't seem to be immutable (at least volume configuration is not)

Choose a reason for hiding this comment

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

If someone changed the image tag, for example.
I doubt someone is using this with overridden image tag or image name, but still.

If you use a different tools image version, this will still work - the only requirements are bash and kubectl. If someone put something completely custom here, I don't think we support that.

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.

Can you also add checking the pvc cleaner script with shellcheck to the CI?

@sumo-drosiek
Copy link
Contributor Author

Can you also add checking the pvc cleaner script with shellcheck to the CI?

It is already https://github.com/SumoLogic/sumologic-kubernetes-collection/actions/runs/4636802238/jobs/8205047669?pr=2973

Checking ./deploy/helm/sumologic/conf/pvc-cleaner/pvc-cleaner.sh with shellcheck

@swiatekm swiatekm self-requested a review April 11, 2023 13:54
@sumo-drosiek sumo-drosiek merged commit d6b1da9 into main Apr 11, 2023
@sumo-drosiek sumo-drosiek deleted the drosiek-pvc-cleaner branch April 11, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants