-
Notifications
You must be signed in to change notification settings - Fork 546
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: custom image settings for k8s upgrade #8283
Conversation
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 PR sounds good, but it has a subtle issue - previously talosctl
would always set the proper image ref, but right now it will try to update the previous one.
upstream k8s images, as well as Talos-provided kubelet images were migrated a couple of times, and this change will break that migration
k8s.gcr.io -> registry.k8s.io
ghcr.io/talos-systems -> ghcr.io/siderolabs
the reason I brought this up is that probably a more proper way is to use flags which give you base image references for each component, and you can override them if you want, but the tool still always doesn't patch the tag, but sets a complete reference. also there's another ticket about pinning the images using digests (SHAs), which this PR would not support (but currently upgrade-k8s doesn't support them either) |
Another commit will be pushed to add a flag for upgrade options |
@smira I'm trying to manage proxy image too but I'm not sure where is managed .ProxyImage in DaemonSet template and if the manifest is managed differently between init and upgrade be cause as far as I know the version of the proxy image is base on talosctl default kubernetes version |
not sure what you mean, but |
dbed97c
to
e46224f
Compare
Hello, I just have tested new flag and proxy kubelet images management that works well. Any change to see the PR moving forward ? (FYI gpg has been generated in a wsl, and i'm can't understand what is appening on number of commits. Regards |
it's not quite the way I saw this change, but I can take a look if I can twist the way I'd like to. We'll get it reviewed. |
Ok thank you, I wanted to wait this on to be merged before make a proposal on the digest capability base on that code. |
/ok-to-test |
@mstrohl please take a look, I reworked it to be a bit different, and more flexible |
@smira look good to me |
/promote integration-provision |
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.
Maybe a release notes too?
probably, and also some words in the docs |
Allows to use custom registry/images. Fixes: siderolabs#8275 Co-authored-by: @g3offrey Signed-off-by: Matthieu STROHL <mstrohl@dive-in-it.com> Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
/m |
/m |
Pull Request
Fixes: #8275
What? (description)
Due to the usage of authentication for image pullin adding this in the pre-pull feature was to big that is why we enable the feature only on --pre-pull=false.
It could be another feature.
Why? (reasoning)
The cli patch MachineConfig using public registries. This break automatic upgrade because of public registries overwriting.
Acceptance
Please use the following checklist:
make conformance
)make fmt
)make lint
)make docs
)make unit-tests
)Co-author : @g3offrey