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

cir-socket flag for kubeadm init does not override config value #2039

Closed
johscheuer opened this issue Feb 26, 2020 · 21 comments
Closed

cir-socket flag for kubeadm init does not override config value #2039

johscheuer opened this issue Feb 26, 2020 · 21 comments
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@johscheuer
Copy link

What keywords did you search in kubeadm issues before filing this one?

cri, config

Is this a BUG REPORT or FEATURE REQUEST?

BUG REPORT

Versions

kubeadm version (use kubeadm version):

kubeadm version
kubeadm version: &version.Info{Major:"1", Minor:"17", GitVersion:"v1.17.3", GitCommit:"06ad960bfd03b39c8310aaf92d1e7c12ce618213", GitTreeState:"clean", BuildDate:"2020-02-11T18:12:12Z", GoVersion:"go1.13.6", Compiler:"gc", Platform:"linux/amd64"}

Environment:

  • Kubernetes version (use kubectl version):
Client Version: version.Info{Major:"1", Minor:"17", GitVersion:"v1.17.1", GitCommit:"d224476cd0730baca2b6e357d144171ed74192d6", GitTreeState:"clean", BuildDate:"2020-01-14T21:04:32Z", GoVersion:"go1.13.5", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"17", GitVersion:"v1.17.1", GitCommit:"d224476cd0730baca2b6e357d144171ed74192d6", GitTreeState:"clean", BuildDate:"2020-01-14T20:56:50Z", GoVersion:"go1.13.5", Compiler:"gc", Platform:"linux/amd64"}
  • Cloud provider or hardware configuration:
    VM's on GCP
  • OS (e.g. from /etc/os-release):
cat /etc/os-release
NAME="Ubuntu"
VERSION="18.04.4 LTS (Bionic Beaver)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 18.04.4 LTS"
VERSION_ID="18.04"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=bionic
UBUNTU_CODENAME=bionic
  • Kernel (e.g. uname -a):
uname -a
Linux master 5.0.0-1031-gcp #32-Ubuntu SMP Tue Feb 11 03:55:48 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
  • Others:

What happened?

For some reasons I was running Docker and CRI-O on the same machine and kubeadm complaint with the following error:

Found multiple CRI sockets, please use --cri-socket to select one: /var/run/dockershim.sock, /var/run/crio/crio.sock

But even when I specified the cri-socket with a flag I still get the error:

$ sudo kubeadm init --cri-socket=/var/run/crio/crio.sock --config=LFS458/SOLUTIONS/s_03/kubeadm-config.yaml --upload-certs 
Found multiple CRI sockets, please use --cri-socket to select one: /var/run/dockershim.sock, /var/run/crio/crio.sock

The error only goes away when I specified an InitConfiguration:

apiVersion: kubeadm.k8s.io/v1beta2
kind: InitConfiguration
nodeRegistration:
  criSocket: /var/run/crio/crio.sock

and this was the kubeadm config file:

apiVersion: kubeadm.k8s.io/v1beta2
kind: ClusterConfiguration
kubernetesVersion: 1.17.1
controlPlaneEndpoint: "k8smaster:6443"
networking:
  podSubnet: 192.168.0.0/16

What you expected to happen?

I would expect that the --cri-socket flag was actually be used (or at least kubeadm would tell me I should use the config file).

How to reproduce it (as minimally and precisely as possible)?

Install Docker and CRI-O side by side and use the minimal kubeadm config to init the cluster.

Anything else we need to know?

Heres the output with -v5:

sudo kubeadm init --cri-socket=/var/run/crio/crio.sock --config=LFS458/SOLUTIONS/s_03/kubeadm-config.yaml --upload-certs --v=5 
I0224 10:06:36.980224   28281 initconfiguration.go:207] loading configuration from "LFS458/SOLUTIONS/s_03/kubeadm-config.yaml"
Found multiple CRI sockets, please use --cri-socket to select one: /var/run/dockershim.sock, /var/run/crio/crio.sock
k8s.io/kubernetes/cmd/kubeadm/app/util/runtime.detectCRISocketImpl
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/util/runtime/runtime.go:214
k8s.io/kubernetes/cmd/kubeadm/app/util/runtime.DetectCRISocket
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/util/runtime/runtime.go:220
k8s.io/kubernetes/cmd/kubeadm/app/util/config.SetNodeRegistrationDynamicDefaults
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/util/config/initconfiguration.go:99
k8s.io/kubernetes/cmd/kubeadm/app/util/config.SetInitDynamicDefaults
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/util/config/initconfiguration.go:50
k8s.io/kubernetes/cmd/kubeadm/app/util/config.documentMapToInitConfiguration
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/util/config/initconfiguration.go:329
k8s.io/kubernetes/cmd/kubeadm/app/util/config.BytesToInitConfiguration
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/util/config/initconfiguration.go:242
k8s.io/kubernetes/cmd/kubeadm/app/util/config.LoadInitConfigurationFromFile
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/util/config/initconfiguration.go:214
k8s.io/kubernetes/cmd/kubeadm/app/util/config.LoadOrDefaultInitConfiguration
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/util/config/initconfiguration.go:226
k8s.io/kubernetes/cmd/kubeadm/app/cmd.newInitData
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/init.go:330
k8s.io/kubernetes/cmd/kubeadm/app/cmd.NewCmdInit.func3
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/init.go:191
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).InitData
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow/runner.go:183
k8s.io/kubernetes/cmd/kubeadm/app/cmd.NewCmdInit.func1
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/init.go:139
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).execute
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/vendor/github.com/spf13/cobra/command.go:826
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).ExecuteC
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/vendor/github.com/spf13/cobra/command.go:914
k8s.io/kubernetes/vendor/github.com/spf13/cobra.(*Command).Execute
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/vendor/github.com/spf13/cobra/command.go:864
k8s.io/kubernetes/cmd/kubeadm/app.Run
	/workspace/anago-v1.17.1-beta.0.42+d224476cd0730b/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/kubeadm.go:50
main.main
	_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/kubeadm.go:25
runtime.main
	/usr/local/go/src/runtime/proc.go:203
runtime.goexit
	/usr/local/go/src/runtime/asm_amd64.s:1357
@SataQiu
Copy link
Member

SataQiu commented Feb 26, 2020

This seems like a bug. I'll take a look. Thanks for your feedback @johscheuer
/assign

@neolit123
Copy link
Member

cc @rosti

@rosti
Copy link

rosti commented Feb 26, 2020

Ok, so the problem here is that we are doing the command line overwrite for the --cri-socket flag in the wrong place (too late). It's actually done after the check.
@SataQiu if you haven't started yet, I can tackle that. It needs some refactoring to get it straight.

/kind bug
/priority important-longterm
/assign
/lifecycle active

@k8s-ci-robot k8s-ci-robot added lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Feb 26, 2020
@SataQiu
Copy link
Member

SataQiu commented Feb 26, 2020

OK! It does look like refactoring is needed.
/unassign

@neolit123 neolit123 added this to the v1.18 milestone Feb 28, 2020
@neolit123 neolit123 modified the milestones: v1.18, v1.19 Mar 8, 2020
@fabriziopandini
Copy link
Member

Uhhmm.. I see a different problem here.
--cri-socket flag should not be allowed when you are using --config

@BenTheElder
Copy link
Member

AIUI most kubernetes tools will override the config field with the CLI flag if set, what prevents us from doing this here?

@fabriziopandini
Copy link
Member

ATM in kubeadm --config and flags touching component config fields are exclusive in all the kubeadm commands.
The main reason behind this choice is to avoid to hard code flags/component config preference rules for the 3 different component config API types file managed today and for all their supported versions (potentially more than 1).

This problem is in the scope of the component config WG, but I'm not aware of progress in this area

@BenTheElder
Copy link
Member

BenTheElder commented Apr 26, 2020 via email

@neolit123
Copy link
Member

AIUI most kubernetes tools will override the config field with the CLI flag if set, what prevents us from doing this here?

kube-proxy is doing it backwards, kubeadm too in some places.
kubelet is deprecating all flags.

i'm +1 on starting to slowly remove some flags in kubeadm that overlap with config and not adding new ones.

@neolit123
Copy link
Member

/remove-lifecycle active
please re-set if needed.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label May 30, 2020
@neolit123 neolit123 modified the milestones: v1.19, v1.20 Jul 27, 2020
@yagonobre
Copy link
Member

/lifecycle active
/assign

@k8s-ci-robot k8s-ci-robot added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Sep 23, 2020
@yagonobre
Copy link
Member

yagonobre commented Sep 23, 2020

What should I do? Make the flag works or remove the flag?
cc @neolit123 @fabriziopandini

@neolit123
Copy link
Member

@rosti mentioned above that we are doing the flag override too late.
so ideally --cri-socket should override the user provided value in config.

@yagonobre
Copy link
Member

I can't reproduce it on kubeadm v1.19.2. @johscheuer can you test it with this version?

@neolit123
Copy link
Member

neolit123 commented Oct 6, 2020

adding some detail here; i was able to reproduce the problem with 1.20-pre.

the problem happens here:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/cmd/init.go#L334

at this point options.externalInitCfg contains the user provided --cri-socket written in options.externalInitCfg.NodeRegistration.CRISocket.

however LoadOrDefaultInitConfiguration ends up calling:
https://github.com/kubernetes/kubernetes/blob/838e7bb27805fd1f18ac12d6ce27bcc34ee6664c/cmd/kubeadm/app/util/config/initconfiguration.go#L46 to perform dynamic defaulting, which ends up calling the CRI socket detection code. https://github.com/kubernetes/kubernetes/blob/838e7bb27805fd1f18ac12d6ce27bcc34ee6664c/cmd/kubeadm/app/util/config/initconfiguration.go#L99

at that point, if the config file does not have the explicit CRI socket the socket detection code will error out in case of multiple sockets on the machine.

later "init" has means to make the user flag override the config file:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/cmd/init.go#L350

but this is never reached.

one immediate problem to me is the following: kubeadm should not load config and perform dynamic defaulting withing a single step.

the process should be the following:

  • load config from file
  • apply flag overrides
  • add dynamic defaults

which roughly means, the following calls should not be part of the document split code:
https://github.com/kubernetes/kubernetes/blob/838e7bb27805fd1f18ac12d6ce27bcc34ee6664c/cmd/kubeadm/app/util/config/initconfiguration.go#L304-L312

if we change that we need to ensure all callers of LoadOrDefaultInitConfiguration() need to also apply the dynamic defaults / validation separately.

@neolit123 neolit123 modified the milestones: v1.20, v1.21 Dec 2, 2020
@neolit123 neolit123 removed the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Jan 8, 2021
@neolit123 neolit123 changed the title kubeadm should use flag for cri socket cir-socket flag for kubeadm init does not override config value Mar 9, 2021
@neolit123 neolit123 modified the milestones: v1.21, v1.22 Mar 9, 2021
@neolit123 neolit123 added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Mar 9, 2021
@pacoxu
Copy link
Member

pacoxu commented Apr 15, 2021

adding some detail here; i was able to reproduce the problem with 1.20-pre.

the problem happens here:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/cmd/init.go#L357

Update the comment links.
/assign @KofClubs

@k8s-ci-robot
Copy link
Contributor

@pacoxu: GitHub didn't allow me to assign the following users: kofClubs.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

adding some detail here; i was able to reproduce the problem with 1.20-pre.

the problem happens here:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/cmd/init.go#L357

Update the comment links.
/assign kofClubs

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.

@KofClubs
Copy link

/assign
I shall try.

@neolit123
Copy link
Member

on kubernetes/kubernetes#101600 we voted to disallow the mixture of --config and --cri-socket.
this is consistent behavior with other flags and we want to remove some flags that already exist in config eventually.
--cri-socket is such a flag.

@neolit123 neolit123 modified the milestones: v1.22, v1.23 Jul 5, 2021
@pacoxu
Copy link
Member

pacoxu commented Sep 6, 2021

/close
since kubernetes/kubernetes#101600 was merged.

reopen if I misunderstand

@k8s-ci-robot
Copy link
Contributor

@pacoxu: Closing this issue.

In response to this:

/close
since kubernetes/kubernetes#101600 was merged.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

No branches or pull requests

10 participants