Skip to content
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 kubeadm images command #63450

Merged
merged 1 commit into from May 10, 2018

Conversation

@chuckha
Copy link
Member

chuckha commented May 4, 2018

Closes kubernetes/kubeadm#388

Signed-off-by: Chuck Ha ha.chuck@gmail.com

What this PR does / why we need it:
This PR adds a list-images subcommand to kubeadm config. We need this to make installing kubernetes on air-gapped environments a little easier. This command will print out a list of images it expects to use for the master node.

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#388

Special notes for your reviewer:

Release note:

Adds a list-images subcommand to kubeadm that lists required images for a kubeadm install.
@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented May 4, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 4, 2018

@chuckha: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cluster-lifecycle-pr-reviews

In response to this:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews

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.

@ericchiang

This comment has been minimized.

Copy link
Member

ericchiang commented May 4, 2018

/ok-to-test

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented May 5, 2018

/test pull-kubernetes-integration

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented May 5, 2018

/test pull-kubernetes-verify


// NewImages returns a "kubeadm images" command
func NewImages(cfgPath string, cfg *kubeadmapi.MasterConfiguration) (*Images, error) {
if cfgPath != "" {

This comment has been minimized.

@fabriziopandini

fabriziopandini May 7, 2018

Contributor

Please use configutil.ConfigFileAndDefaultsToInternalConfig; this will ensure a consistent initialisation for the master-configuration e.g. by invoking SetDynamicDefaults (same as other kubeadm commands)

}

// AddImagesFlags adds the flags required for "kubeadm images"
func AddImagesFlags(flagSet *flag.FlagSet, cfgPath *string) {

This comment has been minimized.

@fabriziopandini

fabriziopandini May 7, 2018

Contributor

Personally I'm in favour of providing a simpler UX (less flags) than kubeadm init/kubeadm phases, but I'm a little bit concerned if the users will find the lack of flags confusing when invoking the command without a config file.
@chuckha What about making the --config flag mandatory? (So there is only a supported path fully consistent with kubeadm init/kubeadm phases)

This comment has been minimized.

@chuckha

chuckha May 8, 2018

Author Member

@fabriziopandini is a config file required for kubeadm init if no flags are provided? I didn't know that. I agree the UX should be as simple as possible. I think I agree here if kubeadm init without a config or flags means nothing then kubeadm config list-images should mean nothing as well.

This comment has been minimized.

@detiber

detiber May 9, 2018

Member

Someone brought this up the other day, and it is definitely tangential to this PR, but we really do need to have a command to generate a config populated with the default values...

Now back on topic, I think requiring a config file is a bit of a poor experience, especially since we should have a happy path to an easy default install. I also think that it makes sense to have a handful of command line flags that can be used for common and trivial config options.

This comment has been minimized.

@detiber

detiber May 9, 2018

Member

Using the existing set of options exposed in v1.10.2:

vagrant@ubuntu:~$ kubeadm init --help          
Run this command in order to set up the Kubernetes master.                                    

Usage:                                         
  kubeadm init [flags]                         

Flags:                                         
      --apiserver-advertise-address string   The IP address the API Server will advertise it's listening on. Specify '0.0.0.0' to use the address of the default network interface.          
      --apiserver-bind-port int32            Port for the API Server to bind to. (default 6443)                                                                                              
      --apiserver-cert-extra-sans strings    Optional extra Subject Alternative Names (SANs) to use for the API Server serving certificate. Can be both IP addresses and DNS names.          
      --cert-dir string                      The path where to save and store the certificates. (default "/etc/kubernetes/pki")                                                              
      --config string                        Path to kubeadm config file. WARNING: Usage of a configuration file is experimental.                                                            
      --cri-socket string                    Specify the CRI socket to connect to. (default "/var/run/dockershim.sock")                                                                      
      --dry-run                              Don't apply any changes; just output what would be done.                                                                                        
      --feature-gates string                 A set of key=value pairs that describe feature gates for various features. Options are:                                                         
Auditing=true|false (ALPHA - default=false)    
CoreDNS=true|false (BETA - default=false)      
DynamicKubeletConfig=true|false (ALPHA - default=false)                                       
SelfHosting=true|false (ALPHA - default=false) 
StoreCertsInSecrets=true|false (ALPHA - default=false)                                        
  -h, --help                                 help for init                                    
      --ignore-preflight-errors strings      A list of checks whose errors will be shown as warnings. Example: 'IsPrivilegedUser,Swap'. Value 'all' ignores errors from all checks.          
      --kubernetes-version string            Choose a specific Kubernetes version for the control plane. (default "stable-1.10")                                                             
      --node-name string                     Specify the node name.                           
      --pod-network-cidr string              Specify range of IP addresses for the pod network. If set, the control plane will automatically allocate CIDRs for every node.                        --service-cidr string                  Use alternative range of IP address for service VIPs. (default "10.96.0.0/12")                                                                  
      --service-dns-domain string            Use alternative domain for services, e.g. "myorg.internal". (default "cluster.local")                                                           
      --skip-token-print                     Skip printing of the default bootstrap token generated by 'kubeadm init'.                                                                       
      --token string                         The token to use for establishing bidirectional trust between nodes and masters.                                                                
      --token-ttl duration                   The duration before the bootstrap token is automatically deleted. If set to '0', the token will never expire. (default 24h0m0s)   

There appear to be a few different "classes" of options that we expose today.

  • command options that do not make sense in the config file:
    • --config
    • -h, --help
    • --dry-run
    • --ignore-preflight-errors
    • --skip-token-print
  • simple, common config overrides
    • --kubernetes-version
    • --node-name
    • --pod-network-cidr
    • --service-cidr
    • --cri-socket
  • feature gates
  • more complex config options, or less common overrides that could have wider implications
    • --apiserver-advertise-address
    • --apiserver-bind-port
    • --apiserver-cert-extra-sans
    • --cert-dir
    • --service-dns-domain

I think the first two "classes" definitely make sense to bubble up as command line options. For less common config options or where common configuration overrides would require multiple command line options to be specified (as in the case of overriding the api server advertise address and port combination) make more sense in the config file.

For feature gates, I think that is a bigger discussion that needs to happen. I think we've been a bit too cavalier with our use of feature gates to date. I don't see an issue with bubbling feature gates up to the command line options, but I think we need to be more judicious in what becomes a feature gate vs a new configuration option.

This comment has been minimized.

@chuckha

chuckha May 9, 2018

Author Member

So luckily for this PR the only one of those that will affect the output here are

--kubernetes-version and feature-gates=CoreDNS=true.

I can add the kubernetes version and the feature gate flags and then in a following PR I will want to refactor the config subcommand to be a package.

@fabriziopandini
Copy link
Contributor

fabriziopandini left a comment

Thanks for this PR! The offline support was one of the topics discussed at kubeconf!
I'm ok with the overall approach, only few comments to make the user experience more consistent with other kubeadm commands.

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented May 7, 2018

So the real question here is whether this should rise to a 1st class subcommand or if it would fit under the config subcommand which we will need.

we need to discuss this one on the sig call, please add to the agenda for this week.

/hold

@chuckha chuckha force-pushed the chuckha:images branch 3 times, most recently from 4b8c057 to fc5567f May 7, 2018

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented May 7, 2018

/test pull-kubernetes-bazel-build

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented May 8, 2018

/test pull-kubernetes-bazel-build

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented May 8, 2018

@chuckha kudos for adding good u-test coverage.

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented May 8, 2018

As discussed in sig-cluster-lifecycle, this is moving to kubeadm config list-images.

@timothysc timothysc added approved and removed do-not-merge/hold labels May 8, 2018

@chuckha chuckha force-pushed the chuckha:images branch from fc5567f to e2ef5a3 May 8, 2018

@chuckha chuckha force-pushed the chuckha:images branch 3 times, most recently from d165e56 to 90d68a0 May 8, 2018

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented May 9, 2018

/test pull-kubernetes-e2e-kops-aws

1 similar comment
@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented May 9, 2018

/test pull-kubernetes-e2e-kops-aws

)

// NewCmdImages returns the "kubeadm images" command
func NewCmdImages(out io.Writer) *cobra.Command {

This comment has been minimized.

@timothysc

timothysc May 9, 2018

Member

If we're being consistent, this would whole subcommand would be in config.go

This comment has been minimized.

@chuckha

chuckha May 9, 2018

Author Member

config is going to have to get refactored. I didn't want to expand the scope of this PR -- how does opening a ticket to refactor config into a package (vs top level files) sound? Then I can stick images in that package and split out config.go into two files which would be more consistent with how we approach other multi-command subcommands.

defaultNumberOfImages = 8
)

func TestNewCmdImages(t *testing.T) {

This comment has been minimized.

@timothysc

timothysc May 9, 2018

Member

Same re: s/images/config

// Run runs the images command and writes the result to the io.Writer passed in
func (i *Images) Run(out io.Writer) error {
imgs := []string{}
imgs = append(imgs, images.GetCoreImage(constants.KubeAPIServer, i.cfg.ImageRepository, i.cfg.KubernetesVersion, i.cfg.UnifiedControlPlaneImage))

This comment has been minimized.

@timothysc

timothysc May 9, 2018

Member

can't we wrap this into a utility f(n)? images.GetAllImages()

This comment has been minimized.

@chuckha

chuckha May 9, 2018

Author Member

We certainly can. I usually leave that for when it's required, but I'm happy to refactor that if you think it's useful.

@chuckha chuckha force-pushed the chuckha:images branch 2 times, most recently from 73a1930 to 668b3e9 May 10, 2018

Adds kubeadm images command
Closes kubernetes/kubeadm#388

Signed-off-by: Chuck Ha <ha.chuck@gmail.com>

@chuckha chuckha force-pushed the chuckha:images branch from 668b3e9 to f5e6881 May 10, 2018

@timothysc
Copy link
Member

timothysc left a comment

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 10, 2018

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: chuckha, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 10, 2018

Automatic merge from submit-queue (batch tested with PRs 62665, 62194, 63616, 63672, 63450). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 828ffd5 into kubernetes:master May 10, 2018

16 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation chuckha authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@luxas
Copy link
Member

luxas left a comment

Super small nit only.

Thanks for this @chuckha, good UX improvement 👍

imgs = append(imgs, GetCoreImage(constants.KubeControllerManager, cfg.ImageRepository, cfg.KubernetesVersion, cfg.UnifiedControlPlaneImage))
imgs = append(imgs, GetCoreImage(constants.KubeScheduler, cfg.ImageRepository, cfg.KubernetesVersion, cfg.UnifiedControlPlaneImage))
imgs = append(imgs, fmt.Sprintf("%v/%v-%v:%v", cfg.ImageRepository, constants.KubeProxy, runtime.GOARCH, kubeadmutil.KubernetesVersionToImageTag(cfg.KubernetesVersion)))
imgs = append(imgs, fmt.Sprintf("%v/pause-%v:%v", cfg.ImageRepository, runtime.GOARCH, "3.1"))

This comment has been minimized.

@luxas

luxas May 11, 2018

Member

Could we source this constant from somewhere? Also this might get updated with new Kubernetes versions, so we maybe should track it in constants like we do with etcd versions...

This comment has been minimized.

@chuckha

chuckha May 11, 2018

Author Member

I looked but the best place I found was in the test framework. I could have overlooked something though. It's a surprisingly hard thing to find.

You're right though. Looking back it looks like it did get bumped in 1.10 to 3.1. I think that will be outside the kubeadm scope but we should have the mechanism in place for getting the correct version given the k8s version.

Might be worth consolidating as we have some logic in constants (etcd) and some logic in the dns package. I'll open a ticket to investigate this work.

This comment has been minimized.

@luxas

luxas May 11, 2018

Member

Thanks!

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented May 14, 2018

@chuckha do you plan to update website for this one?
if not i can do it, but can you dump an output of the command to include it as an example.

@chuckha

This comment has been minimized.

Copy link
Member Author

chuckha commented May 14, 2018

@neolit123 Yeah, I was going to after the code freeze this week. I'm trying to get one more change to this feature before the end of the week so the documentation might be better put off until next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.