switch pytests and neon_local to control_plane_hooks_api #11195
Conversation
7909 tests run: 7526 passed, 0 failed, 383 skipped (full report)Flaky tests (2)Postgres 17
Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
5fe7fa8 at 2025-03-12T16:13:53.467Z :recycle: |
| @@ -101,7 +101,7 @@ changes such as a pageserver node becoming unavailable, or the tenant's shard co | |||
| postgres clients to handle such changes, the storage controller calls an API hook when a tenant's pageserver | |||
| location changes. | |||
|
|
|||
| The hook is configured using the storage controller's `--compute-hook-url` CLI option. If the hook requires | |||
| The hook is configured using the storage controller's `--control-plane-url` CLI option. If the hook requires | |||
There was a problem hiding this comment.
This should specify what the sub-paths of control-plane-url are -- otherwise the reader would have to read the code to find out that their notification hook needs to be called notify-attach
| @@ -363,6 +363,15 @@ pub struct Config { | |||
| /// assume it is running in a test environment and try to update neon_local. | |||
| pub compute_hook_url: Option<String>, | |||
There was a problem hiding this comment.
For external folks using our binaries (people in discord have asked about the hook before), I guess we will need to maintain this for some time & enable working with a pageserver hook but no safekeeper hook. That's okay, we can clean it up eventually, just have to give folks in #neon-oss and #self-hosted some advance notice.
There was a problem hiding this comment.
Fair points, do you think we should also keep support of that field in neon_local?
We want to switch away from and deprecate the
--compute-hook-urlparam for the storcon in favour of--control-plane-urlbecause it allows us to construct urls withnotify-safekeepers.This PR switches the pytests and neon_local from a
control_plane_compute_hook_apito a new param namedcontrol_plane_hooks_apiwhich is supposed to point to the parent of thenotify-attachURL.We still support reading the old url from disk to not be too disruptive with existing deployments, but we just ignore it.
Follow-up of #11173
Part of #11163