-
Notifications
You must be signed in to change notification settings - Fork 183
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
Conversation
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
| `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` | |
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.
Technically, that sounds like a breaking change (although I doubt that many customers are using it now), do we have any procedures for that?
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.
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
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.
Sounds good, but I guess that it still should be discussed once @perk-sumo is back.
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 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?
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 someone changed the image tag, for example.
I doubt someone is using this with overridden image tag or image name, but still.
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.
Because we add a new volume?
volumeMount
is immutable in k8s
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 double checked and pod spec in cronjob doesn't seem to be immutable (at least volume configuration is not)
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 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.
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.
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
|
The motivation is to use the minimal image required to provide the functionality and I propose to use bitnami/kubectl
Checklist