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

Implement `kubeadm upgrade diff` #63930

Merged
merged 3 commits into from May 17, 2018

Conversation

@liztio
Member

liztio commented May 16, 2018

What this PR does / why we need it:

Some users want to see the changes kubeadm woulda apply before actually running kubeadm upgrade apply. This shows the changes that will be made to the static pod manifests before applying them. This is a narrower case than kubeadm upgrade apply --dry-run, which specifically focuses on the static pod manifests.

Which issue(s) this PR fixes:
Part of kubeadm/489
Fixes kubernetes/kubeadm#830

Special notes for your reviewer:

Release note:

adds the `kubeadm upgrade diff` command to show how static pod manifests will be changed by an upgrade.
@liztio

This comment has been minimized.

Member

liztio commented May 16, 2018

I couldn't think of any tests that would provide much useful signal here and wouldn't be brittle across versions. If anyone has any suggestions I'm open to them!

@liztio

This comment has been minimized.

Member

liztio commented May 16, 2018

@luxas luxas self-assigned this May 16, 2018

k8sVer, err := version.ParseSemantic(flags.newK8sVersionStr)
if err != nil {
kubeadmutil.CheckErr(err)

This comment has been minimized.

@neolit123

neolit123 May 16, 2018

Member

it's really hard to write unit tests around CheckErr() as it effectively calls exit(1) on error.
:(

i would return an error from this function an only call a single CheckErr() on that error in the Cobra Run().

This comment has been minimized.

@luxas

luxas May 16, 2018

Member

Can you please file an issue to improve that? Such unit testing would be a perfect contribution from you 👍

This comment has been minimized.

@neolit123

neolit123 May 16, 2018

Member

shouldn't a unit test be added as part of this PR? @liztio WDYT?

This comment has been minimized.

@liztio

liztio May 16, 2018

Member

@neolit123 like I said, I couldn't figure out what good unit tests would look like since the diffs are likely to change a lot between versions. I'm open to ideas on what they could look like.

This comment has been minimized.

@neolit123

neolit123 May 16, 2018

Member

possible tests:

  • non existing config path in flags.parent.cfgPath
  • flags.newK8sVersionStr == "", but no version arg is provided
  • bad version format in flags.newK8sVersionStr to make ParseSemantic() fail
  • make kubeadmutil.MarshalToYaml fail somehow.
  • fail ioutil.ReadFile(path) by providing bad 'path'

i don't think that the diff output itself should be tested - e.g. making sure that a diff buffer looks exactly like another diff buffer.

one problem i have with Lucas' request is that my setup is quite broken at the moment, so if you want me to add tests for this, i would need a fully working master configuration yaml aaand i might ask some questions too - because i simply don't understand some of the things here.

This comment has been minimized.

@neolit123

neolit123 May 16, 2018

Member

@liztio TLDR if you are out of time and want me to help send me a YAML that returns nil for runDiff().

This comment has been minimized.

@liztio

liztio May 16, 2018

Member

Those sound good. Unfortunately, I'm out thursday and friday this week, and it's basically the end of my day. So given the choice between adding unit tests and missing the 0.11 deadline, I'll defer the unit tests until I get back. I made an issue so I don't forget to follow up.

@luxas

A couple of small comments only, generally LGTM

k8sVer, err := version.ParseSemantic(flags.newK8sVersionStr)
if err != nil {
kubeadmutil.CheckErr(err)

This comment has been minimized.

@luxas

luxas May 16, 2018

Member

Can you please file an issue to improve that? Such unit testing would be a perfect contribution from you 👍

}
// Convert to an internal representation
internalcfg := &kubeadmapi.MasterConfiguration{}

This comment has been minimized.

@luxas

luxas May 16, 2018

Member

It already is internal, why have these lines of code here?

This comment has been minimized.

@liztio

liztio May 16, 2018

Member

whoops, leftover from a rebase.

corev1 "k8s.io/api/core/v1"
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/scheme"
kubeadmapiext "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1alpha2"

This comment has been minimized.

@luxas

luxas May 16, 2018

Member

We use kubeadmv1alpha2 as the name everywhere else currently. If you care strongly about the naming there, please open an issue to change it to something else, but please be consistent for now.

var cfg *kubeadmapi.MasterConfiguration
// If the version is specified in config file, pick up that value.
if flags.parent.cfgPath != "" {

This comment has been minimized.

@luxas

luxas May 16, 2018

Member

Technically I don't think you should need to do this, as ConfigFileAndDefaultsToInternalConfig has this conditional inside of itself.

continue
}
new, err := kubeadmutil.MarshalToYaml(&pod, corev1.SchemeGroupVersion)

This comment has been minimized.

@luxas

luxas May 16, 2018

Member

is new a keyword? can you make the name a bit longer like newPodYAML?

cmd := &cobra.Command{
Use: "diff [version]",
Short: "Show what differences would be applied to existing static pod manifests",

This comment has been minimized.

@luxas

luxas May 16, 2018

Member

Can you please add a reference to apply --dry-run here so that it's more discoverable?

@luxas luxas changed the title from Kubeadm init diff to Implement `kubeadm init diff` May 16, 2018

liztio added some commits May 16, 2018

Add a `kubeadm upgrade diff` command
This command takes an upgrade version, and shows how the static pod manifests
will be changed by a given upgrade.
path = flags.schedulerManifestPath
default:
glog.Errorf("[diff] unknown spec %v", spec)
continue

This comment has been minimized.

@neolit123

neolit123 May 16, 2018

Member

is it OK to skip unknown specs here?
should we return fmt.Errorf() instead.

This comment has been minimized.

@liztio

liztio May 16, 2018

Member

I'd prefer to err on the side of not breaking this for users. This shouldn't come up, but in case it does I don't want it to break upgrade diff altogether.

This comment has been minimized.

@neolit123

neolit123 May 16, 2018

Member

sounds good.

@k8s-ci-robot k8s-ci-robot added the lgtm label May 16, 2018

@luxas

luxas approved these changes May 16, 2018

/lgtm
/approve

Thanks Liz, this is a very useful UX improvement 🎉!

@liztio

This comment has been minimized.

Member

liztio commented May 16, 2018

/assign @pwittrock
this is just boilerplate it looks like, but the tests grumbled if I didn't add it

@luxas

This comment has been minimized.

Member

luxas commented May 16, 2018

/retest

I'm just gonna add it manually, makes no sense to have a dedicated approver for that old stuff we don't care about anymore

@luxas luxas added the approved label May 16, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: liztio, luxas

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented May 17, 2018

Automatic merge from submit-queue (batch tested with PRs 63865, 57849, 63932, 63930, 63936). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 2fda6e5 into kubernetes:master May 17, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation liztio authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@luxas luxas changed the title from Implement `kubeadm init diff` to Implement `kubeadm upgrade diff` May 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment