rfcs: Finalize the cluster upgrade process automatically. #24377

windchan7 · 2018-03-30T19:50:25Z

This RFC proposes to automate the finalization process to upgrade a cluster.

Release note: None

cockroach-teamcity · 2018-03-30T19:50:30Z

This change is

vilterp · 2018-03-30T19:52:43Z

Formatting nit: try to wrap your lines at 80 characters, as in other RFCs. I think there are editor plugins which do this automatically (though last time I wrote a big markdown doc I did it by hand, annoyingly).

bdarnell · 2018-04-01T16:14:49Z

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 30 at r1 (raw file):

We will likely add the following logic to `pkg/server/server.go`:

- When a node starts, let's call it <b>A</b>, it will poll version information of each node from <b>crdb\_internal.gossip\_nodes</b>.

What is the opt-out? A cluster setting? How do we ensure that we have the right setting when starting up?

docs/RFCS/auto_finalize_cluster_upgrade.md, line 32 at r1 (raw file):

- When a node starts, let's call it <b>A</b>, it will poll version information of each node from <b>crdb\_internal.gossip\_nodes</b>.
- If:
   - every other server's node\_executable\_version is the same as <b>A</b>'s node\_executable\_version.

Formatting nit: Use monospace formatting for node_executable_version and then you won't have to escape the underscores. Use markdown formatting (asterisks instead of <b>, for example) whenever possible.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 50 at r1 (raw file):

- Unreachable Node:
  - We don't guarantee the upgrade will always be successful if there're unreachable nodes.
  - If a node is unreachable, we won't ever upgrade because it could be unsafe if the unreachable node is running in a lower version.

If a node is unreachable, it won't show up in gossip, so how would the surviving nodes know not to upgrade?

I think we need to add a condition that there are no under-replicated ranges.

Comments from Reviewable

bdarnell · 2018-04-01T22:04:07Z

I believe that we should auto-upgrade clusters by default. I also believe that our docs should recommend disabling the auto-upgrade mechanism (the auto-upgrade is to protect users who don't read the docs).

The mechanism to opt out of automatic upgrades should be on a per-version basis. If I follow the docs and upgrade from 2.0 to 2.1 with auto-upgrade disabled, that shouldn't prevent auto-upgrades when I go to 2.2 (when I may just do a rolling upgrade without reading the docs).

benesch · 2018-04-02T04:54:14Z

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 1 at r1 (raw file):

- Feature Name: Finalize CockroachDB's cluster upgrade process automatically

Let's call this "Finalize cluster upgrades automatically"

docs/RFCS/auto_finalize_cluster_upgrade.md, line 4 at r1 (raw file):

- Status: Draft
- Start Date: 2018 4/1
- Authors: Nikhil, Nathan, Victor

Just list yourself here! :)

docs/RFCS/auto_finalize_cluster_upgrade.md, line 5 at r1 (raw file):

- Start Date: 2018 4/1
- Authors: Nikhil, Nathan, Victor
- RFC PR: (PR # after acceptance of initial draft)

Fill in #24377 on the next update.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 22 at r1 (raw file):

in order to finalize the cluster upgrade process.

However, this step is easy to forget and without it, operators are not able to unlock features in the latest version. Therefore, it is necessary to add a version-check mechanism that automatically runs the finalization command once knowing all the nodes in the cluster are running the new version.

Specifically, it unlocks backwards-incompatible features. Backwards compatible features are available without finalizing the upgrade.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 30 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

What is the opt-out? A cluster setting? How do we ensure that we have the right setting when starting up?

Yep. We can consistently read it from system.settings?

docs/RFCS/auto_finalize_cluster_upgrade.md, line 47 at r1 (raw file):

  - As soon as the operator just replaced the last node running v1.1 with a node running v2.0, the auto-finalization will start running the set cluster version command.
  - However, if right before the command get executed, the operator erroneously joined another node running 1.1 version. Then the command get executed and the cluster version is set to be 2.0, with new features enabled.
  - But the node running 1.1 version can not perform some of the 2.0 version operations and it will cause trouble.

I've been thinking more about this race. I think the best way to handle it is by introducing (yet another) version, a handshake_version. Teach nodes to reject RPCs at handshake time when the source's node_executable_version is less than the target's handshake_version.

Then the upgrade process looks like this:

Verify that all known nodes are running the same node_executable_version(), and that that version is greater than the current cluster version.
Ratchet handshake_version up to node_executable_version().
Wait for all known nodes to report that their handshake_version has been updated.
Ensure that no new nodes were added to the cluster. (Can we just poll gossip again, or is that racy? Is polling the node status key ok, or is that racy too?)
Update the cluster version.

I think original credit for this idea belongs to @nvanbenschoten.

@bdarnell, does this overcomplicate matters?

docs/RFCS/auto_finalize_cluster_upgrade.md, line 50 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

If a node is unreachable, it won't show up in gossip, so how would the surviving nodes know not to upgrade?

I think we need to add a condition that there are no under-replicated ranges.

~~What is it, then, that allows us to mark unreachable nodes as "suspect" or "dead" in the admin UI? I.e., where is that state stored?~~

Looks like we write node status keys for every node in the cluster. Could we repurpose those? That is, only upgrade once we've verified that every node with a status key is either running the right version or decommissioned and dead?

Comments from Reviewable

benesch · 2018-04-02T14:03:56Z

Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 47 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I've been thinking more about this race. I think the best way to handle it is by introducing (yet another) version, a handshake_version. Teach nodes to reject RPCs at handshake time when the source's node_executable_version is less than the target's handshake_version.

Then the upgrade process looks like this:

Verify that all known nodes are running the same node_executable_version(), and that that version is greater than the current cluster version.

Ratchet handshake_version up to node_executable_version().

Wait for all known nodes to report that their handshake_version has been updated.

Ensure that no new nodes were added to the cluster. (Can we just poll gossip again, or is that racy? Is polling the node status key ok, or is that racy too?)

Update the cluster version.

I think original credit for this idea belongs to @nvanbenschoten.

@bdarnell, does this overcomplicate matters?

It occurs to me that SET CLUSTER SETTING version should do the handshake_version ratcheting internally, so that manual finalizations have the same safety guarantees as automatic finalizations.

Comments from Reviewable

windchan7 · 2018-04-02T14:36:59Z

I agree that we should auto upgrade by default. However, upgrade on a per-version basis could be confusing to the users.

Let's say user turned auto_upgrade off when updating to 2.1. The auto_upgrade should remain off so users still need to manually run the finalization command when upgrading to 2.2. If we turned it on internally without notifying users and simply say it in the docs, users could treat it as a bug rather than a feature.

We should always make the setting consistent by having a default value and let users flip that around if they want to. @bdarnell

BramGruneir · 2018-04-02T15:26:18Z

This has no mention of testing. Please add some. This might be a large section.

Is there no way to set how long until this finalization step occurs? I don't see any mention of it.
I think having a new cluster setting with a duration AUTO_UPGRADE_AFTER that shows how long after a cluster is fully up (since the last node was added) will give more flexibility. You could turn it off by setting it to 0. We could have a default of 1 hour (or maybe 6?). But it would let operators determine how long to wait for their cluster. If I was running a critical system, I think 24 or even 48 hours would be appropriate.

Also, there needs to be a UI component to this RFC. We need to surface in both the CLI and the Admin UI if a cluster is going to be upgraded and when. Or if it needs to be done manually. If we are going to step it one upgrade at a time, then this should show that.

Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

Comments from Reviewable

windchan7 · 2018-04-02T15:36:02Z

Do you mean give operators a grace period just in case they want to cancel the upgrade? @BramGruneir

BramGruneir · 2018-04-02T15:52:12Z

Yes. Exactly that.

Instead of this being an on/off feature. Have it wait X mins/hours before upgrading. Thus ensuring that the cluster is stable and it give the operators a chance to turn it off, change the length of the time before the auto-upgrade happens or upgrade immediately.

Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

Comments from Reviewable

windchan7 · 2018-04-02T15:57:18Z

If so, setting AUTO_UPGRADE_AFTER to 0 to disable auto upgrade is a bit counter intuitive because it literally means auto upgrade immediately after user replaces the final node.

Maybe set it to -1 to disable this feature? But I'm also wondering if duration could be negative or not.

benesch · 2018-04-02T16:22:32Z

Also, there needs to be a UI component to this RFC. We need to surface in both the CLI and the Admin UI if a cluster is going to be upgraded and when. Or if it needs to be done manually. If we are going to step it one upgrade at a time, then this should show that.

I'd prefer if we tackled the UI in a separate RFC. I don't want the scope of this feature to explode on Victor.

Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

Comments from Reviewable

piyush-singh · 2018-04-02T17:00:50Z

Since we already have an issue for the web UI to warn users for this scenario that we were planning on tackling in 2.1, we could adapt the issue to instead warn them about the impending auto-update (when applicable). To avoid increasing scope (to Nikhil's point) we can coordinate the update on our end with Victor so they happen at the same time. If that works for everyone, I'll go ahead and update the linked issue with the changes we would need based on this RFC.

BramGruneir · 2018-04-02T17:01:11Z

Maybe set it to -1 to disable this feature? But I'm also wondering if duration could be negative or not.

It's not uncommon to use 0 to indicate that the feature is turned off, we just need to describe it in the notes. And looking at a negative time might seem weird. We could also set the minimum wait to 1h or 30min or something, to further not confuse people. If they wanted to upgrade right away, they can manually run the upgrade command.

I'd prefer if we tackled the UI in a separate RFC. I don't want the scope of this feature to explode on Victor.
I disagree. The UI (both CLI and Admin UI) changes should be scoped with the project. Both are not very big and we should be encouraging all engineers to work full stack. We leave the admin UI out too many times.

More than that, there should also be a function to find out how long until the upgrade occurs. This would be useful for both UIs.

Comments from Reviewable

bdarnell · 2018-04-02T19:31:47Z

Let's say user turned auto_upgrade off when updating to 2.1. The auto_upgrade should remain off so users still need to manually run the finalization command when upgrading to 2.2. If we turned it on internally without notifying users and simply say it in the docs, users could treat it as a bug rather than a feature.

I think this is just a question of how the setting is worded. It's true that it would look buggy if they said set auto_upgrade = off and we just reversed it. But if the command looked something like use manual upgrade for 2.1, then it wouldn't be surprising when it only affected that version and needed to be repeated in the next upgrade.

I think that making the opt-out of auto-upgrade permanent instead of per-release will lead to more surprises since people will forget how they set it between releases.

We could have a default of 1 hour (or maybe 6?).

I'm not sure a time-based auto-update makes sense. If it doesn't auto-upgrade as soon as the rolling restart is finished, users will still be confused that their new features aren't working yet. A few hours doesn't buy you much time to roll back (you at least want to see how the new version performs under your daily peak load before you commit, right?)

Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 30 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Yep. We can consistently read it from system.settings?

Ah, right. This isn't at early startup like our store-level migrations; it can wait until the cluster is up and able to serve consistent reads.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 47 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

It occurs to me that SET CLUSTER SETTING version should do the handshake_version ratcheting internally, so that manual finalizations have the same safety guarantees as automatic finalizations.

Introducing a new cluster version variable seems over-complicated to me. Can we do this by making the checks in rpc.checkVersion stricter? It currently enforces the rule that 2.0 binaries can't talk to anything older than 1.1, but it doesn't appear to enforce the rule that you can't talk to anything older than your cluster version.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 50 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

~~What is it, then, that allows us to mark unreachable nodes as "suspect" or "dead" in the admin UI? I.e., where is that state stored?~~

Looks like we write node status keys for every node in the cluster. Could we repurpose those? That is, only upgrade once we've verified that every node with a status key is either running the right version or decommissioned and dead?

Yeah, that could do it. It would make decommissioning mandatory in a way that it's not today, but it probably makes sense for this use case.

Comments from Reviewable

benesch · 2018-04-02T20:29:15Z

@windchan7 let's find some time tomorrow to go through this feedback!

Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 47 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Introducing a new cluster version variable seems over-complicated to me. Can we do this by making the checks in rpc.checkVersion stricter? It currently enforces the rule that 2.0 binaries can't talk to anything older than 1.1, but it doesn't appear to enforce the rule that you can't talk to anything older than your cluster version.

Doesn't it already do that? rpc.checkVersion is comparing the peer's version to clusterVersion.Version().MinimumVersion. Isn't that the current value of the version cluster setting?

Anyway, let's suppose rpc.checkVersion is indeed looking at the version cluster setting. Here's the race I see:

Node A (v2.0) executes SET CLUSTER SETTING version = '2.0'.
Node A starts using new 2.0 behavior and writes a table descriptor that only a 2.0 node can understand.
Node C (v1.1) contacts node B (v2.0) and asks to join the cluster. Node B, not knowing the cluster version is now 2.0, agrees to let node C join the cluster.
Node C inadvertently erases the 2.0 features in a table descriptor that node A just wrote.
Node B gets the gossip update and bumps its knowledge of the minimum version.
Node B terminates its connection with Node C, but the damage is done.

I'm not sure how we get around this without a separate version that limits what nodes you can talk to without actually turning on the backwards-incompatible features.

Comments from Reviewable

windchan7 · 2018-04-04T15:05:33Z

I had a discussion with Nikhil yesterday and agreed that use manual upgrade for version syntax makes more sense. So I come up with a second draft that modified the check version mechanism, added testing and UI components.

bdarnell · 2018-04-04T20:59:35Z

Review status: 0 of 1 files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 47 at r1 (raw file):

Isn't that the current value of the version cluster setting?

Yeah, I think so. I was getting it mixed up with the constant BinaryMinimumSupportedVersion.

I'm not sure how we get around this without a separate version that limits what nodes you can talk to without actually turning on the backwards-incompatible features.

True. A bulletproof solution here would need an extra step here. So far we've just been relying on the fact that new nodes running the old version are unlikely, especially when the upgrade is done manually a significant interval later.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 57 at r2 (raw file):

```sql
use manual/auto upgrade for VERSION;

When I said "use manual upgrade" I didn't necessarily mean that literal syntax. I'm not sure it's worth the complexity of custom syntax here since it's only going to be used when you're copy/pasting from the docs here.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 60 at r2 (raw file):

Add a new field cluster.auto_finalize.disable_at_version

Bikeshed: cluster.preserve_downgrade_option = '1.1' (and RESET CLUSTER SETTING cluster.preserve_downgrade_option).

Comments from Reviewable

benesch · 2018-04-06T19:38:54Z

Code review tip: hit the "Done" button once you've addressed someone's feedback!

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 5 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Fill in #24377 on the next update.

Please linkify these when you get the chance:

- RFC PR: [#24377]
- Cockroach Issue: [#23912], [#22686] (UI)

[#24377]: https://github.com

docs/RFCS/auto_finalize_cluster_upgrade.md, line 47 at r1 (raw file):

True. A bulletproof solution here would need an extra step here. So far we've just been relying on the fact that new nodes running the old version are unlikely, especially when the upgrade is done manually a significant interval later.

Right. So my question is how much do you care about this race?

I'm inclined to call it future work, but perhaps work that we mandate makes it into 2.1. It's looking like the daemon is going to be an isolated chunk of work that blindly runs SET CLUSTER SETTING version = '2.0'. If we want to prevent the race, we can do so in the implementation of SET CLUSTER SETTING version so that manual bumps get the new safety guarantees too.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 57 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

When I said "use manual upgrade" I didn't necessarily mean that literal syntax. I'm not sure it's worth the complexity of custom syntax here since it's only going to be used when you're copy/pasting from the docs here.

Victor and I were unable to come up with a setting name that was sufficiently self-descriptive. If we don't introduce custom syntax I think we'll be stuck with a slightly confusing setting name, but perhaps that's ok.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 60 at r2 (raw file):

cluster.preserve_downgrade_option = '1.1'

cluster.preserve_downgrade_option = '1.1' could be confusing if, say, you're upgrading from 2.0 to 2.1. You might think you can still downgrade to 1.1 when you look at the value of cluster.preserve_downgrade_option, when in fact you'll be auto-upgrading.

I'll toss out cluster.upgrades.prevent_automatic_finalization = '2.0'.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 54 at r3 (raw file):

## Detailed Design

### Add a new field `cluster.preserve_downgrade_option`

s/field/setting/

docs/RFCS/auto_finalize_cluster_upgrade.md, line 110 at r3 (raw file):

- (Maybe) We need a page that shows the version of each node in the cluster
along with the cluster version.

The node list page already shows the node's executable version, I think. We could easily extend this to also show their view of the cluster version.

Comments from Reviewable

benesch · 2018-04-06T19:39:09Z

This is looking really good, Victor!

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

Comments from Reviewable

windchan7 · 2018-04-08T15:42:03Z

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 1 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Let's call this "Finalize cluster upgrades automatically"

Done.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 4 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Just list yourself here! :)

Done.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 22 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Specifically, it unlocks backwards-incompatible features. Backwards compatible features are available without finalizing the upgrade.

Done.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 30 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Ah, right. This isn't at early startup like our store-level migrations; it can wait until the cluster is up and able to serve consistent reads.

Done.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 32 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Formatting nit: Use monospace formatting for node_executable_version and then you won't have to escape the underscores. Use markdown formatting (asterisks instead of <b>, for example) whenever possible.

Done.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 50 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Yeah, that could do it. It would make decommissioning mandatory in a way that it's not today, but it probably makes sense for this use case.

Done.

Comments from Reviewable

bdarnell · 2018-04-09T15:46:16Z

Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 47 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

True. A bulletproof solution here would need an extra step here. So far we've just been relying on the fact that new nodes running the old version are unlikely, especially when the upgrade is done manually a significant interval later.

Right. So my question is how much do you care about this race?

I'm inclined to call it future work, but perhaps work that we mandate makes it into 2.1. It's looking like the daemon is going to be an isolated chunk of work that blindly runs SET CLUSTER SETTING version = '2.0'. If we want to prevent the race, we can do so in the implementation of SET CLUSTER SETTING version so that manual bumps get the new safety guarantees too.

I'm OK with deferring this for future work.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 60 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

cluster.preserve_downgrade_option = '1.1'

cluster.preserve_downgrade_option = '1.1' could be confusing if, say, you're upgrading from 2.0 to 2.1. You might think you can still downgrade to 1.1 when you look at the value of cluster.preserve_downgrade_option, when in fact you'll be auto-upgrading.

I'll toss out cluster.upgrades.prevent_automatic_finalization = '2.0'.

I was thinking that you'd be unable to set the cluster version to 2.0 without clearing the preserve_downgrade_option setting. Or more radically, we could get rid of the (user-facing) SET CLUSTER SETTING version step completely and rely solely on the automatic process. The final step would then be RESET CLUSTER SETTING cluster.preserve_downgrade_option.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 59 at r3 (raw file):

`cluster.preserve_downgrade_option = 2.0`

By default, the value should be `0.0` to indicate we are always using auto

The setting should be a string, not a float (like the version setting), so the default should be an empty string.

Comments from Reviewable

windchan7 · 2018-04-09T15:51:23Z

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 54 at r3 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

s/field/setting/

Do you mind clarify what it means? Thanks Nikhil.

Comments from Reviewable

bdarnell · 2018-04-09T16:05:44Z

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 54 at r3 (raw file):

Previously, windchan7 (Victor Chen) wrote…

Do you mind clarify what it means? Thanks Nikhil.

s/xxx/yyy/ is shorthand for "replace xxx with yyy". (It dates back to ancient unix history (ed(1)) and lives on in tools like sed and vi)

Comments from Reviewable

couchand · 2018-04-09T18:06:57Z

I don't think this RFC needs to address the specifics of the UI implementation, but it would be useful to consider the information that we'll want to surface on the CLI and web UI. I'm sensitive to the concern about scope creep, but I'm still bruised from previous RFCs that played fast & loose with the UI side.

What new status information does this RFC create, and what does the API look like to retrieve this status? From a first pass, it's something like:

have any nodes been upgraded, and how many? (I think we may already have this API, mentioning it for completeness)
is the auto-upgrade disabled or not?
if the auto-upgrade is not disabled:
- has the auto-upgrade begun?
- (if there is a timeout) has the timeout begun, and how much time is left?
if the auto-upgrade is disabled:
- (if reset is not part of the upgrade) have we upgraded but not reset it?

Including this information is critical to making sure this RFC gives a complete view of the process.

Review status: 0 of 1 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

Comments from Reviewable

windchan7 · 2018-04-11T02:07:41Z

Review status: 0 of 1 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 5 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Please linkify these when you get the chance:
- RFC PR: [#24377]
- Cockroach Issue: [#23912], [#22686] (UI)

[#24377]: https://github.com

Done.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 47 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I'm OK with deferring this for future work.

Done.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 57 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Victor and I were unable to come up with a setting name that was sufficiently self-descriptive. If we don't introduce custom syntax I think we'll be stuck with a slightly confusing setting name, but perhaps that's ok.

Done.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 54 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

s/xxx/yyy/ is shorthand for "replace xxx with yyy". (It dates back to ancient unix history (ed(1)) and lives on in tools like sed and vi)

Done.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 59 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

The setting should be a string, not a float (like the version setting), so the default should be an empty string.

Done.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 110 at r3 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

The node list page already shows the node's executable version, I think. We could easily extend this to also show their view of the cluster version.

Done.

Comments from Reviewable

windchan7 · 2018-04-11T03:14:55Z

Done.

Review status: 0 of 1 files reviewed at latest revision, 6 unresolved discussions.

Comments from Reviewable

benesch · 2018-04-11T03:30:27Z

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

docs/RFCS/auto_finalize_cluster_upgrade.md, line 47 at r1 (raw file):

Previously, windchan7 (Victor Chen) wrote…

Done.

Ok, sounds like we're good to go. Victor, can you file an issue to track the future work here, then link it in? ("settings: version upgrade races with booting old node")

docs/RFCS/auto_finalize_cluster_upgrade.md, line 60 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I was thinking that you'd be unable to set the cluster version to 2.0 without clearing the preserve_downgrade_option setting. Or more radically, we could get rid of the (user-facing) SET CLUSTER SETTING version step completely and rely solely on the automatic process. The final step would then be RESET CLUSTER SETTING cluster.preserve_downgrade_option.

Ooh, I like that.

Comments from Reviewable

windchan7 · 2018-04-11T15:17:41Z

bors r+

windchan7 · 2018-04-11T15:36:09Z

bors r-

craig · 2018-04-11T15:36:14Z

Canceled

benesch · 2018-04-11T15:41:33Z

Pending final review! @bdarnell, @couchand, @BramGruneir, thoughts?

couchand · 2018-04-11T15:47:31Z

Didn't look at the whole thing, but the additional detail about the API for upgrade state LGTM

bdarnell · 2018-04-11T17:29:47Z

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

docs/RFCS/20180411_finalize_cluster_upgrade_automatically.md, line 16 at r6 (raw file):

command.

Meanwhile, we will also add a feature feature that allows operators to opt out

s/feature feature/feature/

docs/RFCS/20180411_finalize_cluster_upgrade_automatically.md, line 107 at r6 (raw file):

## UI
- If the user opt out of the auto upgrade, we should have a banner that alert

We should also check for the state when all live nodes are running the new version but some nodes are down and not decommissioned, to alert the operator to the need to either revive or decommission them.

Comments from Reviewable

windchan7 · 2018-04-11T19:31:56Z

I'm finalizing the design, please let me know if you have any other comments.

Issue: cockroachdb#23912, cockroachdb#22686 Release note: None

windchan7 · 2018-04-12T12:58:24Z

bors r+

24377: rfcs: Finalize the cluster upgrade process automatically. r=windchan7 a=windchan7 This RFC proposes to automate the finalization process to upgrade a cluster. Issue: #23912 Release note: None Co-authored-by: Victor Chen <victor@cockroachlabs.com>

craig · 2018-04-12T13:20:46Z

Build succeeded

GitHub CI (Cockroach)

windchan7 requested a review from a team as a code owner March 30, 2018 19:50

windchan7 force-pushed the rfcs branch from 1f14aa2 to ed6e1db Compare March 30, 2018 19:59

bdarnell mentioned this pull request Apr 1, 2018

RFCs: add range merges RFC #24394

Merged

windchan7 force-pushed the rfcs branch from ed6e1db to 4bd2c3b Compare April 4, 2018 14:52

windchan7 force-pushed the rfcs branch from 4bd2c3b to 62847b7 Compare April 5, 2018 14:21

windchan7 force-pushed the rfcs branch from 62847b7 to 33c92f6 Compare April 9, 2018 16:35

windchan7 force-pushed the rfcs branch from 33c92f6 to afb1a23 Compare April 11, 2018 03:14

windchan7 mentioned this pull request Apr 11, 2018

settings: version upgrade races with booting old node #24670

Closed

windchan7 force-pushed the rfcs branch 3 times, most recently from 32720b0 to 9e6e65b Compare April 11, 2018 14:13

windchan7 force-pushed the rfcs branch from 9e6e65b to abae009 Compare April 11, 2018 18:18

windchan7 force-pushed the rfcs branch from abae009 to 14ff960 Compare April 11, 2018 20:57

This RFC propoases to finalize cluster upgrade automatically.

2dd9c6b

Issue: cockroachdb#23912, cockroachdb#22686 Release note: None

windchan7 force-pushed the rfcs branch from 14ff960 to 2dd9c6b Compare April 12, 2018 03:46

craig bot merged commit 2dd9c6b into cockroachdb:master Apr 12, 2018

rfcs: Finalize the cluster upgrade process automatically. #24377

rfcs: Finalize the cluster upgrade process automatically. #24377

Conversation

windchan7 commented Mar 30, 2018

cockroach-teamcity commented Mar 30, 2018

vilterp commented Mar 30, 2018

bdarnell commented Apr 1, 2018

bdarnell commented Apr 1, 2018

benesch commented Apr 2, 2018

benesch commented Apr 2, 2018

windchan7 commented Apr 2, 2018 • edited Loading

BramGruneir commented Apr 2, 2018

windchan7 commented Apr 2, 2018

BramGruneir commented Apr 2, 2018

windchan7 commented Apr 2, 2018 • edited Loading

benesch commented Apr 2, 2018

piyush-singh commented Apr 2, 2018

BramGruneir commented Apr 2, 2018

bdarnell commented Apr 2, 2018

benesch commented Apr 2, 2018

windchan7 commented Apr 4, 2018

bdarnell commented Apr 4, 2018

Add a new field cluster.auto_finalize.disable_at_version

benesch commented Apr 6, 2018

benesch commented Apr 6, 2018

windchan7 commented Apr 8, 2018

bdarnell commented Apr 9, 2018

windchan7 commented Apr 9, 2018

bdarnell commented Apr 9, 2018

couchand commented Apr 9, 2018

windchan7 commented Apr 11, 2018

windchan7 commented Apr 11, 2018

benesch commented Apr 11, 2018

windchan7 commented Apr 11, 2018

windchan7 commented Apr 11, 2018

craig bot commented Apr 11, 2018

Canceled

benesch commented Apr 11, 2018

couchand commented Apr 11, 2018

bdarnell commented Apr 11, 2018

windchan7 commented Apr 11, 2018

windchan7 commented Apr 12, 2018

craig bot commented Apr 12, 2018

Build succeeded

windchan7 commented Apr 2, 2018 •

edited

Loading

windchan7 commented Apr 2, 2018 •

edited

Loading

Add a new field `cluster.auto_finalize.disable_at_version`