Skip to content
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

neon_local: use pageserver.toml as source of truth for struct PageServerConf #7642

Merged
merged 61 commits into from
May 8, 2024

Conversation

problame
Copy link
Contributor

@problame problame commented May 7, 2024

Before this PR, neon_local would store a copy of a subset of the initial pageserver.toml in its .neon/config, e.g, listen_pg_addr. That copy is represented as struct PageServerConf.

This copy was used to inform e.g., neon_local endpoint and other commands that depend on Pageserver about which port to connect to.

The problem with that scheme is that the duplicated information in .neon/config can get stale if pageserver.toml is changed.

This PR fixes that by eliminating populating struct PageServerConf from the pageserver.tomls.

The [[pageservers]] TOML table in the .neon/config is obsolete.
As of this PR, neon_local will fail to start and print an error informing about this change.

Code-level changes:

  • Remove the --pg-version flag, it was only used for some checks during neon_local init
  • Remove the warn-but-continue behavior for when auth key creation fails but auth keys are not required. It's just complexity that is unjustified for a tool like neon_local.
  • Introduce a type-system-level distinction between the runtime state and the two (!) toml formats that are almost the same but not quite.
    • runtime state: struct PageServerConf, now without serde derives
    • toml format 1: the state in .neon/config => struct OnDiskState
    • toml format 2: the neon_local init --config TMPFILE that, unlike struct OnDiskState, allows specifying pageservers
  • Remove [[pageservers]] from the struct OnDiskState and load the data from the individual pageserver.tomls instead.

problame added 30 commits May 3, 2024 12:35
…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.
This allows inlining append_pageserver_param_overrides into NeonCli.init()
…age during init, not start"

This reverts commit 511f593.
…into problame/remove-pageserver-config-overrides
…into problame/remove-pageserver-config-overrides
…into problame/remove-pageserver-config-overrides

Conflicts:
	control_plane/src/pageserver.rs
    => pick ours
@problame problame requested a review from VladLazar May 7, 2024 17:45
@problame problame marked this pull request as ready for review May 7, 2024 17:58
Copy link

github-actions bot commented May 8, 2024

3024 tests run: 2891 passed, 0 failed, 133 skipped (full report)


Code coverage* (full report)

  • functions: 31.4% (6313 of 20125 functions)
  • lines: 47.3% (47575 of 100666 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
c9f63e9 at 2024-05-08T14:40:20.773Z :recycle:

Base automatically changed from problame/neon-local-init-dont-use-config-override to main May 8, 2024 09:03
Copy link
Contributor

@VladLazar VladLazar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot. Played around with it locally and it makes it easier to hack on things.

control_plane/src/pageserver.rs Outdated Show resolved Hide resolved
@problame problame enabled auto-merge (squash) May 8, 2024 14:00
@problame problame merged commit 8728d5a into main May 8, 2024
49 checks passed
@problame problame deleted the problame/neon-local-pageserver-toml-source-of-truth branch May 8, 2024 14:32
a-masterov pushed a commit that referenced this pull request May 20, 2024
…ServerConf` (#7642)

Before this PR, `neon_local` would store a copy of a subset of the
initial `pageserver.toml` in its `.neon/config`, e.g, `listen_pg_addr`.
That copy is represented as `struct PageServerConf`.

This copy was used to inform e.g., `neon_local endpoint` and other
commands that depend on Pageserver about which port to connect to.

The problem with that scheme is that the duplicated information in
`.neon/config` can get stale if `pageserver.toml` is changed.

This PR fixes that by eliminating populating `struct PageServerConf`
from the `pageserver.toml`s.

The `[[pageservers]]` TOML table in the `.neon/config` is obsolete.
As of this PR, `neon_local` will fail to start and print an error
informing about this change.

Code-level changes:

- Remove the `--pg-version` flag, it was only used for some checks
during `neon_local init`
- Remove the warn-but-continue behavior for when auth key creation fails
but auth keys are not required. It's just complexity that is unjustified
for a tool like `neon_local`.
- Introduce a type-system-level distinction between the runtime state
and the two (!) toml formats that are almost the same but not quite.
  - runtime state: `struct PageServerConf`, now without `serde` derives
  - toml format 1: the state in `.neon/config` => `struct OnDiskState`
- toml format 2: the `neon_local init --config TMPFILE` that, unlike
`struct OnDiskState`, allows specifying `pageservers`
- Remove `[[pageservers]]` from the `struct OnDiskState` and load the
data from the individual `pageserver.toml`s instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants