Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

serverutils: refactor the server test APIs #107058

Closed
knz opened this issue Jul 18, 2023 · 0 comments · Fixed by #109612
Closed

serverutils: refactor the server test APIs #107058

knz opened this issue Jul 18, 2023 · 0 comments · Fixed by #109612
Assignees
Labels
A-multitenancy Related to multi-tenancy A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) db-cy-23 T-multitenant Issues owned by the multi-tenant virtual team

Comments

@knz
Copy link
Contributor

knz commented Jul 18, 2023

Context

As part of our work on #76378 we are observing a pattern:

s, sqlDB, kvDB := serverutils.StartServer(...)

// At this point, sqlDB and kvDB point to the "application layer" already. This is desirable!

// If a test needs something else (e.g. "AdminURL") it needs to do a bit extra work:
ts := s.TenantOrServer() // give access to the application layer.

someuse := ts.ClusterSettings()
someuse := ts.AdminURL()
// etc.

Meanwhile, if the test needs explicit access to the storage layer, it is cumbersome:

_ = s.AdminURL() // this kinda works, but we feel it is ambiguous.
// Very cumbersome to retrieve a SQL connection:
systemSqlDB := serverutils.OpenDBConn(t, s.SQLAddr() /* don't mix with ServingSQLAddr()! */, ...)

Description of problem

There are two shorcomings with this structure:

  • if a test cares about the "application layer", it is too easy to mistakenly reach out for the TestServerInterface instead of thinking to call TenantOrServer() first.
  • if a test cares about the storage layer / system tenant, retrieving a SQL connection is cumbersome.

And most importantly:

  • a single run of the test only exercices EITHER the system tenant OR a secondary tenant, and never both. This halves test coverage.

Solution outline

More explicit testserver APIs

At the outset, we should make the "application" vs "storage" layer separation more clear. That is:

  • don't make TestServerInterface inherit from TestTenantInterface any more
  • instead, make a method StorageLayer() TestTenantInterface inside TestServerInterface so that a test can spell out explicitly that it wants access to the storage layer.
  • rename TenantOrServer() to ApplicationLayer().
  • add a SQLConn() *goSQL.DB method to TestTenantInterface, it's been sorely missing!
  • spell out in docs that the 2nd and 3rd return value of StartServer are equivalent to .ApplicationLayer().SQLConn() and .ApplicationLayer().DB(), for clarity.
  • rename TestTenantInterface to TestClientInterface.

This would already clarify a great deal of tests. But it would still limit each test run to either test one or the other.

Helper to automate working with test servers

To increase test coverage, we should invert control of StartServer, that is deprecate

func StartServer(*testing.T, ...) (TestServerInterface, *sql.DB, *kv.DB)

and instead introduce:

func RunWithServer(*testing.T, testName string, ..., runFn func(*testing.T, TestServerInterface, *sqlDB, *kv.DB))

So a test would change from:

s, sqlDB, kvDB := serverutils.StartServer(t, ...)
defer s.Stopper().Close(...)

ts := s.TenantOrServer() // give access to the application layer.
someuse := ts.ClusterSettings()
someuse := ts.AdminURL()

to:

serverutils.RunWithServer(t, ... func(*testing.T, s ApplicationLayerInterface, sqlDB ..., kvDB ...) { 
   // No need for calling stopper explicitly.
   someuse := ts.ClusterSettings()
   someuse := ts.AdminURL()
})

Once we have this API in place, we can make the sub-test run for both interfaces:

// in serverutils, RunWithServer:
s := startServerInternal(...)
defer s.Stopper().Close()

t.Run("system tenant", func(t *testing.T) {
   ts := s.SystemLayer()
   runFn(t, ts, ts.SQLConn(), ts.DB())
})

t.Run("secondary tenant, serverless-style", func(t *testing.T) {
   tenant := s.StartTenant(...)
   runFn(t, tenant, tenant.SQLConn(), tenant.DB())
})

t.Run("secondary tenant, dedicated-style", func(t *testing.T) {
   tenant := s.StartTenant(...)
   runFn(t, tenant, tenant.SQLConn(), tenant.DB())
})

Jira issue: CRDB-29864

@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-multitenancy Related to multi-tenancy T-multitenant Issues owned by the multi-tenant virtual team A-testing Testing tools and infrastructure labels Jul 18, 2023
@knz knz added the db-cy-23 label Jul 26, 2023
@knz knz self-assigned this Jul 27, 2023
craig bot pushed a commit that referenced this issue Jul 28, 2023
107612: serverutils: clean up some interfaces r=knz a=knz

Epic: CRDB-18499
Part of solving #107058.
Informs #106772.

In this commit:

- `TestTenantInterface` renamed to `ApplicationLayerInterface`.

- `TestServerInterface` split into:

  - `StorageLayerInterface` containing most of the methods previously
    in `TestServerInterface`.

  - `TenantControlInterface` with just the methods relating to
    starting the service for secondary tenants.

- Relevant methods moved from `TestServerInterface` to
  `ApplicationLayerInterface`:

  - `ServingSQLAddr()` - renamed to `AdvSQLAddr()`
  - `RPCAddr()`
  - `LeaseManager()`
  - `DistSenderI()`
  - `MigrationServer()`
  - `SetDistSQLSpanResolver()`
  - `CollectionFactory()`
  - `SystemTableIDResolver()`

- Removed `HostSQLAddr()` - use `.SystemLayer().AdvSQLAddr()` instead.

- `AdvSQLAddr()` (previously `ServingSQLAddr()`) was
  simplified to always return the advertised SQL address for the
  current layer.

- Updated comments/docstrings for the interfaces.

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
craig bot pushed a commit that referenced this issue Jul 28, 2023
107391: roachtest: include link to testeng grafana in issue posts r=smg260,tbg a=annrpom

This adds a link, populated with relevant cluster name and test timeframe, to the testeng grafana instance for failed roachtests.

Fixes: #105894
Release note: None

107659: serverutils: provide SQLConn/SQLConnE in ApplicationLayerInterface r=stevendanna a=knz

Fixes  #107672.
Part of solving #107058.
Informs #106772.

Epic: CRDB-18499



107697: rpc: avoid crash in newPeer r=erikgrinaker a=tbg

It was previously possible to make a new peer while the old one was in the
middle of being deleted, which caused a crash due to to acquiring child metrics
when they still existed.

Luckily, this is easy enough to fix: just remove some premature optimization
where I had tried to be too clever.

Fixes #105335.

Epic: CRDB-21710
Release note: None (bug never released)

107721: asim: skip TestAllocatorSimulatorDeterministic and example_fulldisk r=wenyihu6 a=wenyihu6

We found some non-deterministic behavior in the allocator simulator (see #105904
for more details). For now, we are skipping these potentially flaky tests.

Release Note: None 
Epic: None

107728: persistedsqlstats: specify background qos for compaction job r=xinhaoz a=xinhaoz

The compaction job can be an expensive operation so we should de-prioritize it with the `UserLow` qos setting.

Fixes: #99949

Release note: None

107750: ui: fix app = empty string filter on stmts page r=xinhaoz a=xinhaoz

The filter on app name = empty string was not working on
the stmts page. This was due to the fact that we use (unset)
as the option in the filter to represent selecting the empty
string app name. However when filtering statements, the empty
string app name on the stmt was not changed accordingly.
this commit fixes this and also adds testing for the unset case.

Epic: none
Fixes: #107748

Release note (bug fix): Filter on stmts page works for
app name = empty string (represented as 'unset').

https://www.loom.com/share/2fee4f0fb7b04208803e0dac1d9694ab?sid=5cabecf9-1c2a-406b-89a8-b378ed07d329



107753: backupccl: deflake TestBackupAndRestoreJobDescription r=stevendanna a=adityamaru

This change sorts the jobs based on when they
were created to ensure we get a stable sort of
job descriptions.

Fixes: #107684
Release note: None

Co-authored-by: Annie Pompa <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: wenyihu6 <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: adityamaru <[email protected]>
@craig craig bot closed this as completed in d9c137d Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) db-cy-23 T-multitenant Issues owned by the multi-tenant virtual team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant