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

@@ -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

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()
 }

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
-}

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

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")

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()

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 {

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.

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
 	}