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

Changing tenant configuration writes only changed values to disk, continues with different config #3471

Closed
koivunej opened this issue Jan 27, 2023 · 0 comments · Fixed by #3534
Labels
c/storage/pageserver Component: storage: pageserver

Comments

@koivunej
Copy link
Member

Modifying the added test in #3446 to instead of changing gc_horizon twice to in different http PUT requests:

  1. set gc_horizon
  2. set pitr_interval

Will write to disk as last version:

# This file contains a specific per-tenant's config.
#  It is read in case of pageserver restart.

[tenant_config]
pitr_interval = "0s"

But adding logging to Tenant::update_tenant_config shows that the tenant config is first updated to:

2023-01-27T15:27:39.514582Z  INFO tenant_config{tenant=db6238f481f8459ce03705f1fa96ab42}: updated config TenantConfOpt {
    checkpoint_distance: None,
    checkpoint_timeout: None,
    compaction_target_size: None,
    compaction_period: None,
    compaction_threshold: None,
    gc_horizon: Some(
        1000000,
    ),
    gc_period: None,
    image_creation_threshold: None,
    pitr_interval: None,
    walreceiver_connect_timeout: None,
    lagging_wal_timeout: None,
    max_lsn_wal_lag: None,
    trace_read_requests: None,
}

Then by the second request:

2023-01-27T15:27:39.521233Z  INFO tenant_config{tenant=db6238f481f8459ce03705f1fa96ab42}: updated config TenantConfOpt {
    checkpoint_distance: None,
    checkpoint_timeout: None,
    compaction_target_size: None,
    compaction_period: None,
    compaction_threshold: None,
    gc_horizon: Some(
        1000000,
    ),
    gc_period: None,
    image_creation_threshold: None,
    pitr_interval: Some(
        0ns,
    ),
    walreceiver_connect_timeout: None,
    lagging_wal_timeout: None,
    max_lsn_wal_lag: None,
    trace_read_requests: None,
}

What should happen, the written to disk version should be the union of settings (with default tenant config values removed):

# This file contains a specific per-tenant's config.
#  It is read in case of pageserver restart.

[tenant_config]
gc_horizon = 1000000
pitr_interval = "0s"

Perhaps we should also add a GET /v1/tenant/:tenant_id/config to get the current tenant config out, so that we can easily compare it always matches the one on disk after single-threaded updates over HTTP.

Origin:

@koivunej koivunej added the c/storage/pageserver Component: storage: pageserver label Jan 27, 2023
SomeoneToIgnore pushed a commit that referenced this issue Feb 3, 2023
The PR adds an endpoint to show tenant's current config: `GET
/v1/tenant/:tenant_id/config`

Tenant's config consists of two parts: tenant overrides (could be
changed via other management API requests) and the default part,
substituting all missing overrides (constant, hardcoded in pageserver).
The API returns the custom overrides and the final tenant config, after
applying all the defaults.

Along the way, it had to fix two things in the config:

* allow to shorten the json version and omit all `null`'s (same as toml
serializer behaves by default), and to understand such shortened format
when deserialized. A unit test is added
* fix a bug, when `PUT /v1/tenant/config` endpoint rewritten the local
file with what had came in the request, but updating (not rewriting the
old values) the in-memory state instead.
That got uncovered during adjusting the e2e test and fixed to do the
replacement everywhere, otherwise there's no way to revert existing
overrides. Fixes #3471 (commit
dc688af)
* fixes #3472 by reordering
the config saving operations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/storage/pageserver Component: storage: pageserver
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant