kubeadm: support kubeadm join --dry-run #103027

Haleygo · 2021-06-20T18:12:36Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

add --dry-run flag for "kubeadm join", run "kubeadm join --dry-run" to see what would be done

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#2505

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: add support for dry running "kubeadm join". The new flag "kubeadm join --dry-run" is similar to the existing flag for "kubeadm init/upgrade" and allows you to see what changes would be applied.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-06-20T18:12:44Z

Hi @Haleygo. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /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 by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

pacoxu · 2021-06-21T09:03:21Z

/ok-to-test
thanks for taking this.

Haleygo · 2021-06-21T09:46:14Z

/retest

cmd/kubeadm/app/cmd/join.go

cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

neolit123 · 2021-06-21T16:23:39Z

cmd/kubeadm/app/cmd/phases/join/controlplaneprepare.go

+	// If we're dry-running, set CertificatesDir to default value to get the right cert path in static pod yaml
+	if data.DryRun() {
+		cfg.CertificatesDir = filepath.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.DefaultCertificateDir)
+	}


but during dryrun the directory where the certs were written is now data.CertificateWriteDir().
are you getting errors if this is not added here?

[1]

It won't get errors here, but if I don't change it, it will write tmp cert dir path to static pod yaml instead of "/etc/kubernetes/pki/".
And If I remake CreateStaticPodFiles() to accept path directly, GetStaticPodSpecs need to do the same and other places that use them.
any suggestion? @neolit123

making these functions accept a directory seems like a good idea for dry-run / testing?
...that likely is going to make this change a lot bigger though.

cmd/kubeadm/app/cmd/phases/join/controlplaneprepare.go

neolit123 · 2021-06-21T16:24:25Z

cmd/kubeadm/app/cmd/phases/join/controlplaneprepare.go

+	// If we're dry-running, download certs to tmp dir
+	if data.DryRun() {
+		cfg.CertificatesDir = data.CertificateWriteDir()
+	}


related question to [1]

Actually, it's very tricky to set CertificatesDir during the whole dry-run process.
First, I need to download certs to the right tmp dir but I can't pass data.CertificateWriteDir() to copycerts.DownloadCerts and certsphase.CreatePKIAssets directly. And remake the args of those two func will be a big work.
Then, when it comes to CreateStaticPodFiles(), CertificatesDir should be set as "/etc/kubernetes/pki" cause it will be written into static pod yaml and printed later, so I change it again

cmd/kubeadm/app/cmd/phases/join/kubelet.go

neolit123 · 2021-06-29T14:25:34Z

thank you very much for the efforts @Haleygo
this has become a big change and i need to find time to review it again.

one problem is code freeze (for 1.22) is next week (July 8th) and we need to make sure that if we are going to merge this it does not make regressions...so that is a bit of a concern.

in the meantime, more reviews from others are appreciated.

Haleygo · 2021-06-29T14:44:49Z

thank you very much for the efforts @Haleygo
this has become a big change and i need to find time to review it again.

one problem is code freeze (for 1.22) is next week (July 8th) and we need to make sure that if we are going to merge this it does not make regressions...so that is a bit of a concern.

in the meantime, more reviews from others are appreciated.

of course, in the meantime, I will do more test on this.

neolit123 · 2021-07-22T20:40:59Z

i will get back to reviewing this PR after code freeze for 1.22.

neolit123 · 2021-08-09T12:30:11Z

would you have time to work on this again @Haleygo ?
PR needs rebase. looks mostly good to me, but i need to have another review pass.

Haleygo · 2021-08-09T15:27:00Z

would you have time to work on this again @Haleygo ?
PR needs rebase. looks mostly good to me, but i need to have another review pass.

@neolit123 Sure thing, done.

cmd/kubeadm/app/phases/etcd/local.go

neolit123 · 2021-08-09T16:34:56Z

^ found one potential problem.

about these comments:
#103027 (comment)
#103027 (comment)

i think it's still not ideal that we are mutating a cfg field during dry-run, but this PR is already quite big and i think it can be fixed separately.

Haleygo · 2021-08-10T13:47:04Z

^ found one potential problem.

about these comments:
#103027 (comment)
#103027 (comment)

i think it's still not ideal that we are mutating a cfg field during dry-run, but this PR is already quite big and i think it can be fixed separately.

+1, maybe create another task? and I would like to help with that.
so should I add some todo comment on it now?

neolit123 · 2021-08-10T15:08:14Z

+1, maybe create another task? and I would like to help with that.
so should I add some todo comment on it now?

sure, you can open an issue with some details in kubernetes/kubeadm and assign yourself.

cmd/kubeadm/app/phases/etcd/local.go

neolit123 · 2021-08-10T17:26:53Z

/lgtm
/approve

thanks, let's see if we get some failures after the change.
our e2e tests should be able to catch them.

neolit123 · 2021-08-10T17:26:58Z

/retest

k8s-ci-robot · 2021-08-10T17:27:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Haleygo, neolit123

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:

~~cmd/kubeadm/OWNERS~~ [neolit123]

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

k8s-ci-robot requested review from RA489 and SataQiu June 20, 2021 18:13

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 20, 2021

Haleygo force-pushed the feature/add-kubeadm-join-dryrun branch from 6eea44b to 2d87eb7 Compare June 21, 2021 03:38

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 21, 2021

Haleygo force-pushed the feature/add-kubeadm-join-dryrun branch from 2d87eb7 to fe349f6 Compare June 21, 2021 09:45