Kubeadm graduate preflight phase #69666

fabriziopandini · 2018-10-11T10:17:02Z

What this PR does / why we need it:
This PR graduates phase preflight in the kubeadm init workflow.

As a result the command kubeadm aplha phase preflight is removed and instead the kubeadm init command is modified and now it prints the following help:

The "init" command executes the following internal workflow:

preflight  Run master pre-flight checks

nb (other phases in kubeadm init workflow are not yet graduated)

Additionally following sub commands are automatically added to kubeadm by the PhaseRunner:

kubeadm init phase 
kubeadm init phase preflight [flags]

Which issue(s) this PR fixes :
Rif kubernetes/kubeadm#1163

Special notes for your reviewer:
Depends on

kubeadm refactor cmd init #69622

/sig cluster-lifecycle
/kind documentation
/assign @neolit123
/cc @timothysc
@kubernetes/sig-cluster-lifecycle-pr-reviews
@chuckha
@rosti

Release note:

NONE

neolit123

thanks @fabriziopandini
added a couple of minor comments.

LGTM
(not adding /lgtm to avoid the bot auto-retesting)

neolit123 · 2018-10-11T12:16:58Z

cmd/kubeadm/app/cmd/phases/preflight.go

+		return nil
+	}
+
+	// TODO: consider if to split this into a nested phase preflight/pull


yes, we should probably consider something like that.

e.g.
...
preflight/validate
preflight/image-pull
...

I postponed this change after completing the implementation of the first list of phases agreed on kubernetes/kubeadm#1163

neolit123 · 2018-10-11T12:24:46Z

cmd/kubeadm/app/cmd/init.go

@@ -161,7 +162,7 @@ func NewCmdInit(out io.Writer) *cobra.Command {
 	options.bto.AddTTLFlag(cmd.PersistentFlags())

 	// initialize the workflow runner with the list of phases
-	// TODO: add the phases to the runner. e.g. initRunner.AppendPhase(phases.PreflightMaster)
+	initRunner.AppendPhase(phases.PreflightMaster)


neolit123 · 2018-10-11T12:27:46Z

cmd/kubeadm/app/cmd/phases/preflight.go

+}
+
+// PreflightMaster defines that kubeadm workflow phase that implements preflight checks for a new master node.
+var PreflightMaster = workflow.SimplePhase{


probably better to make such phase variables private and have a public getter instead.
GetPreflightMaster() { return preflightMaster }

otherwise any package can modify the PreflightMaster variable.
also we might want to prefix these with Phase - e.g.
PhasePreflightMaster ?

neolit123 · 2018-10-11T12:30:55Z

cmd/kubeadm/app/cmd/phases/preflight.go

@@ -70,47 +115,11 @@ func NewCmdPreFlight() *cobra.Command {
 	options.AddConfigFlag(cmd.PersistentFlags(), &cfgPath)
 	options.AddIgnorePreflightErrorsFlag(cmd.PersistentFlags(), &ignorePreflightErrors)

-	cmd.AddCommand(NewCmdPreFlightMaster(&cfgPath, &ignorePreflightErrors))
 	cmd.AddCommand(NewCmdPreFlightNode(&cfgPath, &ignorePreflightErrors))


i wonder, if we have plans to make the worker preflight phase-able too eventually...
might be wise to keep that in mind in terms of code organization, even if we never do it.

Not for v1.13 IMO

fabriziopandini · 2018-10-11T14:28:24Z

/hold
wating for dependencies to merge

rosti · 2018-10-11T14:33:08Z

cmd/kubeadm/app/cmd/phases/preflight.go

+			return err
+		}
+	} else {
+		fmt.Println("[preflight] Would pull the required images (like 'kubeadm config images pull')")


It would be useful to dump the images that were supposed to be pulled here.

I agree this is a useful suggestion but I think we should graduate phases as much as possible without changes because there is still a lot of work to do, and then eventually address possible improvements with separated PR.
This one, if you want to file an issue, could be a good issue for first-time contributors 😉

i think we already dump them with --v>=0
no?

I don't think so, but I am not 100% certain. We have to check on that.

Once this gets merged, I'll file an issue.

--v=1 seems to show them:

[preflight/images] Pulling images required for setting up a Kubernetes cluster [preflight/images] This might take a minute or two, depending on the speed of your internet connection [preflight/images] You can also perform this action in beforehand using 'kubeadm config images pull' I1017 18:33:05.090825 8365 checks.go:840] pulling k8s.gcr.io/kube-apiserver:v1.12.1 I1017 18:33:25.091417 8365 checks.go:840] pulling k8s.gcr.io/kube-controller-manager:v1.12.1 ...

probably fine to leave it like that, unless we want to show them with v=0 (aka fmt.Print).

That seems to be the case where there is no --dry-run. My idea is to have this with --dry-run.

timothysc · 2018-10-11T16:48:36Z

I'll loop back to this after we agree on #69684

fabriziopandini · 2018-10-17T08:38:45Z

/hold cancel
@timothysc ready for final review

rosti · 2018-10-17T10:53:10Z

/test pull-kubernetes-e2e-kops-aws

neolit123 · 2018-10-17T15:38:42Z

/lgtm

neolit123 · 2018-10-17T15:44:14Z

/remove-kind documentation
/kind feature

@fabriziopandini
something else to consider here is release-note planning.
might be best that we add release note to all the PRs that graduate commands.
we can then decide to consolidate then once the whole list is up.

otherwise we are going to end up with NONE for all of them.

fabriziopandini · 2018-10-17T18:01:18Z

@neolit123, since we are doing this via multiple PRs (and probably also a final iteration to finalize details) in my opinion release notes should be added only at the end.

timothysc

/lgtm
/approve

let's keep rolling but please change the usage of 'ctx'

timothysc · 2018-10-17T18:47:36Z

cmd/kubeadm/app/cmd/init.go

@@ -305,23 +307,20 @@ func newInitData(cmd *cobra.Command, options *initOptions) (initData, error) {
 	}, nil
 }

+func (ctx initData) Cfg() *kubeadmapi.InitConfiguration {


just name the var anything other than ctx.

fabriziopandini · 2018-10-18T10:49:00Z

@timothysc

I remove all the ctx
I have added all the initData methods that IMO are necessary for implementing remaining phases, so we can now go faster/in parallel without too many conflicts
If you prefer 2 moved in another PR let me know

neolit123 · 2018-10-18T10:49:52Z

i wish github allowed to show what changed since the last push. :
~~i assume you've added more dry-run handling?~~
LGTM.

fabriziopandini · 2018-10-18T13:02:57Z

@neolit123 +1
rif. lines 323-408 in init.go; basically only getters on InitData struct

neolit123 · 2018-10-19T20:40:03Z

/lgtm

@fabriziopandini @timothysc
you are both now approvers of docs/admin/OWNERS

so this PR should just merge if /approve is re-applied.

fabriziopandini · 2018-10-20T05:28:33Z

/approve

fabriziopandini · 2018-10-20T07:21:06Z

@neolit123 we are still blocked
I guess it is for docs/.generated_docs only 😣

neolit123 · 2018-10-20T09:08:05Z

@fabriziopandini

I guess it is for docs/.generated_docs only

i forgot about that file :...when new generated docs are added they need to be reflected in docs/.generated_docs too and this means that we would need approvers in /docs/OWNERS (/docs/admin is not sufficient)!

@spiffxp @timothysc @ixdy @tengqm

there is no point to generate these place holder docs on every commit.
the sig-docs team generates them only once before a release. if there are errors (which is unlikely) things can be addressed before the release.

this is a big UX blocker for contributors and there are a couple of solutions:

add more more apporvers in /docs/OWNERS (not ideal)
~~disable verify.generated-docs on presubmits (better)~~
don't generate and verify the placeholder files

i will send a PR for the later to see how that gets received.

k8s-ci-robot · 2018-10-20T11:49:00Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: fabriziopandini, 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:

~~cmd/kubeadm/OWNERS~~ [fabriziopandini,timothysc]
docs/OWNERS

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 added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 11, 2018

k8s-ci-robot assigned neolit123 Oct 11, 2018

k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. kind/documentation Categorizes issue or PR as related to documentation. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 11, 2018

k8s-ci-robot requested a review from timothysc October 11, 2018 10:17

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubeadm labels Oct 11, 2018

neolit123 reviewed Oct 11, 2018

View reviewed changes

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

rosti reviewed Oct 11, 2018

View reviewed changes

timothysc added the do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. label Oct 11, 2018

fabriziopandini force-pushed the kubeadm-graduate-preflight branch from d6a2a53 to afe4a74 Compare October 13, 2018 15:22

fabriziopandini force-pushed the kubeadm-graduate-preflight branch from afe4a74 to 8fe2e99 Compare October 17, 2018 08:33

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2018

fabriziopandini force-pushed the kubeadm-graduate-preflight branch from 8fe2e99 to e289ef2 Compare October 17, 2018 08:38

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 17, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2018

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/documentation Categorizes issue or PR as related to documentation. labels Oct 17, 2018

timothysc approved these changes Oct 17, 2018

View reviewed changes

k8s-ci-robot assigned timothysc Oct 17, 2018

fabriziopandini added 2 commits October 18, 2018 11:52

kubeadm graduate preflight phase

abac950

autogenerated

22da6a6

fabriziopandini force-pushed the kubeadm-graduate-preflight branch from e289ef2 to 22da6a6 Compare October 18, 2018 10:20

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2018

neolit123 mentioned this pull request Oct 18, 2018

tracking issue for kubeadm init phases kubernetes/kubeadm#1163

Closed

17 tasks

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2018

timothysc added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2018

k8s-ci-robot merged commit 0bf8a51 into kubernetes:master Oct 20, 2018

fabriziopandini deleted the kubeadm-graduate-preflight branch October 20, 2018 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubeadm graduate preflight phase #69666

Kubeadm graduate preflight phase #69666

fabriziopandini commented Oct 11, 2018 •

edited

neolit123 left a comment

neolit123 Oct 11, 2018

fabriziopandini Oct 13, 2018

neolit123 Oct 11, 2018

neolit123 Oct 11, 2018

neolit123 Oct 11, 2018

fabriziopandini Oct 13, 2018

fabriziopandini commented Oct 11, 2018

rosti Oct 11, 2018

fabriziopandini Oct 13, 2018

neolit123 Oct 17, 2018

rosti Oct 17, 2018

neolit123 Oct 17, 2018

rosti Oct 18, 2018

timothysc commented Oct 11, 2018

fabriziopandini commented Oct 17, 2018

rosti commented Oct 17, 2018

neolit123 commented Oct 17, 2018

neolit123 commented Oct 17, 2018

fabriziopandini commented Oct 17, 2018

timothysc left a comment

timothysc Oct 17, 2018

fabriziopandini commented Oct 18, 2018

neolit123 commented Oct 18, 2018 •

edited

fabriziopandini commented Oct 18, 2018

neolit123 commented Oct 19, 2018

fabriziopandini commented Oct 20, 2018

fabriziopandini commented Oct 20, 2018

neolit123 commented Oct 20, 2018 •

edited

k8s-ci-robot commented Oct 20, 2018

Kubeadm graduate preflight phase #69666

Kubeadm graduate preflight phase #69666

Conversation

fabriziopandini commented Oct 11, 2018 • edited

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc commented Oct 11, 2018

fabriziopandini commented Oct 17, 2018

rosti commented Oct 17, 2018

neolit123 commented Oct 17, 2018

neolit123 commented Oct 17, 2018

fabriziopandini commented Oct 17, 2018

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Oct 18, 2018

neolit123 commented Oct 18, 2018 • edited

fabriziopandini commented Oct 18, 2018

neolit123 commented Oct 19, 2018

fabriziopandini commented Oct 20, 2018

fabriziopandini commented Oct 20, 2018

neolit123 commented Oct 20, 2018 • edited

k8s-ci-robot commented Oct 20, 2018

fabriziopandini commented Oct 11, 2018 •

edited

neolit123 commented Oct 18, 2018 •

edited

neolit123 commented Oct 20, 2018 •

edited