[Listing] Resolve undecidable list entries against the locked read path - #221
Open
mrjavadseydi wants to merge 2 commits into
Open
mrjavadseydi wants to merge 2 commits into
mrjavadseydi wants to merge 2 commits into
Conversation
Member
|
Thanks a lot @mrjavadseydi, this is exactly the investigation #218 needed. Tying all 25 omissions to a single exit, the deterministic shape A test, and showing that the fallback read has to take the lock are all really valuable. We tested the PR against its base (2fde3cf) and found a few issues to sort out before merging: 1. Commit 2 fails non-latest-only callers on stale minority entries. When
2. Commit 1 can turn existing objects into 404s. 3. Two gaps in the fallback.
Suggestions
Thanks again for digging into this so thoroughly. Looking forward to the next revision! Probes (put in
|
| Test | main 2fde3cf | #221 510da3c |
|---|---|---|
StaleMinorityIgnoredByListings |
PASS | FAIL |
StaleMinorityIgnoredByIAMListing |
PASS | FAIL |
CrossParityMinorityStaysVisible/g0-on-drive-0 |
PASS | FAIL |
CrossParityMinorityStaysVisible/g0-on-drive-11 |
FAIL | FAIL |
SplitDirectoryObjectListed |
FAIL | FAIL |
package cmd
// Review probes for pgsty/silo#221. They reuse the fixtures in
// metacache-null-quorum_test.go (nullQuorumBackend and friends).
import (
"bytes"
"context"
"encoding/xml"
"fmt"
"io"
"net/http"
"slices"
"testing"
"time"
)
// reviewOfflineDisk models a drive of a node that is away.
type reviewOfflineDisk struct{ StorageAPI }
func (d *reviewOfflineDisk) IsOnline() bool { return false }
func (d *reviewOfflineDisk) DiskInfo(context.Context, DiskInfoOptions) (DiskInfo, error) {
return DiskInfo{}, errDiskNotFound
}
func (d *reviewOfflineDisk) WalkDir(context.Context, WalkDirOptions, io.Writer) error {
return errDiskNotFound
}
func (d *reviewOfflineDisk) ReadXL(context.Context, string, string, bool) (RawFileInfo, error) {
return RawFileInfo{}, errDiskNotFound
}
func (d *reviewOfflineDisk) ReadVersion(context.Context, string, string, string, string, ReadOptions) (FileInfo, error) {
return FileInfo{}, errDiskNotFound
}
// reviewFailWalkDisk fails only its walk stream, as in the PR's shape A test.
type reviewFailWalkDisk struct{ StorageAPI }
func (d *reviewFailWalkDisk) WalkDir(context.Context, WalkDirOptions, io.Writer) error {
return errDiskNotFound
}
func reviewPut(t *testing.T, z *erasureServerPools, bucket, object string, body []byte, opts ObjectOptions) {
t.Helper()
if _, err := z.PutObject(t.Context(), bucket, object, mustGetPutObjReader(t, bytes.NewReader(body), int64(len(body)), "", ""), opts); err != nil {
t.Fatal(err)
}
}
func reviewWalk(t *testing.T, z *erasureServerPools, bucket string) (names []string, err error) {
t.Helper()
ch := make(chan itemOrErr[ObjectInfo], 100)
if err := z.Walk(t.Context(), bucket, "", ch, WalkOptions{}); err != nil {
return nil, err
}
for it := range ch {
if it.Err != nil {
err = it.Err
continue
}
names = append(names, it.Item.Name)
}
return names, err
}
func reviewListV2(t *testing.T, router http.Handler, bucket string) (int, []string) {
t.Helper()
rec := nullQuorumRequest(t, router, http.MethodGet, getListObjectsV2URL("", bucket, "", "1000", "", "", ""))
if rec.Code != http.StatusOK {
return rec.Code, nil
}
var list ListObjectsV2Response
if err := xml.Unmarshal(rec.Body.Bytes(), &list); err != nil {
t.Fatal(err)
}
var keys []string
for _, c := range list.Contents {
keys = append(keys, c.Key)
}
return rec.Code, keys
}
func reviewListVersions(t *testing.T, router http.Handler, bucket string) (int, []string) {
t.Helper()
rec := nullQuorumRequest(t, router, http.MethodGet, getListObjectVersionsURL("", bucket, "", "1000", ""))
if rec.Code != http.StatusOK {
return rec.Code, nil
}
var list struct {
Versions []struct {
Key string `xml:"Key"`
} `xml:"Version"`
}
if err := xml.Unmarshal(rec.Body.Bytes(), &list); err != nil {
t.Fatal(err)
}
var keys []string
for _, v := range list.Versions {
keys = append(keys, v.Key)
}
return rec.Code, keys
}
// An object deleted while four drives were away keeps its xl.meta on those
// four drives until it is healed. main drops that minority entry everywhere.
func TestReview221StaleMinorityIgnoredByListings(t *testing.T) {
z, bucket, router := nullQuorumBackend(t)
disks := z.serverPools[0].sets[0].getDisks()
body := bytes.Repeat([]byte{'a'}, 8192)
reviewPut(t, z, bucket, "gone", body, ObjectOptions{})
reviewPut(t, z, bucket, "keep", body, ObjectOptions{})
for _, disk := range disks[4:] {
if err := disk.Delete(t.Context(), bucket, "gone", DeleteOptions{Recursive: true, Immediate: true}); err != nil {
t.Fatal(err)
}
}
if names, err := reviewWalk(t, z, bucket); err != nil || !slices.Equal(names, []string{"keep"}) {
t.Errorf("Walk: got %v, %v; want [keep], nil", names, err)
}
if code, keys := reviewListVersions(t, router, bucket); code != http.StatusOK || !slices.Equal(keys, []string{"keep"}) {
t.Errorf("ListObjectVersions: got %d %v; want 200 [keep]", code, keys)
}
if _, err := z.DeleteObject(t.Context(), bucket, "keep", ObjectOptions{}); err != nil {
t.Fatal(err)
}
if err := z.DeleteBucket(t.Context(), bucket, DeleteBucketOptions{}); err != nil {
t.Errorf("DeleteBucket of a bucket holding only the stale minority: %v", err)
}
}
// The IAM loader lists .minio.sys/config/iam through the same Walk.
func TestReview221StaleMinorityIgnoredByIAMListing(t *testing.T) {
z, _, _ := nullQuorumBackend(t)
body := []byte(`{"version":1}`)
alive := iamConfigUsersPrefix + "alive/identity.json"
ghost := iamConfigUsersPrefix + "ghost/identity.json"
reviewPut(t, z, minioMetaBucket, alive, body, ObjectOptions{})
reviewPut(t, z, minioMetaBucket, ghost, body, ObjectOptions{})
for _, disk := range z.serverPools[0].getHashedSet(ghost).getDisks()[4:] {
if err := disk.Delete(t.Context(), minioMetaBucket, ghost, DeleteOptions{Recursive: true, Immediate: true}); err != nil {
t.Fatal(err)
}
}
items, err := newIAMObjectStore(z, MinIOUsersSysType).listAllIAMConfigItems(t.Context())
if err != nil {
t.Fatalf("IAM listing failed on a stale minority (retriable=%v): %v", configRetriableErrors(err), err)
}
if got := items["users/"]; !slices.Equal(got, []string{"alive/identity.json"}) {
t.Errorf("IAM users: got %v; want [alive/identity.json]", got)
}
}
// G0 was written 8+8 while a node was away. G1 was later written 12+4 but
// missed one drive, which still holds G0. Then a node holding four G1 drives
// goes away: eleven G1 and one G0 remain online. G1 exists, but lacks one of
// its twelve data shards until the node returns, so HEAD should report 503
// rather than 404, and LIST should still show the key.
func TestReview221CrossParityMinorityStaysVisible(t *testing.T) {
for _, g0Drive := range []int{0, 11} {
t.Run(fmt.Sprintf("g0-on-drive-%d", g0Drive), func(t *testing.T) {
z, bucket, router := nullQuorumBackend(t)
set := z.serverPools[0].sets[0]
disks := set.getDisks()
const object = "object"
mtime := time.Now().UTC().Add(-time.Hour)
reviewPut(t, z, bucket, object, bytes.Repeat([]byte{'a'}, 256<<10), ObjectOptions{MTime: mtime, MaxParity: true})
g0 := mustReadNullQuorumMeta(t, disks[g0Drive], bucket, object)
var x xlMetaV2
if err := x.LoadOrConvert(g0); err != nil {
t.Fatal(err)
}
if h := x.versions[0].header; h.EcM != 8 || h.EcN != 8 {
t.Fatalf("G0 is %d+%d, want 8+8", h.EcM, h.EcN)
}
g1Body := bytes.Repeat([]byte{'b'}, 256<<10)
reviewPut(t, z, bucket, object, g1Body, ObjectOptions{MTime: mtime.Add(time.Minute)})
if err := disks[g0Drive].WriteAll(t.Context(), bucket, object+"/"+xlStorageFormatFile, g0); err != nil {
t.Fatal(err)
}
online := slices.Clone(disks)
for i := 12; i < len(online); i++ {
online[i] = &reviewOfflineDisk{StorageAPI: disks[i]}
}
set.getDisks = func() []StorageAPI { return online }
if code := nullQuorumRequest(t, router, http.MethodHead, getHeadObjectURL("", bucket, object)).Code; code == http.StatusNotFound {
t.Errorf("HEAD with a node away: 404 for an existing object")
}
if code, keys := reviewListV2(t, router, bucket); code != http.StatusOK || !slices.Equal(keys, []string{object}) {
t.Errorf("ListObjectsV2 with a node away: got %d %v; want 200 [%s]", code, keys, object)
}
set.getDisks = func() []StorageAPI { return disks }
if get := nullQuorumRequest(t, router, http.MethodGet, getGetObjectURL("", bucket, object)); get.Code != http.StatusOK || !bytes.Equal(get.Body.Bytes(), g1Body) {
t.Fatalf("GET after the node returns: %d", get.Code)
}
})
}
}
// The PR's shape A for a directory object. Listing entries carry the decoded
// name ("dir/"), while the object is stored and locked as "dir__XLDIR__".
func TestReview221SplitDirectoryObjectListed(t *testing.T) {
z, bucket, router := nullQuorumBackend(t)
set := z.serverPools[0].sets[0]
disks := set.getDisks()
const object = "dir/"
enc := encodeDirObject(object)
mtime := time.Now().UTC().Add(-time.Hour)
reviewPut(t, z, bucket, object, nil, ObjectOptions{MTime: mtime})
old := make([][]byte, 3)
for i := range old {
old[i] = mustReadNullQuorumMeta(t, disks[i], bucket, enc)
}
reviewPut(t, z, bucket, object, nil, ObjectOptions{MTime: mtime.Add(time.Minute)})
for i := range old {
if err := disks[i].WriteAll(t.Context(), bucket, enc+"/"+xlStorageFormatFile, old[i]); err != nil {
t.Fatal(err)
}
}
walkers := slices.Clone(disks)
for i := 8; i < len(walkers); i++ {
walkers[i] = &reviewFailWalkDisk{StorageAPI: disks[i]}
}
set.getDisks = func() []StorageAPI { return walkers }
if code, keys := reviewListV2(t, router, bucket); code != http.StatusOK || !slices.Equal(keys, []string{object}) {
t.Errorf("ListObjectsV2: got %d %v; want 200 [%s]", code, keys, object)
}
}
…d path Refs: pgsty#218 Signed-off-by: mr javad seydi <[email protected]>
Signed-off-by: mr javad seydi <[email protected]>
mrjavadseydi
force-pushed
the
fix/issue-218-listobjects-rolling-restart
branch
from
September 23, 2026 15:34
510da3c to
8c784f6
Compare
Author
|
Thanks for the detailed review. I’ve force pushed an updated revision addressing the feedback:
All CI checks are green. The PR is ready for another review |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Closes #218
Contribution Licensing (no CLA, inbound=outbound, DCO required)
This pull request contributes to PGSTY SILO (
pgsty/silo). Code contributionsare accepted under AGPL-3.0-or-later, the same license as the server.
This project does not use a CLA or require a separate Apache-2.0 license grant.
By submitting this pull request I represent that I have the right to contribute
the code changes under this repository's
GNU Affero General Public License v3.0 or later
and retain copyright in my original work. Existing copyright and license
notices remain intact; separately licensed material keeps its applicable terms.
Every commit must carry a DCO
Signed-off-bytrailer(
git commit -s) certifying theDeveloper Certificate of Origin — see
CONTRIBUTING.md.
Description
A successful
ListObjectsresponse could omit a readable key when per-drivewalk snapshots disagreed during a rolling restart with concurrent overwrites.
metaCacheEntries.resolvereturned no entry, andlistPathRaw'spartialcallback silently dropped it. For synchronization tools, a false absence is
more dangerous than a request-level error.
This revision contains two commits:
258b167b18c784f613For latest-only listings,
resolveListEntrynow falls back only when the entrycould still reach listing quorum: valid walk entries plus failed readers must
be at least
objQuorum. Non-latest-only callers retain the previous behavior,so stale minority metadata is ignored by
Walk, version listings,DeleteBucket, and IAM initialization.The fallback:
getObjectFileInfoafter acquiring the lock;getObjectFileInfoalso maps inconsistent metadata and empty merges tonot-found;
the resolved metadata when it returns a live object.
listPathRaw'spartialcallback gains anerrorreturn so an undecidablelatest-only entry can fail the request instead of disappearing from an HTTP
200 page.
Why the lock is load-bearing
An earlier draft performed the fallback read without the namespace lock. In
the rolling-restart investigation, an instrumented build probed each dropped
entry at the point of omission:
getObjectFileInfoObject not found— 51/51Object not found— 47/51A lock-free fallback therefore traded one silent omission for another.
TestListFallbackTakesObjectReadLockpins the lock behavior.Scope after review
mergeXLV2Versionschange was removed from both the diffand branch history. Cross-parity selection is a separate, pre-existing issue
and should receive its own narrow fix and differential tests.
TestListCrossParityMinorityStaysVisibleprotects the baseline drive-0 casefrom regression without expanding this PR into that fix.
WalkDirstill tolerates a minorityxl.metaread failure instead offailing the remaining stream.
TestListObjectsSurvivesTruncatedMetadataMinoritypins this behavior.
Motivation and context
Issue #218 reported readable keys missing from successful listings during a
4-node × 4-drive rolling restart with concurrent overwrites. Instrumentation
captured 25 omissions and tied all of them to merges that selected no
generation. The deterministic shape-A regression uses eight readable walk
streams split 3/5 across two generations while the other eight walk streams
fail. The locked fallback re-reads the stable object state instead of treating
that snapshot disagreement as absence.
How to test this PR
New regression coverage:
TestListObjectsFallsBackToReadableMetadataTestListFallbackTakesObjectReadLockTestListObjectsSurvivesTruncatedMetadataMinorityTestListStaleMinorityIgnoredWalk, andDeleteBucketcallers ignore a stale minorityTestIAMListingIgnoresStaleMinorityTestListSplitDirectoryObjectdir/is locked and read asdir__XLDIR__TestListFallbackDoesNotTrustNotFoundTestListCrossParityMinorityStaysVisibleManual reproduction evidence
The following measurements are historical evidence from the pre-review
revision at
510da3c7c, which still included the now-removed cross-paritycommit. The external rolling-restart harness was not rerun for this narrowed
revision, so these rows must not be read as current-head benchmark results.
8d06424b1, no fix)510da3c7c, run 1510da3c7c, run 2The retained harness, instrumented builds, and raw logs remain outside the
repository as required by
AGENTS.md.Compatibility impact
xl.metaformat change.listPathRawOptions.partialgains an error return; existing non-listing callers returnnil.Best-effort upstream MinIO compatibility is preserved. The maintained and
tested target remains the coordinated PGSTY SILO stack.
Types of changes
Checklist
Signed-off-bytrailers.go test ./cmd/ -count=1passes on the narrowed revision.Verification notes
Verified locally on
darwin/arm64, based onmainat2fde3cf53:go test ./cmd/ -count=1— pass (330.026s);git diff --check— clean;