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

kubeadm: Add a 'kubeadm config migrate' command #64232

Merged
merged 2 commits into from May 30, 2018

Conversation

@luxas
Copy link
Member

luxas commented May 23, 2018

What this PR does / why we need it:

This is an UX improvement so users may easier "upgrade" their configuration files from the an old version (e.g. v1alpha1) version to a new one (e.g. v1alpha2), can do this locally and seamlessly without touching a cluster. We talked about this in the SIG meeting; getting the users to be able to convert their checked-in configuration files to new API versions will be crucial.

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 kubernetes/kubeadm#912

Special notes for your reviewer:

Release note:

kubeadm: Add a 'kubeadm config migrate' command to convert old API types to their newer counterparts in the new, supported API types. This is just a client-side tool, it just executes locally without requiring a cluster to be running. You can think about this as an Unix pipe that upgrades config files.

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

@@ -138,6 +141,59 @@ func getDefaultAPIObjectBytes(apiObject string) ([]byte, error) {
return []byte{}, fmt.Errorf("--api-object needs to be one of %v", availableAPIObjects)
}

// NewCmdConfigMigrate returns cobra.Command for "kubeadm config migrate" command
func NewCmdConfigMigrate(out io.Writer) *cobra.Command {

This comment has been minimized.

@timothysc

timothysc May 24, 2018

Member

What's the user story here? I'm just wondering if adding a cli makes sense and will be used, b/c once created it will be near impossible to take out.

especially seeing how we have diff and upgrade...

/cc @liztio

This comment has been minimized.

@luxas

luxas May 30, 2018

Author Member

This is client-side migration of config files (bytes on disk) only, it doesn't touch any upgrading code nor is a replacement for anything upgrade-specific.

internalcfg, err := configutil.ConfigFileAndDefaultsToInternalConfig(cfgPath, &kubeadmapiv1alpha2.MasterConfiguration{})
kubeadmutil.CheckErr(err)

outputBytes, err = kubeadmutil.MarshalToYamlForCodecs(internalcfg, kubeadmapiv1alpha2.SchemeGroupVersion, kubeadmscheme.Codecs)

This comment has been minimized.

@dixudx

dixudx May 24, 2018

Member
outputBytes, err = kubeadmutil.MarshalToYamlForCodecs(internalcfg, kubeadmapiv1alpha2.SchemeGroupVersion, kubeadmscheme.Codecs)

This is common for both MasterConfig and NodeConfig, right? Shall we extract and handle it for all?

This comment has been minimized.

@luxas

luxas May 30, 2018

Author Member

internalcfg is of different types so instead of doing some casting in both places I'd like to keep this small duplication for now

This comment has been minimized.

@luxas

luxas May 30, 2018

Author Member

I added a TODO

kubeadmutil.CheckErr(fmt.Errorf("Didn't recognize type with GroupVersionKind: %v", gvk))
}

fmt.Fprintf(out, string(outputBytes))

This comment has been minimized.

@dixudx

dixudx May 24, 2018

Member

I think we can provide another flag to write the content to a file. WDYT?

This comment has been minimized.

@luxas

luxas May 30, 2018

Author Member

Done

@liztio

This comment has been minimized.

Copy link
Member

liztio commented May 25, 2018

I don't think I understand why we would want this vs just automatically doing this for kubeadm upgrade. If a user really needs to apply changes, would kubeadm upgrade diff | patch -p0 not already be sufficient?

fmt.Fprintf(out, string(outputBytes))
},
}
cmd.Flags().StringVar(&cfgPath, "config", "", "Path to a kubeadm config file. WARNING: Usage of a configuration file is experimental.")

This comment has been minimized.

@chuckha

chuckha May 25, 2018

Member

This warning doesn't really make sense if it's mandatory

edit:

suggestion: remove the warning

This comment has been minimized.

@luxas

luxas May 30, 2018

Author Member

Done, also splitted to --old-config and --new-config

@chuckha

This comment has been minimized.

Copy link
Member

chuckha commented May 25, 2018

I think this is good to have, but I strongly agree with what @liztio suggested. This should be the default behavior in upgrade.

I can see migration behavior as being a "phase" but if we're trying to not lump everything under phases, config is the right place for it.

I see this as the first step to making the config migration automatically happen in the upgrade path while also exposing just this behavior under config for cases where a user wants to see what the new config would look like or maybe run upgrades manually.

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented May 25, 2018

@luxas

i'm ok with a new sub-command that has a specific use.

This command lets you convert configuration objects of older versions to the latest supported version.
In this version of kubeadm, the following API versions are supported:

migrate implies a big action. relocating to something completely different (e.g. birds moving south).
given the command only handles converting a config to a new version it should be called kubeadm config upgrade.

if the command will have future implications that allow it to convert a certain config to any given config version it should be called kubeadm config convert as a safe bet:

kubeadm config convert --to=latest --config=<some-config>
kubeadm config convert --to=1.8 --config=<some-1.10-config>

having --to=latest, equals not specifying it.

@liztio

I don't think I understand why we would want this vs just automatically doing this for kubeadm upgrade. If a user really needs to apply changes, would kubeadm upgrade diff | patch -p0 not already be sufficient?

it's ok, but it needs documentation outside of cobra and is kind of bound to Unix, because if kubeadm ever supports native Win32 one would need patch installed from something like msys, since that's not a command that's bundled by default.

if this command goes under kubeadm upgrade config --config=<some-file>: it can still work, but it's then it would only imply upgrade.


my vote for a long term flexible command would be:

kubeadm config convert --to=latest --config=<some-config>

also one thing that the command has to do is to create backups and write about that to stdout.
[config] Upgrading config <file-path> from version <x> to version <y>config>.backup-1.10
[config] Backup file created: <file-path>.backup-<x>

@timothysc
Copy link
Member

timothysc left a comment

/lgtm

Per conversation with the SIG call, but please update the release note to outline the user story to be clear. Also update the long description.

@k8s-ci-robot k8s-ci-robot added the lgtm label May 29, 2018

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 29, 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.

3 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 30, 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.

Copy link

fejta-bot commented May 30, 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.

Copy link

fejta-bot commented May 30, 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.

luxas added some commits May 30, 2018

@luxas luxas force-pushed the luxas:kubeadm_config_migrate branch from dbca428 to 7914dce May 30, 2018

@k8s-ci-robot k8s-ci-robot added size/L and removed lgtm size/M labels May 30, 2018

@dixudx

This comment has been minimized.

Copy link
Member

dixudx commented May 30, 2018

/lgtm

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 30, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@dixudx @liztio @luxas @timothysc

Pull Request Labels
  • sig/cluster-lifecycle: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/feature: New functionality.
Help

@luxas luxas added the approved label May 30, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 30, 2018

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

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

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 30, 2018

Automatic merge from submit-queue (batch tested with PRs 64322, 64210, 64458, 64232, 64370). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 897a4b4 into kubernetes:master May 30, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation luxas 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-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.