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.

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

Add proposal for kubeadm feature flags #35689

Closed
wants to merge 1 commit into from
Closed

Add proposal for kubeadm feature flags #35689

wants to merge 1 commit into from

Conversation

bogdando
Copy link

@bogdando bogdando commented Oct 27, 2016

What this PR does / why we need it: See proposal in the gdoc https://goo.gl/K90i3N (duplicated here as well)

Release note:

Related #35133
This one must be amended to the original proposal, kubernetes/enhancements#11,
once its design accepted.

Signed-off-by: Bogdan Dobrelya [email protected]


This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot 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 will still work. Regular contributors should join the org to skip this step.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 27, 2016
@errordeveloper
Copy link
Member

cc @kubernetes/sig-cluster-lifecycle

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 27, 2016
@luxas
Copy link
Member

luxas commented Oct 27, 2016

@kubernetes/sig-cluster-lifecycle Any others than me that thinks that this could just be a gdoc instead of a proposal in the main repo?

We already have the original kubeadm proposal, and
a) this PR should modify that proposal
b) this should be just a gdoc
c) we can discuss this design in an issue

WDYT?
This doesn't seem big enough for a full proposal IMO, but on the other hand, I don't know if we have any formalized process for what should and shouldn't be written as a proposal.

Copy link
Contributor

@brendandburns brendandburns left a comment

Choose a reason for hiding this comment

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

I think that this lacking some stuff as is (and I think I second amending the existing proposal, rather than creating an alternate one)

I think that I'd like to see a crisp proposal for what is to be added. There is a general overview of the goals, and some concrete user examples, but there is no specific specification of the complete new commands, syntax, etc. I'd like to see more detail so that we actually know what is being proposed, rather than just the problem statement.

Thanks!


## Motivation

Kubernetes is hard to install. Kubeadm and numerous external installers solve that issue well. None of them are excellent, although together they might be a perfect team. We believe that flexibility that feature flags may bring to the former make this possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit kubeadm here and elsewhere.


## Goals

Have a set of cluster bootstrapping tasks the kubeadm does as a list of configurable optional steps.
Copy link
Contributor

Choose a reason for hiding this comment

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

kubeadm


## Goals

Have a set of cluster bootstrapping tasks the kubeadm does as a list of configurable optional steps.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is awkward, rephrase?

@bogdando
Copy link
Author

bogdando commented Oct 28, 2016

@luxas how could I amend the PR to modify the original kubeadm proposal? In gerrit it is as simple as uploading a new revision, but I have no idea how to do the same with open github PRs :(

@luxas
Copy link
Member

luxas commented Oct 28, 2016

@bogdando What about a google doc first? I think that would be best.
That's how we started the other sig-cluster-lifecycle proposals as well, and it's best if everyone can easily edit that's in the group initially.

If you want to update your pr, just push or force-push to your branch

@bogdando
Copy link
Author

bogdando commented Oct 28, 2016

@luxas Sorry for confusing words, I meant the original PR:

We already have the original kubeadm proposal, and
a) this PR should modify that proposal

how to update it with this change?
As for google doc, I think the reviewable extension gives a lots more than gdocs for a generic review process. See https://reviewable.kubernetes.io/reviews/kubernetes/kubernetes/35689 . I found that more convinient than sharing some google docs to some ppl (which ones?) in my private space, or Mirantis gdoc space? That becomes too much complicated, sorry.

@bogdando
Copy link
Author

@luxas here is the gdoc https://goo.gl/K90i3N

@luxas
Copy link
Member

luxas commented Oct 28, 2016

I think it should be possible to restrict the editing to folks in the [email protected] google group

@lukemarsden knows how I guess

@luxas
Copy link
Member

luxas commented Oct 31, 2016

I agree with Brendan, this isn't really a proposal and shouldn't be a doc on its own.
Right now we only have the problem statement, which is documented in a lot of other places and a lot of flags that can be appended to init and join (which can't easily be implemented either)

Please, can you just file an issue in kubernetes/kubernetes where you're proposing what you'd like to see or make a gdoc like everyone else does?

This is also clearly an unmaintainable design which we can't/won't implement IMO.
We're going towards a config-file-based configuration approach, so flags to init and join will be kept minimal
And just think about what if you like to run kubeadm init --certificates and specify an argument to the certificates process? We would end up with 20s of flags that conflict with each other and the opinionated setup process is gone.

Instead we should make a new, advanced command called kubeadm manual or something like that and subcommands that does the specific tasks kubeadm should do. This will require quite a lot of refactoring work indeed, but it's clean and worth it.

We should identify later which phases there are in the kubeadm setup, and propose a comprehensive kubeadm manual design, but not in this proposal document/PR.

@bogdando
Copy link
Author

bogdando commented Oct 31, 2016

@luxas runtime options allow to opt-in required features only and invoke kubeadm multiple times just simple as changing opt-in flags one want to call (wrapped in docker run perhaps) and make it stupid simple. While manipulating with config file is tedious and bad design, specifically for feature flags. I agree on proposed steps and kubeadm manual and refactoring. But the primary task is removing a blocker for external CM tools integration, which is inability to invoke kubeadm for a specific action only. Once that blocker removed, we shall proceed to further design.

This is about KISS implementation for the blocker removal, with minimum of required changes and nothing more complex.

@bogdando
Copy link
Author

bogdando commented Oct 31, 2016

@luxas : @brendandburns

I agree with Brendan, this isn't really a proposal and shouldn't be a doc on its own.

Folks, let's followup please. I understood the Brendan's comment as a request for more details on implementation, thus this directly conflicts to the cited statement. And at least this must be amended to the original spec kubernetes/enhancements#11, before closed.

@brendandburns
Copy link
Contributor

@bogdando the intent of my comment was that before the proposal can be considered, there has to be sufficient detail to understand what it is proposing before we can even evaluate the proposal.

I think I'm with @luxas here that I think the right way forward is to add more detail to the existing kubernetes/enhancements#11, and then re-factor this proposal to describe how it solves those issues.

Once that's done, I think that we'll be in good shape to understand if the proposal should be accepted or not.

Thanks!
(and sorry, I realize this is kind of bureaucratic, but I think for things like this where there can really only be one kubeadm and much of the community will (hopefully) use it, we need to be crisp and get a clear understanding of what we are building before we commit to building it)

@bgrant0607
Copy link
Member

@bogdando Could you please describe scenarios for how you imagine these options to be used? Do you need to perform other operations in between the steps? Retry steps? Reorder steps?

Small numbers of flags and seem targeted at manual, interactive usage. Large numbers of flags aren't really easy to deal with for any use case. That approach basically requires a configuration management tool to translate from options specified some other way. In general, we find that configuration is more composable and therefore more amenable to higher-level automation.

Subcommands to invoke individual steps that read a subset of the configuration could potentially work, but this may expose too many implementation details. I assume it would be a problem if steps were added or removed.

@bogdando
Copy link
Author

bogdando commented Nov 2, 2016

@bgrant0607 I described example workflow and use cases and goals in the gdoc https://goo.gl/K90i3N

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@apelisse apelisse removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 22, 2016
This one mustbe amended to the original proposal,
kubernetes/enhancements#11,
once its design accepted.

Signed-off-by: Bogdan Dobrelya <[email protected]>
@bogdando
Copy link
Author

Updated the spec and gdoc with that @jbeda proposes in #35907 (comment).
@luxas what should be next steps to get that merged into the main kubeadm spec and changes to the spec being maintained there since then?

@k8s-github-robot k8s-github-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 22, 2016
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 22, 2016
@k8s-github-robot
Copy link

Adding label:do-not-merge because PR changes docs prohibited to auto merge
See http://kubernetes.io/editdocs/ for information about editing docs

@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. kind/old-docs do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Dec 1, 2016
@luxas
Copy link
Member

luxas commented Dec 1, 2016

Proposals should now target the community repo, so closing this one.

Also, here's my GDOC that includes the refactoring of the binary which will lead to separately invokable commands (which will solve this issue when done): https://docs.google.com/document/d/1v4k7eN-eetHbbpTAG7WuioRAMsK_FVGi1yC3VIn4Inw

@luxas luxas closed this Dec 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. kind/design Categorizes issue or PR as related to design. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants