-
Notifications
You must be signed in to change notification settings - Fork 309
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
Add a blog post to summarize the issue in 3.5 to 3.6 upgrade #974
Conversation
6d4da0b
to
55613bc
Compare
also cc @neolit123 |
Should user care at all if v3.6 will come with a complete fix and no user action is needed? or maybe I didn't understood etcd-io/etcd#19636 |
It answers the question " |
Also it's part of the fixes. The summary should be complete. All the links are just for experienced users references. I don't think normal users will understand the details of the PRs without expertise of etcd. It's exactly the reason why we need to ensure the blog is clear and easy to understand. Also we have a "TL; DR" section for entry-level users or anyone who don't care about the details. |
With etcd-io/etcd#19636 would it just work? |
Obviously it won't work. Please see the the first case as mentioned in |
To be clearer, the etcd-io/etcd#19636 is only to address the second case as mentioned in the two possible cases if users still upgrade from 3.5.1-3.5.19 to 3.6.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny grammatical change, but also I think we really should change the title to be more grabbing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ahrtr !! I have added few small comments inline.
55613bc
to
8701d69
Compare
942757c
to
8db3f37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for quickly addressing the review comments @ahrtr !!
etcd-io/etcd#19636 would just work if v2store snapshot is saved. If not, we have to assume the v3store is correct, and prevent too many learners. |
Oh, I forgot that in v3.6 the etcd will bootstrap membership from v3 storage. Sorry, I'm 100% focused on KubeCon. This means that etcd might have incorrect information until fully we process local WAL. With etcd-io/etcd#19636 in wouldn't a complete fix just require on more step? When patching storage during bootstrap also read WAL for any PromoteLearner proto, and we are done? No need to communicate, no need for user action, etc. |
Yes. It's exactly the reason why we sync the v3store not only on bootstrap (inside
Right, no further action is needed from users. We just need to clarify it in the blog to avoid any potential confusion or questions. Overall, users are only required to upgrade to 3.5.20 (or a higher version) before upgrading to 3.6.0. We also clarify what will happen if users do not follow this guide (we guarantee that we won't silently swallow any errors). |
it was discovered in Kubernetes' workflow test. To address this gap, we added a similar e2e | ||
test via [19634][], which was also backported to release-3.6 via [19662][]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
it was discovered in Kubernetes' workflow test. To address this gap, we added a similar e2e | |
test via [19634][], which was also backported to release-3.6 via [19662][]. | |
it was discovered in Kubernetes' workflow test. To address this gap, we added a Kubernetes style upgrade e2e | |
test via [19634][], which was also backported to release-3.6 via [19662][]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try not to create new concept "Kubernetes style upgrade
" to avoid any unnecessary confusion/questions, also avoid wordy explanation.
I used "Kubeadm style upgrade
" in the description of etcd-io/etcd#19557. It's OK in that informal case, but suggest not for this formal (and to be widely broadcasted) blog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left a comment with a minor nit. Thanks, @ahrtr.
Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
8db3f37
to
d49f19c
Compare
@jberkus we will release etcd v3.6.0-rc.3 tonight. I am going to merge this PR right after the release being out. Do you have any further comments? |
Nope, it's good to go /hold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, ivanvc, jberkus, neolit123, siyuanfoundation, spzala 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 |
/unhold v3.6.0-rc.3 is out :) |
Thanks. @ivanvc |
Thanks all for the review. Do you think we should broadcast this blog now (given v3.6.0-rc.3 is already out) or wait until v3.6.0 is out? @fuweid @jberkus @jmhbnz @ivanvc @serathius @spzala @wenjiaswe |
Considering the blog post is public and upgrade to rc versions can potentially hit the issue covered in the blog, I think it's probably a good idea to broadcast (after KubeCon). It can also serve as a good reminder/heads-up for v3.6.0, and at that time we can amplify more. Just thoughts if others agree. Thanks @ahrtr !! |
The blog is to summarize the upgrade issue etcd-io/etcd#19557. We need to broadcast the blog post to increase awareness, so that users take action before upgrading to v3.6.0[-rc.x].
cc @fuweid @ivanvc @jberkus @jmhbnz @serathius @siyuanfoundation @spzala @wenjiaswe