remove neon_local --pageserver-config-override
#7614
Merged
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
…side of `neon_local init` The `NeonCli.init()` persists the non-default pageserver config values for remote storage & `NeonEnvBuilder.pageserver_config_override` in `pageserver.toml`. We don't need to repeat them on each pageserver start after that.
…blame/test-suite-narrow-pageserver-config-override
Rewrite the pageserver.toml instead.
…--pageserver-config-override`
This allows inlining append_pageserver_param_overrides into NeonCli.init()
…ng init, not start
…age during init, not start" This reverts commit 511f593.
…into problame/remove-pageserver-config-overrides
…akes a toml tempfile
…into problame/remove-pageserver-config-overrides
…into problame/remove-pageserver-config-overrides
Conflicts:
control_plane/src/pageserver.rs
=> pick ours
…into problame/remove-pageserver-config-overrides
problame
changed the title
neon_local --pageserver-config-override
problame
changed the base branch from
problame/remove-pageserver-update-config-flag
to
problame/test-suite-narrow-pageserver-config-override
koivunej
reviewed
May 6, 2024
problame
commented
May 6, 2024
|
Congrats! This had so large test report message that github refused it :) This is the first I've seen such. From the logs this seems to be quite usual failure: https://neon-github-public-dev.s3.amazonaws.com/reports/pr-7614/8967600476/index.html#suites/7745dadbd815ab87f5798aa881796f47/baabb8517aad95b1 |
Base automatically changed from
problame/test-suite-narrow-pageserver-config-override
to
main
May 6, 2024 20:31
2970 tests run: 2843 passed, 0 failed, 127 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
3ab3007 at 2024-05-07T16:37:50.860Z :recycle: |
koivunej
approved these changes
May 7, 2024
|
The compat tests are failing because the old This is immaterial to
|
…erver-config-overrides
problame
added
the
forward compatibility breakage
The test_forward_compatibility test runs the old production binaries, but is supposed to always run the latest neon_local binary. I think commit 6acbee2 broke that by accident because in that commit, from_repo_dir is introduced and runs an `init_start()` before the `test_forward_compatibility` gets a chance to patch up the neon_local_binpath.
problame
removed
the
forward compatibility breakage
|
Extracted fix for test_forward_compatibility into PR #7637 , this one will be rebased after it gets merged. |
conradludgate
pushed a commit
that referenced
this pull request
May 8, 2024
Preceding PR #7613 reduced the usage of `--pageserver-config-override`. This PR builds on top of that work and fully removes the `neon_local --pageserver-config-override`. Tests that need a non-default `pageserver.toml` control it using two options: 1. Specify `NeonEnvBuilder.pageserver_config_override` before `NeonEnvBuilder.init_start()`. This uses a new `neon_local init --pageserver-config` flag. 2. After `init_start()`: `env.pageserver.stop()` + `NeonPageserver.edit_config_toml()` + `env.pageserver.start()` A few test cases were using `env.pageserver.start(overrides=("--pageserver-config-override...",))`. I changed them to use one of the options above. Future Work ----------- The `neon_local init --pageserver-config` flag still uses `pageserver --config-override` under the hood. In the future, neon_local should just write the `pageserver.toml` directly. The `NeonEnvBuilder.pageserver_config_override` field should be renamed to `pageserver_initial_config`. Let's save this churn for a separate refactor commit.
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.
Preceding PR #7613 reduced the usage of
--pageserver-config-override.This PR builds on top of that work and fully removes the
neon_local --pageserver-config-override.Tests that need a non-default
pageserver.tomlcontrol it using two options:NeonEnvBuilder.pageserver_config_overridebeforeNeonEnvBuilder.init_start(). This uses a newneon_local init --pageserver-configflag.init_start():env.pageserver.stop()+NeonPageserver.edit_config_toml()+env.pageserver.start()A few test cases were using
env.pageserver.start(overrides=("--pageserver-config-override...",)).I changed them to use one of the options above.
Future Work
The
neon_local init --pageserver-configflag still usespageserver --config-overrideunder the hood. In the future, neon_local should just write thepageserver.tomldirectly.The
NeonEnvBuilder.pageserver_config_overridefield should be renamed topageserver_initial_config. Let's save this churn for a separate refactor commit.