safekeeper: refactor static remote storage usage to use arc

[oldmanfleming]

Greetings! Please add w=1 to github url when viewing diff (sepcifically wal_backup.rs)

Problem

This PR is aimed at addressing the remaining work of #8200. Namely, removing static usage of remote storage in favour of arc. I did not opt to pass Arc<RemoteStorage> directly since it is actually Optional<RemoteStorage> as it is not necessarily always configured. I wanted to avoid having to pass Arc<Optional<RemoteStorage>> everywhere with individual consuming functions likely needing to handle unwrapping.

Instead I've added a WalBackup struct that holds Optional<RemoteStorage> and handles initialization/unwrapping RemoteStorage internally. wal_backup functions now take self and Arc<WalBackup> is passed as a dependency through the various consumers that need it.

Summary of changes

• Add WalBackup that holds Optional<RemoteStorage> and handles initialization and unwrapping
• Modify wal_backup functions to take WalBackup as self (Add w=1 to github url when viewing diff here)
• Initialize WalBackup in safekeeper root
• Store Arc<WalBackup> in GlobalTimelineMap and pass and store in each Timeline as loaded
• use WalBackup through Timeline as needed

[github-actions]

7121 tests run: 6824 passed, 0 failed, 297 skipped (full report)

---

Flaky tests (3)

Postgres 17

• test_idle_checkpoints: debug-x86-64
• test_pageserver_small_inmemory_layers[True]: debug-x86-64
• test_pageserver_small_inmemory_layers[False]: debug-x86-64

Code coverage* (full report)

• functions: 31.3% (8396 of 26820 functions)
• lines: 48.0% (66643 of 138945 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
0f9064d at 2024-12-18T15:18:46.358Z :recycle:}

[erikgrinaker]

Thanks for the PR, appreciate the cleanup!

I did not opt to pass Arc<RemoteStorage> directly since it is actually Optional<RemoteStorage> as it is not necessarily always configured. I wanted to avoid having to pass Arc<Optional<RemoteStorage>> everywhere with individual consuming functions likely needing to handle unwrapping.

Instead I've added a WalBackup struct that holds Optional<RemoteStorage> and handles initialization/unwrapping RemoteStorage internally.

I think checking the Option everywhere is a feature here, not a bug. I wonder if we should try to tighten this up a bit.

Currently in this PR, the caller must check either SafeKeeperConf::is_wal_backup_enabled() or WalReader::enable_remote_read before calling any methods on WalBackup, otherwise the code may panic. This is pretty easy to get wrong, and can't easily be verified for current callers at a cursory glance.

We could get the type system to enforce this for us at compile-time by storing Option<WalBackup> in Timeline and WalReader, with Some only if WAL backups should be enabled. Then it's only possible to call methods on it when it is actually enabled, avoiding the panic entirely. This also allows us to get rid of the redundant SafeKeeperConf::is_wal_backup_enabled() and WalReader::enable_remote_read.

Wdyt?

[oldmanfleming]

@erikgrinaker Yup that makes a lot of sense. Let me work on a revision along those lines