kubeadm: Upload configuration used at 'kubeadm init' time to ConfigMap for easier upgrades #50320

luxas · 2017-08-08T14:37:51Z

What this PR does / why we need it:

Uploads config used to a ConfigMap so we can upgrade a cluster seamlessly without forcing the user to re-specify all options they specified the first time.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
fixes: kubernetes/kubeadm#373

Special notes for your reviewer:

This should be a cherrypick-candidate for easier upgrading to v1.8
cc @wojtek-t

Release note:

kubeadm: Upload configuration used at 'kubeadm init' time to ConfigMap for easier upgrades

@kubernetes/sig-cluster-lifecycle-pr-reviews @timothysc

luxas · 2017-08-08T19:06:07Z

/retest

pipejakob · 2017-08-08T20:38:10Z

cmd/kubeadm/app/cmd/init.go

@@ -269,6 +270,11 @@ func (i *Init) Run(out io.Writer) error {
 		return fmt.Errorf("couldn't parse kubernetes version %q: %v", i.cfg.KubernetesVersion, err)
 	}

+	// Upload currently used configuration to the cluster
+	if err := saveconfigphase.SaveConfiguration(i.cfg, client); err != nil {


I have a bit of a nitpick about the verb here. "Save" is a little vague to me, and immediately brings up questions like "what happens if the config already exists? Will it get clobbered? Will it just save a new copy? Is it expected that this configuration was already created, and I'm just saving an update to it?"

I like how our other APIs are extremely informative about their contracts, like tokenphase.CreateBootstrapConfigMapIfNotExists() or tokenphase.UpdateOrCreateToken(). Can the phase be named something like configphase and the method be CreateOrUpdateConfiguration()? That also gives us the flexibility of also having other complementary APIs in the same phase like CreateConfigIfNotExists() in the future if there's demand for other variants.

I changed this to UploadConfiguration sounds good to you?

To me, "upload" is just as vague as "save," since it isn't obvious what happens if the data already exists. We already have the pattern of explicitly saying "CreateOrUpdate" elsewhere in our phases, so I'm wondering why we don't want to follow that. If we're trying to have kubeadm taken seriously as a library / toolkit, I think having a consistent API experience is pretty important.

But, three different people had three different ideas for the naming in this PR :-). Since this is basically introducing a new kubeadm API, maybe we should cast the net a little wider and get a little bit of consensus on how to name it?

Or, if we already have precedent of "all of the phases are alpha anyway, so we can rename them whenever we want," maybe it's not so important to get it right the first time. I haven't been involved in the kubeadm adoption working group, so I'm not sure what the sentiment is around using these phases.

pipejakob · 2017-08-08T20:48:36Z

cmd/kubeadm/app/cmd/phases/saveconfig.go

+func NewCmdSaveConfig() *cobra.Command {
+	var cfgPath, kubeConfigFile string
+	cmd := &cobra.Command{
+		Use:     "save-config <node-name>",


node-name? Copy-paste error?

Good catch, yeah, copy-paste error, shouldn't be anything there

pipejakob · 2017-08-08T20:51:15Z

cmd/kubeadm/app/cmd/phases/saveconfig.go

+		Use:     "save-config <node-name>",
+		Short:   "Upload the currently used configuration for kubeadm to a ConfigMap in the cluster.",
+		Aliases: []string{"saveconfig"},
+		Run: func(_ *cobra.Command, args []string) {


I don't know if we have this pattern for the other commands, but if we're not expecting anything in args, I would error in the case anything is provided. Someone might run kubeadm phase saveconfig my-file thinking that they're overriding the filename to use, but not realize that they forgot --config and accidentally use the wrong file.

I can add a comment about that as follow up. My node bootstrap token PR has a func that can do that.

pipejakob · 2017-08-08T20:54:28Z

cmd/kubeadm/app/phases/saveconfig/saveconfig.go

+)
+
+// SaveConfiguration saves the MasterConfiguration used for later reference (when upgrading for instance)
+func SaveConfiguration(cfg *kubeadmapi.MasterConfiguration, client clientset.Interface) error {


Are there tests we can write for this? In particular, if we want this to be idempotent like our other phases so that kubeadm init can be used for upgrading, we should assert that AlreadyExist errors are handled properly, while other errors are terminal.

jbeda · 2017-08-08T21:12:18Z

cmd/kubeadm/app/cmd/phases/saveconfig.go

+func NewCmdSaveConfig() *cobra.Command {
+	var cfgPath, kubeConfigFile string
+	cmd := &cobra.Command{
+		Use:     "save-config <node-name>",


I'm not sure of the name of the phase here. Save sounds like it is absolutely necessary whereas this is more of a helper type of thing. Perhaps "upload-config"?

Also, change the description a little to something like "Upload the currently used configuration for kubeadm to a ConfigMap in the cluster for future use in reconfiguration and upgrades of the cluster."

jbeda · 2017-08-08T21:14:56Z

LGTM modulo @pipejakob's comments.

/approve

…p for easier upgrades

mattmoyer · 2017-08-09T19:18:23Z

/lgtm

I had some questions about using an object with a stricter schema instead of a ConfigMap, but @timothysc said that was already discussed and punted until later.

luxas · 2017-08-09T19:23:30Z

/retest

…

On 09 Aug 2017, at 22:20, Kubernetes Submit Queue ***@***.***> wrote: [APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jbeda, luxas, mattmoyer Associated issue: 373 The full list of commands accepted by this bot can be found here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

pipejakob · 2017-08-09T19:30:08Z

Same request for new unit tests for the new code. It will never be easier to add tests than it is right now.

timothysc

/lgtm

luxas · 2017-08-09T19:45:34Z

I had some questions about using an object with a stricter schema instead of a ConfigMap, but @timothysc said that was already discussed and punted until later.

As discussed on Slack; there won't be a mechanism in the API for validating that configuration you're uploading to the cluster is valid anytime soon.

To me, "upload" is just as vague as "save," since it isn't obvious what happens if the data already exists. We already have the pattern of explicitly saying "CreateOrUpdate" elsewhere in our phases, so I'm wondering why we don't want to follow that. If we're trying to have kubeadm taken seriously as a library / toolkit, I think having a consistent API experience is pretty important.

I agree with you. This is still in the design phase and I love input here. Please create an issue @pipejakob

But, three different people had three different ideas for the naming in this PR :-). Since this is basically introducing a new kubeadm API, maybe we should cast the net a little wider and get a little bit of consensus on how to name it?
Or, if we already have precedent of "all of the phases are alpha anyway, so we can rename them whenever we want," maybe it's not so important to get it right the first time. I haven't been involved in the kubeadm adoption working group, so I'm not sure what the sentiment is around using these phases.

Yes -- right. We're defining APIs here, but the hard thing is the timeline here and the fact that this PR is in a dependency chain for upgrades. Adding this functionality is not trivial in the chain of ~10 PRs I have posted or am posting as we speak. Getting through the SQ is not trivial either for multiple PRs in a row.

So, my proposal is that you'd create an issue for your concerns. We will discuss things there and spawn a PR with a naming-only change as soon as we find a better name.

Until that, we're not blocking the upgrade chain, instead merging this now-ish lets us work in parallell.

Same request for new unit tests for the new code. It will never be easier to add tests than it is right now.

Again -- totally agree. However, I'm gonna be away for 3.5-days-ish so I'd really want to get this merged now. I can promise to make an unit test for this before the next SIG meeting if you let me.

Does this at all sound reasonable? Sorry for the bad timing -- help here would be very appreciated.

pipejakob

/lgtm

k8s-github-robot · 2017-08-09T19:58:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbeda, luxas, mattmoyer, pipejakob, timothysc

Associated issue: 373

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

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

~~cmd/kubeadm/OWNERS~~ [jbeda,luxas]

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 · 2017-08-09T22:24:18Z

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

k8s-github-robot · 2017-08-10T00:14:18Z

Automatic merge from submit-queue

k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 8, 2017

luxas assigned timothysc Aug 8, 2017

luxas added the cherrypick-candidate label Aug 8, 2017

luxas added this to the v1.7 milestone Aug 8, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 8, 2017

pipejakob reviewed Aug 8, 2017

View reviewed changes

jbeda approved these changes Aug 8, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2017

timothysc assigned jbeda Aug 9, 2017

luxas added 2 commits August 9, 2017 19:22

kubeadm: Upload configuration used at 'kubeadm init' time to ConfigMa…

ad7012e

…p for easier upgrades

autogenerated bazel

728d0f9

luxas force-pushed the kubeadm_saveconfig_phase branch from b02f736 to 728d0f9 Compare August 9, 2017 16:31

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2017

k8s-ci-robot assigned mattmoyer Aug 9, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2017

timothysc approved these changes Aug 9, 2017

View reviewed changes

pipejakob approved these changes Aug 9, 2017

View reviewed changes

k8s-ci-robot assigned pipejakob Aug 9, 2017

luxas mentioned this pull request Aug 9, 2017

Automated cherry pick of #50320 #50393

Closed

k8s-github-robot merged commit b23af4b into kubernetes:master Aug 10, 2017

This was referenced Aug 10, 2017

kubeadm: Move all node bootstrap token related code in one phase package #49982

Merged

Implement the kubeadm upgrade command #48899

Merged

pipejakob mentioned this pull request Aug 10, 2017

Create unit tests for uploadconfigphase kubernetes/kubeadm#379

Closed

wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Aug 11, 2017

wojtek-t removed cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cherrypick-candidate labels Nov 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Upload configuration used at 'kubeadm init' time to ConfigMap for easier upgrades #50320

kubeadm: Upload configuration used at 'kubeadm init' time to ConfigMap for easier upgrades #50320

luxas commented Aug 8, 2017

luxas commented Aug 8, 2017

pipejakob Aug 8, 2017

luxas Aug 9, 2017

pipejakob Aug 9, 2017

pipejakob Aug 8, 2017

luxas Aug 9, 2017 •

edited

pipejakob Aug 8, 2017

luxas Aug 9, 2017

pipejakob Aug 8, 2017

jbeda Aug 8, 2017

jbeda commented Aug 8, 2017

mattmoyer commented Aug 9, 2017

luxas commented Aug 9, 2017 via email

pipejakob commented Aug 9, 2017

timothysc left a comment

luxas commented Aug 9, 2017 •

edited

pipejakob left a comment

k8s-github-robot commented Aug 9, 2017

k8s-github-robot commented Aug 9, 2017

k8s-github-robot commented Aug 10, 2017

kubeadm: Upload configuration used at 'kubeadm init' time to ConfigMap for easier upgrades #50320

kubeadm: Upload configuration used at 'kubeadm init' time to ConfigMap for easier upgrades #50320

Conversation

luxas commented Aug 8, 2017

luxas commented Aug 8, 2017

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

luxas Aug 9, 2017 • edited

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

jbeda commented Aug 8, 2017

mattmoyer commented Aug 9, 2017

luxas commented Aug 9, 2017 via email

pipejakob commented Aug 9, 2017

timothysc left a comment

Choose a reason for hiding this comment

luxas commented Aug 9, 2017 • edited

pipejakob left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Aug 9, 2017

k8s-github-robot commented Aug 9, 2017

k8s-github-robot commented Aug 10, 2017

luxas Aug 9, 2017 •

edited

luxas commented Aug 9, 2017 •

edited