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.

Sign up for GitHub

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

Jump to bottom

pageserver: consistent validation of tenant conf #5300

Closed
jcsp opened this issue Sep 13, 2023 · 0 comments · Fixed by #5310
Closed

pageserver: consistent validation of tenant conf #5300

jcsp opened this issue Sep 13, 2023 · 0 comments · Fixed by #5310
Labels
a/tech_debt Area: related to tech debt c/storage/pageserver Component: storage: pageserver m/good_first_issue Moment: when doing your first Neon contributions

Comments

@jcsp
Copy link
Contributor

jcsp commented Sep 13, 2023 •
edited
Loading

The pageserver gets tenant configs into memory three ways:

  1. A TenantConf that is part of PageServerConf, loaded from pageserver.conf during startup
  2. A TenantConfOpt that is saved to disk for each tenant, and loaded on subsequent startups
  3. Calls into the HTTP API where /attach and /config calls can provide a TenantConfOpt.

TOML is serde compatible, but we have a bunch of hand-rolled code in parse_toml_tenant_conf that mainly exists to provide nicer errors than serde would give, if something invalid is in the config. This is useful for files that are liable to be edited like a human, like pageserver.conf.

That specialized loading code is only used in cases 1 & 2. In case 3, when we receive an API call relating to configuration, it is not used: a caller would get a serde-style error if they provide something invalid. In case 2, we may stop using the special loading code as well (see #5299), since the data we save per tenant has already passed through an API call and deserialized successfully there.

All this is kind of messy. We should:
@jcsp jcsp added c/storage/pageserver Component: storage: pageserver a/tech_debt Area: related to tech debt m/good_first_issue Moment: when doing your first Neon contributions labels Sep 13, 2023
@rmodpur rmodpur mentioned this issue Sep 14, 2023
Merged
shanyp added a commit that referenced this issue Nov 30, 2023
@rmodpur @shanyp @koivunej
refactor: use serde for TenantConf deserialization Fixes: #5300 (#5310)
50d959f 
Remove handcrafted TenantConf deserialization code. Use
`serde_path_to_error` to include the field which failed parsing. Leaves
the duplicated TenantConf in pageserver and models, does not touch
PageserverConf handcrafted deserialization.

Error change:
- before change: "configure option `checkpoint_distance` cannot be
negative"
- after change: "`checkpoint_distance`: invalid value: integer `-1`,
expected u64"

Fixes: #5300
Cc: #3682

---------

Signed-off-by: Rahul Modpur <rmodpur2@gmail.com>
Co-authored-by: Shany Pozin <shany@neon.tech>
Co-authored-by: Joonas Koivunen <joonas@neon.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
a/tech_debt Area: related to tech debt c/storage/pageserver Component: storage: pageserver m/good_first_issue Moment: when doing your first Neon contributions
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: use serde for TenantConf deserialization Fixes: #5300 rmodpur/neon
1 participant
@jcsp