cockroachdb / cockroachdb/cockroach
logstore: sideloaded storage is not atomic
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
### Background
Raft log storage is mostly contained in Pebble, which provides atomicity guarantee for writes: each write batch is either fully applied or fully discarded. For example, if the node crashes before a write has been flushed/synced, the write will be fully discarded post restart.
There is a specialized part of the raft log storage: [sideloaded](https://github.com/cockroachdb/cockroach/blob/d1c7403bcf1f51440332b0eea7f33c52f75dc4b2/pkg/kv/kvserver/logstore/sideload_disk.go#L35) storage. It is used for storing `AddSSTable` raft entries, which are typically large, in the order of MiB. The motivation there is avoiding the write amplification of Pebble - `AddSSTable` commands are stored directly as files, and "referenced" from the raft log entries. Another goal here is being able to directly ingest these files into Pebble.
The implementation of the sideloaded storage does not provide atomicity guarantees like Pebble:
1. A file can be partially written (e.g. if interrupted by a crash).
2. When multiple files are written in one "batch", it can be interrupted (by a crash) and complete only partially.
3. When entries are overwritten (e.g. when a new leader regresses a follower's log), multiple entries at the same log index can be present simultaneously.
4. Writes are not atomic with the corresponding Pebble writes, so files can be written, but end up "dangling" if there is a crash before the subsequent Pebble write.
This historically caused bugs like #38566 and #113135. The workaround for this lack of atomicity is to sync newly added files before committing "references" to them in Pebble, and to sync the truncated state changes in Pebble (i.e. "unreference" the files) before removing the files.
### Issues
#### Log size tracking is imprecise.
The log storage size delta [computations](https://github.com/cockroachdb/cockroach/blob/d1c7403bcf1f51440332b0eea7f33c52f75dc4b2/pkg/kv/kvserver/replica_application_result.go#L511) are sensitive to these partial writes. The [raftLogSizeTrusted](https://github.com/cockroachdb/cockroach/blob/d1c7403bcf1f51440332b0eea7f33c52f75dc4b2/pkg/kv/kvserver/replica.go#L475-L477) field aims to catch some situations when the size might be imprecise, but it doesn't consider all corner cases.
For example, if there is a crash during `TruncateTo` call (after we have already durably applied the new truncated state), the next truncation post restart will observe (and account) more entries than the actual delta between the old and new truncated state. But we will not notice this impreciseness.
If there is a crash during a situation (3) when leader overwrites a follower's entries, we can end up with multiple entries at the same log index (but different terms). This is fine/correct w.r.t. raft (we read entries by index/term pair in the file name). But the `TruncateTo` call (and other raft log size recomputation funcs) will count them all (because it filters files only by index).
#### Entry removals are imprecise.
When [removing](https://github.com/cockroachdb/cockroach/blob/caa5aaed069b9f5d745de38ea397818550bcd1a0/pkg/kv/kvserver/logstore/logstore.go#L368-L375) sideloaded entries in case (3), the code assumes that all these entries have the same term. This is generally not true, so some files can be left dangling until some other `TruncateTo` call removes them as a drive-by.
Jira issue: CRDB-45026
Contributor guide
Assessment
This issue has not been assessed yet.