Skip to content
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.

Sign up for GitHub

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

Jump to bottom

kubeadm: Demote --self-hosted to master config file. #41991

Merged
merged 1 commit into from Feb 26, 2017
Merged

kubeadm: Demote --self-hosted to master config file. #41991

merged 1 commit into from Feb 26, 2017

Conversation

dgoodwin
Copy link
Contributor

What this PR does / why we need it:

kubeadm init --self-hosted was meant to be a short lived hack to enable self-hosted deployments until we're ready to make them the default. Rather than shipping this in 1.6 (for the first time) we will move this to the config file as it is presently only an advanced feature, leaving us with more well supported ways to remove it in the future.

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:

CC @luxas @pires @errordeveloper @dmmcquay

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 23, 2017
@k8s-reviewable
Copy link

This change is Reviewable

dmmcquay
dmmcquay approved these changes Feb 23, 2017
View reviewed changes
Copy link
Contributor

@dmmcquay dmmcquay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a better idea. lgtm

luxas
luxas reviewed Feb 23, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but one nit needs fixing

cmd/kubeadm/app/cmd/init.go Outdated
// Temporary control plane is up, now we create our self hosted control
// plane components and remove the static manifests:
fmt.Println("[init] Creating self-hosted control plane...")
if err := kubemaster.CreateSelfHostedControlPlane(i.cfg, client); err != nil {
return err
}
} else {
fmt.Println("[init] ############ NOT SELF-HOSTED")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this, it would be very confusing for end users

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damnit sorry that was purely debug purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Feb 23, 2017
@luxas luxas assigned luxas and unassigned errordeveloper Feb 23, 2017
@luxas
Copy link
Member

luxas commented Feb 23, 2017

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2017
jbeda
jbeda approved these changes Feb 23, 2017
View reviewed changes
Copy link
Contributor

@jbeda jbeda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much cleaner!

cmd/kubeadm/app/apis/kubeadm/types.go Outdated
@@ -40,6 +40,7 @@ type MasterConfiguration struct {
KubernetesVersion string
CloudProvider string
AuthorizationMode string
SelfHosted bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should start documenting these types. Can you add a description here and mark this as alpha?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect nearly everything here will be completely rewritten in future releases -- in line with my phases proposal for example. Let's discuss the proposed, long-term goals there and then implement that for future releases

Copy link
Contributor Author

@dgoodwin dgoodwin Feb 24, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment, is there a way to mark this field specifically as alpha? (or just mention it in comment?)

@dmmcquay
Copy link
Contributor

@k8s-bot test this

seems like some tests failed to start

@luxas
Copy link
Member

luxas commented Feb 24, 2017

@k8s-bot test this

pires
pires approved these changes Feb 24, 2017
View reviewed changes
Copy link
Contributor

@pires pires left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're adding flags for other stuff like CA file path but self-hosted (alpha) can't be a flag?

Other than my doubts about the rationale behind this decision, the code lgtm.

@luxas
Copy link
Member

luxas commented Feb 24, 2017

@pires We're aiming to make the kubeadm init/join/reset UX beta in v1.6, which basically says we shouldn't change it much after that. The node CA path flag for join you're referring to is essential for the secure communication configuration we're aiming for in v1.6.

Further, given our plans to enable selfhosting by default, it's awkward to pass --self-hosting=false to kubeadm init to get a static pod-hosted master. We just need more time to think about this, hence this move for the v1.6 release, we'll revisit when that release is out of the door.

@luxas
Copy link
Member

luxas commented Feb 24, 2017

@k8s-bot bazel test this

@pires
Copy link
Contributor

pires commented Feb 24, 2017

The node CA path flag for join you're referring to is essential for the secure communication configuration we're aiming for in v1.6.

I'm not arguing this isn't important but it is part of the API, yes? So why a flag too? Or have you removed it by this time?

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: dgoodwin, luxas

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

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

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2017
@luxas luxas added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2017
@dgoodwin
Copy link
Contributor Author

@k8s-bot test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 41857, 41864, 40522, 41835, 41991)

@k8s-github-robot k8s-github-robot merged commit 3cc1429 into kubernetes:master Feb 26, 2017
@dgoodwin dgoodwin deleted the demote-self-hosted branch March 14, 2017 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luxas luxas luxas left review comments

@jbeda jbeda jbeda approved these changes

@dmmcquay dmmcquay dmmcquay approved these changes

@pires pires pires approved these changes

Assignees

@luxas luxas

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@dgoodwin @k8s-reviewable @luxas @dmmcquay @pires @k8s-github-robot @jbeda @errordeveloper @k8s-ci-robot