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: kustomize core #80905

Merged

Conversation

@fabriziopandini
Copy link
Member

commented Aug 2, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR introduces a new feature that allows applying kustomize patches to static pod manifests generated by kubeadm. See KEP

Which issue(s) this PR fixes:
xref #kubernetes/kubeadm#1379

Special notes for your reviewer:
WRT to the POC [https://github.com//pull/80580] this PR:

  • contains only the core part of the kustomize implementation in kubeadm (UX changes will be implemented as a follow up PR starting from placeholders added in the second commit)
  • address all the feedbacks collected in the POC
  • adds unit test

Known limitations:
this first implementation as some known limitations:

  • it does not work with customization stored in a git repository
  • it does not support patchJson6902 patches

Does this PR introduce a user-facing change?:

Implement a new feature that allows applying kustomize patches to static pod manifests generated by kubeadm. 

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

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-cluster-lifecycle/kubeadm/20190722-Advanced-configurations-with-kubeadm-(Kustomize).md

/sig cluster-lifecycle
/priority important-soon
/assign @rosti @yagonobre @neolit123

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

/retest

@ereslibre
Copy link
Member

left a comment

Two minor comments, SGTM on a first pass.

cmd/kubeadm/app/util/kustomize/kunstruct.go Outdated Show resolved Hide resolved
fmt.Fprintf(&kustomization, " - %s\n", name)
}

kustomization.WriteString("patches:\n")

This comment has been minimized.

Copy link
@ereslibre

ereslibre Aug 2, 2019

Member

I think it's fine for now but would a template look more clear?

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Aug 6, 2019

Author Member

I'm working on a follow-up PR for overcoming known limitations, and I will fix this in that PR

This comment has been minimized.

Copy link
@rosti

rosti Aug 6, 2019

Member

We already import some Kustomize packages, importing the types package, filling in a struct and marshaling it to YAML looks like the cleanest way to me.

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@yagonobre
Copy link
Member

left a comment

Thanks @fabriziopandini
After address @ereslibre comments LGTM

cmd/kubeadm/app/phases/etcd/local.go Show resolved Hide resolved

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:kubeadm-kustomize-core branch from 6eb522d to 6be2c1c Aug 6, 2019

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

@yagonobre @ereslibre thanks for the feedback! Everything should be addressed/commented now

@rosti
Copy link
Member

left a comment

Thanks @fabriziopandini !
Overall, this looks good!

// create an in memory fs to use for the kustomization
memFS := fs.MakeFakeFS()

var kustomization bytes.Buffer

This comment has been minimized.

Copy link
@rosti

rosti Aug 6, 2019

Member

Ideally this should be var kustomization kustomizetypes.Kustomization (where kustomizetypes "sigs.k8s.io/kustomize/pkg/types") and then simply marshal it to YAML.

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Aug 6, 2019

Author Member

Agreed, that's the approach I'm already following in the follow-up PR for overcoming known limitations

fmt.Fprintf(&kustomization, " - %s\n", name)
}

kustomization.WriteString("patches:\n")

This comment has been minimized.

Copy link
@rosti

rosti Aug 6, 2019

Member

We already import some Kustomize packages, importing the types package, filling in a struct and marshaling it to YAML looks like the cleanest way to me.

@ereslibre

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

/lgtm
/hold

Adding hold for leaving time to address @rosti comments, feel free to unhold. I'm always unsure if a single lgtm will merge right away.

@@ -138,6 +141,56 @@ func TestCreateStaticPodFilesAndWrappers(t *testing.T) {
}
}

func TestCreateStaticPodFilesKustomize(t *testing.T) {
// Create temp folder for the test case
tmpdir := testutil.SetupTempDir(t)

This comment has been minimized.

Copy link
@neolit123

neolit123 Aug 6, 2019

Member

non-blocking: we should stop importing k8s.io/kubernetes/cmd/kubeadm/test" in .../app.

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

ok, lets get this merge this and let @fabriziopandini send a follow up.
/hold cancel
/retest

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 7, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Would like the CLI team's feedback on the invocation of kustomize to avoid drifting implementations.

More broadly, at first glance, this seems like it makes every change to the generated static manifests a potentially breaking change (even specifying a flag can break a user that was already adding that flag themselves). What happens if user-targeted customizations no longer match generated resources (name changes, etc). Are the customizations silently dropped or does that produce an error?

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

@liggitt thanks for comments.
The risk of "change to the generated static manifests" was discussed in the KEP, and for the alpha release of this feature, it was decided to simply add AI to release notes for changes and to improve documentation for upgrades suggesting user to --dry-run before upgrades in case of patches.
The KEP introduces also the need for an improvement to kubeadm error management "ensuring that the node remains in a consistent state in case of errors", but this will be implemented in follow up PRs.

@timothysc timothysc added this to the v1.16 milestone Aug 7, 2019

@timothysc

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

The risk of "change to the generated static manifests" was discussed in the KEP

TL;DR It's a power user feature that has been asked for, and we plan to clause it as such.

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

The risk of "change to the generated static manifests" was discussed in the KEP

TL;DR It's a power user feature that has been asked for, and we plan to clause it as such.

ack

@dims

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

/retest

@dims dims added this to Backlog in code-organization subproject Aug 8, 2019

@dims

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Let's reach out to @monopole for a review of how kustomize is used. no issues about vendoring it back in.

@dims dims moved this from Backlog to In progress in code-organization subproject Aug 8, 2019

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:kubeadm-kustomize-core branch from 148abe2 to 7fee8bc Aug 9, 2019

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:kubeadm-kustomize-core branch from 7fee8bc to 5eca049 Aug 12, 2019

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 12, 2019

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@liggitt If I'm not wrong all the comment are now addressed or properly tracked

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

/approve
for go.mod changes

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, fabriziopandini, liggitt, neolit123

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 merged commit c08ee9d into kubernetes:master Aug 13, 2019

23 checks passed

cla/linuxfoundation fabriziopandini authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
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.