-
Notifications
You must be signed in to change notification settings - Fork 1.7k
replace load docker-image with load container-image to support multiple providers
#4053
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
base: main
Are you sure you want to change the base?
replace load docker-image with load container-image to support multiple providers
#4053
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Blaimi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @Blaimi! |
|
Hi @Blaimi. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
650366c to
7f09e46
Compare
load docker-image with load container-image to support multiple providers
stmcginnis
left a comment
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.
With this implementation, you can only use the same provider for loading images as is used for running kind. I.e. it is not possible to load an image from podman and have the cluster running in docker.
I think that's fine and would probably be the expected/desired behavior for most users.
| Short: "Loads docker images from host into nodes", | ||
| Long: "Loads docker images from host into all or specified nodes by name", | ||
| Deprecated: "Please use 'kind load container-image' instead of 'kind load docker-image', 'docker-image' will be removed in future releases and is restricted to the docker-provider.", | ||
| RunE: func(cmd *cobra.Command, args []string) error { |
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 haven't looked closely, but can we have this command just call the new code?
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.
No. This would break existing installations, where the image is built/available via docker but the cluster runs on podman/nerdctl. I'm a huge fan of not breaking stuff because of new features ;-)
7f09e46 to
720bccc
Compare
BenTheElder
left a comment
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.
couple quick thoughts
I think this overall makes sense, but it's also not quite the UX discussed in the issue (can't override source, only supports locally stored images ...).
| } | ||
|
|
||
| // ContainerSave saves images to dest, as in `docker save` | ||
| func (p *provider) ContainerSave(images []string, dest string) error { |
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.
Saving images from storage isn't really a property of implementing nodes, and this is already a pretty complicated object. I think we should probably split this out.
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.
We also lose the ability to import across providers cleanly, which while a bit weird we probably already have users doing.
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.
Saving images from storage isn't really a property of implementing nodes, and this is already a pretty complicated object. I think we should probably split this out.
Any idea for locations? I would suggest pkg/cmd/kind/load/providers/*/provider.go
We also lose the ability to import across providers cleanly, which while a bit weird we probably already have users doing.
Can you explain that? load docker-image will be backwards compatible.
| @@ -0,0 +1,262 @@ | |||
| /* | |||
| Copyright 2019 The Kubernetes Authors. | |||
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.
new files should drop the year (previously they should've had the year of creation, so 2025 here)
dropping the year is ... new kubernetes/steering#299
| return | ||
| } | ||
| exists = true | ||
| sanitizedImage = sanitizeImage(imageName) |
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 may be inaccurate on podman where the user may have configured a different default image host because :reasons:
for our own node images, forcing the default host we actually use makes sense, for user's images ... maybe 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.
Any suggestion? Just not sanitizing will lead to an error then—or make the command interactive which might lead to new problems …
BenTheElder
left a comment
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.
renaming the existing command, modifying it, and then back-adding a supposedly compatible version seems ... far more confusing than just introducing a new command to begin with and leaving the existing one ~untouched
That's for the long run of the git history. Once the then deprecated |
This POC abstracts
docker saveto the providers so that images from podman and nerdctl can also be used with theload container-imagecommand can be used instead of restrict it to docker withload docker-image.TBD
load {docker,podman,nerdctl}-imageand/or have additional configuration options likeKIND_EXPERIMENTAL_PROVIDER.TODO
load docker-imagebecause it is actually copy&paste with very small ajustments.Fix: #2038
Relates: GoogleContainerTools/skaffold#9901