Skip to content

kubeadm-upgrade.md: add more details for upgrade rollbacks#18424

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
neolit123:1.18-kubeadm-ugprade-rollback
Jan 17, 2020
Merged

kubeadm-upgrade.md: add more details for upgrade rollbacks#18424
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
neolit123:1.18-kubeadm-ugprade-rollback

Conversation

@neolit123
Copy link
Copy Markdown
Member

Introduce a couple of paragraphs explaining what backup folders kubeadm creates during upgrade.

note: this is not a new feature, thus i'm sending it to the master branch here.

fixes kubernetes/kubeadm#1694

/assign @fabriziopandini @jimangel
/sig cluster-lifecycle
/kind cleanup

@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jan 2, 2020
@neolit123 neolit123 force-pushed the 1.18-kubeadm-ugprade-rollback branch from b5143f0 to 9b31bd1 Compare January 2, 2020 19:05
Copy link
Copy Markdown
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

@neolit123 thanks
Only one curiosity, but nothing blocking


`kubeadm-backup-etcd` contains a backup of the local etcd member data for this control-plane Node.
In case of an etcd upgrade failure and if the automatic rollback does not work, the contents of this folder
can be manually restored in `/var/lib/etcd`. In case of HA setup with multiple local etcd members
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we skipping the etcd backed in case of multi-controlplane... I don't remember this detail 😳

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

before writing this update, digging the source code i found that we clean the etcd local member backup in case of external etcd or HA:
https://github.com/kubernetes/kubernetes/blob/ce2102f3637134519ec189f096da5277af6072a6/cmd/kubeadm/app/phases/upgrade/staticpods.go#L595

https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/upgrade/staticpods.go#L102

https://github.com/kubernetes/kubernetes/blob/ce2102f3637134519ec189f096da5277af6072a6/cmd/kubeadm/app/phases/upgrade/staticpods.go#L182

In case of HA setup with multiple local etcd members

perhaps i need to clarify that the backup is always written, but after upgrade:

  • the folder contents will be cleaned for HA setup.
  • the folder contents will persist for single control-plane setup.

or maybe we need to change this logic and always keep the backup.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that making consistent how we managed stacked etcd (HA or not) makes more sense

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this has to be a k/k PR.
in my opinion we should always persist the etcd backup for stacked.

i'm inclined to remove the last sentence from this k/website PR:

In case of HA setup with multiple local etcd members ...

WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok to remove the last sentence and open an issue (a PR) to track the change on update

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@fabriziopandini
i've updated the PR to only have the "external etcd" part of the sentence.
created kubernetes/kubeadm#1998 to track the k/k cleanup/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

/hold
until the discussion in kubernetes/kubeadm#1998 is resolved.

Introduce a couple of paragraphs explaining what backup folders
kubeadm creates during upgrade.
@neolit123 neolit123 force-pushed the 1.18-kubeadm-ugprade-rollback branch from 9b31bd1 to 70a48b7 Compare January 5, 2020 22:56
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 6, 2020
@sftim
Copy link
Copy Markdown
Contributor

sftim commented Jan 16, 2020

I'm going to
/approve

this so that a future /lgtm and /hold cancel will let it go live.

@sftim
Copy link
Copy Markdown
Contributor

sftim commented Jan 16, 2020

Alternatively, /close it it the decision goes another way.

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Jan 16, 2020
@neolit123
Copy link
Copy Markdown
Member Author

neolit123 commented Jan 17, 2020

/hold cancel
looking for a LGTM on this.

the following PR is merging so that we now have the update in this current docs PR accurate:
kubernetes/kubernetes#86861
/cc @SataQiu

@k8s-ci-robot k8s-ci-robot requested a review from SataQiu January 17, 2020 00:29
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2020
@SataQiu
Copy link
Copy Markdown
Member

SataQiu commented Jan 17, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit 08ce770 into kubernetes:master Jan 17, 2020
wawa0210 pushed a commit to wawa0210/website that referenced this pull request Mar 2, 2020
…s#18424)

Introduce a couple of paragraphs explaining what backup folders
kubeadm creates during upgrade.
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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. language/en Issues or PRs related to English language 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. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

include more details about the kubeadm upgrade backup process

6 participants