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

pageserver/storcon: add patch endpoints for tenant config metrics #10020

Merged
merged 13 commits into from
Dec 11, 2024

Conversation

VladLazar
Copy link
Contributor

@VladLazar VladLazar commented Dec 4, 2024

Problem

Cplane and storage controller tenant config changes are not additive. Any change overrides all existing tenant configs. This would be fine if both did client side patching, but that's not the case.

Once this merges, we must update cplane to use the PATCH endpoint.

Summary of changes

High Level

Allow for patching of tenant configuration with a PATCH /v1/tenant/config endpoint.
It takes the same data as it's PUT counterpart. For example the payload below will update gc_period and unset compaction_period. All other fields are left in their original state.

{
  "tenant_id": "1234",
  "gc_period": "10s",
  "compaction_period": null
}

Low Level

  • PS and storcon gain PATCH /v1/tenant/config endpoints. PS endpoint is only used for cplane managed instances.
  • storcon_cli is updated to have separate commands for set-tenant-config and patch-tenant-config

Related https://github.com/neondatabase/cloud/issues/21043

Copy link

github-actions bot commented Dec 4, 2024

7792 tests run: 7461 passed, 0 failed, 331 skipped (full report)


Flaky tests (10)

Postgres 17

Postgres 15

Postgres 14

  • test_pgdata_import_smoke[8-1024-RelBlockSize.MULTIPLE_RELATION_SEGMENTS]: release-arm64
  • test_pgdata_import_smoke[None-1024-RelBlockSize.MULTIPLE_RELATION_SEGMENTS]: release-arm64

Code coverage* (full report)

  • functions: 31.4% (8382 of 26736 functions)
  • lines: 47.7% (65943 of 138163 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
c1115bf at 2024-12-11T19:25:57.851Z :recycle:

We need a type with patch semantics for each field
of the tenant config. This commit introduces TenantConfigPatch
along with conversions into the tenant config types used by
the pageserver and storage controller.

This type supports JSON serde:
* null values are interpreted as unsets
* non-null values are upserts
* missing values are no-ops
Parse the patch, apply it to the current in-memory tenant config
and trigger location confs to update the pageservers.
This endpoint will be used for control plane managed pageservers.
@VladLazar VladLazar force-pushed the vlad/patch-tenant-configs branch 2 times, most recently from e2ee73a to 39f8a7b Compare December 5, 2024 12:58
@VladLazar VladLazar force-pushed the vlad/patch-tenant-configs branch from 39f8a7b to 93d5b67 Compare December 5, 2024 13:32
@VladLazar VladLazar marked this pull request as ready for review December 5, 2024 17:55
@VladLazar VladLazar requested a review from a team as a code owner December 5, 2024 17:55
libs/pageserver_api/src/models.rs Outdated Show resolved Hide resolved
libs/pageserver_api/src/models.rs Show resolved Hide resolved
libs/pageserver_api/src/models.rs Show resolved Hide resolved
pageserver/src/http/routes.rs Outdated Show resolved Hide resolved
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.

In principle, I applaud this.

I don't know if inventing our own patching format is the way to go, as opposed to using sth like https://jsonpatch.com/.

But, not asking to do it here, and AFAICT this will be only used internally, right?

Or are we planning for cplane to use this new API to set pitr_interval?

We could offer a dedicated pitr endpoint for cplane to sidestep the issue.

libs/pageserver_api/src/models.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/config.rs Outdated Show resolved Hide resolved
@VladLazar
Copy link
Contributor Author

VladLazar commented Dec 11, 2024

Or are we planning for cplane to use this new API to set pitr_interval?

Yeah. That's the idea.

We could offer a dedicated pitr endpoint for cplane to sidestep the issue.

I'd rather offer something generic so we don't have to do this again when cplane wants to set a new field.
In any case, the pageserver stuff goes away when we finish the migration to storcon, so perhaps not
super relevant either way.

@problame
Copy link
Contributor

I'd rather offer something generic so we don't have to do this again when cplane wants to set a new field.
In any case, the pageserver stuff goes away when we finish the migration to storcon, so perhaps not
super relevant either way.

The pageserver-side yes, but the API will continue to exist, cplane will simply call storcon instead of PS.

Won't block the PR on this though as it's a lateral move in terms of API surface towards cplane.

@VladLazar
Copy link
Contributor Author

I'd rather offer something generic so we don't have to do this again when cplane wants to set a new field.
In any case, the pageserver stuff goes away when we finish the migration to storcon, so perhaps not
super relevant either way.

The pageserver-side yes, but the API will continue to exist, cplane will simply call storcon instead of PS.

Which API will continue to exist? The pageserver tenant config APIs can be removed once the migration is complete.
On storcon they will still exist and interface with the pageservers via location_config.

@problame
Copy link
Contributor

Right, but cplane will still call storcon with an API to set (or eventually, patch) the tenant config directly, instead of a well-defined API to set just the pitr interval and leave tenant config be a storage-internal concept.

@VladLazar
Copy link
Contributor Author

Right, but cplane will still call storcon with an API to set (or eventually, patch) the tenant config directly, instead of a well-defined API to set just the pitr interval and leave tenant config be a storage-internal concept.

Okay, I see your point now. I don't think it makes much difference. We should allow cplane to set mutiple "project configs". I agree that project configs are a subset of tenant configs and we could model it as such, but I don't see a huge benefit in doing that.

@VladLazar VladLazar requested a review from problame December 11, 2024 16:42
@VladLazar VladLazar enabled auto-merge December 11, 2024 17:15
@VladLazar VladLazar added this pull request to the merge queue Dec 11, 2024
Merged via the queue into main with commit a3e8044 Dec 11, 2024
83 checks passed
@VladLazar VladLazar deleted the vlad/patch-tenant-configs branch December 11, 2024 19:18
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.

3 participants