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: config migrate handles more valid configs #71315

Merged
merged 1 commit into from
Nov 22, 2018
Merged

kubeadm: config migrate handles more valid configs #71315

merged 1 commit into from
Nov 22, 2018

Conversation

rosti
Copy link
Contributor

@rosti rosti commented Nov 21, 2018

What type of PR is this?
/kind bug

What this PR does / why we need it:

kubeadm config migrate uses AnyConfigFileAndDefaultsToInternal, which can
unmarshal config from file only if InitConfiguration or JoinConfiguration are
present. Even with that in mind, it can only return a singlie config object,
with InitConfiguration taking precendence over JoinConfiguration. Thus, the
following cases were not handled properly, while they were perfectly valid for
kubeadm init/join:

  • ClusterConfiguration only file caused kubeadm config migrate to exit with
    error.
  • Init + Join configurations in the same file caused Init + Cluster
    configuration to be produced (ignoring JoinConfiguration). The same is valid
    when the combo is Init + Cluster + Join configurations.
  • Cluster + Join configuration ignores ClusterConfiguration and only
    JoinConfiguration gets migrated.

To fix this, the following is done:

  • Introduce MigrateOldConfigFromFile which migrates old config from a file,
    while ensuring that all kubeadm originated input config kinds are taken care
    of. Add comprehensive unit tests for this.
  • Replace the use of AnyConfigFileAndDefaultsToInternal in
    kubeadm config migrate with MigrateOldConfigFromFile.
  • Remove the no longer used and error prone AnyConfigFileAndDefaultsToInternal.

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#1262

Special notes for your reviewer:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/assign @fabriziopandini
/assign @timothysc
/assign @luxas

Does this PR introduce a user-facing change?:

NONE

@rosti
kubeadm: config migrate handles more valid configs
037fb61 
kubeadm config migrate uses AnyConfigFileAndDefaultsToInternal, which can
unmarshal config from file only if InitConfiguration or JoinConfiguration are
present. Even with that in mind, it can only return a singlie config object,
with InitConfiguration taking precendence over JoinConfiguration. Thus, the
following cases were not handled properly, while they were perfectly valid for
kubeadm init/join:

- ClusterConfiguration only file caused kubeadm config migrate to exit with
  error.
- Init + Join configurations in the same file caused Init + Cluster
  configuration to be produced (ignoring JoinConfiguration). The same is valid
  when the combo is Init + Cluster + Join configurations.
- Cluster + Join configuration ignores ClusterConfiguration and only
  JoinConfiguration gets migrated.

To fix this, the following is done:
- Introduce MigrateOldConfigFromFile which migrates old config from a file,
  while ensuring that all kubeadm originated input config kinds are taken care
  of. Add comprehensive unit tests for this.
- Replace the use of AnyConfigFileAndDefaultsToInternal in
  kubeadm config migrate with MigrateOldConfigFromFile.
- Remove the no longer used and error prone AnyConfigFileAndDefaultsToInternal.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 21, 2018
@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Nov 21, 2018
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 21, 2018
dixudx
dixudx reviewed Nov 21, 2018
View reviewed changes
cmd/kubeadm/app/util/config/common.go

// Migrate InitConfiguration and ClusterConfiguration if there are any in the config
if kubeadmutil.GroupVersionKindsHasInitConfiguration(gvks...) || kubeadmutil.GroupVersionKindsHasClusterConfiguration(gvks...) {
o, err := ConfigFileAndDefaultsToInternalConfig(cfgPath, &kubeadmapiv1beta1.InitConfiguration{})
Copy link
Member

Choose a reason for hiding this comment

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

I think we can do some refactoring for ConfigFileAndDefaultsToInternalConfig, since the file will be read twice. There is no need.

Copy link
Contributor Author

@rosti rosti Nov 21, 2018 •
edited

Choose a reason for hiding this comment

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

Yes, that's on my agenda for the next release cycle. Decoupling InitConfiguration and ClusterConfiguration is one of my priorities. If we refactor the APIs and make ClusterConfiguration more standalone and introduce an internal type that houses the usual pair (ClusterConfiguration + NodeRegistrationOptions for the current node) we can eliminate most of the confusion and optimize things at all levels.
But for now at the end of this release cycle it's best to keep the changes as small as possible and just fix the bug at hand.

cmd/kubeadm/app/util/config/common.go

// Migrate JoinConfiguration if there is any
if kubeadmutil.GroupVersionKindsHasJoinConfiguration(gvks...) {
o, err := JoinConfigFileAndDefaultsToInternalConfig(cfgPath, &kubeadmapiv1beta1.JoinConfiguration{})
Copy link
Member

Choose a reason for hiding this comment

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

same as above.

@rosti
Copy link
Contributor Author

rosti commented Nov 21, 2018

/test pull-kubernetes-integration

@neolit123
Copy link
Member

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 21, 2018
neolit123
neolit123 approved these changes Nov 21, 2018
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

thanks @rosti
LGTM

@neolit123
Copy link
Member

/retest

1 similar comment
@neolit123
Copy link
Member

/retest

@fabriziopandini
Copy link
Member

/milestone v1.13

@k8s-ci-robot k8s-ci-robot added this to the v1.13 milestone Nov 21, 2018
fabriziopandini
fabriziopandini approved these changes Nov 21, 2018
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

@rosti thanks for this quick fix!
/approve
@luxas @timothysc could you kindly take a look / give final lgtm?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, rosti

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2018
@neolit123
Copy link
Member

/retest

@rosti
Copy link
Contributor Author

rosti commented Nov 21, 2018

/test pull-kubernetes-integration

@neolit123
Copy link
Member

/lgtm
/hold

further comments? unhold by EOD tomorrow.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 22, 2018
luxas
luxas approved these changes Nov 22, 2018
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
/hold cancel
Thanks for the bugfix!

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2018
@k8s-ci-robot k8s-ci-robot merged commit 8d9ac26 into kubernetes:master Nov 22, 2018
@rosti rosti deleted the config-migrate-fix branch November 22, 2018 15:23
@rosti rosti mentioned this pull request Feb 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dixudx dixudx dixudx left review comments

@neolit123 neolit123 neolit123 approved these changes

@fabriziopandini fabriziopandini fabriziopandini approved these changes

@luxas luxas luxas approved these changes

Assignees

@timothysc timothysc

@neolit123 neolit123

@fabriziopandini fabriziopandini

@luxas luxas

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.13
Development

Successfully merging this pull request may close these issues.

kubeadm config migrate v1alpha3 -> v1beta1 bugs
7 participants
@rosti @neolit123 @fabriziopandini @k8s-ci-robot @luxas @dixudx @timothysc