storage scrubber: GC ancestor shard layers

[jcsp]

Problem

After a shard split, the pageserver leaves the ancestor shard's content in place. It may be referenced by child shards, but eventually child shards will de-reference most ancestor layers as they write their own data and do GC. We would like to eventually clean up those ancestor layers to reclaim space.

Summary of changes

• Extend the physical GC command with --mode=full, which includes cleaning up unreferenced ancestor shard layers
• Add test test_scrubber_physical_gc_ancestors
• Remove colored log output: in testing this is irritating ANSI code spam in logs, and in interactive use doesn't add much.

Checklist before requesting a review

• I have performed a self-review of my code.
• If it is a core feature, I have added thorough tests.
• Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
• If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

• Do not forget to reformat commit message to not include the above checklist

[github-actions]

2958 tests run: 2841 passed, 0 failed, 117 skipped (full report)

---

Flaky tests (1)

Postgres 15

• test_pg_regress[None]: debug

Code coverage* (full report)

• functions: 32.7% (6913 of 21142 functions)
• lines: 50.0% (54203 of 108388 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
88dfeaa at 2024-07-01T11:29:26.502Z :recycle:}

[problame]

We should clean up the variable names when speaking about tenants (tenant_shard_id vs tenant_id).

---

I would prefer we call it parent shard, so at least there is a minimal distinction from the word ancestor which we already use for Timeline.

---

What if the remote storage lists some but not all shards, e.g. because we're in the middle of Tenant::split_prepare ?

Edit: ah, nice, you handled that.

Kind of an important thing to regress-test. Shouldn't be too hard to test this with a pausable failpoint, should it?

Edit 2: but, did you also consider the same condition for the stream_tenant_timelines call made by gc_ancestor? I think it can observe a tenant shard in S3 but some of its timelines' IndexParts haven't been uploaded by Tenant::split_prepare yet.

Let's make sure we understand which races can happen and why they are fine / how they are handled. And: where will we document this? module-level comment doesn't seem like the worst choice, but, hard to discover.

---

About AncestorRefs: why is it not a more phyiscal refcount map, like on the absolute_key ? Sure, a bit less memory efficient but much harder to screw up. Similarly, I don't think we should go through the trouble of only tracking the cross-ancestor references.

I'm feeling kind of strongly about this, I think the code could be much simpler and lower risk.

[problame]

Is that 1s referring to the min_age_secs=1` below?

Suggested change

	# We will use a 1s threshold for deletion, let it pass
	time.sleep(2)
	# We will use a min_age_secs=1 second threshold for deletion, let it pass
	time.sleep(2)

[problame]

Type hinting is useful for "find callers" down the road.
(This suggestion requires NeonEnv to be added to the imports)

Suggested change

	env.storage_controller.tenant_policy_update(tenant_id, {"placement": "Detached"})
	def drop_local_state(env: NeonEnv, tenant_id):

[problame]

This is not summing up ancestor_layers_deleted.
Let's assert it to be zero at this point.

Also, I would recommend destructuring to get compile-time errors when we extend GcSummary in the future

Suggested change

	while let Some(i) = timelines.next().await {
	let tl_summary = i?;
	summary.indices_deleted += tl_summary.indices_deleted;
	summary.remote_storage_errors += tl_summary.remote_storage_errors;
	while let Some(i) = timelines.next().await {
	let GcSummary { indices_deleted, remote_storage_errors, ancestor_layers_deleted } = i?;
	summary.indices_deleted += indices_deleted;
	summary.remote_storage_errors += remote_storage_errors;
	assert_eq!(anecstor_layers_deleted, , "we do ancestor layer deletions below in gc_ancestor()");

[problame]

readability: please use some itertools::partition here so the .filter() condition doesn't have to be repeated and we operate on proper names instead of shard_indices.

[problame]

I would prefer this to be a newtype with a well-defined interface instead of the broad BTreeMap API. Ideally in its own mod { ... } block so visibiltiy rules are enforced.
Mostly because it makes the callsites more expressive.

[problame]

If we bail here, the accumulator doesn't contain this ttid's references. Thus we don't have the global view and must not do gc_ancestor. But currently we do.

[problame]

Commented out code tends to rot. I'm ok with if false { ... }

[problame]

The S3 SDK implements a

impl TryFrom<DateTime> for SystemTime {

[problame]

I think sprinkling in some humantime::Duration would be appreciated by whatever poor soul will have to read these logs?