pageserver: suppress compaction/gc errors while stopping

[jcsp]

Problem

Tenant deletions would sometimes be accompanied by compaction stack traces, because shutdown() puts the tenant into stopping state before it joins background tasks.

Summary of changes

Treat GC+Compaction as no-ops on a Stopping tenant.

[github-actions]

2346 tests run: 2229 passed, 0 failed, 117 skipped (full report)

---

Flaky tests (1)

Postgres 15

• test_timeline_init_break_before_checkpoint: debug

Code coverage (full report)

• functions: 53.6% (8576 of 16010 functions)
• lines: 80.9% (49768 of 61484 lines)

---

_{The comment gets automatically updated with the latest test results
f5b5444 at 2023-10-26T09:52:48.261Z :recycle:}

[koivunej]

Isn't there now repetition with the ensure in both (compaction, gc) cases?

We do not start the background loops before we are active, next possible steps are broken and stopping.

I am also surprised how did you get to merge this without conflicts since I did make the CompactionError visible in the #4938.

[jcsp]

The repetition is a pre-existing thing: the gc_loop and compaction_loop code paths are very similar -- here I'm aiming to make the behavioral change rather than refactor to reduce duplication.

[koivunej]

I meant repeatition between:

if let TenantState::Stopping { .. } = self.current_state()

and

anyhow::ensure!(
    self.is_active(),

And how the stopping OR broken can be the only next state.

Perhaps the suggestion I was working towards is should we instead have:

-if let TenantState::Stopping { .. } = self.current_state() {
+if !self.is_active() {
     return Ok(GcResult::default());
 }
 
-// there is a global allowed_error for this
-anyhow::ensure!(
-    self.is_active()
-);

(did not check how the ensure! block would correctly end).

[jcsp]

Ah, I see. The reason I didn't do that is because this function is called from two places: in the compaction loop I would always want to drop out with Ok(()) if we have no work, but when called via the API (e.g. in a test), I would prefer to give the caller an error if they have tried to compact prematurely during activation.

[koivunej]

Today I found out that the Tenant::compaction_iteration is not called from tests. I see that GC of course is.. I'll do what I proposed in #5881 for that so I can use the better error type.

[koivunej]

prefer to give the caller an error if they have tried to compact prematurely during activation

Related: there is a PR I intend to merge from external contributor I can never find the number of, but it would add waiting until tenants have finished loading by default to all tests so this reason would go away for gc as well.

It never existed for compaction though, as Tenant::compaction_iteration was never exposed like Tenant::gc is.