upgrades: upgrades don't seem to run when developmentBranch is set to false

[knz]

When manually modifying developmentBranch (as in #99387) a couple of tests[1] fail.

For example:

--- FAIL: TestServerStartupGuardrails (0.23s)
    test_server_shim.go:353: migration-manager-find-jobs: system-jobs-scan: relation "system.job_info" does not exist

This suggests that we are not properly running the SQL migrations in that case.

[1] Tests that fail:

• TestAlreadyRunningJobsAreHandledProperly, TestPauseMigration - fixed by release-23.1: upgrademanager: fix upgrade manager tests that relied on wrong invariants #100867
• TestServerStartupGuardrails - fixed by release-23.1: serverccl: conditionally apply DevOffset in TestServerStartupGuardrails #100729

Jira issue: CRDB-26555

[celiala]

3 tests that fail in #99387 are:

(1) TestAlreadyRunningJobsAreHandledProperly. in pkg/upgrade/upgrademanager/upgrademanager_test

=== RUN   TestAlreadyRunningJobsAreHandledProperly
    manager_external_test.go:120:
          Error Trace:  pkg/upgrade/upgrademanager/upgrademanager_test_test/pkg/upgrade/upgrademanager/manager_external_test.go:120
          Error:        Received unexpected error:
                        sql: no rows in result set

(2) TestPauseMigration. in pkg/upgrade/upgrademanager/upgrademanager_test

=== RUN   TestPauseMigration
    manager_external_test.go:464: error scanning '&{<nil> 0xc003f80200}': sql: no rows in result set

(3) TestServerStartupGuardrails. in github.com/cockroachdb/cockroach/pkg/ccl/serverccl

I230405 05:30:21.943950 189123 upgrade/upgrademanager/manager.go:168  [T1,n1] 1170  running permanent upgrades up to version: 1000022.2
I230405 05:30:21.944731 189876 sql/temporary_schema.go:505  [T1,n1] 1171  running temporary object cleanup background job
I230405 05:30:21.945053 189877 sql/sqlstats/persistedsqlstats/provider.go:129  [T1,n1] 1172  starting sql-stats-worker with initial delay: 10m15.992188656s
I230405 05:30:21.946647 189895 kv/kvclient/rangefeed/rangefeedcache/watcher.go:335  [T1,n1] 1173  system-config-cache: established range feed cache
I230405 05:30:21.947169 189864 kv/kvserver/replica_rangefeed.go:701  [T1,n1,rangefeed=table-stats-cache,mux_n=1,gen=2,r21/1:/Table/2{0-1},s1] 1174  RangeFeed closed timestamp is empty
I230405 05:30:21.950611 189876 sql/temporary_schema.go:555  [T1,n1] 1175  found 0 temporary schemas
I230405 05:30:21.950720 189876 sql/temporary_schema.go:558  [T1,n1] 1176  early exiting temporary schema cleaner as no temporary schemas were found
I230405 05:30:21.950747 189876 sql/temporary_schema.go:559  [T1,n1] 1177  completed temporary object cleanup job
I230405 05:30:21.950765 189876 sql/temporary_schema.go:640  [T1,n1] 1178  temporary object cleaner next scheduled to run at 2023-04-05 06:00:21.944702855 +0000 UTC m=+1944.305053323
E230405 05:30:21.994611 189894 spanconfig/spanconfigmanager/manager.go:141  [T1,n1] 1179  error starting auto span config reconciliation job: get-jobs: system-jobs-scan: relation "system.job_info" does not exist
I230405 05:30:22.008682 189123 upgrade/upgrademanager/manager.go:229  [T1,n1] 1180  the last permanent upgrade (v22.2-98) does not appear to have completed; attempting to run all upgrades
I230405 05:30:22.009905 189123 upgrade/upgrademanager/manager.go:270  [T1,n1] 1181  running permanent upgrade for version 0.0-2
    test_server_shim.go:331: migration-manager-find-jobs: system-jobs-scan: relation "system.job_info" does not exist
        (1) attached stack trace
          -- stack trace:
          | github.com/cockroachdb/cockroach/pkg/sql.(*InternalExecutor).execInternal.func1.1

[celiala]

Note that TestPauseMigration passed on March 23[1] but then failed on April 4[2].

[1] March 23: https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_BazelEssentialCi/9223737?showRootCauses=false&expandBuildChangesSection=true&expandBuildProblemsSection=true&expandBuildDeploymentsSection=true&expandBuildTestsSection=true
[2] April 4: https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_BazelEssentialCi/9416194?showRootCauses=false&expandBuildChangesSection=true&expandBuildDeploymentsSection=true&expandBuildProblemsSection=true&expandBuildTestsSection=true

[renatolabs]

TestServerStartupGuardrails shouldn't be using versions that unconditionally apply the dev offset.

cockroach/pkg/ccl/serverccl/server_startup_guardrails_test.go

Lines 33 to 35 in ff2c4a3

	v := func(major, minor int32) roachpb.Version {
	return roachpb.Version{Major: clusterversion.DevOffset + major, Minor: minor}
	}

[knz]

@renatolabs any suggestion as to how to achieve what the test wants to exercise, instead?

[renatolabs]

@renatolabs any suggestion as to how to achieve what the test wants to exercise, instead?

Since that test already relies on version keys to pass a custom TenantLogicalVersionKey, we could be fetching the other version fields in that test by key as well instead of manually creating a roachpb.Version. I can open a PR if you think that's acceptable. Feel free to suggest anything else you may have in mind too.

[knz]

I can open a PR if you think that's acceptable

Yes I would love that ❇️

[renatolabs]

I'm not sure if this has already been identified, but setting developmentBranch to false was fine as far as TestPauseMigration is concerned up until #99458, at which point it started failing with that same error message (error scanning '&{<nil> 0xc009df9600}': sql: no rows in result set).

@adityamaru any ideas on this failure?

[adityamaru]

@renatolabs I'll spend some time understanding what developmentBranch has to do with the failure but applying this diff seems to fix the test. We don't expect system.jobs to contain any payload after 99458, this information has been moved to the system.job_info table. crdb_internal.system_jobs is a virtual table that joins those two tables so we can read the right columns from the right table:

diff --git a/pkg/upgrade/upgrademanager/manager_external_test.go b/pkg/upgrade/upgrademanager/manager_external_test.go
index f268b226cc1..f2960a57a51 100644
--- a/pkg/upgrade/upgrademanager/manager_external_test.go
+++ b/pkg/upgrade/upgrademanager/manager_external_test.go
@@ -454,7 +454,7 @@ func TestPauseMigration(t *testing.T) {
        var id int64
        tdb.QueryRow(t, `
 SELECT id
-  FROM system.jobs
+  FROM crdb_internal.system_jobs
  WHERE (
         crdb_internal.pb_to_json(
             'cockroach.sql.jobs.jobspb.Payload',

[adityamaru]

reopening until the backport is merged

[adityamaru]

backport has been merged!

[celiala]

Thank you, all! 🙌🏼