backupccl: allow backups with proto named BACKUP_MANIFEST

[dt]

This does not change the writing of the current manifest file at the current name -- it only checks for the new name when reading the old one fails.

Adding this check now will allow a future-version to only write at the new name, but still play nice with any old nodes, as during the upgrade all nodes will look at both names.

Release justification: this is a low-risk change with high value, in that it establishes compatibility for a UX polish change we want to do in 20.1.

Release note: none.

[cockroach-teamcity]

This change is 

[dt]

This isn't critical to merge now and I could see declining to do so due to RC stability period rules. However if we do merge a small change to the read-path now to handle either name, it makes changing the write-path in 20.1 while not breaking mixed-version clusters a lot cleaner so I figured I'd send it out if that seems like a strong enough motivation.

[thoszhang]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @lucy-zhang)

---

pkg/ccl/backupccl/backup.go, line 59 at r1 (raw file):

	// BackupManifestName is a future name for the serialized
	// BackupDescriptor proto.
	BackupManifestName = BackupDescriptorName + "_MANIFEST"

Should this just be defined as "BACKUP_MANIFEST"? I don't think we use BackupDescriptorName as a prefix anywhere else.

Also, maybe we should just call these BackupManifestOldName and BackupManifestNewName or something, to make it clear that the files have the same content.

---

pkg/ccl/backupccl/backup.go, line 98 at r1 (raw file):

	backupDesc, err := readBackupDescriptor(ctx, exportStore, BackupDescriptorName)
	if err != nil {
		backupManifest, manifestErr := readBackupDescriptor(ctx, exportStore, BackupManifestName)

We'll have to attempt reading both filenames indefinitely into the future, right? At some point, I think having the if statements in the opposite order will make more sense.

[dt]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)

---

pkg/ccl/backupccl/backup.go, line 59 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Should this just be defined as "BACKUP_MANIFEST"? I don't think we use BackupDescriptorName as a prefix anywhere else.

Also, maybe we should just call these BackupManifestOldName and BackupManifestNewName or something, to make it clear that the files have the same content.

Done w.r.t. literal.

Agreed on the BackupManifestOldName but I'd rather do that when we actually change it for real -- my goal here is very much minimal churn required to just get the foot-in-the-door we need for upgrades later and no more.

---

pkg/ccl/backupccl/backup.go, line 98 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

We'll have to attempt reading both filenames indefinitely into the future, right? At some point, I think having the if statements in the opposite order will make more sense.

Yeah, I think a build that writes the new name would also optimistically read the new name first, but I think this order makes sense for the build that writes the old name.

[dt]

@lucy-zhang I just realized I didn't merge this before I went OOO. Just wanted to re-check: do you still think this is worth merging for 19.2?

[thoszhang]

Yeah, I think it's worth it.

[dt]

bors r+

[dt]

TFTR!

[craig]

Build failed

• GitHub CI (Cockroach)

[dt]

Huh, I assumed the Release justification linter operated on just the commit messages but I guess it wants it in the GitHub PR text too?

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)