mgmt api: share a single tenant config model struct in Rust and OpenAPI #4252

problame · 2023-05-16T14:18:42Z

This is prep for #4255

[1/X] OpenAPI: share a single definition of TenantConfig

DRYs up the pageserver OpenAPI YAML's representation of
tenant config.

All the fields of tenant config are now located in a model schema
called TenantConfig.

The tenant create & config-change endpoints have separate schemas,
TenantCreateInfo and TenantConfigureArg, respectively.
These schemas inherit from TenantConfig, using allOf 1.

The tenant config-GET handler's response was previously named TenantConfig.
It's now named TenantConfigResponse.

None of these changes affect how the request looks on the wire.

The generated Go code will change for Console because the OpenAPI code
generator maps allOf to a Go struct embedding.
Luckily, usage of tenant config in Console is still very lightweigt,
but that will change in the near future.
So, this is a good chance to set things straight.

The console changes are tracked in
https://github.com/neondatabase/cloud/pull/5046

[2/x]: extract the tenant config parts of create & config requests

[3/x]: code movement: move TenantConfigRequestConfig next to TenantCreateRequestConfig

[4/x] type-alias TenantConfigRequestConfig = TenantCreateRequestConfig;
They are exactly the same.

[5/x] switch to qualified use for tenant create/config request api models

[6/x] rename models::TenantConfig{RequestConfig,} and remove the alias

[7/x] OpenAPI: sync tenant create & configure body names from Rust code

[8/x]: dedupe the two TryFrom<...> for TenantConfOpt impls
The only difference is that the TenantConfigRequest impl does

        tenant_conf.max_lsn_wal_lag = request_data.max_lsn_wal_lag;
        tenant_conf.trace_read_requests = request_data.trace_read_requests;

and the TenantCreateRequest impl does

        if let Some(max_lsn_wal_lag) = request_data.max_lsn_wal_lag {
            tenant_conf.max_lsn_wal_lag = Some(max_lsn_wal_lag);
        }
        if let Some(trace_read_requests) = request_data.trace_read_requests {
            tenant_conf.trace_read_requests = Some(trace_read_requests);
        }

As far as I can tell, these are identical.

…cessing With this patch, the attach handler now follows the same pattern as tenant create with regards to instantiation of the new tenant: 1. Prepare on-disk state using `create_tenant_files`. 2. Use the same code path as pageserver startup to load it into memory and start background loops (`schedule_local_tenant_processing`). It's a bit sad we can't use the `PageServerConfig::tenant_attaching_mark_file_path` method inside `create_tenant_files` because it operates in a temporary directory. However, it's a small price to pay for the gained simplicity. During implementation, I noticed that we don't handle failures post `create_tenant_files` well. I left TODO comments in the code linking to the issue that I created for this [^1]. Also, I'll dedupe the spawn_load and spawn_attach code in a future commit. [^1]: #4233

And make error message wording fit both create & attach use case.

This patch DRYs up the pageserver OpenAPI YAML's representation of tenant config. All the fields of tenant config are now located in a model schema called TenantConfig. The tenant create & config-change endpoints have separate schemas, TenantCreateInfo and TenantConfigureArg, respectively. These schemas inherit from TenantConfig, using allOf 1. The tenant config-GET handler's response was previously named TenantConfig. It's now named TenantConfigResponse. None of these changes affect how the request looks on the wire. The generated Go code will change for Console because the OpenAPI code generator maps `allOf` to a Go struct embedding. Luckily, usage of tenant config in Console is still very lightweigt, but that will change in the near future. So, this is a good chance to set things straight. The console changes are tracked in neondatabase/cloud#5046

…eateRequestConfig

They are exactly the same.

…dels

github-actions · 2023-05-16T14:42:16Z

992 tests run: 944 passed, 0 failed, 48 skipped (full report for `764b62e`)

Flaky tests

PostgreSQL 14 (debug build)

✅ test_runner/regress/test_proxy.py::test_close_on_connections_exit[project]

_{The comment gets automatically updated with the latest test results ♻️}

The only difference is that the TenantConfigRequest impl does ``` tenant_conf.max_lsn_wal_lag = request_data.max_lsn_wal_lag; tenant_conf.trace_read_requests = request_data.trace_read_requests; ``` and the TenantCreateRequest impl does ``` if let Some(max_lsn_wal_lag) = request_data.max_lsn_wal_lag { tenant_conf.max_lsn_wal_lag = Some(max_lsn_wal_lag); } if let Some(trace_read_requests) = request_data.trace_read_requests { tenant_conf.trace_read_requests = Some(trace_read_requests); } ``` As far as I can tell, these are identical.

shanyp · 2023-05-16T15:53:27Z

is possible that you are not rebased on main ? I am not seeing the from_request function I added

pageserver/src/http/openapi_spec.yml

…enant-config-model Conflicts: pageserver/src/tenant/config.rs Conflict with Shany's 9cd6f2c Remove duplicated logic in creating TenantConfOpt (#4230 pageserver/src/tenant/mgr.rs Minor.

problame · 2023-05-17T09:01:13Z

is possible that you are not rebased on main ? I am not seeing the from_request function I added

Yeah, I thought you had abandoned your change 9cd6f2c and didn't re-check 😞

Anyway, resolved the conflict now (by making my change win, I think it's better than having that kilometer-long param list)

problame · 2023-05-17T09:02:35Z

By the way, the reason I was able to dedupe the code was by making the TenantCreateRequest and TenantConfigRequest Deref<Target=TenantConfig>. See 8a89c86 if you're interested.

shanyp · 2023-05-17T09:43:25Z

By the way, the reason I was able to dedupe the code was by making the TenantCreateRequest and TenantConfigRequest Deref<Target=TenantConfig>. See 8a89c86 if you're interested.

That was my first approach but it broke the clients, now that I know of "allOf" I understand how can I have done it :-)

problame · 2023-05-17T10:27:37Z

now that I know of "allOf" I understand how can I have done it

Important note: it only works due to the serde(flatten).

This PR adds support for supplying the tenant config upon /attach. Before this change, when relocating a tenant using `/detach` and `/attach`, the tenant config after `/attach` would be the default config from `pageserver.toml`. That is undesirable for settings such as the PITR-interval: if the tenant's config on the source was `30 days` and the default config on the attach-side is `7 days`, then the first GC run would eradicate 23 days worth of PITR capability. The API change is backwards-compatible: if the body is empty, we continue to use the default config. We'll remove that capability as soon as the cloud.git code is updated to use attach-time tenant config (#4282 keeps track of this). unblocks neondatabase/cloud#5092 fixes #1555 part of #886 (Tenant Relocation) Implementation ============== The preliminary PRs for this work were (most-recent to least-recent) * #4279 * #4267 * #4252 * #4235

problame added 9 commits May 15, 2023 16:29

fix test failures caused by changed error message

2e63691

And make error message wording fit both create & attach use case.

[2/x]: extract the tenant config parts of create & config requests

8a89c86

[3/x]: code movement: move TenantConfigRequestConfig next to TenantCr…

899083d

…eateRequestConfig

[4/x] type-alias TenantConfigRequestConfig = TenantCreateRequestConfig;

b7397f1

They are exactly the same.

[5/x] switch to qualified use for tenant create/config request api mo…

d679311

…dels

[6/x] rename models::TenantConfig{RequestConfig,} and remove the alias

b345855

[7/x] OpenAPI: sync tenant create & configure body names from Rust code

3d8db71

problame mentioned this pull request May 16, 2023

attach-time tenant config #4255

Merged

problame requested review from LizardWizzard, shanyp and skyzh May 16, 2023 15:37

problame marked this pull request as ready for review May 16, 2023 15:37

problame requested review from a team as code owners May 16, 2023 15:37

problame requested review from knizhnik and removed request for a team May 16, 2023 15:38

Base automatically changed from problame/attach-time-tenant-config to main May 16, 2023 16:53

shanyp reviewed May 17, 2023

View reviewed changes

pageserver/src/http/openapi_spec.yml Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into problame/api-shared-t…

764b62e

…enant-config-model Conflicts: pageserver/src/tenant/config.rs Conflict with Shany's 9cd6f2c Remove duplicated logic in creating TenantConfOpt (#4230 pageserver/src/tenant/mgr.rs Minor.

shanyp approved these changes May 17, 2023

View reviewed changes

problame changed the title ~~mgmt api: share a single tenant config modle struct in Rust and OpenAPI~~ mgmt api: share a single tenant config model struct in Rust and OpenAPI May 17, 2023

problame merged commit 8930782 into main May 17, 2023

problame deleted the problame/api-shared-tenant-config-model branch May 17, 2023 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgmt api: share a single tenant config model struct in Rust and OpenAPI #4252

mgmt api: share a single tenant config model struct in Rust and OpenAPI #4252

problame commented May 16, 2023 •

edited

Loading

github-actions bot commented May 16, 2023 •

edited

Loading

PostgreSQL 14 (debug build)

shanyp commented May 16, 2023

problame commented May 17, 2023

problame commented May 17, 2023

shanyp commented May 17, 2023

problame commented May 17, 2023

mgmt api: share a single tenant config model struct in Rust and OpenAPI #4252

mgmt api: share a single tenant config model struct in Rust and OpenAPI #4252

Conversation

problame commented May 16, 2023 • edited Loading

github-actions bot commented May 16, 2023 • edited Loading

992 tests run: 944 passed, 0 failed, 48 skipped (full report for 764b62e)

PostgreSQL 14 (debug build)

shanyp commented May 16, 2023

problame commented May 17, 2023

problame commented May 17, 2023

shanyp commented May 17, 2023

problame commented May 17, 2023

problame commented May 16, 2023 •

edited

Loading

github-actions bot commented May 16, 2023 •

edited

Loading

992 tests run: 944 passed, 0 failed, 48 skipped (full report for `764b62e`)