[DNM/PoC]: use lsn leases to temporarily block gc

[yliang412]

Part of

• Epic: pageserver: LSN "leases" to block GC temporarily #7497

Problem

This pull request is a PoC opening for early reviews and comments. The feedbacks will incorporate into later PRs.

With the LSN lease API introduced in #7808, we want to implement the real lease logic to prevent GC from proceeding pass some LSN when garbage collecting layers in a timeline.

Design doc at https://www.notion.so/neondatabase/LSN-Lease-Design-f8aa8333a9b7431d9905785ba7599745?pvs=4.

Summary of changes

• Maintain a in-memory mapping of LSN to lease expiration time in GCInfo.
• Change leasing period to 10 min (based on discussion here to make the lease heartbeat rare, still seeking feedback)
• Remove expired lease from the map when refreshing GCInfo.
• Block GC (similar to how we uses retain_lsns).
• A unit test that demonstrates the concept.
• Delay GC by lease period at startup.

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]

No tests were run or test report is not available

Test coverage report is not available

_{The comment gets automatically updated with the latest test results
db378d1 at 2024-06-17T19:19:33.110Z :recycle:}

[skyzh]

Current implementation would keep everything above the lease. Would be better if this is handled in the same way as retain_lsn so that we only keep necessary images? I forgot where our discussion last time ends up in.

[yliang412]

I'm actually trying to handle it the same way as what retain_lsns does right now. The way current GC uses retain_lsns is that for each layer it loops through the retain_lsn in retain_lsns, and keeping the layer if layer.get_lsn_range().start <= retain_lsn.

In my case, I'm doing layer.get_lsn_range().start <= maximum_lsn_with_valid_lease (If a layer's lsn_range overlaps with the maximum_lsn_with_valid_lease, then all other LSNs with valid leases will do as well).

neon/pageserver/src/tenant/timeline.rs

Lines 5051 to 5083 in 4f13e03

	// 3. Is it needed by a child branch?
	// NOTE With that we would keep data that
	// might be referenced by child branches forever.
	// We can track this in child timeline GC and delete parent layers when
	// they are no longer needed. This might be complicated with long inheritance chains.
	//
	// TODO Vec is not a great choice for `retain_lsns`
	for retain_lsn in &retain_lsns {
	// start_lsn is inclusive
	if &l.get_lsn_range().start <= retain_lsn {
	debug!(
	"keeping {} because it's still might be referenced by child branch forked at {} is_dropped: xx is_incremental: {}",
	l.layer_name(),
	retain_lsn,
	l.is_incremental(),
	);
	result.layers_needed_by_branches += 1;
	continue 'outer;
	}
	}
	// 3.5 Is there a valid lease that requires us to keep this layer?
	if let Some(lsn) = &max_lsn_with_valid_lease {
	if &l.get_lsn_range().start <= lsn {
	debug!(
	"keeping {} because there is a valid lease preventing GC at {}",
	l.layer_name(),
	lsn,
	);
	result.layers_needed_by_leases += 1;
	continue 'outer;
	}
	}

Maybe I miss the part where retain_lsns only keep necessary images? One way we could do that is to maybe check the end of the lsn range as well?

[skyzh]

Okay seems like retain_lsn did not optimize for branches (it keeps everything above the oldest branch). Would be a future optimization.

[yliang412]

layer.get_lsn_range().start <= min_lsn_with_valid_lease <= all other lsn with valid leases.

[yliang412]

The comparison should actually be against max_lsn_with_valid_lease since we keep a layer if the layer start is <= any of the lease.

[koivunej]

Looking good, few smaller comments.

[skyzh]

LGTM. Could have some e2e test once the compute side of this pull request gets merged. Besides the "one compute should hold one lease" problem I mentioned in the review comments, I also prefer that at some point in the future gc_timeline should take all leases instead of only the min lease. In theory, we only need to keep the image layers for each of the lease/branch LSN, as an future optimization.

[skyzh]

I feel there is something not optimal of the lease design. Imagine a compute node updates the lease every second and the default lease length is 300s, we will easily accumulate 300 leases in memory. I feel a better design is to have the lease request come with <compute_unique_id, lsn>, so that one compute maintains exactly one lease in the system.

[skyzh]

something to be fixed in the future, could create an issue

[yliang412]

For this I was thinking that compute will basically consume the valid_until expiration time and renew the lease a little bit before that?

[skyzh]

well, it depends on the cplane people, but I do feel usually heartbeats are designed in a way that the interval is 1/10 of the expiration interval.

[jcsp]

We keep the leases in a map of LSN->lease, right? A given compute will keep requesting the same LSN repeatedly, so the overall number of leases we track shouldn't increase.

[skyzh]

feel free to create a tracking issue if this is something you plan to work on in short term

[yliang412]

The code changes and comments in this PR have been addressed in #8084. Closing it for now.