*: customize the timeouts used by unit tests in Bazel Essential CI
#102719
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
rickystewart
requested changes
May 2, 2023
pkg/BUILD.bazel
Outdated
| @@ -3521,3 +3521,10 @@ test_suite( | |||
| ) | |||
|
|
|||
| unused_checker(srcs = GET_X_DATA_TARGETS) | |||
|
|
|||
| config_setting( | |||
There was a problem hiding this comment.
For the other config_settings, we keep this stuff in build/toolchains/BUILD.bazel. Can you put it there please?
pkg/base/BUILD.bazel
Outdated
| @@ -58,7 +58,10 @@ go_test( | |||
| "node_id_test.go", | |||
| "store_spec_test.go", | |||
| ], | |||
| args = ["-test.timeout=55s"], | |||
| args = select({ | |||
| "//pkg:use_ci_timeouts": ["-test.timeout=55s"], | |||
There was a problem hiding this comment.
Nit: this is pretty confusing. No reason to apply the select and introduce this dependency if the result is the same either way. Can you update the code to only insert this select if necessary (if the test is enormous)?
There was a problem hiding this comment.
Indeed this makes more sense. Done.
We manage unit tests timeouts at two levels: 1. Bazel timeout, by default [60s,300s,900s,3600s] for [small,medium,large,enormous] targets. 2. Go timeout, set to 5 seconds less than the corresponding Bazel timeout [see cockroachdb#86363]. Previously, unit tests used the same timeouts both when running in `Bazel Essential CI` and elsewhere. As a result, enormous test targets inherited a timeout of 1 hour from Bazel's default timeout. This is way beyond the expected time needed by any test target in `Bazel Essential CI`. We can't change enormous targets to large ones for two reasons: 1. `Enormous` is also used to indicate the resources needed by a test target. 2. `Enormous` test targets may still need the large timeout when running locally. To make this possible, we needed to support setting an `attr` value to a `select` using Buildozer. This was done in bazelbuild/buildtools#1153. This change only affects the timeout of `enormous` test targets. It however makes it simple to customize the timeout of other test sizes if desired in the future. Release note: None Epic: none
|
bors r=rickystewart |
|
Build succeeded: |
healthy-pod
pushed a commit
to healthy-pod/cockroach
that referenced
this pull request
May 11, 2023
In cockroachdb#102719, we changed the way we set `-test.timeout` but didn't update the logictests template. This code change updates the template. Release note: None Epic: none
craig bot
pushed a commit
that referenced
this pull request
May 11, 2023
101786: workload: introduce timeout for pre-warming connection pool r=sean- a=sean- Interrupting target instances during prewarming shouldn't cause workload to proceed: introduce a timeout to prewarming connections. Connections will have 15s to 5min to warmup before the context will expire. Epic: none 101987: cli/sql: new option autocerts for TLS client cert auto-discovery r=rafiss a=knz Fixes #101986. See the release note below. An additional benefit not mentioned in the release note is that it simplifies switching from one tenant to another when using shared-process multitenancy. For example, this becomes possible: ``` > CREATE TENANT foo; > ALTER TENANT foo START SERVICE SHARED; > \c cluster:foo root - - autocerts ``` Alternatively, this can also be used to quickly switch from a non-root user in an app tenant to the root user in the system tenant: ``` > \c cluster:system root - - autocerts ``` This works because (currently) all tenant servers running side-by-side use the same TLS CA to validate SQL client certs. ---- Release note (cli change): The `\connect` client-side command for the SQL shell (included in `cockroach sql`, `cockroach demo`, `cockroach-sql`) now recognizes an option `autocerts` as last argument. When provided, `\c` will now try to discover a TLS client certificate and key in the same directory(ies) as used by the previous connection URL. This feature makes it easier to switch usernames when TLS client/key files are available for both the previous and the new username. 102382: c2c: deflake c2c/shutdown roachtests r=stevendanna a=msbutler c2c: deflake c2c/shutdown roachtests This patch addresses to roachtest failure modes: - Prevents roachtest failure if a query fails during a node shutdown. - Prevents the src cluster from returning a single node topology, which could cause the stream ingestion job to hang if the participating src node gets shut down. Longer term, automatic replanning will prevent this. Specifically, this patch changes the kv workload to split and scatter the kv table across the cluster before the c2c job begins. Fixes #101898 Fixes #102111 This patch also makes it easier to reproduce c2c roachtest failures by plumbing a random seed to several components of the roachtest driver. Release note: None c2c: rename completeStreamIngestion to applyCutoverTime Release note: none workload: add --scatter flag to kv workload The user can now run `./workload init kv --scatter ....` which scatters the kv table across the cluster after the initial data load. This flag is best used with `--splits`, `--max-block-bytes`, and `--insert-count`. Epic: none Release note: none 102819: admission: move CreateTime-sequencing below-raft r=irfansharif a=irfansharif These are already reviewed commits from #98308. Part of #95563. --- **admission: move CreateTime-sequencing below-raft** We move kvflowsequencer.Sequencer and its use in kvflowhandle.Handle (above-raft) to admission.sequencer, now used by admission.StoreWorkQueue (below-raft). This variant appeared in an earlier revision of #97599 where we first introduced monotonically increasing CreateTimes for a given raft group. In a subsequent commit, when integrating kvflowcontrol into the critical path for replication traffic, we'll observe that it's quite difficult to create sequencing CreateTimes[^1] above raft. This is because these sequence numbers are encoded as part of the raft proposal[^2], and at encode-time, we don't actually know what log position the proposal is going to end up in. It's hard to explicitly guarantee that a proposal with log-position P1 will get encoded before another with log position P2, where P1 < P2. Naively sequencing CreateTimes at proposal-encode-time could result in over-admission. This is because of how we return flow tokens -- up to some log index[^3], and how use these sequence numbers in below-raft WorkQueues. If P2 ends up with a lower sequence number/CreateTime, it would get admitted first, and when returning flow tokens by log position, in specifying up-to-P2, we'll early return P1's flow tokens despite it not being admitted. So we'd over-admit at the sender. This is all within a <tenant,priority> pair. [^1]: We use CreateTimes as "sequence numbers" in replication admission control. We want to assign each AC-queued work below-raft a "sequence number" for FIFO ordering within a <tenant,priority>. We ensure these timestamps are roughly monotonic with respect to log positions of replicated work by sequencing work in log position order. [^2]: In kvflowcontrolpb.RaftAdmissionMeta. [^3]: See kvflowcontrolpb.AdmittedRaftLogEntries. --- **admission: add intercept points for when replicated work gets admitted** In a subsequent commit, when integrating kvflowcontrol into the critical path for replication traffic, we'll set up the return of flow tokens from the receiver node back to the sender once log entries get (asynchronously) admitted[^4]. So we need to intercept the exact points at which the virtually enqueued work items get admitted, since it all happens asynchronously[^5]. To that end we introduce the following interface: ```go // OnLogEntryAdmitted is used to observe the specific entries // (identified by rangeID + log position) that were admitted. Since // admission control for log entries is asynchronous/non-blocking, // this allows callers to do requisite post-admission // bookkeeping. type OnLogEntryAdmitted interface { AdmittedLogEntry( origin roachpb.NodeID, /* node where the entry originated */ pri admissionpb.WorkPriority, /* admission priority of the entry */ storeID roachpb.StoreID, /* store on which the entry was admitted */ rangeID roachpb.RangeID, /* identifying range for the log entry */ pos LogPosition, /* log position of the entry that was admitted*/ ) } ``` For now we pass in a no-op implementation in production code, but this will change shortly. Seeing as how the asynchronous admit interface is going to be the primary once once we enable replication admission control by default, for IO control, we no longer need the storeWriteDone interfaces and corresponding types. It's being used by our current (and soon-to-be legacy) above-raft IO admission control to inform granters of when the write was actually done, post-admission. For above-raft IO control, at admit-time we do not have sizing info for the writes, so by intercepting these writes at write-done time we're able to make any outstanding token adjustments in the granter. To reflect this new world, we: - Rename setAdmittedDoneModels to setLinearModels. - Introduce a storeReplicatedWorkAdmittedInfo[^6]. It provides information about the size of replicated work once it's admitted (which happens asynchronously from the work itself). This lets us use the underlying linear models for L0 {writes,ingests} to deduct an appropriate number of tokens from the granter, for the admitted work size[^7]. - Rename the granterWithStoreWriteDone interface to granterWithStoreReplicatedWorkAdmitted. We'll still intercept the actual point of admission for some token adjustments, through the the storeReplicatedWorkAdmittedLocked API shown below. There are two callstacks through which this API gets invoked, one where the coord.mu is already held, and one where it isn't. We plumb this information through so the lock is acquired if not already held. The locking structure is unfortunate, but this was a minimally invasive diff. ```go storeReplicatedWorkAdmittedLocked( originalTokens int64, admittedInfo storeReplicatedWorkAdmittedInfo, ) (additionalTokens int64) ``` While here, we also export an admission.TestingReverseWorkPriorityDict. There are at least three tests that have re-invented the wheel. [^4]: This will happen through the kvflowcontrol.Dispatch interface introduced back in #97766, after integrating it with the RaftTransport layer. [^5]: Introduced in #97599, for replicated write work. [^6]: Identical to the previous StoreWorkDoneInfo. [^7]: There's a peculiarity here in that at enqueuing-time we actually know the size of the write, so we could have deducted the right number of tokens upfront and avoid this post-admit granter token adjustment. We inherit this structure from earlier, and just leave a TODO for now. 103116: generate-logic-test: fix incorrect timeout in logictests template r=rickystewart a=healthy-pod In #102719, we changed the way we set `-test.timeout` but didn't update the logictests template. This code change updates the template. Release note: None Epic: none Co-authored-by: Sean Chittenden <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: Michael Butler <[email protected]> Co-authored-by: irfan sharif <[email protected]> Co-authored-by: healthy-pod <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We manage unit tests timeouts at two levels:
go testprocess timeout before bazel kills it #86363].Previously, unit tests used the same timeouts both when running in
Bazel Essential CIand elsewhere. As a result, enormous test targets inherited a timeout of 1 hour from Bazel's default timeout. This is way beyond the expected time needed by any test target inBazel Essential CI. We can't change enormous targets to large ones for two reasons:Enormousis also used to indicate the resources needed by a test target.Enormoustest targets may still need the large timeout when running locally.To make this possible, we needed to support setting an
attrvalue to aselectusing Buildozer. This was done in bazelbuild/buildtools#1153.This change only affects the timeout of
enormoustest targets. It however makes it simple to customize the timeout of other test sizes if desired in the future.Release note: None
Epic: none