-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Feature 2160 add images subcommand #2303
Feature 2160 add images subcommand #2303
Conversation
Welcome @curtbushko! |
Hi @curtbushko. Thanks for your PR. I'm waiting for a kubernetes-sigs 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/test-infra repository. |
We may need to consider
That will not work once this supports multi-arch, perhaps best to head that off now. (see: #2176) |
/ok-to-test |
/retest |
Is there a way I could use What I am aiming for is being being able to build our product images locally and build a node image for all developers to use. That gets difficult if I have to pull images from a private registry while building. |
The problem I found is that if you need to ask docker to pull an image for a platform other than the host and then save it for loading you run into issues (e.g. try pulling the image for each platform you're building for and loading it, it will just save the host platform one, there's no way to save --platform), so the only solution I found was to pull for the platform directly to the node container we're going to commit. I think there's more on this in the linked PR? But without a solution for that multi-arch support is untenable, and multi-arch support is very much needed now. |
Also AFAIK there is no way to ask docker for layers locally (versus images by name), the only way docker exposes layers is through pushing to registries. |
Thanks for the explanation and insight. I did go through your PR to see how you did it and I even took a dive into docker engine to see how it was doing save. It's too bad that they didn't add --platform to save even though they added it to other commands. I found something very interesting! It looks like save does save the correct local image but it changes the manifest as it is writing it. I pulled an odd platform version and built my custom image. I am on a mac.
Inspect the images locally and in the custom node. The rootfs layers match.
Even the creation dates are the same!
Bonus: If I run an alpine pod in my custom node image, the logs give me:
Just for fun, I pulled a mips version of redis, built it into my node and tried running it
Digging into this was quite fun! |
@BenTheElder does it makes sense to duplicate the code for the docker commands |
In general I would say no but should it also go to a shared location? |
eh, I prescribe to WET: Write Everything Thrice. Once or twice is not enough usage to design a good abstraction, DRY it out once you hit at least 3 times. but more importantly the code in any |
Is there any news on this PR? This would be extremely useful! |
Any progress on this PR? This is extremly useful and is already used as a fork in multiple projekt for round about a year. No issues so far, so I don't see any reason to defer a merge. Clean-up tasks could be done in a follow-up PR! |
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've personally been overwhelmed earlier this year, and then the past few weeks on vacation.
Will try to be more responsive here.
@@ -0,0 +1,167 @@ | |||
/* |
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.
In this case we do not need to duplicate this entire package, we can just move it to be pkg/build/internal/container/docker
instead of the existing pkg/build/nodeimage/internal/container/docker
, and not duplicate any of it. It will be available to pkg/build/foo
and pkg/build/bar
then.
if we are going to duplicate it, this command should only need a small subset of it, not the whole package.
} | ||
c.logger.V(0).Info("Building in " + containerID) | ||
|
||
// For kubernetes v1.15+ (actually 1.16 alpha versions) we may need to |
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 should not be doing this for arbitrary images we're adding, this is a quirk of component images from Kubernetes's own build process.
|
||
// create build container | ||
c.logger.V(0).Info("Creating build container based on " + c.baseImage) | ||
// pull images to local docker |
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 shouldn't do this anymore, it's too problematic for multi-arch, as discussed in previous review comments.
we pull straight to containerd in the current version of the node image build
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 is also a UX issue that is not thought out here, we should probably have --pull
vs no --pull
.
without --pull
we should use the local docker stored image or tarball, which makes a predictable experience for locally built images
with --pull
+ --arch
we can support multi-arch builds
--arch
without --pull
should possibly be prohibited or throw a warning, unless the inputs are tarballs not image names.
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.
supporting tarballs would make it easier to deal with architectures locally without a registry
for i, tag := range entry.RepoTags { | ||
parts := strings.Split(tag, ":") | ||
if len(parts) > 2 { | ||
return nil, fmt.Errorf("invalid repotag: %s", entry) |
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 check would prevent images stored in a local registry I believe. For example:
localhost:5001/asia.gcr.io/my-gcp-project/my-docker-image:my-tag
localhost:5001/postgres:14.4
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.
not sure if it would be an intended behavior
This is a great feature. I just successfully tested it on my machine, great job! |
Is there anything I can do to help with this feature? |
Looks like there are some comments to be addressed yet. Commits should also be squashed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: curtbushko 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 |
1 similar comment
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: curtbushko 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 |
@curtbushko: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Closing in favour of the more recent #3634 |
Thanks @curtbushko, I think we're close on that one, followed up on the discussion there again. |
This PR addresses Feature 2160 - ability to include additional images in the kindest/node image and hopefully addresses the concerns in the discussion.
The PR:
kind build add-image IMAGE [IMAGES...]
in order to keep the command separate fromkind build node-image
Extras:
An example of the command being used:
Please let me know if there is something you would like to see changed or fixed.