-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
[kubeadm] specify an alternate location for all images and pre pull them #36759
Conversation
Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands will still work. Regular contributors should join the org to skip this step.
If you have questions or suggestions related to this bot's behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
|
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 flag should be documented fully (both in the code and in our main documentation) or moved to an env variable so as not to pollute our help strings.
@@ -155,6 +156,10 @@ func NewCmdInit(out io.Writer) *cobra.Command { | |||
"Port for JWS discovery service to bind to", | |||
) | |||
|
|||
cmd.PersistentFlags().StringVar( | |||
&cfg.ImagePrefix, "image-prefix", gcrPrefix, | |||
"use specific images instead of gcr", |
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 help string should be more descriptive. I'd do something like: Specify an alternate location for all images. Currently this includes: etcd, kube-discovery, kube-apiserver, kube-controller-manager, kube-scheduler, kube-proxy, kubedns, kube-dnsmasq, exechealthz, pause
Ideally that list would be generated and we'd have clear instructions on how to create a custom set of images. Or we'd have a way to override just some of them as necessary. As things stand, this is a pretty advanced move. Unless we are ready to document how to use this we should probably make it an env variable instead of a command line flag in order to not pollute our kubeadm --help
.
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.
A command line flag is more friendly for kubeadm users.
In fact, I have a more radical branch( pre pull images and configurable pod ) for personal dev and test.
If this is a command line flag, it should probably have a release note. |
1e44a9a
to
461fb3d
Compare
461fb3d
to
fb9b508
Compare
need a pre flight check to notify user set kubelet pod-infra-image flag. will do this change tonight. you can review this commit tomorrow. |
fb9b508
to
722bde5
Compare
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.
@bulletRush -- this looks difficult enough that I don't think it should be a command line flag. It isn't clear or easy to use this/these flags. We've been working hard to keep the number of flags minimal here.
The case for doing the pre-pulling of images (vs just letting the kubelet do it) isn't clear.
If we think that this is something that a large percentage of kubeadm users will be using (>30-50%) then I'm game. If not, we should probably leave it as a more obscure env variable. If/as we document this and it becomes more common we can promote it to a flag.
"k8s.io/kubernetes/pkg/api" | ||
"k8s.io/kubernetes/pkg/apis/extensions" | ||
clientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" | ||
certutil "k8s.io/kubernetes/pkg/util/cert" | ||
"k8s.io/kubernetes/pkg/util/wait" | ||
"time" |
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.
Why did time move down here?
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's seem like my ide or ./hack/update-gofmt.sh does that. I'm not sure and I will try to revert this.
"Specify an alternate location for all images. Currently this includes: etcd, kube-discovery, kube-apiserver, kube-controller-manager, kube-scheduler, kube-proxy, kubedns, kube-dnsmasq, exechealthz, pause", | ||
) | ||
|
||
cmd.PersistentFlags().BoolVar( |
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 looks like this defaults to false. What is the scenario here?
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's DefaultImagePrefix
(gcr.io/google_containers). see cmd/kubeadm/app/apis/kubeadm/v1alpha/defaults.go
return | ||
} | ||
warnings = []error{ | ||
fmt.Errorf("custom image prefix(%s) detected, please confirm whether you need to modify the kubelet `pod-infra-container-image` flag", ipc.imagePrefix), |
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.
What does this mean? How/what should be modified here? Without some sort of guide this is not actionable for users.
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.
when start up a pod, kubelet will create a pod-infra-container first. Kubelet will download image from gcr.io/google_containers by default. If you specify a custom registry to download your container image, you may also need change where pod-infra-container image download from.
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.
--pod-infra-container-image string The image whose network/ipc namespaces containers in each pod will use. (default "gcr.io/google_containers/pause-amd64:3.0")
@kubernetes/sig-cluster-lifecycle |
This is docker specific. We need kubeadm to work with any container runtime that k8s supports. |
@jbeda if you have a bad network or you are just in some special country like China, you can't download container image from gcr.io success, and you will wait for control plane to become ready forever. when kubeadm is first released, most people complain they stop at "<master/apiclient> created API client, waiting for the control plane to become ready" and don't know what happened. but if we have pre pulled images and still stop at 'waitting' step after a long time, then we will relize that there must be something else happened. For me, it's invalid hostname, and I have make a PR fixed that. |
If we want to support "mirroring" these images elsewhere, we should think about this from the point of view of the total user experience and make sure that we make that feature usable. First off -- I'm okay with moving the default image pull policy to Second -- we should start with documenting this stuff. I suspect that we can get away with a combination of The tricky part, actually, is getting the list of images necessary. There are just too many of them. Perhaps the best way to go here would be to implement a new command ( Then we tell people to run If we go ahead with |
Seem like this is an unacceptable PR. please close this it and I'll keep this in my private branch. |
I think we should document this for sure, but we have to be very cautious with what we merge into kubeadm, otherwise it will do everything but do the dishes for you :) If you want/can, you could send a PR against kubernetes.github.io and we'll see where we like to put that information. Getting kubeadm smoothly running in China is important for sure 👍 Also, if you want to contribute the parts of this PR that set the imagePullPolicy to IfNotExists and the code that refactored the kube-discovery image path, feel free to open a new PR with those changes, because I think those are good changes. Thanks |
@jbeda hope this command will be implemented |
What this PR does / why we need it: specify an alternate location for all images and pre pull them when
kubeadm init
Which issue this PR fixes (optional, in
fixes #<issue number>(, #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #[kubeadm] pre pull images and configurable pod implement
Special notes for your reviewer:
sample command for this pr:
and I will make a PR to kubernetes docs when this is merged to master.
Release note:
This change is