rfc: hooks to help with cluster version upgrades

[danhhz]

@vivekmenezes This is an idea I had that might help with #7073, among other things. WDYT?

---

This change is

[vivekmenezes]

I do like this idea but it needs to be spelled out in more detail for the current set of examples you list here. The reduction in complexity is not obvious from reading the doc.

[bdarnell]

I think this is valuable even if it's not a net reduction in complexity. As I said in #7073, I think that allowing every node to spam CREATE TABLE IF NOT EXISTS commands at startup is not the best way to do migrations and will tie our hands when our needs get more complicated. I'd rather designate one node to manage the migration.

[danhhz]

Okay, I reworked the doc quite a bit. Let me know if this is any more clear

[bdarnell]

Generally LGTM, although it could use more details about the exact leasing process and how the SystemVersion table gets bootstrapped.

[bdarnell]

In past systems I've found it useful to make the "version" table store a set of strings (i.e. all the migrations that have been applied) instead of a single value. This facilitates development by letting different migrations be merged to the mainline out of order.

[danhhz]

Agreed. I was wary of overengineering, but since you're concerned about this too, I added something about to address it. (Plus I tweaked it a bit so you don't have to know the cockroach version your code is going into, which might not always be straightforward).

[bdarnell]

This seems backwards to me. It's easier for the new code to be compatible with the old data than for the migration to be performed while the old code is still running. I would prefer to roll out new code everywhere (in backwards-compatibility mode) and then perform the update once there are no more old nodes.

[danhhz]

That was my gut feeling too, but if you're adding a system table, then you have to handle it not existing everywhere and there's no problem adding it with old code. I suspect many of our uses will fall into this camp.

And when you need the intermediate state, you can always do two migrations.

[bdarnell]

Unless you're going to continue hard-coding descriptors for these dynamically-created tables, you have to handle the case that the table exists but hasn't been loaded. So I think you could have a single piece of code that loads the table descriptor and creates it (or waits for someone else to do so) if it doesn't exist yet.

[bdarnell]

I think this is valuable even if it's not a net reduction in complexity. As I said in #7073, I think that allowing every node to spam CREATE TABLE IF NOT EXISTS commands at startup is not the best way to do migrations and will tie our hands when our needs get more complicated. I'd rather designate one node to manage the migration.

[bdarnell]

Why? As long as the migration process completed, we know we won't regress. It may be worth repeating the check periodically to see if the node that started the migration process died without finishing it, but I don't see any reason to tie that to rejoining after a partition.

[bdarnell]

I'm not sure that we need this map; I doubt we'll ever have enough migrations here that it's worthwhile to roll them up into a single value. Maintaining this map seems like it would be as much of a headache as giving all the migrations a version number instead of a name.

[bdarnell]

I would define the migrations in an ordered list in the code, and then always execute them in that order (skipping any that are already applied).

[bdarnell]

It sounds like you're talking about running cluster-upgrade with a new binary while the cluster is still running an old version. This sounds like it could be tricky from an operational logistics perspective. It's also troubling from a security perspective since this command would presumably need an all-powerful node certificate instead of a user certificate.

[bdarnell]

This seems dangerous. How does a node tell that it is not the first? (what if the first crashes?) What is the recovery procedure if you've rolled out the new binary everywhere and every node but one has crashed? We don't generally guarantee the ability to roll back to an older version.

[bdarnell]

Why a new column? Don't we just need to change the default value to uuid_v4? Or are you talking about changing it to an int?

[bdarnell]

Walk through the entire process of a multi-step update like this from the operator's perspective. Is it cluster-upgrade (add column), rollout binary version N (read and write both), cluster-upgrade (backfill new column), rollout binary version N+1 (stop using old column), cluster-upgrade (delete old column), rollout binary version N+2 (maybe not necessary)?

That sounds terrible; we really don't want to put our users through a process like that. It's definitely not worth it to delete a tiny experimental function, although I assume we'll eventually have better reasons for making a similarly complex change. I think we need to be able to tolerate intermediate states so this can look like a single step (from the user's perspective).

We don't need to solve this part now; it's more important that we get something that works for the simple cases. But this is part of why I still think it would be better to roll out the new binary before starting the migration process.

[bdarnell]

It might be worth considering a SQL extension for this (or adopting one if another database has done it). This is a problem for user schema changes too.

[bdarnell]

Unless you're going to continue hard-coding descriptors for these dynamically-created tables, you have to handle the case that the table exists but hasn't been loaded. So I think you could have a single piece of code that loads the table descriptor and creates it (or waits for someone else to do so) if it doesn't exist yet.

[a-robinson]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 38 at r2 (raw file):

I'm not sure how to do that.)

The migration coordinator starts by heartbeating a kv entry to ensure that there

Is this a system-wide kv entry or a node-local kv entry? If the former, than how are upgrades supposed to be coordinated across multiple nodes?

---

docs/RFCS/cluster_upgrade_tool.md, line 46 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I would define the migrations in an ordered list in the code, and then always execute them in that order (skipping any that are already applied).

+1, this pattern makes testing much easier

---

docs/RFCS/cluster_upgrade_tool.md, line 61 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This seems dangerous. How does a node tell that it is not the first? (what if the first crashes?) What is the recovery procedure if you've rolled out the new binary everywhere and every node but one has crashed? We don't generally guarantee the ability to roll back to an older version.

We don't? Do we have any plans to support rollbacks?

---

Comments from Reviewable

[bdarnell]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 61 at r2 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

We don't? Do we have any plans to support rollbacks?

Only in the sense that we recognize rollbacks as a highly desirable feature. We have no real plan about how we might get there.

---

Comments from Reviewable

[petermattis]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 30 at r2 (raw file):

version. In the steady state, there is a map in code between migration names and
the cockroach version they were released in. There is also a list of migrations
added since the last cockroach release.

Would be good to be able to get a list of the migrations that are required to upgrade to a version. Should be easy to provide with the info available.

---

docs/RFCS/cluster_upgrade_tool.md, line 55 at r2 (raw file):

starting the migration coordinator. When upgrading a Cockroach version, this
command will be run and pointed at the cluster. After it returns, the cluster
can be rolled onto the new version.

Ah, per my earlier comment, there should be a cluster-upgrade -n option that shows the migrations that would be performed without actually performing them. Also, it will be good for this command to provide user feedback. That doesn't need to be spelled out here, though.

---

docs/RFCS/cluster_upgrade_tool.md, line 61 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Only in the sense that we recognize rollbacks as a highly desirable feature. We have no real plan about how we might get there.

This ease-of-use upgrade seems mildly complicated. To start with I think it would be sufficient for a node to detect that it isn't compatible with the current system tables and exit with an error indicating that an upgrade is needed.

---

docs/RFCS/cluster_upgrade_tool.md, line 75 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Why a new column? Don't we just need to change the default value to uuid_v4? Or are you talking about changing it to an int?

Or we could change the default to `unique_rowid()::bytes`.

---

Comments from Reviewable

[vivekmenezes]

@a-robinson I'm assuming you're taking over this PR?

[a-robinson]

Correct. I've been tied up with some lingering views complexities, but I hope to have some amendments sent out for this by Monday.

[a-robinson]

Sorry for the delay, @vivekmenezes! PTAL, I've expanded on the original RFC with a focus on getting something done in the short term while not limiting our longer term options.

---

Review status: 0 of 1 files reviewed at latest revision, 14 unresolved discussions, some commit checks pending.

---

docs/RFCS/cluster_upgrade_tool.md, line 46 at r2 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

+1, this pattern makes testing much easier

Although if some migrations are backwards compatible and could be applied immediately, we may not want them to have to wait behind non-backwards-compatible migrations that take multiple steps / release cycles...

---

Comments from Reviewable

[a-robinson]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 46 at r2 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Although if some migrations are backwards compatible and could be applied immediately, we may not want them to have to wait behind non-backwards-compatible migrations that take multiple steps / release cycles...

Whoops, I didn't really mean to publish this, but there's definitely a trade-off between a single linear upgrade path and flexibility in not queueing up all upgrades behind the most complex upgrade.

---

Comments from Reviewable

[danhhz]

(but that's expected from me). I particularly like the way you've handled the short term design while laying out how it plays into the options for later plans.

please wait for @bdarnell to be happy before merging

---

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

---

Comments from Reviewable

[petermattis]

I think this is a "definitely", not a "maybe".

[petermattis]

This model doesn't handle "changing the encoding of the RaftAppliedIndexKey and LeaseAppliedIndexKey keys" or "switching from nanoseconds to microseconds for storing timestamps and intervals". I think that's fine, but perhaps worth calling out that the types of migrations this enables is mostly constrained to system table and their schemas.

[a-robinson]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 24 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I think this is a "definitely", not a "maybe".

I've removed the "(maybe)", but for what it's worth I was going off @bdarnell's comment that it isn't worth putting users through so much pain to delete a tiny experimental function: https://github.com//pull/9404#discussion_r80838974

---

docs/RFCS/cluster_upgrade_tool.md, line 75 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This model doesn't handle "changing the encoding of the RaftAppliedIndexKey and LeaseAppliedIndexKey keys" or "switching from nanoseconds to microseconds for storing timestamps and intervals". I think that's fine, but perhaps worth calling out that the types of migrations this enables is mostly constrained to system table and their schemas.

Serious question: are those pressing concerns for the next 3-6 months? If so, we can make the added effort to focus on a more complete solution now.

---

Comments from Reviewable

[bdarnell]

---

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

---

docs/RFCS/cluster_upgrade_tool.md, line 24 at r3 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

I've removed the "(maybe)", but for what it's worth I was going off @bdarnell's comment that it isn't worth putting users through so much pain to delete a tiny experimental function:
#9404 (comment)

I think it's not worth the multi-step add/remove column dance to replace this function. It probably is worth it to change the default since that should be doable in one step.

---

docs/RFCS/cluster_upgrade_tool.md, line 75 at r3 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Serious question: are those pressing concerns for the next 3-6 months? If so, we can make the added effort to focus on a more complete solution now.

I think we should explicitly limit the scope of this to system tables and their schemas. This RFC is for _cluster_-level migrations. Migrations that can be done on a store-by-store basis without synchronizing with the rest of the cluster can simply be done at startup (we already have a few of these). I think the applied index key migrations fall into this category.

---

docs/RFCS/cluster_upgrade_tool.md, line 172 at r4 (raw file):

process if so desired.

#### Overview of changes needed from short-term design

In the short term design, simple migrations like "create table system.jobs" are run automatically at startup. In "option 1", no migrations are run at startup. Would we still have a way to run simple migrations automatically?

---

docs/RFCS/cluster_upgrade_tool.md, line 208 at r4 (raw file):

1. Check release notes for the desired version for any required migrations
  1. If none, just carry out the upgrade
  1. If there are some and the output `./cockroach cluster-upgrade` on their

So this is running all migrations that were optional in the previous version? This seems like a weird workflow; I think we'd want to encourage migrations to be run as soon as practical after an upgrade (with a last-minute check here before the next upgrade).

---

docs/RFCS/cluster_upgrade_tool.md, line 230 at r4 (raw file):

information for the entire cluster and add some code that understands our
semantic versioning scheme sufficiently well to determine which node versions
support which migrations.

This doesn't have to understand our versioning scheme (which may or may not be semantic - our betas are using date versions instead of semantic versions). The nodes could gossip information about the migrations they understand directly instead of condensing it down to a single version identifier designed for humans.

---

Comments from Reviewable

[petermattis]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 75 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I think we should explicitly limit the scope of this to system tables and their schemas. This RFC is for cluster-level migrations. Migrations that can be done on a store-by-store basis without synchronizing with the rest of the cluster can simply be done at startup (we already have a few of these). I think the applied index key migrations fall into this category.

What Ben said. The applied index key migrations are in a different category than the migrations that this RFC is targeted at.

---

Comments from Reviewable

[a-robinson]

Thanks for the reviews!

---

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

---

docs/RFCS/cluster_upgrade_tool.md, line 24 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I think it's not worth the multi-step add/remove column dance to replace this function. It probably is worth it to change the default since that should be doable in one step.

Ack, thanks. Sorry for misrepresenting your opinion!

---

docs/RFCS/cluster_upgrade_tool.md, line 75 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

What Ben said. The applied index key migrations are in a different category than the migrations that this RFC is targeted at.

Thanks for the clarification. Updated the bullet point about it to clarify

---

docs/RFCS/cluster_upgrade_tool.md, line 172 at r4 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

In the short term design, simple migrations like "create table system.jobs" are run automatically at startup. In "option 1", no migrations are run at startup. Would we still have a way to run simple migrations automatically?

Yeah, my thinking when I wrote this was that any migration which didn't have a minimum version specified could be run safely at startup.

However, I apparently forgot to write that down, and it also assumes that we go with the approach of specifying a minimum CockroachDB version, which would require solidifying our versioning scheme if we haven't already.

---

docs/RFCS/cluster_upgrade_tool.md, line 208 at r4 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

So this is running all migrations that were optional in the previous version? This seems like a weird workflow; I think we'd want to encourage migrations to be run as soon as practical after an upgrade (with a last-minute check here before the next upgrade).

Well at some level, it's still the _current_ version :)

The tool would work just as well after an upgrade as before an upgrade, since it doesn't rely on the next version in any way. It's just a matter of admin workflow choice. I laid out the example in this way because waiting to run non-backwards-compatible migrations leaves more room for safe rollbacks, but we could also just describe it as "once you decide you won't want to roll back, run the migrations".

What do you think? Would you like me to change this?

---

docs/RFCS/cluster_upgrade_tool.md, line 230 at r4 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This doesn't have to understand our versioning scheme (which may or may not be semantic - our betas are using date versions instead of semantic versions). The nodes could gossip information about the migrations they understand directly instead of condensing it down to a single version identifier designed for humans.

Very good point. I've added that to the doc as an alternative.

---

Comments from Reviewable

[bdarnell]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 208 at r4 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Well at some level, it's still the current version :)

The tool would work just as well after an upgrade as before an upgrade, since it doesn't rely on the next version in any way. It's just a matter of admin workflow choice. I laid out the example in this way because waiting to run non-backwards-compatible migrations leaves more room for safe rollbacks, but we could also just describe it as "once you decide you won't want to roll back, run the migrations".

What do you think? Would you like me to change this?

I think the implementation is fine, but I'd clarify the description a bit. Something like "2: Verify that all migrations for the current version have been completed by running `cockroach cluster-upgrade` with the current binary; 3: perform the upgrade; 4: once the upgrade has completed and rollbacks are deemed no longer necessary, perform migrations for the new version by running `cockroach cluster-upgrade` with the new binary". We want to encourage people to run migrations soon after upgrading because migrations may enable performance improvements or new features and it's generally better to switch over to the new mode instead of running in backwards-compatibility mode indefinitely.

---

Comments from Reviewable

[a-robinson]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 208 at r4 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I think the implementation is fine, but I'd clarify the description a bit. Something like "2: Verify that all migrations for the current version have been completed by running cockroach cluster-upgrade with the current binary; 3: perform the upgrade; 4: once the upgrade has completed and rollbacks are deemed no longer necessary, perform migrations for the new version by running cockroach cluster-upgrade with the new binary". We want to encourage people to run migrations soon after upgrading because migrations may enable performance improvements or new features and it's generally better to switch over to the new mode instead of running in backwards-compatibility mode indefinitely.

Sounds good, except that checking release notes for the new version is still important in the sense that they may be trying to do an upgrade that skips a few versions, meaning that their desired version may require migrations that aren't available at their current version. I've updated the doc appropriately.

---

Comments from Reviewable

[a-robinson]

Rebased to appease teamcity

[a-robinson]

Merged the two commits together at Tamir's suggestion.

[vivekmenezes]

Review status: 0 of 1 files reviewed at latest revision, 19 unresolved discussions, some commit checks pending.

---

docs/RFCS/cluster_upgrade_tool.md, line 151 at r5 (raw file):

The idea is that before upgrading to a new version of CockroachDB, the admin
will look in the release notes for the version that they want to run and

Does the admin need to look at the release notes? Can you imagine an implementation where you point the new version at the current cluster and it run a diff and suggest commands that need to run before the cluster can be upgraded?

---

Comments from Reviewable

[vivekmenezes]

Review status: 0 of 1 files reviewed at latest revision, 19 unresolved discussions, some commit checks pending.

---

docs/RFCS/cluster_upgrade_tool.md, line 208 at r4 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Sounds good, except that checking release notes for the new version is still important in the sense that they may be trying to do an upgrade that skips a few versions, meaning that their desired version may require migrations that aren't available at their current version. I've updated the doc appropriately.

I just saw this comment. I'm confused as to why you cannot do this automatically? If migrations aren't available the tool can suggest the min version for the upgrade right?

---

Comments from Reviewable

[a-robinson]

Review status: 0 of 1 files reviewed at latest revision, 19 unresolved discussions, some commit checks pending.

---

docs/RFCS/cluster_upgrade_tool.md, line 151 at r5 (raw file):

Previously, vivekmenezes wrote…

Does the admin need to look at the release notes? Can you imagine an implementation where you point the new version at the current cluster and it run a diff and suggest commands that need to run before the cluster can be upgraded?

They don't actually have to look at the notes -- as mentioned in the next paragraph, if they try to start up a node at the new version and all required migrations haven't been run, it should exit with an explanatory error message, so things would work fine if they just try to start up a node with the new version.

---

Comments from Reviewable

[a-robinson]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 208 at r4 (raw file):

Previously, vivekmenezes wrote…

I just saw this comment. I'm confused as to why you cannot do this automatically? If migrations aren't available the tool can suggest the min version for the upgrade right?

In this design, we've been assuming that only the current binary would be used to run the `cluster-upgrade` command, and the current version has no way of knowing what migrations some arbitrary later version requires.

We could support a workflow where the user downloads the new desired binary and runs some sort of compatibility check command with it (which would be similar in implementation to the startup check for required migrations and the cluster-upgrade command), if there's agreement that it's worth the extra effort on top of maintaining release notes / documentation.

---

Comments from Reviewable

[vivekmenezes]

---

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

---

docs/RFCS/cluster_upgrade_tool.md, line 208 at r4 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

In this design, we've been assuming that only the current binary would be used to run the cluster-upgrade command, and the current version has no way of knowing what migrations some arbitrary later version requires.

We could support a workflow where the user downloads the new desired binary and runs some sort of compatibility check command with it (which would be similar in implementation to the startup check for required migrations and the cluster-upgrade command), if there's agreement that it's worth the extra effort on top of maintaining release notes / documentation.

It's not worth the extra effort at this stage.

---

Comments from Reviewable

[tamird]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 20 at r5 (raw file):

software as complex as CockroachDB.

## Examples

heh, this heading is weird. Examples of what? Maybe it's clear when rendered (since this is a subhead of Motivation). Feel free to push back.

---

docs/RFCS/cluster_upgrade_tool.md, line 48 at r5 (raw file):

some way.

Simple migrations can be discussed in terms which versions of the Cockroach

"in terms which" -- something off about this sentence.

---

docs/RFCS/cluster_upgrade_tool.md, line 59 at r5 (raw file):

assumes that the table is present begins the post-migration range.

For simplicity, we assume that at most two versions of CockroachDB are ever

Can we enforce this using the same mechanism as schema leases? Something to think about.

---

docs/RFCS/cluster_upgrade_tool.md, line 82 at r5 (raw file):

Because handling migrations in the general case is a very broad problem
encompassing many potential types of migrations, we choose to first tackle the
simplest case, migrations that are backward-compatible, while leaving the door

nit: either backward-{in,}compatible, or backwards-{in,}compatible, but not both (the difference is the s on backwards)

---

docs/RFCS/cluster_upgrade_tool.md, line 97 at r5 (raw file):

needs to catch up with more than one of them.

The migration coordinator starts by heartbeating a kv entry to ensure that there

rather than "heartbeating a kv entry", can we describe this as a "migration lease"? I'm imagining the machinery to be virtually identical to range leases.

---

docs/RFCS/cluster_upgrade_tool.md, line 101 at r5 (raw file):

migrations while a different node is doing migrations will have to block until
the kv entry is released (or expired in the case of node failure). Then, for
each missing migration, the migration coordinator runs the work function for

"each missing migration" is mentioned twice in this sentence.

---

docs/RFCS/cluster_upgrade_tool.md, line 105 at r5 (raw file):

`/SystemVersion/<MigrationName>`.

Each work function must be idempotent (in case a migration coordinator crashes)

Should be easy enough to smoke-test this in the migration coordinator tests (i.e. before any migrations have been added).

---

docs/RFCS/cluster_upgrade_tool.md, line 115 at r5 (raw file):

accomplished with one hook and immediately used. To add such a migration, you'd
create a new migration called something like "CREATE system.jobs" with a
function that created the new system table and add it to the end of the list of

s/created/creates/?

---

docs/RFCS/cluster_upgrade_tool.md, line 142 at r5 (raw file):

* Listing all migrations that have been run on the cluster
* Listing the available migrations, which migrations they depend on, and which

s/and which/and the/

---

docs/RFCS/cluster_upgrade_tool.md, line 145 at r5 (raw file):

  CockroachDB versions at which it's safe to run each of them
* Running a single migration specified by the admin
* Running all available migrations whose migration dependencies are satisfied

also might be helped by using a "version lease" (same mechanism as schema leases)

---

docs/RFCS/cluster_upgrade_tool.md, line 168 at r5 (raw file):

version.

The CLI tool will be the recommended way of upgrading Cockroach versions for

nit:s/Cockroach\b/CockroachDB/g

---

docs/RFCS/cluster_upgrade_tool.md, line 170 at r5 (raw file):

The CLI tool will be the recommended way of upgrading Cockroach versions for
production scenarios. For ease of small clusters and local development,
one-node clusters will be able to do migrations during their normal startup

How do you determine you're a one-node cluster?

---

docs/RFCS/cluster_upgrade_tool.md, line 172 at r5 (raw file):

one-node clusters will be able to do migrations during their normal startup
process if so desired. Also, migrations with no minimum version specified can
still be run at startup, as in the short-term design, because the lack of a

s/startup, as/startup as/

---

docs/RFCS/cluster_upgrade_tool.md, line 226 at r5 (raw file):

having total control.

Unlike in the short-term, backwards compatible case, we can't necessarily run

s/short-term,/,/, s/backwards /backwards-/

---

docs/RFCS/cluster_upgrade_tool.md, line 233 at r5 (raw file):

enough version.

We could ensure this in at least two different ways:

"version leases" may be relevant here as well.

---

docs/RFCS/cluster_upgrade_tool.md, line 256 at r5 (raw file):

The main difference for this approach is that we'll have to start tracking
either the versions or known migrations of all nodes in the cluster. We could

s/or/of/

---

docs/RFCS/cluster_upgrade_tool.md, line 258 at r5 (raw file):

either the versions or known migrations of all nodes in the cluster. We could
potentially use gossip for this, but have to be sure that we know the state of
all nodes, not just most of them to be confident that it's safe to run a

"version leases" again

---

docs/RFCS/cluster_upgrade_tool.md, line 295 at r5 (raw file):

The short-term design doesn't appear to have any obvious drawbacks, as it
solves a couple immediate needs and can be added without limiting future

couple of

---

docs/RFCS/cluster_upgrade_tool.md, line 300 at r5 (raw file):

The long-term design adds administrative complexity to the upgrade process, no
matter which option we go with. It'll also add a little extra work to the
release process for developers due to the need to include minimum versions for 

trailing space, i think?

---

Comments from Reviewable

[a-robinson]

Thanks for the review!

---

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

---

docs/RFCS/cluster_upgrade_tool.md, line 20 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

heh, this heading is weird. Examples of what? Maybe it's clear when rendered (since this is a subhead of Motivation). Feel free to push back.

Done.

---

docs/RFCS/cluster_upgrade_tool.md, line 48 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

"in terms which" -- something off about this sentence.

Done.

---

docs/RFCS/cluster_upgrade_tool.md, line 59 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Can we enforce this using the same mechanism as schema leases? Something to think about.

Not the exact same mechanism, but the general approach could potentially be use for this. I've opened #10212 to track the idea.

---

docs/RFCS/cluster_upgrade_tool.md, line 82 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: either backward-{in,}compatible, or backwards-{in,}compatible, but not both (the difference is the s on backwards)

Done throughout the file.

---

docs/RFCS/cluster_upgrade_tool.md, line 97 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

rather than "heartbeating a kv entry", can we describe this as a "migration lease"? I'm imagining the machinery to be virtually identical to range leases.

Sure, changed the wording.

---

docs/RFCS/cluster_upgrade_tool.md, line 101 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

"each missing migration" is mentioned twice in this sentence.

Done.

---

docs/RFCS/cluster_upgrade_tool.md, line 115 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/created/creates/?

Done.

---

docs/RFCS/cluster_upgrade_tool.md, line 142 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/and which/and the/

Done.

---

docs/RFCS/cluster_upgrade_tool.md, line 168 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit:s/Cockroach\b/CockroachDB/g

Done.

---

docs/RFCS/cluster_upgrade_tool.md, line 170 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

How do you determine you're a one-node cluster?

That's a good question that I don't currently know the answer to. I kept this around from Dan's plan and naively assumed that checking the used node IDs from the store was fairly simple, but perhaps that's not the case? I'm curious if you have an answer, but either way I don't think answering that blocks the RFC as a whole.

---

docs/RFCS/cluster_upgrade_tool.md, line 172 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/startup, as/startup as/

That doesn't flow well to me, switched to a parenthetical instead

---

docs/RFCS/cluster_upgrade_tool.md, line 226 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/short-term,/,/, s/backwards /backwards-/

Done.

---

docs/RFCS/cluster_upgrade_tool.md, line 256 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/or/of/

No, "or" is correct there, to account for the design space of choosing between tracking node versions or tracking which migrations each node knows about.

---

docs/RFCS/cluster_upgrade_tool.md, line 295 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

couple of

Done.

---

docs/RFCS/cluster_upgrade_tool.md, line 300 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

trailing space, i think?

Done.

---

Comments from Reviewable

[tamird]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 170 at r5 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

That's a good question that I don't currently know the answer to. I kept this around from Dan's plan and naively assumed that the we could checking the used node IDs from the store was fairly simple, but perhaps that's not the case? I'm curious if you have an answer, but either way I don't think answering that blocks the RFC as a whole.

Hm, probably the most conservative way is to scan all the node descriptors, if there is only one, we can do this stuff.

---

docs/RFCS/cluster_upgrade_tool.md, line 256 at r5 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

No, "or" is correct there, to account for the design space of choosing between tracking node versions or tracking which migrations each node knows about.

Ah, my mistake. I think "known migrations of all nodes" is what's confusing here, it seems to imply that a node can have run or unrun migrations.

Actually, the two things we could track that are mentioned here are exactly equivalent, so it might be better to only mention the one we directly care about.

---

docs/RFCS/cluster_upgrade_tool.md, line 100 at r6 (raw file):

to ensure that there is only ever one running at a time. Other nodes that start
up and require migrations while a different node is doing migrations will have
to block until the kv entry is released (or expired in the case of node

s/kv entry/lease/

---

Comments from Reviewable

[a-robinson]

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

---

docs/RFCS/cluster_upgrade_tool.md, line 100 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/kv entry/lease/

Done.

---

Comments from Reviewable

[tamird]

---

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

---

Comments from Reviewable