Use config file to join nodes to the cluster by aojea · Pull Request #616 · kubernetes-sigs/kind

aojea · 2019-06-14T16:28:45Z

This avoids having to write configuration files on runtime, ~~however, seems that the control plane nodes can't use it~~

aojea · 2019-06-14T16:29:04Z

/assign @neolit123
/assign @BenTheElder

neolit123

thanks for the PR @aojea
aren't we already passing the main template with the --- separate different kubeadm config types to joining nodes?

it should also work for control-plane nodes, fine.

neolit123 · 2019-06-14T20:53:09Z

pkg/cluster/internal/kubeadm/config.go

+  kubeletExtraArgs:
+    fail-swap-on: "false"
+    {{ if .APIAdvertiseAddress -}}
+    node-ip: "{{ .APIAdvertiseAddress }}"


could you please explain why we need node-ip here?

Ipv6 needs it, otherwise Kubelet picks the ipv4

understood.
would the kubelet long term always default to an ipv4 from the default interface...or would it be able to somehow pick a ipv6?

IIRC the logic implemented in Kubernetes always defaults to the IPv4 address ... don´t think that´s a behavior that can change soon ... will add it to my backlog, it can be something interesting to implement though

neolit123 · 2019-06-14T20:55:02Z

pkg/cluster/internal/kubeadm/config.go

 	// assume the latest API version, then fallback if the k8s version is too low
 	templateSource := ConfigTemplateBetaV2
-	if ver.LessThan(version.MustParseSemantic("v1.12.0")) {
-		templateSource = ConfigTemplateAlphaV2


did we drop the kind 1.11 tests already?

Yeah, they aren't running, also 1.15 is close to be ready... No strong feelings but we should move on sooner or later :)

We only did because one of the tests is broken and officially Kubernetes CI won't support it. We should make best effort to support feasible versions.

I dropped the 1.11 in a separate commit, let me remove that commit and figure out the code for the v1alpha2 config files

neolit123 · 2019-06-14T20:56:54Z

pkg/cluster/internal/kubeadm/config.go

+// JoinConfigTemplate is the kubadm join config template
+const JoinConfigTemplate = `# config generated by kind
+# no-op entry that exists solely so it can be patched
+apiVersion: kubeadm.k8s.io/{{ .KubedamAPI }}


IMO, the correct way is separate constants - JoinConfigTemplate<version>.
the API is v1betav2, but still not final.

but also we already have JoinConfiguration as part of the other config templates, which is there for the sake of being patchable from kustomize. isn't it already used when nodes are joining?

Joins are done with the command line, i took one option but I'm missing lot of context, appreciate any feedback and advice

i think we can pass the same kubeadm.cfg to both init and join (with the multi-yaml doc; --- separated). i was under the impression we are already doing that.

I abandoned that idea because I thought that will break the kubeadm join and it could simplify the join file creation since it needs fewer parameters, any way you're the expert here

pkg/container/docker/restart.go

neolit123 · 2019-06-14T21:03:19Z

pkg/cluster/internal/create/actions/kubeadmjoin/join.go

 	}

+	if err = writeKubeadmJoinConfiguration(node, joinAddress); err != nil {
+		return errors.Wrapf(err, "failed to generate kubeadm join configuration")


...the kubeadm JoinConfiguration

neolit123 · 2019-06-14T21:03:33Z

pkg/cluster/internal/create/actions/kubeadmjoin/join.go

+	// get installed kubernetes version from the node image
+	kubeVersion, err := node.KubeVersion()
+	if err != nil {
+		return errors.Wrap(err, "failed to get kubernetes version from node")


kubernetes -> Kubernetes

aojea · 2019-06-14T22:57:22Z

seems join configuration for v1.12 needs a different template, it fails with

16:38:43.002425 435 interface.go:244] Interface \"eth0\" has 1 addresses :[172.17.0.3/16].\nI0614 16:38:43.002581 435 interface.go:211] Checking addr 172.17.0.3/16.\nI0614 16:38:43.003448 435 interface.go:218] IP found 172.17.0.3\nI0614 16:38:43.003517 435 interface.go:250] Found valid IPv4 address 172.17.0.3 for interface \"eth0\".\nI0614 16:38:43.003609 435 interface.go:395] Found active IP 172.17.0.3 \n[tlsBootstrapToken: Invalid value: \"\": the bootstrap token is invalid, discovery: Invalid value: \"\": discoveryToken or discoveryFile must be set]"

neolit123 · 2019-06-14T23:26:39Z

it needs a token:

apiVersion: kubeadm.k8s.io/v1alpha3
kind: JoinConfiguration
token: "{{ .Token }}"
apiServerEndpoint: ........
discoveryTokenUnsafeSkipCAVerification: true

apiVersion: kubeadm.k8s.io/v1beta1
kind: JoinConfiguration
discovery:
  bootstrapToken:
    apiServerEndpoint: ........
    token: "{{ .Token }}"
    unsafeSkipCAVerification: true

apiVersion: kubeadm.k8s.io/v1beta2
kind: JoinConfiguration
discovery:
  bootstrapToken:
    apiServerEndpoint: ........
    token: "{{ .Token }}"
    unsafeSkipCAVerification: true

neolit123 · 2019-06-14T23:28:26Z

pkg/cluster/internal/create/actions/kubeadmjoin/join.go

 		// increase verbosity for debugging
 		"--v=6",
 		"--cri-socket=/run/containerd/containerd.sock",
+		"--config=/kind/kubeadm-join.conf",


needs to be done for CP nodes too.
EDIT: for v1alpha3 joining CP nodes needs a bool
https://github.com/kubernetes/kubernetes/blob/release-1.12/cmd/kubeadm/app/apis/kubeadm/v1alpha3/types.go#L299-L302

for v1beta* it uses a pointer to an object where the endpoint is:
https://github.com/kubernetes/kubernetes/blob/release-1.14/cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go#L315-L323

https://github.com/kubernetes/kubernetes/blob/release-1.15/cmd/kubeadm/app/apis/kubeadm/v1beta2/types.go#L316-L324

neolit123 · 2019-06-14T23:30:38Z

pkg/cluster/internal/create/actions/kubeadmjoin/join.go

 		"--ignore-preflight-errors=all",
 		// increase verbosity for debugging
 		"--v=6",
 		"--cri-socket=/run/containerd/containerd.sock",


no need to pass if present in config.

neolit123 · 2019-06-14T23:32:08Z

pkg/cluster/internal/kubeadm/config.go

+    node-ip: "{{ .NodeAddress }}"
+discovery:
+  bootstrapToken:
+    apiServerEndpoint: "{{ .JoinAddress }}"


for v1alpha3 it's different:
https://github.com/kubernetes/kubernetes/blob/release-1.12/cmd/kubeadm/app/apis/kubeadm/v1alpha3/types.go#L299-L302

(thus the recommendation to have separate constants)

for v1beta* this seems OK.

aojea · 2019-06-15T22:15:16Z

pkg/cluster/internal/create/actions/kubeadmjoin/join.go

 		// TODO(bentheelder): limit the set of acceptable errors
 		"--ignore-preflight-errors=all",
-		// increase verbosity for debugging
+		// increase verbosity for debug


seems I cleaned more than necessary 😓

BenTheElder

Please do not unnecessarily break v1.11.X support.
Also:

there should be a single config template per version
the config file should be generated per node but with the same mechanism
this should correctly handle each node's Kubernetes version

pkg/cluster/internal/create/actions/config/config.go

resolved

BenTheElder · 2019-06-19T07:27:09Z

I want to take another pass when I'm a little more awake but this looks right now and like a great improvement, thank you!

BenTheElder · 2019-06-19T20:29:12Z

failing to join workers locally with 1.11, investigating...
/hold

BenTheElder · 2019-06-19T22:52:59Z

v1.11 doesn't have kubernetes/kubernetes@9109e62 🙃
will fix in a follow-up
/hold cancel

k8s-ci-robot · 2019-06-19T22:53:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, BenTheElder

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [BenTheElder]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

BenTheElder · 2019-06-19T23:10:09Z

pkg/cluster/internal/create/actions/config/config.go

+		return err
+	}
+	if len(workers) > 0 {
+		kubeadmClusterConfig.ControlPlane = false


this can cause all config templates to be controlplane=false

that's because while the function calls will get a pass by value of this struct, they copy from the same shared instance of the struct and we assign to it here

I'm fixing this in the patch to fix v1.11

BenTheElder · 2019-06-19T23:13:03Z

/hold

BenTheElder · 2019-06-19T23:13:35Z

since this hadn't merged by the time I filed the follow up, I'm just including these commits in #632 😅

BenTheElder · 2019-06-19T23:51:34Z

included in #632

thank you!!

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2019

k8s-ci-robot requested review from amwat and neolit123 June 14, 2019 16:28

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 14, 2019

k8s-ci-robot assigned BenTheElder and neolit123 Jun 14, 2019

aojea force-pushed the kubeadmjoin branch from 652f569 to f0fbf82 Compare June 14, 2019 16:30

neolit123 reviewed Jun 14, 2019

View reviewed changes

aojea force-pushed the kubeadmjoin branch from f0fbf82 to 2e6c71e Compare June 15, 2019 22:07

aojea commented Jun 15, 2019

View reviewed changes

BenTheElder previously requested changes Jun 15, 2019

View reviewed changes

pkg/cluster/internal/create/actions/config/config.go Show resolved Hide resolved

aojea added 5 commits June 16, 2019 18:02

Use kubeadm config files to create the cluster

db97575

Always get a control plane endpoint

84350fc

Remove getJoinAddress function

91154e1

Fix kubeadm v1alpha3 control plane

779f9e9

Use APIendpoint type correctly

81154d1

aojea force-pushed the kubeadmjoin branch from 0f3a6a4 to 81154d1 Compare June 16, 2019 16:04

aojea added 2 commits June 16, 2019 18:36

Fix templating errors

c4ba0a3

Add kubeadm v1alpha2 template join config

6a85e1c

BenTheElder changed the title ~~WIP Use config file to join nodes to the cluster~~ Use config file to join nodes to the cluster Jun 19, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 19, 2019

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 19, 2019

BenTheElder approved these changes Jun 19, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2019

BenTheElder reviewed Jun 19, 2019

View reviewed changes

BenTheElder mentioned this pull request Jun 19, 2019

Use config file to join nodes to the cluster [fixed] #632

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 19, 2019

BenTheElder closed this Jun 19, 2019

BenTheElder mentioned this pull request Jun 19, 2019

Add IPv6 support #348

Merged

aojea deleted the kubeadmjoin branch August 9, 2020 08:45

Conversation

aojea commented Jun 14, 2019 • edited by BenTheElder Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aojea commented Jun 14, 2019

Uh oh!

neolit123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neolit123 Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea commented Jun 14, 2019

Uh oh!

neolit123 commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neolit123 Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenTheElder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BenTheElder commented Jun 19, 2019

Uh oh!

BenTheElder commented Jun 19, 2019

Uh oh!

BenTheElder commented Jun 19, 2019

Uh oh!

k8s-ci-robot commented Jun 19, 2019

Uh oh!

BenTheElder Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenTheElder commented Jun 19, 2019

Uh oh!

BenTheElder commented Jun 19, 2019

Uh oh!

BenTheElder commented Jun 19, 2019

Uh oh!

Reviewers

aojea commented Jun 14, 2019 •

edited by BenTheElder

Loading

neolit123 Jun 14, 2019 •

edited

Loading

neolit123 commented Jun 14, 2019 •

edited

Loading

neolit123 Jun 14, 2019 •

edited

Loading

BenTheElder Jun 19, 2019 •

edited

Loading