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: Fix small-ish bugs for v1.11 #64974

Merged
merged 2 commits into from Jun 12, 2018

Conversation

@luxas
Copy link
Member

luxas commented Jun 11, 2018

What this PR does / why we need it:

Fixes a bunch of bugs I noticed when I was reading the source code:

  • --cloud-provider should also be propagated to the kubelet when converting configs from v1alpha1 to v1alpha2
  • The validation for .NodeRegistration.Name is practically non-existent, just verifies the name isn't in upper case. Instead we currently do that validation in preflight checks, which is in the totally wrong place.
  • Now that we pull images in preflight checks, the timeout for the kubelet to start the Static Pods should be kinda short, as it doesn't depend on internet connection
  • I think the shorthand for kubeadm reset --force ought to be -f
  • The common flags between upgrade apply and upgrade plan were registered as global flags for the upgrade command, although they make no sense for upgrade diff and/or upgrade node config. Hence, I moved them to be locally registered.
  • Just because we vendor glog we have a lot of unnecessary/annoying flags registered in glog's init() function. Let's hide these properly.
  • I saw that kubeadm upgrade apply doesn't write down the new kubelet config that should be used, now that is the case. Also, the CRISocket annotation information is now preserved properly on upgrade (and is configurable using the --cri-socket flag)
  • If kubeadm join is run against a v1.10 cluster without the kubelet-config-1.10 configmap, it shouldn't fail.

What I will still investigate:

  • kubeadm token create should have a flag called --ttl, not --token-ttl as it is now (this snuck in in this dev cycle)
  • That --dry-run works properly for upgrade, end to end.

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

Special notes for your reviewer:

Release note:

NONE

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

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented Jun 11, 2018

/assign @fabriziopandini @timothysc
/milestone v1.11
/status approved-for-milestone
/priority critical-urgent
/kind bug

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 11, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@fabriziopandini @luxas @timothysc

Pull Request Labels
  • sig/cluster-lifecycle: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/bug: Fixes a bug discovered during the current release.
Help
@chuckha
Copy link
Member

chuckha left a comment

Not enough context to review the upgrade part right now but I'll read through that source more closely over the next few days.

{"1234", nil, true}, // supported
{"valid-nodename", nil, true}, // supported
{"INVALID-NODENAME", nil, false}, // Upper cases is invalid
{"", "/some/path", true}, // node name can't be empty

This comment has been minimized.

@chuckha

chuckha Jun 11, 2018

Member

These will be easier to debug if run as sub tests (t.Run(...)

This comment has been minimized.

@luxas

luxas Jun 11, 2018

Author Member

If I have time I'll do that, otherwise just leave up to a more general refactor of our tests

@@ -138,6 +138,7 @@ func NewCmdToken(out io.Writer, errW io.Writer) *cobra.Command {
"config", cfgPath, "Path to kubeadm config file (WARNING: Usage of a configuration file is experimental)")
createCmd.Flags().BoolVar(&printJoinCommand,
"print-join-command", false, "Instead of printing only the token, print the full 'kubeadm join' flag needed to join the cluster using the token.")
// TODO: This is wrong, the added flag here should be named --ttl, not --token-ttl

This comment has been minimized.

@chuckha

chuckha Jun 11, 2018

Member

Why is this wrong? The TTL is for the token so --token-ttl makes sense to me. --ttl seems ambiguous

This comment has been minimized.

@timothysc

timothysc Jun 11, 2018

Member

I'm a meh, b/c it's under create [token]

This comment has been minimized.

@luxas

luxas Jun 11, 2018

Author Member

This is for the command kubeadm token create, where --ttl makes sense I think.
Well, at this point it should be --ttl as that has been a thing since v1.6 and we shouldn't break the CLI

@timothysc
Copy link
Member

timothysc left a comment

/approve
/lgtm

I don't see anything obvious.
Please unmark WIP and PR the individual fixes for any subsequent items.

@@ -138,6 +138,7 @@ func NewCmdToken(out io.Writer, errW io.Writer) *cobra.Command {
"config", cfgPath, "Path to kubeadm config file (WARNING: Usage of a configuration file is experimental)")
createCmd.Flags().BoolVar(&printJoinCommand,
"print-join-command", false, "Instead of printing only the token, print the full 'kubeadm join' flag needed to join the cluster using the token.")
// TODO: This is wrong, the added flag here should be named --ttl, not --token-ttl

This comment has been minimized.

@timothysc

timothysc Jun 11, 2018

Member

I'm a meh, b/c it's under create [token]

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 11, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 11, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, 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

@luxas luxas force-pushed the luxas:kubeadm_v111_bugs branch from 44fd72f to 00adec9 Jun 11, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jun 11, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 11, 2018

New changes are detected. LGTM label has been removed.

@luxas luxas added the lgtm label Jun 11, 2018

@luxas luxas force-pushed the luxas:kubeadm_v111_bugs branch from 00adec9 to a4b6fa2 Jun 12, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 12, 2018

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jun 12, 2018

@luxas luxas changed the title [WIP] kubeadm: Fix small-ish bugs for v1.11 kubeadm: Fix small-ish bugs for v1.11 Jun 12, 2018

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented Jun 12, 2018

Fixed golint so now ready to merge

@luxas luxas added the lgtm label Jun 12, 2018

luxas added some commits Jun 12, 2018

@luxas luxas force-pushed the luxas:kubeadm_v111_bugs branch from a4b6fa2 to f126f78 Jun 12, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 12, 2018

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jun 12, 2018

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented Jun 12, 2018

Rebased on top of #64988, race in the merge queue

@timothysc timothysc added the lgtm label Jun 12, 2018

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented Jun 12, 2018

/retest

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented Jun 12, 2018

/retest

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 12, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 5286647 into kubernetes:master Jun 12, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-kops-aws
Details
cla/linuxfoundation luxas 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
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.