kubeadm: Adds dry-run support for kubeadm using the `--dry-run` option #50631

luxas · 2017-08-14T18:49:27Z

What this PR does / why we need it:

Adds dry-run support to kubeadm by creating a fake clientset that can get totally fake values (like in the init case), or delegate GETs/LISTs to a real API server but discard all edits like POST/PUT/PATCH/DELETE

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

fixes: kubernetes/kubeadm#389

Special notes for your reviewer:

This PR depends on #50626, first three commits are from there
This PR is a dependency for #48899 (kubeadm upgrades)

I have some small things to fixup and I'll yet write unit tests, but PTAL if you think this is going in the right direction

Release note:

kubeadm: Adds dry-run support for kubeadm using the `--dry-run` option

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

mattmoyer

This will be great to have! It's nice for init and token, and for debugging during development, but it will be especially nice for upgrade.

My only general comment is it needs unit tests. It would also be good to get a review from someone more familiar with the API machinery/client-go code.

mattmoyer · 2017-08-14T21:22:22Z

cmd/kubeadm/app/cmd/init.go

@@ -248,15 +258,11 @@ func (i *Init) Run(out io.Writer) error {
 		return err
 	}

-	client, err := kubeconfigutil.ClientSetFromFile(kubeadmconstants.GetAdminKubeConfigPath())
+	client, err := createClientsetAndOptionallyWaitForReady(i.cfg, i.dryRun)


I think it might be clearer to put if dryRun { here and rename createClientsetAndOptionallyWaitForReady() to just createClientsetAndWaitForReady().

mattmoyer · 2017-08-14T21:23:18Z

cmd/kubeadm/app/cmd/init.go

+
+	fmt.Printf("[init] Waiting for the kubelet to boot up the control plane as Static Pods from directory %q\n", kubeadmconstants.GetStaticPodDirectory())
+	// TODO: Adjust this timeout or start polling the kubelet API
+	if err := apiclient.WaitForAPI(client, 30*time.Minute); err != nil {


30*time.Minute should probably be pulled out into a constant.

added a TODO; not for this PR

mattmoyer · 2017-08-14T21:36:46Z

cmd/kubeadm/app/util/apiclient/wait.go

 	start := time.Now()
-	wait.PollInfinite(kubeadmconstants.APICallRetryInterval, func() (bool, error) {


Nice, does this take some latency out of init as well?

If so it might be worth cherry picking a simpler fix (just s/PollInfinite/PollImmediateInfinite/).

I don't think so; as it often takes ~15 secs for control plane to come up
Whether we start polling immediately or after 0.5secs doesn't seem to matter.

But this is cleaner :)

mattmoyer · 2017-08-14T21:37:15Z

staging/src/k8s.io/client-go/testing/actions.go

@@ -319,6 +319,18 @@ type DeleteAction interface {
 	GetName() string
 }

+type DeleteCollectionAction interface {


I don't see DeleteCollectionAction used elsewhere. Unused/WIP code?

No, more for consistency.
That generic iface and PatchAction were missing somehow...

mattmoyer · 2017-08-14T21:39:19Z

cmd/kubeadm/app/util/apiclient/wait.go

 			return false, nil
 		}
-		for _, pod := range apiPods.Items {
-			fmt.Printf("[apiclient] Pod %s status: %s\n", pod.Name, pod.Status.Phase)


This Printf seems useful. Is it just too noisy?

this was in the last PR; it was way too noisy; spitting things out twice every second
Instead this now prints on changed state

But in the last PR, not this one

mattmoyer · 2017-08-14T22:02:47Z

cmd/kubeadm/app/util/apiclient/init_dryrun.go

+		return true, nil, fmt.Errorf("unable to get first IP address from the given CIDR (%s): %v", svcSubnet.String(), err)
+	}
+
+	// We can safely return a NotFound error here as the code will just proceed normally and don't care about modifying this clusterrolebinding


The comment here is wrong .

mattmoyer · 2017-08-14T22:02:56Z

cmd/kubeadm/app/util/apiclient/init_dryrun.go

+		// We can't handle this event
+		return false, nil, nil
+	}
+	// We can safely return a NotFound error here as the code will just proceed normally and don't care about modifying this clusterrolebinding


This comment is wrong as well.

mattmoyer · 2017-08-14T22:04:03Z

cmd/kubeadm/app/util/apiclient/init_dryrun.go

+				{
+					Name:       "https",
+					Port:       443,
+					TargetPort: intstr.FromInt(6443),


This target port might vary per cluster, right?

this doesn't matter; only important thing here is the ClusterIP

mattmoyer · 2017-08-14T22:05:22Z

cmd/kubeadm/app/util/apiclient/init_dryrun.go

+
+// handleGetNode
+func (idr *InitDryRunGetter) handleGetNode(action core.GetAction) (bool, runtime.Object, error) {
+	if action.GetResource().Resource != "nodes" {


Should we also check against idr.masterName here?

Good idea, now done

mattmoyer · 2017-08-14T22:06:50Z

cmd/kubeadm/app/util/apiclient/init_dryrun.go

+		// We can't handle this event
+		return false, nil, nil
+	}
+	// We can safely return a NotFound error here as the code will just proceed normally and don't care about modifying this clusterrolebinding


This comment says "clusterrolebinding" but this is the bootstrap token.

timothysc · 2017-08-15T13:32:44Z

poke me when the dependent PRs are merged.

luxas · 2017-08-15T13:34:40Z

@timothysc ack, I'll do that.

luxas · 2017-08-15T19:52:31Z

@timothysc @mattmoyer Rebased and fixed up a small things. Should now work very well.

My only general comment is it needs unit tests.

I stated above that "I'll yet write unit tests" 😄
If you have suggestions about what unit tests I should create, please comment on the PR

cc @sttts @ncdc You are familiar with the API machinery, could you take a look and check if this is sane usage of it?

timothysc

@ncdc is there a de-facto client-set stub for validating client behavior that you are aware of?

timothysc · 2017-08-16T00:20:38Z

cmd/kubeadm/app/cmd/init.go

+func createClientsetAndOptionallyWaitForReady(cfg *kubeadmapi.MasterConfiguration, dryRun bool) (clientset.Interface, error) {
+	if dryRun {
+		// If we're dry-running; we should create a faked client that answers some GETs in order to be able to do the full init flow and just logs the rest of requests
+		dryRunGetter := apiclient.NewInitDryRunGetter(cfg.NodeName, cfg.Networking.ServiceSubnet)


So there are a number of different clientset stubs, I'm wondering if we can dedupe.

timothysc · 2017-08-16T00:32:48Z

cmd/kubeadm/app/phases/selfhosting/selfhosting.go

@@ -33,6 +33,11 @@ import (
 	"k8s.io/kubernetes/pkg/api"
 )

+const (
+	// selfHostingWaitTimeout describes the maximum amount of time a self-hosting wait process should wait before timing out
+	selfHostingWaitTimeout = 2 * time.Minute


This seems orthogonal to dry-run

ncdc · 2017-08-16T13:25:08Z

@timothysc could you expand on the exact use case here please?

…ix_race Automatic merge from submit-queue kubeadm: Fix self-hosting race condition **What this PR does / why we need it**: Splitted out from: kubernetes#50766 Waits for the Static Pod to be deleted before proceeding with checking the API health. Otherwise there is a race condition where we're checking the health on the static pod API server; not the self-hosted one that we expect. Also improves the logging output and adds reasonable timeouts for the process **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: Dependency for - kubernetes#50766 - kubernetes#50631 - kubernetes#48899 **Release note**: ```release-note NONE ``` @kubernetes/sig-cluster-lifecycle-pr-reviews

timothysc · 2017-08-17T19:52:26Z

@ncdc This use case is for a set of stub'd out interactions to the main api. You basically want a hook mechanism to route some calls to another function.

mml · 2017-08-17T20:47:45Z

cc @caesarxuchao

mml · 2017-08-17T20:48:22Z

/assign @caesarxuchao

luxas · 2017-08-17T23:40:34Z

@timothysc @mattmoyer @caesarxuchao @ncdc @sttts This is now rebased. Please take a look and review this. Now unit tests also are in place.

caesarxuchao · 2017-08-18T00:29:15Z

/approve

the client-go changes lgtm.

mattmoyer · 2017-08-18T20:03:00Z

/lgtm

timothysc

/lgtm

TBH - I don't want to block this, but I'm not a really a fan of much of what is done in here. The dry-run behavior stubbing is kind of weird.

Given that it's stub code and not critical path, I'm ok with it. But it looks like you may be missing some UTs in this patch.

For now I'll unblock but could you open a follow on issue to address cleanup & re-eval.

timothysc · 2017-08-18T19:48:46Z

cmd/kubeadm/app/util/apiclient/clientbacked_dryrun.go

+var _ DryRunGetter = &ClientBackedDryRunGetter{}
+
+// NewClientBackedDryRunGetter creates a new ClientBackedDryRunGetter instance based on the rest.Config object
+func NewClientBackedDryRunGetter(config *rest.Config) *ClientBackedDryRunGetter {


So the naming here is weird.
clientbacked_dryrun.NewClientBackedDryRunGetter() ?

timothysc · 2017-08-18T19:51:02Z

cmd/kubeadm/app/util/apiclient/clientbacked_dryrun.go

+}
+
+// NewClientBackedDryRunGetterFromKubeconfig creates a new ClientBackedDryRunGetter instance from the given KubeConfig file
+func NewClientBackedDryRunGetterFromKubeconfig(file string) (*ClientBackedDryRunGetter, error) {


k8s-github-robot · 2017-08-18T20:05:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, luxas, mattmoyer, timothysc

Associated issue: 389

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]
~~staging/src/k8s.io/client-go/OWNERS~~ [caesarxuchao]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

luxas · 2017-08-19T06:05:18Z

/retest

…

On 19 Aug 2017, at 06:35, k8s-ci-robot ***@***.***> wrote: @luxas: The following test failed, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-gce-etcd3 0bf84aa link /test pull-kubernetes-e2e-gce-etcd3 Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

fejta-bot · 2017-08-19T08:41:28Z