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.

Sign up for GitHub

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

Jump to bottom

RFE: backup manifest dir gets wiped after upgrade #489

Closed
jipperinbham opened this issue Oct 10, 2017 · 15 comments
Closed

RFE: backup manifest dir gets wiped after upgrade #489

jipperinbham opened this issue Oct 10, 2017 · 15 comments
Assignees
@jipperinbham @stealthybox @liztio
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone
v1.11

Comments

@jipperinbham
Copy link

jipperinbham commented Oct 10, 2017 •
edited
Loading

What keywords did you search in kubeadm issues before filing this one?

  • temp
  • tmp

BUG REPORT

Versions

kubeadm version (use kubeadm version): 1.8.0

Environment:

  • Kubernetes version (use kubectl version): 1.7.6
  • Cloud provider or hardware configuration: aws

What happened?

While watching TGIK, @jbeda ran sudo kubeadm upgrade apply v1.8.0 and the upgrade ran successfully but the previous manifests in /etc/kubernetes/tmp were deleted.

What you expected to happen?

The general comments were the previous manifests should have been kept around even with a successful upgrade.

How to reproduce it (as minimally and precisely as possible)?

  • sudo kubeadm upgrade apply v1.8.0
  • ls -alh /etc/kubernetes/tmp

Anything else we need to know?

This looks to be the expected behavior per https://github.com/kubernetes/kubernetes/blob/73d1b38604d6f39ec77a94f0f411a46c765d1c4f/cmd/kubeadm/app/phases/upgrade/staticpods.go#L172 so the question is whether the current behavior needs to be altered.
@jbeda
Copy link

jbeda commented Oct 10, 2017

Thanks for filing the issue! It was on my list to follow up.

@luxas -- I think you wrote this code. What do you think about leaving the old manifests there so that users can see what changed. It also enables a (dangerous) rollback path.

@luxas
Copy link
Member

luxas commented Oct 10, 2017

If you think the command should "leak" directories for the user -- feel free to change.
I thought that if the user actually wants to do that -- he/she would just do cp -r /etc/kubernetes{,-preupgrade} before kubeadm upgrade
Documentation improvement or code change?

jipperinbham added a commit to jipperinbham/kubernetes that referenced this issue Dec 12, 2017
@jipperinbham
add note prior to interactive confirm of kubeadm upgrade apply
73108a3 
some users may want to do a comparison of the changes between versions but not think to copy off the existing config files prior to running the upgrade

addresses kubernetes/kubeadm#489
@jipperinbham jipperinbham mentioned this issue Dec 12, 2017
Closed
@jipperinbham
Copy link
Author

@luxas @jbeda I submitted kubernetes/kubernetes#57116 as an attempt to address the concern here. If y'all think it's better to go the route of adding verbiage to the upgrade docs, I'm happy to go that route instead.

@luxas
Copy link
Member

luxas commented Dec 25, 2017

@jipperinbham Thanks! I think more documentation would be better 👍

@luxas luxas added documentation/improvement kind/documentation Categorizes issue or PR as related to documentation. labels Dec 25, 2017
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 25, 2018
@timothysc
Copy link
Member

/assign @liztio

@timothysc timothysc added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 7, 2018
@timothysc timothysc added this to the v1.11 milestone Apr 7, 2018
@stealthybox
Copy link
Member

The manifests are a pretty insignificant amount of data.
I think we gain more by having a longterm backup directory than deleting a temp dir.

@timothysc mentioned it would also be useful to store the old kubeadm-config as a file in the backupdir

We'd probably store the etcd backup in the same directory?
This is significantly more data to store.

related #651

@timothysc
Copy link
Member

We should definitely outline best practices as part of an upgrade, but as we mentioned on the call rollback will become unsupportable for HA with the current methods and I think kubeadm overstepped here.

@jipperinbham
Copy link
Author

Sorry for the lack of feedback here, I think some of the original thoughts behind this issue revolved around providing the user some way of previewing the changes prior to actually running the upgrade.

Something I've enjoyed when using kops is the delta it provides when running kops update cluster ... as can be seen in this gist.

I know being able to provide some sort of preview delta is a much bigger change for kubeadm. If a current goal is around outlining best practices and providing good feedback to the user, then it may or may not be something worth pursuing.

@timothysc timothysc added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/feature Categorizes issue or PR as related to a new feature. and removed documentation/improvement kind/documentation Categorizes issue or PR as related to documentation. labels Apr 26, 2018
@timothysc timothysc changed the title backup manifest dir gets wiped after upgrade RFE: backup manifest dir gets wiped after upgrade Apr 26, 2018
@stealthybox
Copy link
Member

@jipperinbham it's a considerable change for UX, but the technical parts are already there to support what you're talking about.
We already render the upgrade manifests beforehand. We would just have to call a diff.
It should definitely be split out into more phases.
Then it would just be a matter of moving it into the plan vs. upgrade.

@timothysc timothysc added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label May 14, 2018
@luxas
Copy link
Member

luxas commented May 14, 2018

FWIW; we kinda already support this "preview" feature with upgrade --dry-run

@timothysc
Copy link
Member

So the following makes a lot of sense to me:

  1. Add a diff to e.g. upgrade plan --diff
  2. Create a backup timestamped directory for backing up manifests on every upgrade.
  3. Don't backup etcd in the HA use case b/c a rollback of the directory could be bad (docs).

@liztio liztio mentioned this issue May 16, 2018
Merged
@liztio liztio removed the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label May 16, 2018
@liztio
Copy link

liztio commented May 16, 2018

I'm not going to have time to get to 2 or 3 before I leave for holiday.

here's where the manifests are deleted
here's where the directory names are created

k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue May 17, 2018
Merge pull request #63930 from liztio/kubeadm-init-diff
2fda6e5 
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Implement `kubeadm init diff`

**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](kubernetes/kubeadm#489 (comment))

**Special notes for your reviewer**:

**Release note**:

```release-note
adds the `kubeadm upgrade diff` command to show how static pod manifests will be changed by an upgrade.
```
@luxas luxas mentioned this issue May 17, 2018
Closed
@liztio liztio added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label May 21, 2018
@liztio
Copy link

liztio commented May 21, 2018

going to resume this now that we have more time until freeze

@liztio liztio mentioned this issue May 25, 2018
Merged
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue May 31, 2018
Merge pull request #64337 from liztio/save-etcd-dir
9fee771 
Automatic merge from submit-queue (batch tested with PRs 62460, 64480, 63774, 64540, 64337). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Save kubeadm manifest backup directories

**What this PR does / why we need it**:

Kubeadm will now preserves previous manifests after `kubeadm upgrade apply`. Previously these files would be deleted after the upgrade succeeded

**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 # [kubeadm/489](kubernetes/kubeadm#489)

**Special notes for your reviewer**:

**Release note**:

```release-note
kubeadm now preserves previous manifests after upgrades
```
@liztio
Copy link

liztio commented Jun 1, 2018

#2 and #3 finished by #64337

@liztio liztio closed this as completed Jun 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jipperinbham jipperinbham

@stealthybox stealthybox

@liztio liztio

Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Milestone
v1.11
Development

Successfully merging a pull request may close this issue.

add note prior to interactive confirm of kubeadm upgrade apply jipperinbham/kubernetes
8 participants
@jbeda @timothysc @jipperinbham @stealthybox @luxas @k8s-ci-robot @fejta-bot @liztio