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

Kubeadm graduate preflight phase #69666

Open
wants to merge 3 commits into
base: master
from

Conversation

@fabriziopandini
Contributor

fabriziopandini commented Oct 11, 2018

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

Please consider only the last two commits:

  • kubeadm-graduate-preflight-phase
  • autogenerated

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

Release note:

NONE
@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Oct 11, 2018

Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fabriziopandini
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: brendandburns

If they are not already assigned, you can assign the PR to them by writing /assign @brendandburns in a comment when ready.

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:

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

Contributor

k8s-ci-robot commented Oct 11, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fabriziopandini
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: brendandburns

If they are not already assigned, you can assign the PR to them by writing /assign @brendandburns in a comment when ready.

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:

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

@neolit123

thanks @fabriziopandini
added a couple of minor comments.

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

Show outdated Hide outdated cmd/kubeadm/app/cmd/phases/preflight.go
Show outdated Hide outdated cmd/kubeadm/app/cmd/init.go
Show outdated Hide outdated 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))

This comment has been minimized.

@neolit123

neolit123 Oct 11, 2018

Member

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.

@neolit123

neolit123 Oct 11, 2018

Member

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.

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 13, 2018

Contributor

Not for v1.13 IMO

@fabriziopandini

fabriziopandini Oct 13, 2018

Contributor

Not for v1.13 IMO

@fabriziopandini

This comment has been minimized.

Show comment
Hide comment
@fabriziopandini

fabriziopandini Oct 11, 2018

Contributor

/hold
wating for dependencies to merge

Contributor

fabriziopandini commented Oct 11, 2018

/hold
wating for dependencies to merge

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

This comment has been minimized.

@rosti

rosti Oct 11, 2018

Contributor

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

@rosti

rosti Oct 11, 2018

Contributor

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

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 13, 2018

Contributor

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 😉

@fabriziopandini

fabriziopandini Oct 13, 2018

Contributor

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 😉

@timothysc

This comment has been minimized.

Show comment
Hide comment
@timothysc

timothysc Oct 11, 2018

Member

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

Member

timothysc commented Oct 11, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment