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

Dump current tenant config #3534

Merged
merged 7 commits into from
Feb 3, 2023
Merged

Dump current tenant config #3534

merged 7 commits into from
Feb 3, 2023

Conversation

SomeoneToIgnore
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore commented 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:

@SomeoneToIgnore SomeoneToIgnore requested review from a team as code owners February 3, 2023 11:50
@SomeoneToIgnore SomeoneToIgnore requested review from funbringer, hlinnaka and problame and removed request for a team February 3, 2023 11:50
@koivunej
Copy link
Member

koivunej commented Feb 3, 2023

The second bullet point sounds like this will fix #3471. Could you confirm, edit Fixes #3471 into the description?

@SomeoneToIgnore
Copy link
Contributor Author

SomeoneToIgnore commented Feb 3, 2023

Yes, looks it is the one. I've embedded the test for this fix with the new http endpoint value checks, but presumably can use old sql api to check these values too and separate into another PR.
Not sure if it's worth it, though.

The fix without tests is in its own commit: dc688af

Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

I think this is looking good. As always, some related and unrelated questions :)

libs/pageserver_api/src/models.rs Show resolved Hide resolved
pageserver/src/http/routes.rs Show resolved Hide resolved
pageserver/src/tenant.rs Show resolved Hide resolved
pageserver/src/http/routes.rs Show resolved Hide resolved
Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Oh yeah, we should update openapi.

@SomeoneToIgnore
Copy link
Contributor Author

we should update openapi

This time, I agree fully.
Interesting, that the API schema is simple and reuses the response schema which exactly what I'd like to see in the rust code.

Copy link
Contributor

@problame problame left a comment

Choose a reason for hiding this comment

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

This mostly LGTM, apart from the discussion regarding persistence of the tenant config.

Let's unblock this PR by changing it to the old behavior (apply in memory, and best-effort save to disk).

pageserver/src/http/routes.rs Outdated Show resolved Hide resolved
pageserver/src/http/routes.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/mgr.rs Show resolved Hide resolved
pageserver/src/tenant/mgr.rs Show resolved Hide resolved
Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Aah yes, lets unblock this. Please include the other fixed issue in the description as well! Both #3471 and #3472. Well, at least mention it if the old behaviour on disk-full is preserved.

@SomeoneToIgnore
Copy link
Contributor Author

Thank you, now have understood indeed and added another issue in the description.
Looks like the current non-updateable behavior is fine for now, so merging the PR.

@SomeoneToIgnore SomeoneToIgnore enabled auto-merge (squash) February 3, 2023 23:11
@SomeoneToIgnore SomeoneToIgnore merged commit ec3a3ae into main Feb 3, 2023
@SomeoneToIgnore SomeoneToIgnore deleted the kb/dump-tenant-config branch February 3, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants