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

Advanced configurations with kubeadm (Kustomize) #1159

Merged
merged 4 commits into from
Jul 29, 2019

Conversation

fabriziopandini
Copy link
Member

This PR adds a KEP for defining a new kubeadm feature that will allow users to bootstrap
a Kubernetes cluster with configuration options - control-plane or kubelet settings -
not supported by the Kubeadm config API.

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

/cc @neolit123
/cc @rosti
/cc @ereslibre
/cc @detiber
/cc @vincepri

/assign @timothysc
/assign @luxas

@k8s-ci-robot 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 Jul 22, 2019
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 22, 2019
@chuckha
Copy link
Contributor

chuckha commented Jul 22, 2019

oh hey this is definitely up my alley
/cc
/cc @amy

@k8s-ci-robot k8s-ci-robot requested review from amy and chuckha July 22, 2019 12:53
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 for the writeup @fabriziopandini

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.

So I think we should limit the scope for the 1st batch to focus solely on control plane manifest customizations and follow the sematics of kubectl https://kubernetes.io/docs/tasks/manage-kubernetes-objects/kustomization/

Copy link

@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.

Looking good, thank you @fabriziopandini, some comments on a first pass.

@fabriziopandini
Copy link
Member Author

@neolit123 @ereslibre @timothysc @rosti Thanks for the highly valuable feedback!

Most of the comments are already addressed; main changes (according to feedbacks):

  • 1st iteration focuses now on static pod manifests only (control-plane and etcd). kubelet is now out of scope
  • Better focus on new use cases enabled by this feature/differences with existing features:
    • Kustomize = full control of static pod manifest generated by Kubeadm at node level
    • kubeadm component config abstractions = control-plane/etcd command line args only, cluster wide
  • Users have to specify patch locations like in kubectl -k (no more feature flag; no more /etc/kubernetes/kustomize default folder)
  • Added a new risk "Component version change during upgrades"

There are still one/two points to be discussed in the kubeadm office hours.

Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thank you @fabriziopandini !
Looks great and other than a few minor things, it's all good to go from my perspective.

@fabriziopandini
Copy link
Member Author

@timothysc @neolit123 @rosti
Kep updated with feedbacks from 24/07/2019 kubeadm office hours

  • documented UX showed in the demo
  • documented acceptable limitation for patchesJson6902 (to be addressed before beta)
  • added a new risk about error management to be improved due to potential kustomize errors

name: kube-controller-manager
namespace: kube-system
{your kube-controller-manager patch here}
EOF
Copy link
Member

Choose a reason for hiding this comment

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

Should there also be an example here for a net new static pod?

Copy link
Member Author

Choose a reason for hiding this comment

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

@detiber I'm not sure what do you mean with "net new static pod".

This feature targets only the 4 static pods manifests (3 in case of external etcd) created by kubeadm in the /etcd/kubernetes/manifests folder

that Kustomize patches will remain stored in the custom location on the machine file system
for the necessary time, and that this location will be passed to kubeadm upgrade with a CLI
flag; this point could be reconsidered in the future, by e.g. defining a method for
allowing higher-level tools/users to define Kustomize patches using a new CRD.
Copy link
Member

Choose a reason for hiding this comment

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

How will the kustomize patches be applied?

Generally kustomize operates on a single yaml file applying all defined patches. Would this proposal mean that the currently generated static manifests would be merged into a single file prior to applying the kustomize patches first?

Would they still be broken up before writing them to disk? If so, what are the plans for doing that splitting and for naming the resulting static manifest files? I'm assuming the existing static manifest file names would stay the same, but how would additional passed static pod files be named?

Are there plans to vendor kustomize to do the patching operations or to shell out to kubectl? If the later, doesn't this introduce a new dependency on the kubectl binary that didn't exist previously?

Copy link
Member Author

@fabriziopandini fabriziopandini Jul 26, 2019

Choose a reason for hiding this comment

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

@detiber

How will the kustomize patches be applied? ...

In the POC I'm exploring a different approach:

  • read all the patches
  • group patches by the target object (without using any naming conventions, but relying on TypeMeta/ObjetMeta embedded in patches)
  • when rendering a static pod manifest, apply only relevant patches

how would additional passed static pod files be named?

ATM the KEP does not consider additional passed static pod, because of they are out of the kubeadm responsibility. But if you can provide more details about your use case I'm more than open to reconsidering this (TBD if in this or in future iterations of this KEP)

Are there plans to vendor kustomize

In the POC I'm exploring vendoring, because it allows me to do the kustomize process in memory and have full control on it; I also agree with you that the resulting solution is self-contained, but there are different opinion on this, see e.g. #1159 (comment)

Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thanks @fabriziopandini !
I am in favor of merging this now. Looks like a good plan to start with and gather some user feedback from an implementation.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2019
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.

/lgtm
/approve

If we need to refine we can just update.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, 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 Jul 29, 2019
@k8s-ci-robot k8s-ci-robot merged commit a26254d into kubernetes:master Jul 29, 2019
@fabriziopandini fabriziopandini deleted the kubeadm-kustomize branch July 30, 2019 14:20
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

9 participants