Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
We now link the name attribute of the go_binary, as it appears in the corresponding BUILD.bazel. The primary motivation for this change is to exclude non-cockroach binaries (i.e., roachtest and roachprod) from the initialization of the metamorphic constants. Note, to fully support interop between a roachtest runner, (executing the roachtest binary), and a node, executing the cockroach binary compiled with assertions (i.e., crdb_test), the roachtest binary must also be compiled with assertions to establish the same serialization on the wire. E.g., KVNemesisSeq is used on the wire only under crdb_test builds. The corresponding (sequence) Container type is conditional on the binary being compiled under crdb_test. (See pkg/kv/kvpb/api.proto) Thus, a request serialized from the roachtest runner is compatible with the cockroach binary iff both binaries are built under crdb_test. Epic: none Release note: None
c8fdb8b to
50520d9
Compare
rail
left a comment
There was a problem hiding this comment.
I wonder if we can explicitly mark these binaries as eligible instead of guessing it from the name. 🤔 Something similar to EnabledAssertions maybe?
| optional string env_channel = 11 [(gogoproto.nullable) = false]; | ||
| // enabled_assertions returns the value of 'CrdbTestBuild' (true iff compiled with 'crdb_test' tag) | ||
| optional bool enabled_assertions = 12 [(gogoproto.nullable) = false]; | ||
| // binary_anme returns the name of the binary, e.g., cockroach, roachtest, roachprod. |
There was a problem hiding this comment.
| // binary_anme returns the name of the binary, e.g., cockroach, roachtest, roachprod. | |
| // binary_name returns the name of the binary, e.g., cockroach, roachtest, roachprod. |
renatolabs
left a comment
There was a problem hiding this comment.
I think it would be helpful to mention an example of a test that breaks without this change.
I'm also curious about how the test runner script will change in light of this. Are we always going to start building roachtest with crdb_test?
| return false | ||
| } | ||
| // Bail out if we are running a non-test binary which also happens to be non-cockroach, e.g., roachprod, roachtest. | ||
| if !bazel.InBazelTest() && !strings.Contains(build.GetInfo().BinaryName, "cockroach") { |
There was a problem hiding this comment.
Can you elaborate a bit on this predicate? Why do we need the !bazel.InBazelTest() part?
Yes, that's the idea. For the nightlies, I don't think there'd be a way to selectively use crdb_test roachtest binary iff the test being run has assertions enabled, since one roachtest call is in charge of running all the tests. |
DarrylWong
left a comment
There was a problem hiding this comment.
Rebased it off my current changes and it looks like it works 👍
LGTM pending the other's comments
| fmt.Fprintf(tw, "C Compiler: %s\n", b.CgoCompiler) | ||
| fmt.Fprintf(tw, "Build Commit ID: %s\n", b.Revision) | ||
| fmt.Fprintf(tw, "Build Type: %s\n", b.Type) | ||
| fmt.Fprintf(tw, "Binary Name: %s\n", b.BinaryName) |
There was a problem hiding this comment.
extremely small nit:
Use spaces instead of tab so it's in line with the others?
| fmt.Fprintf(tw, "Binary Name: %s\n", b.BinaryName) | |
| fmt.Fprintf(tw, "Binary Name: %s\n", b.BinaryName) |
rickystewart
left a comment
There was a problem hiding this comment.
This looks fine, consider that something like the following would also work and will be simpler:
In pkg/build/info.go, add:
var IsCockroachBinary = false
Then, in pkg/cmd/cockroach{,-oss,-short}/main.go, add:
func init() { build.IsCockroachBinary = true }
This is assuming that determining whether a binary is a cockroach binary is the ultimate purpose as the PR description says.
In [1], new cluster settings have been added, e.g., `kv.replica_gc_queue.enabled`, `kv.raft_log_queue.enabled`, to selectively disable work queues per store, at runtime. The new unit tests use a dummy `BoolSetting` which aliases slot 0 of the global cluster setting `registry`. Thus, its value corresponds to whichever cluster setting is registered first during `init()`. Before the change in [2], the first registered cluster setting is `bulkio.backup.merge_file_buffer_size`, one with a non-zero value in slot 0. After the change, it is `keyvisualizer.enabled`, one with a zero value in slot 0. Subsequently, a number of the unit tests failed because `baseQueue.mu.disabled` is now true instead of previously, false. The change in this PR fixes the bug by registering a fresh dummy `BoolSetting`. We also rename `disabledConfig` to match the polarity used in conjunction with `bq.SetDisabled`. [1] cockroachdb#104170 [2] cockroachdb#113113 Epic: none Release note: None
In [1], new cluster settings have been added, e.g., `kv.replica_gc_queue.enabled`, `kv.raft_log_queue.enabled`, to selectively disable work queues per store, at runtime. The new unit tests use a dummy `BoolSetting` which aliases slot 0 of the global cluster setting `registry`. Thus, its value corresponds to whichever cluster setting is registered first during `init()`. Before the change in [2], the first registered cluster setting is `bulkio.backup.merge_file_buffer_size`, one with a non-zero value in slot 0. After the change, it is `keyvisualizer.enabled`, one with a zero value in slot 0. Subsequently, a number of the unit tests failed because `baseQueue.mu.disabled` is now true instead of previously, false. The change in this PR fixes the bug by registering a fresh dummy `BoolSetting`. We also rename `disabledConfig` to match the polarity used in conjunction with `bq.SetDisabled`. [1] cockroachdb#104170 [2] cockroachdb#113113 Epic: none Release note: None
We now link the name attribute of the go_binary, as it appears in the corresponding BUILD.bazel. The primary motivation for this change is to exclude non-cockroach binaries (i.e., roachtest and roachprod) from the initialization of the metamorphic constants.
Note, to fully support interop between a roachtest runner, (executing the roachtest binary), and a node, executing the cockroach binary compiled with assertions (i.e., crdb_test), the roachtest binary must also be compiled with assertions to establish the same serialization on the wire. E.g., KVNemesisSeq is used on the wire only under crdb_test builds. The corresponding (sequence) Container type is conditional on the binary being compiled under crdb_test. (See pkg/kv/kvpb/api.proto) Thus, a request serialized from the roachtest runner is compatible with the cockroach binary iff both binaries are built under crdb_test.
Epic: none
Release note: None