-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
Adds a kubeadm config images pull command #63833
Conversation
cmd/kubeadm/app/images/puller.go
Outdated
return ip.exec.Command("sh", "-c", fmt.Sprintf("docker pull %v", image)).Run() | ||
} | ||
|
||
return ip.exec.Command(ip.crictlPath, "-r", ip.criSocket, "pull", image).Run() |
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.
Don't we need todo version magic checking to ensure that the crictl can pull for the runtimes we are endorsing? We'll need to ship a version with kubeadm that we we vet.
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.
You are 💯% correct. We'll be addressing that in this issue kubernetes/kubeadm#811
cmd/kubeadm/app/cmd/config.go
Outdated
Use: "images", | ||
Short: "Interact with container images used by kubeadm.", | ||
RunE: cmdutil.SubCmdRunE("images"), | ||
} |
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.
did these two PRs overlap?
#63811
oddly, for the other one the bots are saying PR is unable to be automatically merged. Needs rebase.
but github thinks there is no need for rebase.
the refactor and pull
addition could have been separate commits.
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.
Yeah, that was me basing this branch off that one. I forgot to mention that, my mistake. The refactor and the addition are in two separate commits. I just fixed the merge conflict, it should look a lot better now.
cmd/kubeadm/app/cmd/config.go
Outdated
@@ -258,8 +324,8 @@ func (i *ListImages) Run(out io.Writer) error { | |||
return nil | |||
} | |||
|
|||
// AddListImagesConfigFlag adds the flags that configure kubeadm | |||
func AddListImagesConfigFlag(flagSet *flag.FlagSet, cfg *kubeadmapiv1alpha1.MasterConfiguration, featureGatesString *string) { | |||
// AddImagesCommonConfigFlags adds the flags that configure kubeadm (and affect the images kubeadm will use) |
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 OK to remove the ( )
here, but keep the text in there.
cmd/kubeadm/app/cmd/config.go
Outdated
} | ||
|
||
// Puller is an interface for pulling images | ||
type Puller interface { |
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 about to move it in app/images
? This more part of a re-usabe tools than a UX concern...
(Same for types that follows)
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 was thinking about this as if it were an internal interface (which I should have then named it puller
instead) -- not really something for reuse, but I don't see any reason why it can't live in the images package.
The ImagesPull
was not designed for use elsewhere, but looking at it a bit closer, the only thing I can see that is specific to this command is the glog
call.
Would you be ok if I moved the Puller interface out to the images
package but kept ImagesPull internal to this package for now?
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.
Ever if there is no real use case for reuse now, I would like to have the capability to pull a set of images implemented in the app/images
package instead of in the UX
..., the only thing I can see that is specific to this command is the
glog
call.
I don't see this as a constraint, e.g. there are already several glog calls in phases
cmd/kubeadm/app/images/puller.go
Outdated
) | ||
|
||
// ImagePuller is a struct that can pull images and hides the implementation (crictl vs docker) | ||
type ImagePuller struct { |
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.
criImagePuller?
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 make it private? I'd also argue that the name should not reveal the implementation as this uses either docker or crictl to pull images
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.
The initial lower case was a typo, sorry
However I'm still not fully comfortable about naming of ImagePull
and ImagePuller
because those confused me while reading the code. If we are keeping 'ImagePuller' because it's implementation, what about ImagePull
-> ImageSetPuller
or something that makes more clear it's responsibility to coordinate the pull process for a set of images
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.
@chuckha
I suggested some refactoring for a better separation between UX and the image pull machinery.
However the PR overall is fine for me. Thanks!
cmd/kubeadm/app/images/puller.go
Outdated
|
||
// Pull pulls the actual image using either crictl or docker | ||
func (ip *ImagePuller) Pull(image string) error { | ||
if ip.crictlPath == "" { |
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 logic is wrong. We want to use docker if the cri socket is the default docker socket. Otherwise, use crictl
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.
/approve
@fabriziopandini please lgtm when you are happy with the change.
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 couple of comments that need fixing, but overall this LGTM 👍. Thanks for this patch!
cmd/kubeadm/app/cmd/config.go
Outdated
return cmd | ||
} | ||
|
||
// NewCmdConfigImagesPull returns the `config images pull` command | ||
func NewCmdConfigImagesPull() *cobra.Command { | ||
cfg := kubeadmapiv1alpha1.MasterConfiguration{} |
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.
must use &
pointer 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.
Is 'must' the right word here? I don't think it is unless I'm missing something.
I'm passing around the pointer everywhere else so I understand to reduce the number of &
s it would make more sense to initialize it as a pointer. That being said I do agree, it's better as a pointer.
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 technically not a must, but practically. If you init it as a pointer, you don't have to remember to pass &
all the time. I'm pretty sure most things break if someone somehow forgets to pass the pointer at some time. Would be super hard to debug and notice.
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.
fair point 👍 thanks for explaining what you meant
cmd/kubeadm/app/cmd/config.go
Outdated
// NewCmdConfigImagesPull returns the `config images pull` command | ||
func NewCmdConfigImagesPull() *cobra.Command { | ||
cfg := kubeadmapiv1alpha1.MasterConfiguration{} | ||
kubeadmapiv1alpha1.SetDefaults_MasterConfiguration(&cfg) |
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.
kubeadmscheme.Scheme.Default(cfg)
where kubeadmscheme "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/scheme"
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.
sure, seems reasonable. I need to understand this stuff better
cmd/kubeadm/app/cmd/config.go
Outdated
func (ip *ImagesPull) PullAll() error { | ||
for _, image := range ip.images { | ||
if err := ip.puller.Pull(image); err != nil { | ||
return fmt.Errorf("failed to pull image [%v]: %v", image, err) |
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.
nit: maybe use %q
instead of [%v]
?
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.
thanks for this nit, I always default to %v
but will endeavor to improve my formatting strings :)
cmd/kubeadm/app/cmd/config.go
Outdated
if err := ip.puller.Pull(image); err != nil { | ||
return fmt.Errorf("failed to pull image [%v]: %v", image, err) | ||
} | ||
glog.Infof("[config/images/pull] Pulled %v", image) |
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.
just [images]
is fine for me here. As image
is a string, use %s\n
(don't forget the newline)
cmd/kubeadm/app/cmd/config.go
Outdated
@@ -254,12 +304,12 @@ func NewImagesList(cfgPath string, cfg *kubeadmapiv1alpha1.MasterConfiguration) | |||
}, nil | |||
} | |||
|
|||
// ImagesList defines the struct used for "kubeadm config images list" | |||
// ImagesList defines the struct used for "kubeadm images" |
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 change this?
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 was probably a merge conflict failing, good 👀
cmd/kubeadm/app/images/puller.go
Outdated
import ( | ||
"fmt" | ||
|
||
kubeadmdefaults "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1alpha1" |
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.
kubeadmapiv1alpha1
cmd/kubeadm/app/images/puller.go
Outdated
// NewImagePuller returns a ready to go ImagePuller | ||
func NewImagePuller(execer utilsexec.Interface, criSocket string) (*ImagePuller, error) { | ||
crictlPath, err := execer.LookPath("crictl") | ||
if crictlPath == "" && err != nil { |
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 happens if crictlPath
is "" but there isn't an 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.
&&
-> ||
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.
Also wrong logic from before -- If we need crictl (our crisocket is not the default) then we fail. Otherwise it's fine that we don't have crictl available (since we use docker instead).
79aeac4
to
c2d0f8b
Compare
This command will use crictl or docker to pull images locally. The dockerfall back is needed because in some cases the kubelet is not yet running so there is no CRI dockershim socket available. Fixes kubernetes/kubeadm#812 Signed-off-by: Chuck Ha <ha.chuck@gmail.com>
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.
/lgtm
Thanks a lot Chuck 🎉!
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: chuckha, luxas, timothysc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
This command will use crictl or docker to pull images locally.
The dockerfall back is needed because in some cases the kubelet is not
yet running so there is no CRI dockershim socket available.
Fixes kubernetes/kubeadm#812
Signed-off-by: Chuck Ha ha.chuck@gmail.com
What this PR does / why we need it:
This PR adds a command to the
kubeadm config images
subcommand to pull images using either crictl or docker.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes kubernetes/kubeadm#812
Special notes for your reviewer:
Release note:
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews