From 074febd9e10170d8f37639d634aed1a3fcdda663 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 11 Apr 2024 10:45:28 -0700 Subject: [PATCH] remove SetDiskLoc() rely on the endpoint values instead (#19475) the disk location never changes in the lifetime of a MinIO cluster, even if it did validate this close to the disk instead at the higher layer. Return appropriate errors indicating an invalid drive, so that the drive is not recognized as part of a valid drive. --- cmd/erasure-sets.go | 19 +--- cmd/naughty-disk_test.go | 2 - cmd/storage-interface.go | 178 -------------------------------- cmd/storage-rest-client.go | 12 +-- cmd/storage-rest-server.go | 4 + cmd/xl-storage-disk-id-check.go | 4 - cmd/xl-storage-format-utils.go | 16 --- cmd/xl-storage.go | 65 ++++++------ cmd/xl-storage_test.go | 19 +++- 9 files changed, 57 insertions(+), 262 deletions(-) diff --git a/cmd/erasure-sets.go b/cmd/erasure-sets.go index cb356deb7..8242489e7 100644 --- a/cmd/erasure-sets.go +++ b/cmd/erasure-sets.go @@ -31,7 +31,6 @@ import ( "time" "github.com/dchest/siphash" - "github.com/dustin/go-humanize" "github.com/google/uuid" "github.com/minio/madmin-go/v3" "github.com/minio/minio-go/v7/pkg/set" @@ -261,7 +260,6 @@ func (s *erasureSets) connectDisks() { } disk.SetDiskID(format.Erasure.This) - disk.SetDiskLoc(s.poolIndex, setIndex, diskIndex) disk.SetFormatData(formatData) s.erasureDisks[setIndex][diskIndex] = disk @@ -455,20 +453,10 @@ func newErasureSets(ctx context.Context, endpoints PoolEndpoints, storageDisks [ if diskID == "" { return } - m, n, err := findDiskIndexByDiskID(format, diskID) - if err != nil { - bootLogIf(ctx, err) - return - } - if m != i || n != j { - bootLogIf(ctx, fmt.Errorf("Detected unexpected drive ordering refusing to use the drive - poolID: %s, found drive mounted at (set=%s, drive=%s) expected mount at (set=%s, drive=%s): %s(%s)", humanize.Ordinal(poolIdx+1), humanize.Ordinal(m+1), humanize.Ordinal(n+1), humanize.Ordinal(i+1), humanize.Ordinal(j+1), disk, diskID)) - s.erasureDisks[i][j] = &unrecognizedDisk{storage: disk} - return - } - disk.SetDiskLoc(s.poolIndex, m, n) - s.erasureDisks[m][n] = disk + s.erasureDisks[i][j] = disk }(disk, i, j) } + innerWg.Wait() // Initialize erasure objects for a given set. @@ -1137,8 +1125,6 @@ func (s *erasureSets) HealFormat(ctx context.Context, dryRun bool) (res madmin.H if disk := storageDisks[index]; disk != nil { if disk.IsLocal() { - disk.SetDiskLoc(s.poolIndex, m, n) - xldisk, ok := disk.(*xlStorageDiskIDCheck) if ok { _, commonDeletes := calcCommonWritesDeletes(currentDisksInfo[m], (s.setDriveCount+1)/2) @@ -1155,7 +1141,6 @@ func (s *erasureSets) HealFormat(ctx context.Context, dryRun bool) (res madmin.H if err != nil { continue } - disk.SetDiskLoc(s.poolIndex, m, n) } s.erasureDisks[m][n] = disk diff --git a/cmd/naughty-disk_test.go b/cmd/naughty-disk_test.go index e36ae82ce..f8134690f 100644 --- a/cmd/naughty-disk_test.go +++ b/cmd/naughty-disk_test.go @@ -102,8 +102,6 @@ func (d *naughtyDisk) GetDiskLoc() (poolIdx, setIdx, diskIdx int) { return -1, -1, -1 } -func (d *naughtyDisk) SetDiskLoc(poolIdx, setIdx, diskIdx int) {} - func (d *naughtyDisk) GetDiskID() (string, error) { return d.disk.GetDiskID() } diff --git a/cmd/storage-interface.go b/cmd/storage-interface.go index 504ef862d..0c80fb603 100644 --- a/cmd/storage-interface.go +++ b/cmd/storage-interface.go @@ -23,7 +23,6 @@ import ( "time" "github.com/minio/madmin-go/v3" - xioutil "github.com/minio/minio/internal/ioutil" ) // StorageAPI interface. @@ -109,182 +108,5 @@ type StorageAPI interface { // Read all. ReadAll(ctx context.Context, volume string, path string) (buf []byte, err error) GetDiskLoc() (poolIdx, setIdx, diskIdx int) // Retrieve location indexes. - SetDiskLoc(poolIdx, setIdx, diskIdx int) // Set location indexes. SetFormatData(b []byte) // Set formatData cached value } - -type unrecognizedDisk struct { - storage StorageAPI -} - -func (p *unrecognizedDisk) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writer) (err error) { - return errDiskNotFound -} - -func (p *unrecognizedDisk) String() string { - return p.storage.String() -} - -func (p *unrecognizedDisk) IsOnline() bool { - return false -} - -func (p *unrecognizedDisk) LastConn() time.Time { - return p.storage.LastConn() -} - -func (p *unrecognizedDisk) IsLocal() bool { - return p.storage.IsLocal() -} - -func (p *unrecognizedDisk) Endpoint() Endpoint { - return p.storage.Endpoint() -} - -func (p *unrecognizedDisk) Hostname() string { - return p.storage.Hostname() -} - -func (p *unrecognizedDisk) Healing() *healingTracker { - return nil -} - -func (p *unrecognizedDisk) NSScanner(ctx context.Context, cache dataUsageCache, updates chan<- dataUsageEntry, scanMode madmin.HealScanMode, shouldSleep func() bool) (dataUsageCache, error) { - return dataUsageCache{}, errDiskNotFound -} - -func (p *unrecognizedDisk) SetFormatData(b []byte) { -} - -func (p *unrecognizedDisk) GetDiskLoc() (poolIdx, setIdx, diskIdx int) { - return -1, -1, -1 -} - -func (p *unrecognizedDisk) SetDiskLoc(poolIdx, setIdx, diskIdx int) { -} - -func (p *unrecognizedDisk) Close() error { - return p.storage.Close() -} - -func (p *unrecognizedDisk) GetDiskID() (string, error) { - return "", errDiskNotFound -} - -func (p *unrecognizedDisk) SetDiskID(id string) { -} - -func (p *unrecognizedDisk) DiskInfo(ctx context.Context, _ DiskInfoOptions) (info DiskInfo, err error) { - return info, errDiskNotFound -} - -func (p *unrecognizedDisk) MakeVolBulk(ctx context.Context, volumes ...string) (err error) { - return errDiskNotFound -} - -func (p *unrecognizedDisk) MakeVol(ctx context.Context, volume string) (err error) { - return errDiskNotFound -} - -func (p *unrecognizedDisk) ListVols(ctx context.Context) ([]VolInfo, error) { - return nil, errDiskNotFound -} - -func (p *unrecognizedDisk) StatVol(ctx context.Context, volume string) (vol VolInfo, err error) { - return vol, errDiskNotFound -} - -func (p *unrecognizedDisk) DeleteVol(ctx context.Context, volume string, forceDelete bool) (err error) { - return errDiskNotFound -} - -func (p *unrecognizedDisk) ListDir(ctx context.Context, origvolume, volume, dirPath string, count int) ([]string, error) { - return nil, errDiskNotFound -} - -func (p *unrecognizedDisk) ReadFile(ctx context.Context, volume string, path string, offset int64, buf []byte, verifier *BitrotVerifier) (n int64, err error) { - return 0, errDiskNotFound -} - -func (p *unrecognizedDisk) AppendFile(ctx context.Context, volume string, path string, buf []byte) (err error) { - return errDiskNotFound -} - -func (p *unrecognizedDisk) CreateFile(ctx context.Context, origvolume, volume, path string, size int64, reader io.Reader) error { - return errDiskNotFound -} - -func (p *unrecognizedDisk) ReadFileStream(ctx context.Context, volume, path string, offset, length int64) (io.ReadCloser, error) { - return nil, errDiskNotFound -} - -func (p *unrecognizedDisk) RenameFile(ctx context.Context, srcVolume, srcPath, dstVolume, dstPath string) error { - return errDiskNotFound -} - -func (p *unrecognizedDisk) RenameData(ctx context.Context, srcVolume, srcPath string, fi FileInfo, dstVolume, dstPath string, opts RenameOptions) (uint64, error) { - return 0, errDiskNotFound -} - -func (p *unrecognizedDisk) CheckParts(ctx context.Context, volume string, path string, fi FileInfo) (err error) { - return errDiskNotFound -} - -func (p *unrecognizedDisk) Delete(ctx context.Context, volume string, path string, opts DeleteOptions) (err error) { - return errDiskNotFound -} - -// DeleteVersions deletes slice of versions, it can be same object or multiple objects. -func (p *unrecognizedDisk) DeleteVersions(ctx context.Context, volume string, versions []FileInfoVersions, opts DeleteOptions) (errs []error) { - errs = make([]error, len(versions)) - - for i := range errs { - errs[i] = errDiskNotFound - } - return errs -} - -func (p *unrecognizedDisk) VerifyFile(ctx context.Context, volume, path string, fi FileInfo) error { - return errDiskNotFound -} - -func (p *unrecognizedDisk) WriteAll(ctx context.Context, volume string, path string, b []byte) (err error) { - return errDiskNotFound -} - -func (p *unrecognizedDisk) DeleteVersion(ctx context.Context, volume, path string, fi FileInfo, forceDelMarker bool, opts DeleteOptions) (err error) { - return errDiskNotFound -} - -func (p *unrecognizedDisk) UpdateMetadata(ctx context.Context, volume, path string, fi FileInfo, opts UpdateMetadataOpts) (err error) { - return errDiskNotFound -} - -func (p *unrecognizedDisk) WriteMetadata(ctx context.Context, origvolume, volume, path string, fi FileInfo) (err error) { - return errDiskNotFound -} - -func (p *unrecognizedDisk) ReadVersion(ctx context.Context, origvolume, volume, path, versionID string, opts ReadOptions) (fi FileInfo, err error) { - return fi, errDiskNotFound -} - -func (p *unrecognizedDisk) ReadXL(ctx context.Context, volume, path string, readData bool) (rf RawFileInfo, err error) { - return rf, errDiskNotFound -} - -func (p *unrecognizedDisk) ReadAll(ctx context.Context, volume string, path string) (buf []byte, err error) { - return nil, errDiskNotFound -} - -func (p *unrecognizedDisk) StatInfoFile(ctx context.Context, volume, path string, glob bool) (stat []StatInfo, err error) { - return nil, errDiskNotFound -} - -func (p *unrecognizedDisk) ReadMultiple(ctx context.Context, req ReadMultipleReq, resp chan<- ReadMultipleResp) error { - xioutil.SafeClose(resp) - return errDiskNotFound -} - -func (p *unrecognizedDisk) CleanAbandonedData(ctx context.Context, volume string, path string) error { - return errDiskNotFound -} diff --git a/cmd/storage-rest-client.go b/cmd/storage-rest-client.go index b37298b2b..140764292 100644 --- a/cmd/storage-rest-client.go +++ b/cmd/storage-rest-client.go @@ -163,21 +163,11 @@ type storageRESTClient struct { formatMutex sync.RWMutex diskInfoCache *cachevalue.Cache[DiskInfo] - - // Indexes, will be -1 until assigned a set. - poolIndex, setIndex, diskIndex int } // Retrieve location indexes. func (client *storageRESTClient) GetDiskLoc() (poolIdx, setIdx, diskIdx int) { - return client.poolIndex, client.setIndex, client.diskIndex -} - -// Set location indexes. -func (client *storageRESTClient) SetDiskLoc(poolIdx, setIdx, diskIdx int) { - client.poolIndex = poolIdx - client.setIndex = setIdx - client.diskIndex = diskIdx + return client.endpoint.PoolIdx, client.endpoint.SetIdx, client.endpoint.DiskIdx } // Wrapper to restClient.Call to handle network errors, in case of network error the connection is makred disconnected diff --git a/cmd/storage-rest-server.go b/cmd/storage-rest-server.go index bf4ea2824..0cebce74d 100644 --- a/cmd/storage-rest-server.go +++ b/cmd/storage-rest-server.go @@ -1198,6 +1198,10 @@ func logFatalErrs(err error, endpoint Endpoint, exit bool) { } else { logger.Fatal(err, "Unable to initialize backend") } + case errors.Is(err, errInconsistentDisk): + if exit { + logger.Fatal(err, "Unable to initialize backend") + } default: if !exit { storageLogOnceIf(GlobalContext, fmt.Errorf("Drive %s returned an unexpected error: %w, please investigate - drive will be offline", endpoint, err), "log-fatal-errs") diff --git a/cmd/xl-storage-disk-id-check.go b/cmd/xl-storage-disk-id-check.go index 3477635c1..4ded5179b 100644 --- a/cmd/xl-storage-disk-id-check.go +++ b/cmd/xl-storage-disk-id-check.go @@ -253,10 +253,6 @@ func (p *xlStorageDiskIDCheck) GetDiskLoc() (poolIdx, setIdx, diskIdx int) { return p.storage.GetDiskLoc() } -func (p *xlStorageDiskIDCheck) SetDiskLoc(poolIdx, setIdx, diskIdx int) { - p.storage.SetDiskLoc(poolIdx, setIdx, diskIdx) -} - func (p *xlStorageDiskIDCheck) Close() error { p.diskCancel() return p.storage.Close() diff --git a/cmd/xl-storage-format-utils.go b/cmd/xl-storage-format-utils.go index b9b0fab2b..11d67a867 100644 --- a/cmd/xl-storage-format-utils.go +++ b/cmd/xl-storage-format-utils.go @@ -141,22 +141,6 @@ func getFileInfo(xlMetaBuf []byte, volume, path, versionID string, data, allPart return fi, nil } -// getXLDiskLoc will return the pool/set/disk id if it can be located in the object layer. -// Will return -1 for unknown values. -func getXLDiskLoc(diskID string) (poolIdx, setIdx, diskIdx int) { - if api := newObjectLayerFn(); api != nil { - if globalIsErasureSD { - return 0, 0, 0 - } - if ep, ok := api.(*erasureServerPools); ok { - if pool, set, disk, err := ep.getPoolAndSet(diskID); err == nil { - return pool, set, disk - } - } - } - return -1, -1, -1 -} - // hashDeterministicString will return a deterministic hash for the map values. // Trivial collisions are avoided, but this is by no means a strong hash. func hashDeterministicString(m map[string]string) uint64 { diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index f46f2aebc..8366dff35 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -25,7 +25,6 @@ import ( "errors" "fmt" "io" - "net/url" "os" pathutil "path" "path/filepath" @@ -103,9 +102,6 @@ type xlStorage struct { diskID string - // Indexes, will be -1 until assigned a set. - poolIndex, setIndex, diskIndex int - formatFileInfo os.FileInfo formatFile string formatLegacy bool @@ -196,15 +192,6 @@ func getValidPath(path string) (string, error) { return path, nil } -// Initialize a new storage disk. -func newLocalXLStorage(path string) (*xlStorage, error) { - u := url.URL{Path: path} - return newXLStorage(Endpoint{ - URL: &u, - IsLocal: true, - }, true) -} - // Make Erasure backend meta volumes. func makeFormatErasureMetaVolumes(disk StorageAPI) error { if disk == nil { @@ -231,9 +218,6 @@ func newXLStorage(ep Endpoint, cleanUp bool) (s *xlStorage, err error) { endpoint: ep, globalSync: globalFSOSync, diskInfoCache: cachevalue.New[DiskInfo](), - poolIndex: -1, - setIndex: -1, - diskIndex: -1, immediatePurge: make(chan string, immediatePurgeQueue), } @@ -315,7 +299,19 @@ func newXLStorage(ep Endpoint, cleanUp bool) (s *xlStorage, err error) { if err = json.Unmarshal(s.formatData, &format); err != nil { return s, errCorruptedFormat } - s.diskID = format.Erasure.This + m, n, err := findDiskIndexByDiskID(format, format.Erasure.This) + if err != nil { + return s, err + } + diskID := format.Erasure.This + if m != ep.SetIdx || n != ep.DiskIdx { + storageLogOnceIf(context.Background(), + fmt.Errorf("unexpected drive ordering on pool: %s: found drive at (set=%s, drive=%s), expected at (set=%s, drive=%s): %s(%s): %w", + humanize.Ordinal(ep.PoolIdx+1), humanize.Ordinal(m+1), humanize.Ordinal(n+1), humanize.Ordinal(ep.SetIdx+1), humanize.Ordinal(ep.DiskIdx+1), + s, s.diskID, errInconsistentDisk), "drive-order-format-json") + return s, errInconsistentDisk + } + s.diskID = diskID s.formatLastCheck = time.Now() s.formatLegacy = format.Erasure.DistributionAlgo == formatErasureVersionV2DistributionAlgoV1 } @@ -408,11 +404,7 @@ func (s *xlStorage) IsLocal() bool { // Retrieve location indexes. func (s *xlStorage) GetDiskLoc() (poolIdx, setIdx, diskIdx int) { - // If unset, see if we can locate it. - if s.poolIndex < 0 || s.setIndex < 0 || s.diskIndex < 0 { - return getXLDiskLoc(s.diskID) - } - return s.poolIndex, s.setIndex, s.diskIndex + return s.endpoint.PoolIdx, s.endpoint.SetIdx, s.endpoint.DiskIdx } func (s *xlStorage) SetFormatData(b []byte) { @@ -421,13 +413,6 @@ func (s *xlStorage) SetFormatData(b []byte) { s.formatData = b } -// Set location indexes. -func (s *xlStorage) SetDiskLoc(poolIdx, setIdx, diskIdx int) { - s.poolIndex = poolIdx - s.setIndex = setIdx - s.diskIndex = diskIdx -} - func (s *xlStorage) Healing() *healingTracker { healingFile := pathJoin(s.drivePath, minioMetaBucket, bucketMetaPrefix, healingTrackerFilename) @@ -873,14 +858,28 @@ func (s *xlStorage) GetDiskID() (string, error) { return "", errCorruptedFormat } + m, n, err := findDiskIndexByDiskID(format, format.Erasure.This) + if err != nil { + return "", err + } + + diskID = format.Erasure.This + ep := s.endpoint + if m != ep.SetIdx || n != ep.DiskIdx { + storageLogOnceIf(GlobalContext, + fmt.Errorf("unexpected drive ordering on pool: %s: found drive at (set=%s, drive=%s), expected at (set=%s, drive=%s): %s(%s): %w", + humanize.Ordinal(ep.PoolIdx+1), humanize.Ordinal(m+1), humanize.Ordinal(n+1), humanize.Ordinal(ep.SetIdx+1), humanize.Ordinal(ep.DiskIdx+1), + s, s.diskID, errInconsistentDisk), "drive-order-format-json") + return "", errInconsistentDisk + } s.Lock() - defer s.Unlock() - s.formatData = b - s.diskID = format.Erasure.This + s.diskID = diskID s.formatLegacy = format.Erasure.DistributionAlgo == formatErasureVersionV2DistributionAlgoV1 s.formatFileInfo = fi + s.formatData = b s.formatLastCheck = time.Now() - return s.diskID, nil + s.Unlock() + return diskID, nil } // Make a volume entry. diff --git a/cmd/xl-storage_test.go b/cmd/xl-storage_test.go index ec6b89257..1e680208a 100644 --- a/cmd/xl-storage_test.go +++ b/cmd/xl-storage_test.go @@ -23,6 +23,7 @@ import ( "crypto/rand" "fmt" "io" + "net/url" "os" slashpath "path" "runtime" @@ -114,13 +115,29 @@ func TestIsValidVolname(t *testing.T) { } } +func newLocalXLStorage(path string) (*xlStorage, error) { + return newLocalXLStorageWithDiskIdx(path, 0) +} + +// Initialize a new storage disk. +func newLocalXLStorageWithDiskIdx(path string, diskIdx int) (*xlStorage, error) { + u := url.URL{Path: path} + return newXLStorage(Endpoint{ + URL: &u, + IsLocal: true, + PoolIdx: 0, + SetIdx: 0, + DiskIdx: diskIdx, + }, true) +} + // creates a temp dir and sets up xlStorage layer. // returns xlStorage layer, temp dir path to be used for the purpose of tests. func newXLStorageTestSetup(tb testing.TB) (*xlStorageDiskIDCheck, string, error) { diskPath := tb.TempDir() // Initialize a new xlStorage layer. - storage, err := newLocalXLStorage(diskPath) + storage, err := newLocalXLStorageWithDiskIdx(diskPath, 3) if err != nil { return nil, "", err }