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 ha upgrade #66973

Merged
merged 2 commits into from Aug 23, 2018

Conversation

@fabriziopandini
Contributor

fabriziopandini commented Aug 3, 2018

What this PR does / why we need it:
This PR implements one of the actions defined by kubernetes/kubeadm#751 (checklist form implementing HA in kubeadm). see KEP 0015 for more context

With this PR, kubeadm implements a new command kubeadm upgrade node experimental-control-plane that managed upgrade of control plane components on a secondary control plane instance.

The entire workflow in case of HA clusters will be:

  • Upgrade the control plane
    • run kubeadm upgrade apply on a first control plane instance
    • run kubeadm upgrade node experimental-control-plane on secondary control plane instances
  • Upgrade nodes

Special notes for your reviewer:
/CC @timothysc @luxas @chuckha @kubernetes/sig-cluster-lifecycle-pr-reviews

Release note:

kubeadm now has the `kubeadm upgrade node experimental-control-plane` command for upgrading secondary control plane instances created with `kubeadm join --experimental-control-plane`. 
@fabriziopandini

This comment has been minimized.

Show comment
Hide comment
@fabriziopandini

fabriziopandini Aug 13, 2018

Contributor

/test pull-kubernetes-integration

Contributor

fabriziopandini commented Aug 13, 2018

/test pull-kubernetes-integration

@chuckha

First pass, some consistency and language comments.

Show outdated Hide outdated cmd/kubeadm/app/cmd/upgrade/node.go
Show outdated Hide outdated cmd/kubeadm/app/cmd/upgrade/node.go
Show outdated Hide outdated cmd/kubeadm/app/cmd/upgrade/node.go
Show outdated Hide outdated cmd/kubeadm/app/cmd/upgrade/node.go
Show outdated Hide outdated cmd/kubeadm/app/cmd/upgrade/node.go
Show outdated Hide outdated cmd/kubeadm/app/phases/upgrade/postupgrade.go
Show outdated Hide outdated cmd/kubeadm/app/phases/upgrade/staticpods.go
@fabriziopandini

This comment has been minimized.

Show comment
Hide comment
@fabriziopandini

fabriziopandini Aug 14, 2018

Contributor

@chuckha many thanks for the feedbacks!
Almost everything is addressed; some comments inline

Contributor

fabriziopandini commented Aug 14, 2018

@chuckha many thanks for the feedbacks!
Almost everything is addressed; some comments inline

@fabriziopandini

This comment has been minimized.

Show comment
Hide comment
@fabriziopandini

fabriziopandini Aug 15, 2018

Contributor

@chuckha @stealthybox everything addressed now!

Contributor

fabriziopandini commented Aug 15, 2018

@chuckha @stealthybox everything addressed now!

@chuckha

This comment has been minimized.

Show comment
Hide comment
@chuckha

chuckha Aug 17, 2018

Member

thanks for the updates!

/lgtm

Member

chuckha commented Aug 17, 2018

thanks for the updates!

/lgtm

@timothysc

I'm confused by why we need this option vs. automating it away. The UX from the consumer perspective would be confusing.

}
cmd := &cobra.Command{
Use: "experimental-control-plane",

This comment has been minimized.

@timothysc

timothysc Aug 17, 2018

Member

Why would we call out the upgrade path for a HA control plan node differently than the master? I think we should detect from on cluster config and just roll with the upgrade.?.?.?

@timothysc

timothysc Aug 17, 2018

Member

Why would we call out the upgrade path for a HA control plan node differently than the master? I think we should detect from on cluster config and just roll with the upgrade.?.?.?

This comment has been minimized.

@chuckha

chuckha Aug 20, 2018

Member

That's fair feedback. I think this keeps the code simpler at the expense of the user. For an experimental thing I think that's fine. Once it becomes not experimental any more I'd fully expect this code to merge with the existing upgrade code. I see your point though and either we do extra work now to integrate it or integrate it later.

@chuckha

chuckha Aug 20, 2018

Member

That's fair feedback. I think this keeps the code simpler at the expense of the user. For an experimental thing I think that's fine. Once it becomes not experimental any more I'd fully expect this code to merge with the existing upgrade code. I see your point though and either we do extra work now to integrate it or integrate it later.

This comment has been minimized.

@timothysc

timothysc Aug 20, 2018

Member

I'm fine with calling this good enough for now, so long as there is a //TODO plus issue opened.

@timothysc

timothysc Aug 20, 2018

Member

I'm fine with calling this good enough for now, so long as there is a //TODO plus issue opened.

@timothysc

This comment has been minimized.

Show comment
Hide comment
@timothysc

timothysc Aug 21, 2018

Member

/test pull-kubernetes-e2e-kops-aws

Member

timothysc commented Aug 21, 2018

/test pull-kubernetes-e2e-kops-aws

@timothysc

/approve

Please file a separate UX issue.

@timothysc

This comment has been minimized.

Show comment
Hide comment
@timothysc

timothysc Aug 22, 2018

Member

/test pull-kubernetes-e2e-kops-aws

Member

timothysc commented Aug 22, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 22, 2018

Contributor

New changes are detected. LGTM label has been removed.

Contributor

k8s-ci-robot commented Aug 22, 2018

New changes are detected. LGTM label has been removed.

@fabriziopandini

This comment has been minimized.

Show comment
Hide comment
@fabriziopandini

fabriziopandini Aug 22, 2018

Contributor

Echoing
/lgtm
After rebase

@timothysc Issue for UX improvement created! kubernetes/kubeadm#1071

Contributor

fabriziopandini commented Aug 22, 2018

Echoing
/lgtm
After rebase

@timothysc Issue for UX improvement created! kubernetes/kubeadm#1071

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 22, 2018

Contributor

@fabriziopandini: you cannot LGTM your own PR.

In response to this:

Echoing
/lgtm
After rebase

@timothysc Issue for UX improvement created! kubernetes/kubeadm#1071

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Contributor

k8s-ci-robot commented Aug 22, 2018

@fabriziopandini: you cannot LGTM your own PR.

In response to this:

Echoing
/lgtm
After rebase

@timothysc Issue for UX improvement created! kubernetes/kubeadm#1071

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@timothysc timothysc added the approved label Aug 22, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 22, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

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

Contributor

k8s-ci-robot commented Aug 22, 2018

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

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

@timothysc timothysc added the lgtm label Aug 22, 2018

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 22, 2018

Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

Contributor

k8s-merge-robot commented Aug 22, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 23, 2018

Contributor

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

Contributor

k8s-merge-robot commented Aug 23, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit 74f4448 into kubernetes:master Aug 23, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation fabriziopandini 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-e2e-kubeadm-gce Skipped
pull-kubernetes-integration 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

@fabriziopandini fabriziopandini deleted the fabriziopandini:kubeadm-ha-upgrade branch Sep 6, 2018

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