storage controller: API + CLI for migrating secondary locations #10284
+210
−19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jcsp
added
t/feature
jcsp
commented
Jan 6, 2025
| @@ -690,7 +690,8 @@ async fn handle_node_list(req: Request<Body>) -> Result<Response<Body>, ApiError | |||
| }; | |||
|
|
|||
| let state = get_state(&req); | |||
| let nodes = state.service.node_list().await?; | |||
| let mut nodes = state.service.node_list().await?; | |||
There was a problem hiding this comment.
Deterministic order to enable the storcon CLI test assertions to run reliably with multiple nodes
jcsp
commented
Jan 6, 2025
| @@ -1742,42 +1760,88 @@ def storcon_cli(args): | |||
| storcon_cli(["node-configure", "--node-id", "1", "--availability", "offline"]) | |||
| assert "Offline" in storcon_cli(["nodes"])[3] | |||
|
|
|||
| # Restore node, verify status changes in CLI output | |||
There was a problem hiding this comment.
This is necessary because we now have two nodes, so making one unavailable kicks off migrations
jcsp
commented
Jan 6, 2025
| env.start() | ||
|
|
||
| tenant_id = TenantId.generate() | ||
| env.create_tenant(tenant_id, placement_policy='{"Attached":1}', shard_count=shard_count) |
There was a problem hiding this comment.
Switch to explicit tenant creation because the default tenant in NeonEnv doesn't have secondaries.
jcsp
commented
Jan 6, 2025
| @@ -935,7 +935,7 @@ def test_storage_controller_debug_apis(neon_env_builder: NeonEnvBuilder): | |||
| that just hits the endpoints to check that they don't bitrot. | |||
| """ | |||
|
|
|||
| neon_env_builder.num_pageservers = 2 | |||
| neon_env_builder.num_pageservers = 3 | |||
There was a problem hiding this comment.
3 pageservers because we need some node that contains neither an attached nor secondary location, in order to migrate a secondary there
7227 tests run: 6875 passed, 0 failed, 352 skipped (full report)Flaky tests (2)Postgres 15
Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
39a98f7 at 2025-01-06T13:33:57.566Z :recycle: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Currently, if we want to move a secondary there isn't a neat way to do that: we just have migration API for the attached location, and it is only clean to use that if you've manually created a secondary via pageserver API in the place you're going to move it to.
Secondary migration API enables:
Summary of changes
/migrate_secondaryAPItenant-shard-migrate-secondaryCLI