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: Group centric component configs #85639

Merged
merged 1 commit into from
Dec 4, 2019
Merged

kubeadm: Group centric component configs #85639

merged 1 commit into from
Dec 4, 2019

Conversation

rosti
Copy link
Contributor

@rosti rosti commented Nov 26, 2019 •
edited
Loading

What type of PR is this?
/kind design

What this PR does / why we need it:

kubeadm's current implementation of component config support is "kind" centric.
This has its downsides. Namely:

  • Kind names and numbers can change between config versions.
    Newer kinds can be ignored. Therefore, detection of a version change is
    considerably harder.
  • A component config can have only one kind that is managed by kubeadm.

Thus a more appropriate way to identify component configs is required.

Probably the best solution identified so far is a config group.
A group name is unlikely to change between versions, while the kind names and
structure can.
Tracking component configs by group name allows us to:

  • Spot more easily config version changes and manage alternate versions.
  • Support more than one kind in a config group/version.
  • Abstract component configs by hiding their exact structure.

Hence, this change rips off the old kind based support for component configs
and replaces it with a group name based one. This also has the following
extra benefits:

  • More tests were added.
  • kubeadm now errors out if an unsupported version of a known component group
    is used.

Which issue(s) this PR fixes:

Refs:

Special notes for your reviewer:

This PR is part of the implementation of the new kubeadm component config management scheme KEP (see link below).

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/priority important-soon
/assign @timothysc @fabriziopandini @neolit123 @ereslibre

Does this PR introduce a user-facing change?:

kubeadm now errors out whenever a not supported component config version is supplied for the kubelet and kube-proxy

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-cluster-lifecycle/kubeadm/20190925-component-configs.md

@rosti
kubeadm: Group centric component configs
b881f19 
kubeadm's current implementation of component config support is "kind" centric.
This has its downsides. Namely:
- Kind names and numbers can change between config versions.
  Newer kinds can be ignored. Therefore, detection of a version change is
  considerably harder.
- A component config can have only one kind that is managed by kubeadm.
Thus a more appropriate way to identify component configs is required.

Probably the best solution identified so far is a config group.
A group name is unlikely to change between versions, while the kind names and
structure can.
Tracking component configs by group name allows us to:
- Spot more easily config version changes and manage alternate versions.
- Support more than one kind in a config group/version.
- Abstract component configs by hiding their exact structure.

Hence, this change rips off the old kind based support for component configs
and replaces it with a group name based one. This also has the following
extra benefits:
- More tests were added.
- kubeadm now errors out if an unsupported version of a known component group
  is used.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Nov 26, 2019
@k8s-ci-robot k8s-ci-robot added the kind/design Categorizes issue or PR as related to design. label Nov 26, 2019
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. area/kubeadm size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 26, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 26, 2019
@rosti
Copy link
Contributor Author

rosti commented Nov 26, 2019

/hold
for discussion.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 26, 2019
neolit123
neolit123 reviewed Nov 26, 2019
View reviewed changes
cmd/kubeadm/app/componentconfigs/configset.go
groups = append(groups, group)
}
sort.Strings(groups) // The sort is needed to make the output predictable
klog.Warningf("WARNING: kubeadm cannot validate component configs for API groups %v", groups)
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 saw this warning being printed in a lot of commands such as kubeadm config images.
should we change it to klog.V(...).Infof()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess so. The code is executed whenever the config is loaded (which happens in almost all kubeadm commands). However it's good to see it as a warning only on kubeadm init/upgrade *.

@neolit123
Copy link
Member

i'm +1 on this refactor. thanks @rosti

i envision the following split in the future:

  • kubelet, and control-plane component configs being managed by kubeadm in a similar way (as core components)
  • kube-proxy moving to addons.

@rosti
Copy link
Contributor Author

rosti commented Nov 27, 2019

i'm +1 on this refactor. thanks @rosti

i envision the following split in the future:

  • kubelet, and control-plane component configs being managed by kubeadm in a similar way (as core components)
  • kube-proxy moving to addons.

Exactly, that's my vision too.

@ereslibre
Copy link
Contributor

On a first sight this SGTM. I plan to review this more thoroughly later today though.

@rosti rosti mentioned this pull request Nov 28, 2019
Closed
ereslibre
ereslibre reviewed Dec 2, 2019
View reviewed changes
Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

LGTM, is a big chunk to review, but I'm +1 overall. A small comment for a tiny improvement.

cmd/kubeadm/app/componentconfigs/configset.go

// FromCluster loads a component from a config map in the cluster
func (h *handler) FromCluster(clientset clientset.Interface, clusterCfg *kubeadmapi.ClusterConfiguration) (kubeadmapi.ComponentConfig, error) {
return h.fromCluster(h, clientset, clusterCfg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we make fromCluster a handler method as well, so there's no need to pass h as argument?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see why you did this on the non-public one... nevermind.

@ereslibre
Copy link
Contributor

/lgtm
/hold
^ for other reviewers.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2019
yastij
yastij reviewed Dec 2, 2019
View reviewed changes
Copy link
Member

@yastij yastij left a comment

Choose a reason for hiding this comment

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

/lgtm

@neolit123
Copy link
Member

/hold cancel
this PR received 3 +1s.

@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 Dec 3, 2019
@k8s-ci-robot k8s-ci-robot merged commit 674695c into kubernetes:master Dec 4, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ereslibre ereslibre ereslibre left review comments

@neolit123 neolit123 neolit123 left review comments

@yastij yastij yastij left review comments

Assignees

@ereslibre ereslibre

@timothysc timothysc

@neolit123 neolit123

@fabriziopandini fabriziopandini

@yastij yastij

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/design Categorizes issue or PR as related to design. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.18
Development

Successfully merging this pull request may close these issues.
7 participants
@rosti @k8s-ci-robot @neolit123 @ereslibre @yastij @timothysc @fabriziopandini