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: fixes for layer path changes #7786

Merged
merged 5 commits into from
May 17, 2024
Merged

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented May 16, 2024

Problem

  • When a layer with legacy local path format is evicted and then re-downloaded, a panic happened because the path downloaded by remote storage didn't match the path stored in Layer.
  • While investigating, I also realized that secondary locations would have a similar issue with evictions.

Closes: #7783

Summary of changes

  • Make remote timeline client take local paths as an input: it should not have its own ideas about local paths, instead it just uses the layer path that the Layer has.
  • Make secondary state store an explicit local path, populated on scan of local disk at startup. This provides the same behavior as for Layer, that our local_layer_path is a default, but the layer path can actually be anything (e.g. an old style one).
  • Add tests for both cases.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@jcsp jcsp added t/bug Issue Type: Bug c/storage/pageserver Component: storage: pageserver labels May 16, 2024
@jcsp jcsp requested a review from VladLazar May 16, 2024 15:43
@jcsp jcsp marked this pull request as ready for review May 16, 2024 15:43
@jcsp jcsp requested a review from a team as a code owner May 16, 2024 15:43
Copy link

github-actions bot commented May 16, 2024

3066 tests run: 2939 passed, 0 failed, 127 skipped (full report)


Code coverage* (full report)

  • functions: 31.5% (6351 of 20188 functions)
  • lines: 47.5% (47950 of 101051 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
4c516fa at 2024-05-17T09:21:38.768Z :recycle:

@jcsp jcsp requested a review from VladLazar May 17, 2024 08:35
@jcsp jcsp merged commit a8e6d25 into main May 17, 2024
55 checks passed
@jcsp jcsp deleted the jcsp/issue-7783-evicted-layers branch May 17, 2024 12:24
a-masterov pushed a commit that referenced this pull request May 20, 2024
## Problem

- When a layer with legacy local path format is evicted and then
re-downloaded, a panic happened because the path downloaded by remote
storage didn't match the path stored in Layer.
- While investigating, I also realized that secondary locations would
have a similar issue with evictions.

Closes: #7783 

## Summary of changes

- Make remote timeline client take local paths as an input: it should
not have its own ideas about local paths, instead it just uses the layer
path that the Layer has.
- Make secondary state store an explicit local path, populated on scan
of local disk at startup. This provides the same behavior as for Layer,
that our local_layer_path is a _default_, but the layer path can
actually be anything (e.g. an old style one).
- Add tests for both cases.
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 t/bug Issue Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pageserver: downloads of evicted layers with pre-v1 local paths panics
2 participants