release-20.1: backupccl: cluster restore import and restore jobs as canceled

[pbardea]

First commit is a backport of #60458.

IMPORT and RESTORE may write non-transactionally, so their writes cannot
be trusted to be included in every backup. As such, they should be
restored in a reverting state to attempt to undo any of their untrusted
writes.

Release note (bug fix): IMPORT and RESTORE jobs are now restored as
reverting so that they cleanup after themselves. Previously, some of the
writes of the jobs while they were running may have been missed by
backup.

[cockroach-teamcity]

This change is 

[ajwerner]

Reviewed 5 of 5 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @pbardea)

---

pkg/ccl/backupccl/restore_job.go, line 1358 at r1 (raw file):

		}

		if err := db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {

in 21.1 you've got a strong claim that no other instance is concurrently running. In 20.1 it's relatively best-effort. It's not likely but it is very possible. It would not hurt to re-read the details in this txn.

---

pkg/ccl/backupccl/restore_job.go, line 1459 at r2 (raw file):

				}

				var updateStatusQuery strings.Builder

nit: I'd pull out the query string construction into a helper function or closure.

---

pkg/ccl/backupccl/restore_job.go, line 1470 at r2 (raw file):

				fmt.Fprint(&updateStatusQuery, ")")

				fmt.Println(updateStatusQuery.String())

detritus?

---

pkg/ccl/backupccl/restore_test.go, line 13 at r1 (raw file):

import (
	"context"
	fmt "fmt"

🤔

---

pkg/jobs/jobspb/jobs.proto, line 99 at r1 (raw file):

  // SystemTablesRestored keeps track of dynamic states that need to happen only
  // once during the lifetime of a job. Note, that this state may be shared
  // between job versions, so updates to this map must be considered carefully.

what do you mean "job versions"?

Also, why is this in details and not progress? I imagine that it's because that ship has sailed but :(

---

pkg/jobs/jobspb/jobs.proto, line 102 at r1 (raw file):

  // It maps system table names to whether or not they have already been
  // restored.
  map<string, bool> system_tables_restored = 17;

This has been backported to every branch after this with the same definition, right?

[pbardea]

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

---

pkg/ccl/backupccl/restore_job.go, line 1358 at r1 (raw file):

Previously, ajwerner wrote…

in 21.1 you've got a strong claim that no other instance is concurrently running. In 20.1 it's relatively best-effort. It's not likely but it is very possible. It would not hurt to re-read the details in this txn.

Moved the reading of the details to be contained within the txn.

---

pkg/ccl/backupccl/restore_job.go, line 1459 at r2 (raw file):

Previously, ajwerner wrote…

nit: I'd pull out the query string construction into a helper function or closure.

Done.

---

pkg/ccl/backupccl/restore_job.go, line 1470 at r2 (raw file):

Previously, ajwerner wrote…

detritus?

Yep, thanks for catching. Removed.

---

pkg/ccl/backupccl/restore_test.go, line 13 at r1 (raw file):

Previously, ajwerner wrote…

🤔

Something seems to be automatically adding these. Perhaps it's something up with my VSCode setup. Looks like we have quite a few violations throughout the codebase.

Perhaps there's a way to lint against these redundant import aliases.

---

pkg/jobs/jobspb/jobs.proto, line 99 at r1 (raw file):

Previously, ajwerner wrote…

what do you mean "job versions"?

Also, why is this in details and not progress? I imagine that it's because that ship has sailed but :(

I think I originally meant "job implementation versions", changed to "cluster versions" for more clarity.

Ya, it is unfortunate. We already have a few of these state-tracking progress metrics in details and didn't break the momentum here. Will try to keep in mind moving forward though.

---

pkg/jobs/jobspb/jobs.proto, line 102 at r1 (raw file):

Previously, ajwerner wrote…

This has been backported to every branch after this with the same definition, right?

Yep:
20.2 -

cockroach/pkg/jobs/jobspb/jobs.proto

Line 169 in 0f4525d

map<string, bool> system_tables_restored = 17;

21.1 -

cockroach/pkg/jobs/jobspb/jobs.proto

Line 192 in 0e1e28e

map<string, bool> system_tables_restored = 17;

[ajwerner]

Reviewed 3 of 5 files at r3, 1 of 2 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @dt)

[pbardea]

Thanks for the review!