server: fix misreported CRDB environment variables during startup

[srosenberg]

During server startup, reportConfiguration (see cli/start.go), logs the values of CRDB and other, unredacted environment variables to the Ops channel. These environment variables are read from envVarRegistry.cache (see GetEnvVarsUsed) which is assumed to have been populated during the initialization of the (Go) runtime. Specifically, EnvOrDefaultXXX must be invoked, which has a side-effect of populating the cache. When it's invoked outside of the initializer (e.g., inside a func that's not invoked until after initilization), the corresponding environment variable will not be in envVarRegistry.cache. Hence, it will not be reported.

Reporting values of modified environment variables is useful for quickly assembling a configuration context during debugging. It's unfortunate that the current design relies on indirectly populating envVarRegistry.cache during the initialization without any sort of enforcement. The change in this PR doesn't attempt to fix it. Instead, the following misreported environment variables are now reported,

• COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING
• COCKROACH_UPGRADE_TO_DEV_VERSION

Release note: None
Epic: none

[cockroach-teamcity]

This change is 

[herkolategan]

Just a small typo in the commit message to fix-up, otherwise looks good to me.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @smg260 and @srosenberg)

---

-- commits line 18 at r1:
Small typo in the commit message.

Code quote:

withoout

[knz]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @smg260 and @srosenberg)

[andreimatei]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @smg260 and @srosenberg)

---

pkg/clusterversion/cockroach_versions.go line 618 at r2 (raw file):

)

// N.B. EnvOrDefaultBool has the desired side-effect of populating envVarRegistry.cache.

I don't think this is a good place for this comment (should we put it on all callers?).
Put it on the function definition.

---

pkg/roachprod/config/config.go line 90 at r2 (raw file):

		"COCKROACH_ENABLE_RPC_COMPRESSION=false",
		// Get rid of an annoying popup in the UI.
		"COCKROACH_UI_RELEASE_NOTES_SIGNUP_DISMISSED=true",

For future reference, I think this belonged better in a different commit. I had to ask why this was going away, then I happened to read the commit message again.

---

pkg/roachprod/config/config.go line 93 at r2 (raw file):

		// the tests will fail even on release branches when attempting to upgrade previous (stable) release to an alpha.
		// [1] https://github.com/cockroachdb/cockroach/pull/87468
		// TODO(SR): should remove this in light of https://github.com/cockroachdb/cockroach/issues/92608

nit: the point of the name in the todo is for readers to know who to ask about it. Consider a more identifiable name.

Also phrased like this, the TODO amounts to little more than clickbait for a reader. Also the "should" phrasing is ambiguous about whether we're waiting for something, or if this action dependent on that issue. I'd either say more or, better, drop the TODO.

---

pkg/settings/cluster/cluster_settings.go line 90 at r2 (raw file):

)

// TelemetryOptOut is a place for controlling whether to opt out of telemetry or not.

since you're taking the blame, consider rephrasing the bizarre "is a place".
And also consider leaving this where it was, unless there was a reason to move it.

[srosenberg]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @smg260)

---

pkg/clusterversion/cockroach_versions.go line 618 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I don't think this is a good place for this comment (should we put it on all callers?).
Put it on the function definition.

Right, the placement of the comment feels awkward here. I'll move it to the function defs.

---

pkg/roachprod/config/config.go line 90 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

For future reference, I think this belonged better in a different commit. I had to ask why this was going away, then I happened to read the commit message again.

Fair. It deserves a separate commit. I'll move it out.

---

pkg/roachprod/config/config.go line 93 at r2 (raw file):

point of the name in the todo is for readers to know who to ask about it. Consider a more identifiable name.

Yep, I am still undoing my habit of using initials which were easily identifiable in smaller codebases; github username seems to be the standard here.

Also phrased like this, the TODO amounts to little more than clickbait for a reader. Also the "should" phrasing is ambiguous about whether we're waiting for something, or if this action dependent on that issue. I'd either say more or, better, drop the TODO.

Fair. This was more of a note-to-self, than a TODO. At the same time, the "clickbait" is actually not as bad as you make it sound; the issue actually describes why we want to drop this env. var.; i.e., the URL encodes sufficient information to serve as a standalone TODO, imo.

---

pkg/settings/cluster/cluster_settings.go line 90 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

since you're taking the blame, consider rephrasing the bizarre "is a place".
And also consider leaving this where it was, unless there was a reason to move it.

Sure, I'll paraphrase the comment; I can only surmise that the original author meant, "is a placeholder". It was moved to be grouped with the other var definitions, but I now see that wasn't necessary since the func was already in the wrong place (grouping-wise); so, we'll keep it there.

[srosenberg]

TFTR!

P.S. TestImportDefaultWithResume was the only failure during Bazel Essential; TC indicated the unit test to be flaky, so there is a diminishing return in rerunning the whole pipeline.

[srosenberg]

bors r=knz,andreimatei,herkolategan

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)