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

Rewrite the etcd doc to be about upgrading etcd. #2767

Merged
merged 1 commit into from
Mar 24, 2017

Conversation

mml
Copy link
Contributor

@mml mml commented Mar 10, 2017

First draft, nearly verbatim copy. Please rip to shreds.


This change is Reviewable

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 10, 2017
@mml mml force-pushed the etcd! branch 3 times, most recently from 8b84e6d to b6d2900 Compare March 10, 2017 23:50
@wojtek-t
Copy link
Member

Just some minor comments.

@wojtek-t
Copy link
Member

@mml ping

@mml
Copy link
Contributor Author

mml commented Mar 15, 2017

@wojtek-t I don't see any comments from you.


* This is a custom tool, which means it doesn’t have any guarantees and only
best-effort support from etcd team (though they fixed some bugs that we found
in the meantime).
Copy link
Member

Choose a reason for hiding this comment

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

It would be useful to link to this script here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

## Default configuration
Going forward, the docker image for etcd in version X will contain multiple
versions of etcd. For example 3.0.14 image will contain all: 2.2.1, 2.3.7 and
3.0.14 binaries of etcd and etcdctl. This will allow running etcd in multiple
Copy link
Member

Choose a reason for hiding this comment

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

s/3.0.14/3.0.17/

everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


## Configuration: high-level goals
Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave this section - it seems useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I made it a subsection of "About etcd" and renamed it.

@wojtek-t
Copy link
Member

Sorry - seems I forgot to click submit.


This document describes how to do this migration.

### Etcd upgrade limitations
Copy link

@xiang90 xiang90 Mar 15, 2017

Choose a reason for hiding this comment

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

etcd upgrade limitations. (do not use Etcd, we prefer etcd :P)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :-). But I will defer to whatever our style guide is about capitalizing words like etcd when they start a heading or sentence.

Copy link

Choose a reason for hiding this comment

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

Where is the rule about capitalizing words in the style guide? I cannot find it here: https://kubernetes.io/docs/contribute/style-guide/#documentation-formatting-standards.


### Etcd upgrade limitations

The way etcd was designed introduces some significant limitations on how the
Copy link

Choose a reason for hiding this comment

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

i do not think this is a design issue to be honest. we have done quite some internal work to allow removing these limitations from very beginning. we just do not know the scope of the use case and the effort. We should resolve it here:

etcd-io/etcd#7308

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer a different phrasing? Regardless of why, the limitations are there.


#### No rollback

Etcd doesn’t support rollback procedure at all (by design). That means, that if
Copy link

@xiang90 xiang90 Mar 15, 2017

Choose a reason for hiding this comment

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

use etcd instead of Etcd?

@xiang90
Copy link

xiang90 commented Mar 15, 2017

This doc looks good to me in general.

etcd team is OK to support rollback once etcd-io/etcd#7308 is resolved.

This is a very huge limitation - we really need a rollback procedure if we face
some problems with the new release.

To make things better, we were provided a custom rollback tool by CoreOS (etcd
Copy link

Choose a reason for hiding this comment

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

to be clear, this is a offline data rollback tool with very limited scope. it probably only works for v2 to v3 upgrades for k8s.

@ravilr
Copy link

ravilr commented Mar 16, 2017

@mml @wojtek-t can we also add some notes around how to deal with running pods in the cluster. I understand that the etcd migration is offline, during which kube-apiserver won't be able to talk to etcd. What happens to the already running pods in the cluster. Would the node controller evict nodes/pods due to lack of heartbeats as soon as the etcd is available but before the nodes starts heartbeating again. How to avoid affecting running pods in the cluster during this migration.

[etcd](https://coreos.com/etcd/docs/latest/) is a highly-available key value
store which Kubernetes uses for persistent storage of all of its REST API
objects.
objects. For the mechanics behind how kubernetes builds, distributes and
deploys Kubernetes, see _some doc_.
Copy link

Choose a reason for hiding this comment

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

did you mean 'how kubernetes builds,.. deploys etcd'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, fixed.

patch releases you can switch versions at any time back and forth). That
basically means, that we cannot upgrade directly e.g. from 2.1.x to 2.3.x.

Fortunately, this limitation it is easy to workaround - it is enough to start a
Copy link
Member

Choose a reason for hiding this comment

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

I think no comma after Fortunately, and s/it is/is/g

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comma belongs, but I'll defer to a "doc review" at some point.

See "sentence adverbs" at http://theeditorsblog.net/2016/02/21/a-tale-of-adverbs-and-the-comma/

I fixed the "it is".

you migrate your cluster e.g. from 2.2.x to 2.3.y, there is no way to get back
to 2.2.x (other than restoring from backup data from the moment when 2.2.x was
running, though if we were running 2.3.y for some time, we will lose all data
that were written in the meantime).
Copy link
Member

Choose a reason for hiding this comment

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

may be gramatically less controversial to say "all data written in the meantime"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

running, though if we were running 2.3.y for some time, we will lose all data
that were written in the meantime).

This is a very huge limitation - we really need a rollback procedure if we face
Copy link
Member

Choose a reason for hiding this comment

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

"very huge" is apt but perhaps not the bestest wording

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to "significant"

This path can be prefixed by using the [kube-apiserver](/docs/admin/kube-apiserver) flag
`--etcd-prefix="/foo"`.
1. Detect which version of etcd we were previously running.
For that purpose, we have added a dedicated (metadata) file that will be
Copy link
Member

Choose a reason for hiding this comment

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

This file is version.txt I believe - should we specify that? (I don't have a strong opinion either way...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I also tidied up the tense there to be more consistent, but I think it's still all over the map across the doc.

The important thing here is that those etcd will not list on the default etcd
port. They are hardcoded to listen on ports which the apiserver is not
configured to connect to, which means that apiserver won’t be able to connect
to those etcd's and no new data will be written in the meantime.
Copy link
Member

Choose a reason for hiding this comment

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

Although it does remain part of the etcd cluster, because we don't change the peer port, I believe, so new data will be written. Should we clarify the "no new data" statement? (And is this actually safe?)

Copy link
Member

Choose a reason for hiding this comment

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

I actually opened an issue about this before I knew about this doc: kubernetes/kubernetes#43364

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are relying on the fact that the port we run on is obscure and "shouldn't" be exposed the way most masters are configured. We should clarify that a bit.

If by safe, we mean we guarantee no one will try to write to it during this transition, then strictly speaking no.

1. Run the migration script
If the previously run version is already in the desired version, this will be
no-op.
1. Start etcd (in the desired version)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should link to the salt manifest, and probably make this more explicit

i.e.

Change "/usr/local/bin/etcd ..." to be
"if [ -e /usr/local/bin/migrate-if-needed.sh ]; then /usr/local/bin/migrate-if-needed.sh 1>>/var/log/etcd.log 2>&1; fi; /usr/local/bin/etcd..."

Copy link
Contributor Author

@mml mml Mar 20, 2017

Choose a reason for hiding this comment

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

Where exactly are you suggesting I link to? (i.e. Can you paste a URL?)

Copy link
Member

Choose a reason for hiding this comment

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

I was suggesting to link to https://github.com/kubernetes/kubernetes/blob/master/cluster/saltbase/salt/etcd/etcd.manifest, as it may be easier to point to an example rather than saying it in words.

```shell
curl -fs -X PUT "http://${host}:${port}/v2/keys/_test"
```
1. Run the migration script
Copy link
Member

Choose a reason for hiding this comment

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

We also need:

  • Specify the image as 3.0.17

  • Specify these env vars for upgrade:

TARGET_STORAGE=etcd3
TARGET_VERSION=3.0.17
DATA_DIRECTORY=???
  • Specify these env vars for downgrade (and clarify that the image should still be 3.0.17, as I made that mistake before engaging brain):
TARGET_STORAGE=etcd2
TARGET_VERSION=2.2.1
DATA_DIRECTORY=???

We also do specify this flag for etcd3 in salt; not sure if it matters: --quota-backend-bytes=4294967296

Copy link
Member

Choose a reason for hiding this comment

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

from kubernetes/kubernetes#43366 (comment):

[downgrading is] supported assuming that:

  1. you are still using 'json' as the storage data format (even though the default is protobuf in 1.6)
  2. you're not downgrading back to 2.2.1 image, but just using "3.0.17" image and setting ETCD_VERSION to 2.2.1

Documentation needs to make it crystal clear that you have to explicitly set --storage-media-type=application/json if you want to have the option to downgrade from etcd3 to etcd2.

It also needs to explain the "use the 3.0.17 image but run in 2.2.1 mode"... I'm not seeing that mentioned in this doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified this, I think. PTAL.

I'd like to rearrange the doc so that the reader can get the recipe first and read the gory details about implementation later, if they wish. Next revision, tho.

@justinsb
Copy link
Member

Great document - thank you!

Q: Is there any reason to stick with the 2.2.1 image? My understanding is that the 3.0.17 image will still run etcd 2.2.1 if TARGET_STORAGE=="etcd2". Sticking with one image would mean we wouldn't need to remember whether we were "etcd2 on a downgrade" or "etcd2 always", which would be great...

@justinsb
Copy link
Member

We should also document that mixing versions of etcd-main and etcd-events does not work (unless there's some magic syntax for etcd-servers-overrides)

in the meantime).

* The rollback can be done only from 3.0.x version (that is using v3 API) to
2.2.1 version (that is using v2 API).
Copy link
Member

Choose a reason for hiding this comment

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

is this accurate? is that what @wojtek-t meant at kubernetes/kubernetes#43366 (comment):

you're not downgrading back to 2.2.1 image, but just using "3.0.17" image and setting ETCD_VERSION to 2.2.1

Copy link
Member

Choose a reason for hiding this comment

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

and only if all apiservers started against the etcd3 server ran with --storage-media-type=application/json, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is accurate, yes. What @wojtek-t meant by that comment was that we don't need to change images. The 3.0.17 image also contains the 2.2.1 binaries (and 2.3.7 and the rollback tool!).

@ethernetdan
Copy link

ethernetdan commented Mar 21, 2017

Has the feedback here been implemented? I'd like to close the issue kubernetes/kubernetes#43366

Copy link

@hongchaodeng hongchaodeng left a comment

Choose a reason for hiding this comment

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

@mml

I think the docs is way too subjective -- mentioning words like "significant limitations on etcd". This breaks the spirit of open source community and not healthy for the project in long term. etcd team have been writing the tool and fixing all issues in fast responsive manner, despite all of this being none of etcd or company's work.

Thus, please correct some wording.

Last but not least, please mention the most important point for users: A good design of etcd cluster deployment should include: 1. have 3 or 5 members for HA; 2. have backup periodically. HA cluster is safe enough. If entire cluster went down, users need to do disaster recovery from backup.


### etcd upgrade limitations

The way etcd was designed introduces some significant limitations on how the

Choose a reason for hiding this comment

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

The way etcd was designed introduces some significant limitations on how the upgrade can be done. These are the main limitations.

I disagree with the wording here. It sounds more like complaints than clarifying the limits to users.

Maybe rephrase like:

There are some limits in etcd upgrade process.


#### One minor release at a time

It is possible to upgrade only by one minor release at a time (though, within

Choose a reason for hiding this comment

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

To emphasize and point out the upgrade path more explicitly, rephrase:

Upgrade only one minor release at a time. That means we cannot upgrade directly e.g. from 2.1.x to 2.3.x . Within patch releases, versions can be switched back and forth.

patch releases you can switch versions at any time back and forth). That
basically means, that we cannot upgrade directly e.g. from 2.1.x to 2.3.x.

Fortunately, this limitation is easy to workaround - it is enough to start a

Choose a reason for hiding this comment

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

Fortunately, this limitation is easy to workaround

cluster for any intermediate minor release, wait until it is healthy and
functional and stop it then. This will do the migration itself underneath.

As an example: to upgrade from 2.1.x to 2.3.y version, it is enough to start

Choose a reason for hiding this comment

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

Combine this with above paragraph.


#### Rollback via special tool

etcd versions through 3.0 don’t support general rollback. That is, in general

Choose a reason for hiding this comment

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

etcd versions through 3.0 3.0+

etcd versions through 3.0 don’t support general rollback. That is, in general
after migrating from M.N to M.N+1, there is no way to go back to M.N.

This is a significant limitation - we really need a rollback procedure if we

Choose a reason for hiding this comment

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

Remove this paragraph. What can users get from reading this paragraph?

This is a significant limitation - we really need a rollback procedure if we
face some problems with the new release.

To make things better, CoreOS has provided a

Choose a reason for hiding this comment

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

For the sake of open source spirit:

CoreOS -> etcd

To test whether `etcd` is running correctly, you can try writing a value to a
test key. On your master VM (or somewhere with firewalls configured such that
you can talk to your cluster's etcd), try:
This section describes how we are going to do the migration given all the

Choose a reason for hiding this comment

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

given all the limitations we have given above conditions.

no-op.
1. Start etcd (in the desired version)

Starting in 1.6, this has been done in the manifests for new GCE clusters. You
Copy link
Member

Choose a reason for hiding this comment

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

Should this be put in the top?
@liggitt for input.

@wojtek-t
Copy link
Member

Has the feedback here been implemented? I'd like to close the issue kubernetes/kubernetes#43366

@ethernetdan - the modification to the doc, yes. Though there was also request to explicitly mention it in release note, and i think it's not yet done.

@jaredbhatti jaredbhatti added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 21, 2017
@jaredbhatti
Copy link
Contributor

This appears to be a 1.6 release-bound item. It's not in the tracking sheet (https://docs.google.com/spreadsheets/d/1nspIeRVNjAQHRslHQD1-6gPv99OcYZLMezrBe3Pfhhg/edit#gid=0) nor is it targeted against the 1.6 brach. Can you please fix these two issues?

@mml
Copy link
Contributor Author

mml commented Mar 22, 2017

@calebamiles #2898 is not targeted for 1.6, so in the interim the current doc will disappear. Is that cool?

Copy link
Member

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

I just have two minor comments.

#### One minor release at a time

Upgrade only one minor release at a time, e.g. we cannot upgrade directly from 2.1.x to 2.3.x.
Within patch releases it is possible to upgrade and downgrade between arbitrary versions.It is fairly
Copy link
Member

Choose a reason for hiding this comment

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

I'm not native speaker, but for me something is missing before "It is fairly easy...". I'm missing some context for this, sth like:
"Fortunately, just starting a cluster for any intermediate minor release, waiting until it is healthy and functional and then shutting it down is itself performing the migration."

WDYT?

* The rollback can be done only from 3.0.x version (that is using v3 API) to
2.2.1 version (that is using v2 API).

* The tool only works if the data is still stored in `application/json` format.
Copy link
Member

Choose a reason for hiding this comment

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

Make it bold?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now called out as a warning per @steveperry-53's style guidance.

@wojtek-t
Copy link
Member

LGTM - but I guess we also need LGTM signal from tech writer.
@devin-donnelly ?

@wojtek-t
Copy link
Member

Also - please squash commits.

@@ -1,49 +1,196 @@
---
Copy link

Choose a reason for hiding this comment

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

@mml @wojtek-t can we change the name of the md file to etcd_upgrade.md to match the title?

I am going to revive the original generic etcd doc soon, which should be named as etcd.md.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not good at naming - I'm fine with that, but will leave that decision to others.

Copy link
Contributor

@calebamiles calebamiles Mar 23, 2017

Choose a reason for hiding this comment

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

I'll add the original etcd.md to this PR @xiang90 and I can make some small changes to update that doc if you'd like. Is that ok with you @devin-donnelly

@chenopis chenopis added this to the 1.6 milestone Mar 23, 2017
[etcd](https://coreos.com/etcd/docs/latest/) is a highly-available key value
store which Kubernetes uses for persistent storage of all of its REST API
objects.
objects. For the mechanics behind how kubernetes builds, distributes and
deploys etcd, see _some doc_.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is some doc just a placeholder for a link? If so, this needs to put in before we can Docs LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a placeholder. I'll just take the sentence out until some doc is ready. I do not want to hold up this PR any further.

@steveperry-53
Copy link
Contributor

@mml, @wojtek-t, See cb07168 for suggested edits.

rollback might require restarting all Kubernetes components on all nodes.

_To verify: It seems that both Kubelet and KubeProxy are using “resource
version” only for watching (i.e. are not using resource versions for anything
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this text under "To verify" meant to be in the document? It almost looks like a review comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I will rephrase that.

@mml
Copy link
Contributor Author

mml commented Mar 24, 2017

@steveperry-53 edits mostly look good, but some of the seemingly excessive bolding is because there are some mistakes users can make here that could cut off their option for rollback or that would lead them into waters where we have not tested. These are charted waters, but the chart is not the territory.

So I guess the question is one of doc style. How do we warn users about something risky and important? This is like those sidebars usually indicated by something like (!) in "for dummies" books.

Thanks!

@steveperry-53
Copy link
Contributor

steveperry-53 commented Mar 24, 2017

@mml, If you feel it's important, go ahead and return some important statements to bold. Usually what we do, instead of bold, is to separate important material as a Note or Warning. We bold the word Note or Warning but not the text. Here is an example. I'll leave the choice up to you. I don't need to review this again.

@steveperry-53 steveperry-53 added Docs LGTM and removed Docs Review: Open Issues do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Mar 24, 2017
@mml
Copy link
Contributor Author

mml commented Mar 24, 2017

@steveperry-53 Thanks, I'll try to restructure things to follow that style. I am re-labeling do-not-merge until I finish those changes and squash.

@mml mml added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 24, 2017
@mml mml added lgtm and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Mar 24, 2017
@mml
Copy link
Contributor Author

mml commented Mar 24, 2017

We are ready to go.

@steveperry-53
Copy link
Contributor

@mml, Would you like me to merge this now?

@mml
Copy link
Contributor Author

mml commented Mar 24, 2017

@steveperry-53 yes, pleas.

@steveperry-53 steveperry-53 merged commit 2b9b002 into kubernetes:release-1.6 Mar 24, 2017
@mml mml deleted the etcd! branch March 24, 2017 22:29

### New etcd Docker image

We decided to completely change the the content of the etcd image and the way it works.
Copy link

Choose a reason for hiding this comment

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

typo: "the the"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.