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: Control plane config moved to substructs #70371

Merged
merged 1 commit into from
Nov 2, 2018

Conversation

rosti
Copy link
Contributor

@rosti rosti commented Oct 29, 2018

What type of PR is this?
/kind api-change

What this PR does / why we need it:

In v1alpha3's, control plane component config options were nested directly into the ClusterConfiguration structure. This is cluttering the config structure and makes it hard to maintain. Therefore the control plane config options must be separated into different substructures in order to graduate the format to beta.

This change does the following:

  • Introduces a new structure called ControlPlaneComponent, that contains fields common to all control plane component types. These are currently extra args and extra volumes.

  • Introduce a new structure called APIServer that contains ControlPlaneComponent and APIServerCertSANs field (from ClusterConfiguration)

  • Replace all API Server, Scheduler and Controller Manager options in ClusterConfiguration with APIServer, ControllerManager and Scheduler fields of APIServer and ControlPlaneComponent types.

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

Special notes for your reviewer:

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

This will be followed by a PR that introduces an API server timeout option (as discussed at office hours on Oct 24th.

Does this PR introduce a user-facing change?:

kubeadm: Control plane component configs are separated into ClusterConfiguration sub-structs.

@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 Oct 29, 2018
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Oct 29, 2018
@k8s-ci-robot k8s-ci-robot added 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. labels Oct 29, 2018
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

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2018
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 PR!
I have no strong opinion on the usage of a shared struct vs dedicated struct.
Everything else seems ok

Only one question. The Change about HostPathMount.Writable to ReadOnly is going to be addressed in another PR?

@rosti
Copy link
Contributor Author

rosti commented Oct 31, 2018

/test pull-kubernetes-e2e-gce-100-performance

@timothysc timothysc removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 1, 2018
@k8s-ci-robot k8s-ci-robot added the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Nov 1, 2018
@timothysc timothysc added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Nov 1, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Nov 1, 2018
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/approve

LGTM but will defer to @fabriziopandini for final call

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

This appears to be blocking a number of your other PRs.

@fabriziopandini
Copy link
Member

@rosti please rebase and then ping me for lgtm

In v1alpha3's, control plane component config options were nested directly into
the ClusterConfiguration structure. This is cluttering the config structure and
makes it hard to maintain. Therefore the control plane config options must be
separated into different substructures in order to graduate the format to beta.

This change does the following:

- Introduces a new structure called ControlPlaneComponent, that contains fields
  common to all control plane component types. These are currently extra args
  and extra volumes.

- Introduce a new structure called APIServer that contains
  ControlPlaneComponent and APIServerCertSANs field (from ClusterConfiguration)

- Replace all API Server, Scheduler and Controller Manager options in
  ClusterConfiguration with APIServer, ControllerManager and Scheduler fields
  of APIServer and ControlPlaneComponent types.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@fabriziopandini
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2018
@rosti
Copy link
Contributor Author

rosti commented Nov 2, 2018

/test pull-kubernetes-e2e-kops-aws

@fabriziopandini
Copy link
Member

/test pull-kubernetes-integration
/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot k8s-ci-robot merged commit b83a947 into kubernetes:master Nov 2, 2018
@rosti rosti deleted the control-plane-substructs branch November 22, 2018 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants