fix: fsync snap directory when saving a received snapshot db - #22314
Conversation
kubernetes-prow
Bot
added
area/robustness-testing
area/testing
size/L
needs-ok-to-test
labels
|
Hi @gyuho. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| @@ -62,6 +73,23 @@ func (s *Snapshotter) SaveDBFrom(r io.Reader, id uint64) (int64, error) { | |||
| return n, err | |||
| } | |||
|
|
|||
| // gofail: var snapDBRenameBeforeDirSync struct{} | |||
|
|
|||
| // A rename is not durable until the containing directory is fsynced. | |||
There was a problem hiding this comment.
To clarify the sequence, 1. etcd receives a snapshot, 2. writes it to a temp file, 3. fsyncs the file, 4. renames it into place as snap/db -- but never fsynced the snap/ directory. A power-off crash in that window could lose the rename, so the freshly received snapshot silently "disappears" on restart.
|
Was also able to reproduce this using something like
|
|
it could happen. I run into such kind of issues in production several times for containerd snapshot files. It's easy to reproduce it with https://github.com/etcd-io/bbolt/tree/main/tests/dmflakey for power-off. |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 15 files with indirect coverage changes @@ Coverage Diff @@
## main #22314 +/- ##
==========================================
- Coverage 69.76% 69.73% -0.04%
==========================================
Files 448 448
Lines 38159 38074 -85
==========================================
- Hits 26622 26551 -71
+ Misses 10107 10105 -2
+ Partials 1430 1418 -12 Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
/retest |
|
Ran real 3-member etcd clusters on AWS and forced a member to receive a database snapshot: injected failpoints (pausing the code between the snap.db rename and the directory fsync, or making that fsync return an error), sent SIGKILL to the etcd process, deleted the snap.db file after the WAL record was durable, and then injected a real machine crash via To prevent filesystem journaling from masking the bug, also put the member's data on a dedicated non-journaled ext2 EBS volume. On the unfixed build this reproduced the field failure: after the crash, the member panicked with failed to find database snapshot file. On the fixed build the snap.db directory entry survived every one of these failures, and the member reopened the snapshot, rejoined the cluster, and converged to the same data as the other two members -- verified by comparing KV hashes across all three. |
| @@ -30,8 +30,12 @@ import ( | |||
|
|
|||
| var ErrNoDBSnapshot = errors.New("snap: snapshot file doesn't exist") | |||
|
|
|||
| // SaveDBFrom saves snapshot of the database from the given reader. It | |||
| // guarantees the save operation is atomic. | |||
| // SaveDBFrom atomically saves a database snapshot from r. Before returning | |||
There was a problem hiding this comment.
| // SaveDBFrom atomically saves a database snapshot from r. Before returning | |
| // SaveDBFrom atomically saves a database snapshot from the reader. Before returning |
| // every successful return keeps the durability guarantee above. | ||
| if err = s.fsyncDir(s.dir); err != nil { | ||
| return n, err | ||
| } | ||
| return n, nil |
There was a problem hiding this comment.
Returning n when the f is removed is a little weird. It should return the existing file's size. But it isn't introduced in this PR.
It's even nice if we have a verification to ensure the existing file has exactly the same content as the new one.
|
/ok-to-test |
21b7d75 to
e82fa68
Compare
SaveDBFrom synced a received snapshot database before renaming it into place, but did not sync the containing directory. Linux fsync(2) documents that syncing a file does not necessarily persist its directory entry; the directory must also be synced. The snapshot receiver calls SaveDBFrom before processing the Raft message, which can later sync the WAL snapshot record. Sync the snapshot directory after rename and on the existing-file retry path, and return sync errors so snapshot handling stops before Raft processing. Add direct unit coverage that both SaveDBFrom paths invoke directory sync and propagate failures. Keep gofail E2E coverage scoped to failure/retry and crash-before-return control flow; remove tests and claims that did not establish directory-entry durability. Signed-off-by: Gyuho Lee <gyuhol@nvidia.com>
e82fa68 to
cf31e1f
Compare
|
can you raise an issue to track the backport effort? thx |
fuweid
left a comment
fuweid
left a comment
There was a problem hiding this comment.
LGTM and left one comment for e2e test
Signed-off-by: Gyuho Lee <gyuhol@nvidia.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, fuweid, gyuho The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherry-pick release-3.7 |
|
@fuweid: #22314 failed to apply on top of branch "release-3.6": DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@fuweid: new pull request created: #22378 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
SaveDBFrom synced a received snapshot database before renaming it into place, but did not sync the containing directory. Linux fsync(2) documents that syncing a file does not necessarily persist its directory entry; the directory must also be synced.
The snapshot receiver calls SaveDBFrom before processing the Raft message, which can later sync the WAL snapshot record. Sync the snapshot directory after rename and on the existing-file retry path, and return sync errors so snapshot handling stops before Raft processing.
Add direct unit coverage that both SaveDBFrom paths invoke directory sync and propagate failures. Keep gofail E2E coverage scoped to failure/retry and crash-before-return control flow; remove tests and claims that did not establish directory-entry durability.