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: Allows to specify custom flag values for control plane components #58080

Merged
merged 1 commit into from Jan 20, 2018

Conversation

@simonferquel
Contributor

simonferquel commented Jan 10, 2018

This makes it possible to override / add flag values to the k8s api server, controller manager and scheduler components on kubeadm init and kubeadm alpha controlplane <component>

What this PR does / why we need it:
This PR makes kubeadm a little more flexible by allowing to specify flag values (or override kubeadm defaults) for the control plane components.
One good example is to deploy Kubernetes with a different admission-control flag on API server

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

Special notes for your reviewer:
Not sure about what should be fixed. The PR merely adds flags to the CLI exposing existing functionality (which I suppose is already tested)

Release note:

kubeadm now accept `--apiserver-extra-args`, `--controller-manager-extra-args` and `--scheduler-extra-args` to override / specify additional flags for control plane components
@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 10, 2018

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@simonferquel

This comment has been minimized.

Contributor

simonferquel commented Jan 10, 2018

CLA signed

@dims

This comment has been minimized.

Member

dims commented Jan 10, 2018

/ok-to-test

the change looks good to me! 👍

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 10, 2018

/retest

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 10, 2018

The build errors look a little odd, also the change looks simple enough and I don't see how it could relate to those build errors...

@simonferquel

This comment has been minimized.

Contributor

simonferquel commented Jan 10, 2018

@errordeveloper I have added an import in init and controlplane subcommands. I think it did not get automatically added to the bazel BUILD file in cmd/kubeadm/app/phase. I submitted a corrective commit. I'll squash them if it succeeds

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 10, 2018

I think it did not get automatically added to the bazel BUILD file in cmd/kubeadm/app/phase

Ah, that's right! I'm still new to bazel.

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 10, 2018

@simonferquel have you tried creating a config file actually? I sort of recollect that these flags were considered too low-level to be exposed in kubeadm init, hence could only be accessed via the config file...

@dims

This comment has been minimized.

Member

dims commented Jan 10, 2018

@simonferquel : if you run hack/update-bazel.sh it fixes up the bazel build files.

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 10, 2018

I sort of recollect that these flags were considered too low-level to be exposed in kubeadm init...

But I don't know if there was a discussion about exposing these in the controlplane phase subcommand, which seems like a good idea to me.

@simonferquel

This comment has been minimized.

Contributor

simonferquel commented Jan 10, 2018

I am aware that the config file exposes the feature, but I thought it would be far easier if this was exposed trough init (would make our CI scripts simpler for spawning clusters with custom admission control)

@simonferquel

This comment has been minimized.

Contributor

simonferquel commented Jan 10, 2018

@errordeveloper maybe we could define a set of flags on each control plane component that are frequently customized, and surface them instead of surfacing the whole extra-args settings. WDYT?

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 10, 2018

@simonferquel we are gonna have a kubeadm office hours meeting today, feel free to add this to the agenda :)

https://groups.google.com/d/msg/kubernetes-sig-cluster-lifecycle/Gh4HK-Z_oSQ/lF3dgusUBwAJ

@dixudx

A small nit. Others lgtm.

}
if properties.use == "all" || properties.use == "controller-manager" {
cmd.Flags().StringVar(&cfg.Networking.PodSubnet, "pod-network-cidr", cfg.Networking.PodSubnet, "The range of IP addresses used for the Pod network")
cmd.Flags().Var(utilflag.NewMapStringString(&cfg.ControllerManagerExtraArgs), "controller-manager-extra-args", "A set of extra flags to pass to the Controller Manager or override default ones in form of <flagname>=<value>")
}

This comment has been minimized.

@dixudx

dixudx Jan 10, 2018

Member

Add a newline here?

@simonferquel

This comment has been minimized.

Contributor

simonferquel commented Jan 10, 2018

Fixed @dixudx nit + squashed

@dixudx

This comment has been minimized.

Member

dixudx commented Jan 10, 2018

/lgtm

ping @luxas for approval.

@simonferquel

This comment has been minimized.

Contributor

simonferquel commented Jan 10, 2018

/test pull-kubernetes-unit

@@ -192,6 +193,10 @@ func AddInitConfigFlags(flagSet *flag.FlagSet, cfg *kubeadmapiext.MasterConfigur
)
flagSet.StringVar(featureGatesString, "feature-gates", *featureGatesString, "A set of key=value pairs that describe feature gates for various features. "+
"Options are:\n"+strings.Join(features.KnownFeatures(&features.InitFeatureGates), "\n"))
flagSet.Var(utilflag.NewMapStringString(&cfg.APIServerExtraArgs), "apiserver-extra-args", "A set of extra flags to pass to the API Server or override default ones in form of <flagname>=<value>")

This comment has been minimized.

@timothysc

timothysc Jan 12, 2018

Member

Do we need this now, could we push for component config in order to resolve this in the long-term?

This comment has been minimized.

@errordeveloper

errordeveloper Jan 15, 2018

Member

I also realised that this would be the best fit for component config, but in the mean time we could add this to phases command.

@timothysc

@dims Would this work for you for the time being?

@fejta-bot

This comment has been minimized.

fejta-bot commented Jan 19, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

2 similar comments
@fejta-bot

This comment has been minimized.

fejta-bot commented Jan 19, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

fejta-bot commented Jan 19, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

kubeadm: Allows to specify custom flag values for control plane compo…
…nents

This makes it possible to override / add flag values to the k8s api server, controller manager and scheduler components on `kubeadm init` and `kubeadm alpha controlplane <component>`

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
@simonferquel

This comment has been minimized.

Contributor

simonferquel commented Jan 19, 2018

Just rebased, hoping it will fix flaky tests

@k8s-merge-robot k8s-merge-robot removed the lgtm label Jan 19, 2018

@simonferquel

This comment has been minimized.

Contributor

simonferquel commented Jan 19, 2018

@dixudx can you re-lgtm
Otherwise, if it gets merged eventually, should I create a cherry-picking PR to sub;it it for an upcoming 1.9 revision, or is there something automagic for that ?

@timothysc

This comment has been minimized.

Member

timothysc commented Jan 19, 2018

/lgtm

This is not a PR that should be cherry-picked back to 1.9 according to policy.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 19, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, simonferquel, timothysc

Associated issue: #58072

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

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 19, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 20, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 7fb295e into kubernetes:master Jan 20, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation simonferquel authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke-gci Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@luxas

I'm happy with this if others are, but IIRC we had frozen the UX of kubeadm init to not clutter things up with too many options...

I see this would be convenient and useful for users though so I'm torn.

We need a longer-term plan what to do with flags for components project-wide, and see how/if this PR fits into that model.

Thoughts?

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 20, 2018

@simonferquel

This comment has been minimized.

Contributor

simonferquel commented Jan 20, 2018

I think this 3 flags (especially the 1 for apiserver) cover such a wide spectrum of customization (admission control, authentication customization, and runtime config, etc.) that the added value counter-balance the added complexity by large (and brings a much simpler ux than having to write a config file).

Actually I wrote this pr out of frustration as a day to day user of kubeadm :).

@luxas

This comment has been minimized.

Member

luxas commented Jan 21, 2018

@simonferquel I totally get you. Please line it up for the next SIG meeting and discuss it there, and LMK what you all decide. I'm not gonna be able to attend sadly, but anyway...

@timothysc

This comment has been minimized.

Member

timothysc commented Jan 22, 2018

IMO these are temporary, long-term it's all configmaps.

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 22, 2018

@timothysc

This comment has been minimized.

Member

timothysc commented Jan 23, 2018

Per sig discussion, we should either move to phases or feature gated flag.

@luxas

This comment has been minimized.

Member

luxas commented Jan 24, 2018

Thanks @timothysc, please open an issue to track that

@kris-nova

This comment has been minimized.

Member

kris-nova commented Jan 30, 2018

I am going to be working on kubernetes/kubeadm#676 and will follow up here as soon as I wrap my head around it 😄

kris-nova added a commit to kris-nova/kubernetes that referenced this pull request Feb 14, 2018

kubeadm: Demote controlplane passthrough flags to phases alpha
After a discussion in sig cluster lifecycle we agreed that the passthrough flags should live in phases alpha, and not be 1st class flags.
Relates to kubernetes/pull/58080
Closes kubernetes/kubeadm/issues/676

k8s-merge-robot added a commit that referenced this pull request Feb 25, 2018

Merge pull request #59882 from kris-nova/kubeadm-demote-controlplane-…
…passthrough-flags-to-phases-alpha

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kubeadm: Demote controlplane passthrough flags to phases alpha

After a discussion in sig cluster lifecycle we agreed that the passthrough flags should live in phases alpha, and not be 1st class flags. They already exist in the alpha command, so just removing from here.



**What this PR does / why we need it**:

We introduced some flags as 1st class flags in #58080 and decided as a sig that the flags should only live in the `alpha` command. This PR removes the flags from the `init` command so they only exist in the `alpha` command

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

relates to /pull/58080
fixes kubernetes/kubeadm/issues/676

**Special notes for your reviewer**:

This is a cosmetic change, and doesn't alter any functionality of the program, only the avenue in which a user access functionality in the program.

**Release note**:

```release-note
kubeadm: Demote controlplane passthrough flags to alpha flags
```

jingxu97 added a commit to jingxu97/kubernetes that referenced this pull request Mar 13, 2018

kubeadm: Demote controlplane passthrough flags to phases alpha
After a discussion in sig cluster lifecycle we agreed that the passthrough flags should live in phases alpha, and not be 1st class flags.
Relates to kubernetes/pull/58080
Closes kubernetes/kubeadm/issues/676
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment