pageserver: remove legacy tenant config code, clean up redundant generation none/broken usages #7947
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jcsp
added
c/storage/pageserver
2934 tests run: 2817 passed, 0 failed, 117 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
0ca7704 at 2024-06-26T19:29:46.396Z :recycle: |
arpad-m
approved these changes
Jun 4, 2024
|
It turns out that we have an automated test of a docker-compose example, which was never updated to use the storage controller. That also highlights that once we always try and write a new-style file, we will give unfriendly errors if someone tries to create a tenant with no generation. Let's follow-through and really make that mandatory at the API level. |
arpad-m
approved these changes
Jun 6, 2024
auto-merge was automatically disabled
June 13, 2024 09:35
Pull request was converted to draft
jcsp
force-pushed
the
jcsp/issue-5388-legacy-tenant-conf
branch
2 times, most recently
from
53e632e
to
580c635
Compare
jcsp
force-pushed
the
jcsp/issue-5388-legacy-tenant-conf
branch
from
580c635
to
cab720d
Compare
jcsp
changed the title
5 tasks
problame
reviewed
Jun 26, 2024
conradludgate
pushed a commit
that referenced
this pull request
Jun 27, 2024
…ration none/broken usages (#7947) ## Problem In #5299, the new config-v1 tenant config file was added to hold the LocationConf type. We left the old config file in place for forward compat, and because running without generations (therefore without LocationConf) as still useful before the storage controller was ready for prime-time. Closes: #5388 ## Summary of changes - Remove code for reading and writing the legacy config file - Remove Generation::Broken: it was unused. - Treat missing config file on disk as an error loading a tenant, rather than defaulting it. We can now remove LocationConf::default, and thereby guarantee that we never construct a tenant with a None generation. - Update some comments + add some assertions to clarify that Generation::None is only used in layer metadata, not in the state of a running tenant. - Update docker compose test to create tenants with a generation
jcsp
added a commit
that referenced
this pull request
Jun 28, 2024
## Problem For some time, we have created tenants with calls to location_conf. The legacy "POST /v1/tenant" path was only used in some tests. ## Summary of changes - Remove the API - Relocate TenantCreateRequest to the controller API file (this used to be used in both pageserver and controller APIs) - Rewrite tenant_create test helper to use location_config API, as control plane and storage controller do - Update docker-compose test script to create tenants with location_config API (this small commit is also present in #7947)
5 tasks
jcsp
added a commit
that referenced
this pull request
Jul 2, 2024
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
VladLazar
pushed a commit
that referenced
this pull request
Jul 8, 2024
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
VladLazar
pushed a commit
that referenced
this pull request
Jul 8, 2024
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
VladLazar
pushed a commit
that referenced
this pull request
Jul 8, 2024
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
VladLazar
pushed a commit
that referenced
this pull request
Jul 8, 2024
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
VladLazar
pushed a commit
that referenced
this pull request
Jul 8, 2024
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
VladLazar
pushed a commit
that referenced
this pull request
Jul 8, 2024
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
In #5299, the new config-v1 tenant config file was added to hold the LocationConf type. We left the old config file in place for forward compat, and because running without generations (therefore without LocationConf) as still useful before the storage controller was ready for prime-time.
Closes: #5388
Summary of changes
Checklist before requesting a review
Checklist before merging