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: simpler, stricter config error handling #8177

Merged
merged 7 commits into from
Jul 2, 2024

Conversation

jcsp
Copy link
Collaborator

@jcsp jcsp commented Jun 26, 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

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 changed the title Jcsp/fatal io errors in config pageserver: simpler, stricter config error handling Jun 26, 2024
Copy link

github-actions bot commented Jun 26, 2024

3025 tests run: 2910 passed, 0 failed, 115 skipped (full report)


Flaky tests (2)

Postgres 14

  • test_location_conf_churn[3]: debug
  • test_pg_regress[None]: debug

Code coverage* (full report)

  • functions: 32.7% (6915 of 21148 functions)
  • lines: 50.0% (54197 of 108318 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
facdd2f at 2024-07-02T12:53:16.746Z :recycle:

@jcsp jcsp force-pushed the jcsp/fatal-io-errors-in-config branch from c21c247 to 05531c7 Compare July 1, 2024 08:30
@jcsp jcsp added c/storage/pageserver Component: storage: pageserver a/tech_debt Area: related to tech debt labels Jul 1, 2024
@jcsp jcsp marked this pull request as ready for review July 1, 2024 08:34
@jcsp jcsp requested a review from a team as a code owner July 1, 2024 08:34
@jcsp jcsp requested review from petuhovskiy and problame and removed request for petuhovskiy July 1, 2024 08:34
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.

general pet peeve: your error contexts / messages use present progressive tense. style guide says present tense, for maximum terseness at no information loss

neon/docs/error-handling.md

Lines 200 to 247 in 4a50483

### Neon Rust code
#### Anyhow Context
When adding anyhow `context()`, use form `present-tense-verb+action`.
Example:
- Bad: `file.metadata().context("could not get file metadata")?;`
- Good: `file.metadata().context("get file metadata")?;`
#### Logging Errors
When logging any error `e`, use `could not {e:#}` or `failed to {e:#}`.
If `e` is an `anyhow` error and you want to log the backtrace that it contains,
use `{e:?}` instead of `{e:#}`.
#### Rationale
The `{:#}` ("alternate Display") of an `anyhow` error chain is concatenation fo the contexts, using `: `.
For example, the following Rust code will result in output
```
ERROR failed to list users: load users from server: parse response: invalid json
```
This is more concise / less noisy than what happens if you do `.context("could not ...")?` at each level, i.e.:
```
ERROR could not list users: could not load users from server: could not parse response: invalid json
```
```rust
fn main() {
match list_users().context("list users") else {
Ok(_) => ...,
Err(e) => tracing::error!("failed to {e:#}"),
}
}
fn list_users() {
http_get_users().context("load users from server")?;
}
fn http_get_users() {
let response = client....?;
response.parse().context("parse response")?; // fails with serde error "invalid json"
}
```

(We should extend that style guide to include thiserror error messages)

pageserver/src/tenant.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/mgr.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/mgr.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/mgr.rs Show resolved Hide resolved
test_runner/regress/test_tenants.py Outdated Show resolved Hide resolved
@jcsp jcsp enabled auto-merge (squash) July 2, 2024 12:20
@jcsp jcsp merged commit 1a0f545 into main Jul 2, 2024
64 checks passed
@jcsp jcsp deleted the jcsp/fatal-io-errors-in-config branch July 2, 2024 12:45
@jcsp jcsp mentioned this pull request Jul 2, 2024
5 tasks
jcsp added a commit that referenced this pull request Jul 2, 2024
I forgot a commit when merging
#8177
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
I forgot a commit when merging
#8177
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
I forgot a commit when merging
#8177
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
I forgot a commit when merging
#8177
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
I forgot a commit when merging
#8177
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
I forgot a commit when merging
#8177
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
I forgot a commit when merging
#8177
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a/tech_debt Area: related to tech debt c/storage/pageserver Component: storage: pageserver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants