-
Notifications
You must be signed in to change notification settings - Fork 396
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
feat(compute_ctl): add periodic lease lsn
requests for static computes
#7994
base: main
Are you sure you want to change the base?
Conversation
2120 tests run: 2051 passed, 0 failed, 69 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
3c988d6 at 2024-08-12T15:30:12.809Z :recycle: |
291b2d3
to
3d1bcc9
Compare
Testing:
|
@jcsp about test_ondemand_download_replica test. I think it's a bug in the test, it actually creates static endpoint instead of replica. https://github.com/neondatabase/neon/blob/main/control_plane/src/bin/neon_local.rs#L816 For hot_standby=false (default) it creates Static. And it doesn't make sense to provide lsn if you expect Replica. here is the test
|
@hlinnaka could you please check my assamption that the test doesn't do what is expected #7994 (comment)? |
It's indeed testing on-demand SLRU download in a static endpoint, rather than a replica that would follow the primary. That's intentional, but I agree the naming is misleading. Is a "static endpoint" a "replica"? Or is it a replica only if it follows the primary? Our terminology is not very well defined. And it would be good to also test on-demand SLRU download in a hot standby replica that follows the primary. We don't have a test for that currently. |
More investigation on failed tests:
env.pageserver.allowed_errors.extend(
[
".*basebackup .* failed: invalid basebackup lsn.*",
".*page_service.*error obtaining lsn lease.*.*tried to request a page version that was garbage collected",
]
)
|
…8254) ## Problem LSN Leases introduced in #8084 is a new API that is made shard-aware from day 1. To support ephemeral endpoint in #7994 without linking Postgres C API against `compute_ctl`, part of the sharding needs to reside in `utils`. ## Summary of changes - Create a new `shard` module in utils crate. - Move more interface related part of tenant sharding API to utils and re-export them in pageserver_api. Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
a3a07db
to
79f9161
Compare
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
…8254) ## Problem LSN Leases introduced in #8084 is a new API that is made shard-aware from day 1. To support ephemeral endpoint in #7994 without linking Postgres C API against `compute_ctl`, part of the sharding needs to reside in `utils`. ## Summary of changes - Create a new `shard` module in utils crate. - Move more interface related part of tenant sharding API to utils and re-export them in pageserver_api. Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
c051403
to
2d9267f
Compare
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
pageserver/src/page_service.rs
Outdated
}); | ||
} | ||
tracing::info!( | ||
"requesting a leased lsn {} below gc cutoff {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so every read request will print a line of info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and having a read lock here seems costly, not sure if it is a good idea to change latest_gc_cutoff_lsn
Rcu into latest_gc_lease_cutoff_lsn
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and having a read lock here seems costly, not sure if it is a good idea to change latest_gc_cutoff_lsn Rcu into latest_gc_lease_cutoff_lsn?
Are you talking about the gc_info
read lock? We could do latest_gc_lease_cutoff_lsn
but don't know what value it should be. It cannot be the lowest leased LSN because we don't know if everything between latest_gc_cutoff_lsn and latest_gc_lease_cutoff_lsn are valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
info log removal: 3c988d6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like with the logging, we will only inspect the read lock when we are actually below the gc cutoff which is already rare, so I don't think the read lock taking will be significant.
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
Signed-off-by: Yuchen Liang <yuchen@neon.tech>
thread::spawn(move || { | ||
if let Err(e) = lsn_lease_bg_task(compute, lsn) { | ||
// TODO: might need stronger error feedback than logging an warning. | ||
warn!("lsn_lease_bg_task failed: {e}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So initially like this, but surely we should kill the compute eventually? Perhaps something to consider is that until we do kill it, it might be good keep re-trying the lease.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posted some similar thoughts bellow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, but left some comments to consider. Let me know what do you think
|
||
let spec = state.pspec.as_ref().expect("spec must be set"); | ||
|
||
let configs = postgres_configs_from_state(&state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List of pageservers is dynamic and can be reconfigured, so that's right that we refresh it. Yet, we do it before acquire_lsn_lease_with_retry
, which I think makes it almost useless
Imagine the case
- We fetched list of shards
- It got changed
- In
acquire_lsn_lease_with_retry
we will hit the offline pageserver, so retries won't help. We will exhaust theMAX_ATTEMPTS
and exit thread with error
I'm thinking that it'd be better to refresh the list of shards/confis before each attempt vs. in each lease iteration
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And NIT here for the code structure: logic from the postgres_configs_from_state
can be just inlined into acquire_lsn_lease_with_retry
.max(valid_duration / 2); | ||
|
||
info!( | ||
"lsn_lease_request succeeded, sleeping for {} seconds", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: lsn_lease_request
here and in the message above looks like a func/method name, but it isn't
lsn: Lsn, | ||
) -> Result<SystemTime> { | ||
let mut client = config.connect(NoTls)?; | ||
let cmd = format!("lease lsn {} {} {} ", tenant_shard_id, timeline_id, lsn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything in the pageserver lease lsn
API that can signal the caller that it's impossible to obtain the lease?
This is actually a broader question about the retry mechanism. If I calculate it right, 10 attempts with current backoff are slightly under 1 min, and we have seen in the past some pageservers being unresponsive >1 min due to restart, iirc
I'd be much more confident in a simpler yet persistent logic -- just catch and retry any errors indefinitely, BUT only if it's not a permanent error, i.e. we already raced with GC, so acquiring the lease is impossible.
thread::spawn(move || { | ||
if let Err(e) = lsn_lease_bg_task(compute, lsn) { | ||
// TODO: might need stronger error feedback than logging an warning. | ||
warn!("lsn_lease_bg_task failed: {e}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posted some similar thoughts bellow
ComputeMode::Static(lsn) => lsn, | ||
_ => return, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started writing that to propose you to add a compute feature flag and use it here
https://github.com/neondatabase/neon/blob/6949b45e1795816507f5025a474e15d718e07456/libs/compute_api/src/spec.rs#L34C23-L34C37
But then realized that currently, the control plane doesn't start static endpoints at all, and there is a separate feature flag to start using them static_ephemeral_endpoints
https://github.com/neondatabase/cloud/blob/cfdadee070fa3503048ef7242da50f26bed1c4b0/goapp/internal/dto/account_settings.go#L146
And it means when we enable it -- your code will kick in, but when it's disabled, it shouldn't affect any other workload
So leaving it just for the context, maybe someone didn't know about these flags :)
Part of #7497
Problem
Static computes pinned at some fix LSN could be created initially within PITR interval but eventually go out it. To make sure that Static computes are not affected by GC, we need to start using the LSN lease API (introduced in #8084) in compute_ctl.
Summary of changes
compute_ctl
test_readonly_node_gc
to test if static compute can read all pages without error.page_service
wait_or_get_last_lsn
will now allowrequest_lsn
less thanlatest_gc_cutoff_lsn
to proceed if there is a lease onrequest_lsn
.Checklist before requesting a review
Checklist before merging