From 9baeda781a60c88875042a15502fb026a57d375b Mon Sep 17 00:00:00 2001 From: Anis Elleuch Date: Tue, 19 May 2020 22:27:20 +0100 Subject: [PATCH] fix storage info output with unordered endpoints arguments (#9610) Shuffling arguments that we pass to MinIO server are supported. However, when that happens, Prometheus returns wrong information about disks usage and online/offline status. The commit fixes the issue by avoiding relying on xl.endpoints since it is not ordered. --- cmd/bitrot_test.go | 2 +- cmd/format-xl.go | 2 +- cmd/naughty-disk_test.go | 8 ++++++-- cmd/object-api-common.go | 2 +- cmd/posix-diskid-check.go | 4 ++++ cmd/posix.go | 15 +++++++++++---- cmd/posix_test.go | 28 ++++++++++++++-------------- cmd/posix_unix_test.go | 4 ++-- cmd/posix_windows_test.go | 4 ++-- cmd/storage-interface.go | 3 ++- cmd/storage-rest-client.go | 4 ++++ cmd/storage-rest-server.go | 2 +- cmd/xl-sets.go | 3 --- cmd/xl-v1.go | 35 ++++++++++++++++++++++++++--------- 14 files changed, 75 insertions(+), 41 deletions(-) diff --git a/cmd/bitrot_test.go b/cmd/bitrot_test.go index 63d43565d..78cf9dc5c 100644 --- a/cmd/bitrot_test.go +++ b/cmd/bitrot_test.go @@ -34,7 +34,7 @@ func testBitrotReaderWriterAlgo(t *testing.T, bitrotAlgo BitrotAlgorithm) { volume := "testvol" filePath := "testfile" - disk, err := newPosix(tmpDir) + disk, err := newPosix(tmpDir, "") if err != nil { t.Fatal(err) } diff --git a/cmd/format-xl.go b/cmd/format-xl.go index 53ec80286..565f647e2 100644 --- a/cmd/format-xl.go +++ b/cmd/format-xl.go @@ -664,7 +664,7 @@ func initXLMetaVolumesInLocalDisks(storageDisks []StorageAPI, formats []*formatX // Compute the local disks eligible for meta volumes (re)initialization var disksToInit []StorageAPI for index := range storageDisks { - if formats[index] == nil || storageDisks[index] == nil || storageDisks[index].Hostname() != "" { + if formats[index] == nil || storageDisks[index] == nil || !storageDisks[index].IsLocal() { // Ignore create meta volume on disks which are not found or not local. continue } diff --git a/cmd/naughty-disk_test.go b/cmd/naughty-disk_test.go index 60c5e90e3..4ad887936 100644 --- a/cmd/naughty-disk_test.go +++ b/cmd/naughty-disk_test.go @@ -54,8 +54,12 @@ func (d *naughtyDisk) IsOnline() bool { return d.disk.IsOnline() } -func (*naughtyDisk) Hostname() string { - return "" +func (d *naughtyDisk) IsLocal() bool { + return d.disk.IsLocal() +} + +func (d *naughtyDisk) Hostname() string { + return d.disk.Hostname() } func (d *naughtyDisk) Close() (err error) { diff --git a/cmd/object-api-common.go b/cmd/object-api-common.go index a480d7fbd..6811d851d 100644 --- a/cmd/object-api-common.go +++ b/cmd/object-api-common.go @@ -82,7 +82,7 @@ func dirObjectInfo(bucket, object string, size int64, metadata map[string]string // Depending on the disk type network or local, initialize storage API. func newStorageAPI(endpoint Endpoint) (storage StorageAPI, err error) { if endpoint.IsLocal { - storage, err := newPosix(endpoint.Path) + storage, err := newPosix(endpoint.Path, endpoint.Host) if err != nil { return nil, err } diff --git a/cmd/posix-diskid-check.go b/cmd/posix-diskid-check.go index 253fc9869..1338d52a4 100644 --- a/cmd/posix-diskid-check.go +++ b/cmd/posix-diskid-check.go @@ -39,6 +39,10 @@ func (p *posixDiskIDCheck) IsOnline() bool { return storedDiskID == p.diskID } +func (p *posixDiskIDCheck) IsLocal() bool { + return p.storage.IsLocal() +} + func (p *posixDiskIDCheck) CrawlAndGetDataUsage(ctx context.Context, cache dataUsageCache) (dataUsageCache, error) { return p.storage.CrawlAndGetDataUsage(ctx, cache) } diff --git a/cmd/posix.go b/cmd/posix.go index 447505f3f..d4454c5de 100644 --- a/cmd/posix.go +++ b/cmd/posix.go @@ -87,7 +87,9 @@ type posix struct { activeIOCount int32 diskPath string - pool sync.Pool + hostname string + + pool sync.Pool diskMount bool // indicates if the path is an actual mount. @@ -228,7 +230,7 @@ func isDirEmpty(dirname string) bool { } // Initialize a new storage disk. -func newPosix(path string) (*posix, error) { +func newPosix(path string, hostname string) (*posix, error) { var err error if path, err = getValidPath(path, true); err != nil { return nil, err @@ -239,6 +241,7 @@ func newPosix(path string) (*posix, error) { } p := &posix{ diskPath: path, + hostname: hostname, pool: sync.Pool{ New: func() interface{} { b := disk.AlignedBlock(readBlockSize) @@ -339,8 +342,8 @@ func (s *posix) String() string { return s.diskPath } -func (*posix) Hostname() string { - return "" +func (s *posix) Hostname() string { + return s.hostname } func (s *posix) Close() error { @@ -352,6 +355,10 @@ func (s *posix) IsOnline() bool { return true } +func (s *posix) IsLocal() bool { + return true +} + func (s *posix) waitForLowActiveIO() { for atomic.LoadInt32(&s.activeIOCount) >= s.maxActiveIOCount { time.Sleep(lowActiveIOWaitTick) diff --git a/cmd/posix_test.go b/cmd/posix_test.go index dfd8f0fc7..0c3cb23f4 100644 --- a/cmd/posix_test.go +++ b/cmd/posix_test.go @@ -121,7 +121,7 @@ func newPosixTestSetup() (StorageAPI, string, error) { return nil, "", err } // Initialize a new posix layer. - storage, err := newPosix(diskPath) + storage, err := newPosix(diskPath, "") if err != nil { return nil, "", err } @@ -376,7 +376,7 @@ func TestPosixNewPosix(t *testing.T) { // Validate all test cases. for i, testCase := range testCases { // Initialize a new posix layer. - _, err := newPosix(testCase.name) + _, err := newPosix(testCase.name, "") if err != testCase.err { t.Fatalf("TestPosix %d failed wanted: %s, got: %s", i+1, err, testCase.err) } @@ -453,7 +453,7 @@ func TestPosixMakeVol(t *testing.T) { } // Initialize posix storage layer for permission denied error. - _, err = newPosix(permDeniedDir) + _, err = newPosix(permDeniedDir, "") if err != nil && !os.IsPermission(err) { t.Fatalf("Unable to initialize posix, %s", err) } @@ -462,7 +462,7 @@ func TestPosixMakeVol(t *testing.T) { t.Fatalf("Unable to change permission to temporary directory %v. %v", permDeniedDir, err) } - posixStorage, err = newPosix(permDeniedDir) + posixStorage, err = newPosix(permDeniedDir, "") if err != nil { t.Fatalf("Unable to initialize posix, %s", err) } @@ -555,7 +555,7 @@ func TestPosixDeleteVol(t *testing.T) { } // Initialize posix storage layer for permission denied error. - _, err = newPosix(permDeniedDir) + _, err = newPosix(permDeniedDir, "") if err != nil && !os.IsPermission(err) { t.Fatalf("Unable to initialize posix, %s", err) } @@ -564,7 +564,7 @@ func TestPosixDeleteVol(t *testing.T) { t.Fatalf("Unable to change permission to temporary directory %v. %v", permDeniedDir, err) } - posixStorage, err = newPosix(permDeniedDir) + posixStorage, err = newPosix(permDeniedDir, "") if err != nil { t.Fatalf("Unable to initialize posix, %s", err) } @@ -811,7 +811,7 @@ func TestPosixPosixListDir(t *testing.T) { defer removePermDeniedFile(permDeniedDir) // Initialize posix storage layer for permission denied error. - _, err = newPosix(permDeniedDir) + _, err = newPosix(permDeniedDir, "") if err != nil && !os.IsPermission(err) { t.Fatalf("Unable to initialize posix, %s", err) } @@ -820,7 +820,7 @@ func TestPosixPosixListDir(t *testing.T) { t.Fatalf("Unable to change permission to temporary directory %v. %v", permDeniedDir, err) } - posixStorage, err = newPosix(permDeniedDir) + posixStorage, err = newPosix(permDeniedDir, "") if err != nil { t.Fatalf("Unable to initialize posix, %s", err) } @@ -938,7 +938,7 @@ func TestPosixDeleteFile(t *testing.T) { defer removePermDeniedFile(permDeniedDir) // Initialize posix storage layer for permission denied error. - _, err = newPosix(permDeniedDir) + _, err = newPosix(permDeniedDir, "") if err != nil && !os.IsPermission(err) { t.Fatalf("Unable to initialize posix, %s", err) } @@ -947,7 +947,7 @@ func TestPosixDeleteFile(t *testing.T) { t.Fatalf("Unable to change permission to temporary directory %v. %v", permDeniedDir, err) } - posixStorage, err = newPosix(permDeniedDir) + posixStorage, err = newPosix(permDeniedDir, "") if err != nil { t.Fatalf("Unable to initialize posix, %s", err) } @@ -1136,7 +1136,7 @@ func TestPosixReadFile(t *testing.T) { defer removePermDeniedFile(permDeniedDir) // Initialize posix storage layer for permission denied error. - _, err = newPosix(permDeniedDir) + _, err = newPosix(permDeniedDir, "") if err != nil && !os.IsPermission(err) { t.Fatalf("Unable to initialize posix, %s", err) } @@ -1145,7 +1145,7 @@ func TestPosixReadFile(t *testing.T) { t.Fatalf("Unable to change permission to temporary directory %v. %v", permDeniedDir, err) } - posixPermStorage, err := newPosix(permDeniedDir) + posixPermStorage, err := newPosix(permDeniedDir, "") if err != nil { t.Fatalf("Unable to initialize posix, %s", err) } @@ -1306,7 +1306,7 @@ func TestPosixAppendFile(t *testing.T) { var posixPermStorage StorageAPI // Initialize posix storage layer for permission denied error. - _, err = newPosix(permDeniedDir) + _, err = newPosix(permDeniedDir, "") if err != nil && !os.IsPermission(err) { t.Fatalf("Unable to initialize posix, %s", err) } @@ -1315,7 +1315,7 @@ func TestPosixAppendFile(t *testing.T) { t.Fatalf("Unable to change permission to temporary directory %v. %v", permDeniedDir, err) } - posixPermStorage, err = newPosix(permDeniedDir) + posixPermStorage, err = newPosix(permDeniedDir, "") if err != nil { t.Fatalf("Unable to initialize posix, %s", err) } diff --git a/cmd/posix_unix_test.go b/cmd/posix_unix_test.go index 02c18c2fe..a6dedd847 100644 --- a/cmd/posix_unix_test.go +++ b/cmd/posix_unix_test.go @@ -49,7 +49,7 @@ func TestIsValidUmaskVol(t *testing.T) { testCase := testCases[0] // Initialize a new posix layer. - disk, err := newPosix(tmpPath) + disk, err := newPosix(tmpPath, "") if err != nil { t.Fatalf("Initializing posix failed with %s.", err) } @@ -91,7 +91,7 @@ func TestIsValidUmaskFile(t *testing.T) { testCase := testCases[0] // Initialize a new posix layer. - disk, err := newPosix(tmpPath) + disk, err := newPosix(tmpPath, "") if err != nil { t.Fatalf("Initializing posix failed with %s.", err) } diff --git a/cmd/posix_windows_test.go b/cmd/posix_windows_test.go index 31284aa93..2ae38d8c7 100644 --- a/cmd/posix_windows_test.go +++ b/cmd/posix_windows_test.go @@ -48,7 +48,7 @@ func TestUNCPaths(t *testing.T) { // Instantiate posix object to manage a disk var fs StorageAPI - fs, err = newPosix(dir) + fs, err = newPosix(dir, "") if err != nil { t.Fatal(err) } @@ -83,7 +83,7 @@ func TestUNCPathENOTDIR(t *testing.T) { defer os.RemoveAll(dir) var fs StorageAPI - fs, err = newPosix(dir) + fs, err = newPosix(dir, "") if err != nil { t.Fatal(err) } diff --git a/cmd/storage-interface.go b/cmd/storage-interface.go index faf440058..7cfe37f8d 100644 --- a/cmd/storage-interface.go +++ b/cmd/storage-interface.go @@ -27,7 +27,8 @@ type StorageAPI interface { String() string // Storage operations. - IsOnline() bool // Returns true if disk is online. + IsOnline() bool // Returns true if disk is online. + IsLocal() bool Hostname() string // Returns host name if remote host. Close() error GetDiskID() (string, error) diff --git a/cmd/storage-rest-client.go b/cmd/storage-rest-client.go index 42009502a..e57e33b94 100644 --- a/cmd/storage-rest-client.go +++ b/cmd/storage-rest-client.go @@ -147,6 +147,10 @@ func (client *storageRESTClient) IsOnline() bool { return atomic.LoadInt32(&client.connected) == 1 } +func (client *storageRESTClient) IsLocal() bool { + return false +} + func (client *storageRESTClient) Hostname() string { return client.endpoint.Host } diff --git a/cmd/storage-rest-server.go b/cmd/storage-rest-server.go index 4b431deca..62abd6287 100644 --- a/cmd/storage-rest-server.go +++ b/cmd/storage-rest-server.go @@ -758,7 +758,7 @@ func registerStorageRESTHandlers(router *mux.Router, endpointZones EndpointZones if !endpoint.IsLocal { continue } - storage, err := newPosix(endpoint.Path) + storage, err := newPosix(endpoint.Path, endpoint.Host) if err != nil { if err == errMinDiskSize { logger.Fatal(config.ErrUnableToWriteInBackend(err).Hint(err.Error()), "Unable to initialize backend") diff --git a/cmd/xl-sets.go b/cmd/xl-sets.go index 1a8cfe3c3..a42f89aa8 100644 --- a/cmd/xl-sets.go +++ b/cmd/xl-sets.go @@ -317,9 +317,7 @@ func newXLSets(ctx context.Context, endpoints Endpoints, storageDisks []StorageA } for i := 0; i < setCount; i++ { - var endpoints Endpoints for j := 0; j < drivesPerSet; j++ { - endpoints = append(endpoints, s.endpoints[i*drivesPerSet+j]) // Rely on endpoints list to initialize, init lockers and available disks. s.xlLockers[i][j] = newLockAPI(s.endpoints[i*drivesPerSet+j]) @@ -344,7 +342,6 @@ func newXLSets(ctx context.Context, endpoints Endpoints, storageDisks []StorageA s.sets[i] = &xlObjects{ getDisks: s.GetDisks(i), getLockers: s.GetLockers(i), - endpoints: endpoints, nsMutex: mutex, bp: bp, mrfUploadCh: make(chan partialUpload, 10000), diff --git a/cmd/xl-v1.go b/cmd/xl-v1.go index 443e9a857..343261676 100644 --- a/cmd/xl-v1.go +++ b/cmd/xl-v1.go @@ -59,8 +59,6 @@ type xlObjects struct { // getLockers returns list of remote and local lockers. getLockers func() []dsync.NetLocker - endpoints Endpoints - // Locker mutex map. nsMutex *nsLockMap @@ -95,7 +93,7 @@ func (d byDiskTotal) Less(i, j int) bool { } // getDisksInfo - fetch disks info across all other storage API. -func getDisksInfo(disks []StorageAPI, endpoints Endpoints) (disksInfo []DiskInfo, onlineDisks, offlineDisks madmin.BackendDisks) { +func getDisksInfo(disks []StorageAPI) (disksInfo []DiskInfo, onlineDisks, offlineDisks madmin.BackendDisks) { disksInfo = make([]DiskInfo, len(disks)) g := errgroup.WithNErrs(len(disks)) @@ -125,7 +123,10 @@ func getDisksInfo(disks []StorageAPI, endpoints Endpoints) (disksInfo []DiskInfo // Wait for the routines. for i, diskInfoErr := range g.Wait() { - peerAddr := endpoints[i].Host + if disks[i] == nil { + continue + } + peerAddr := disks[i].Hostname() if _, ok := offlineDisks[peerAddr]; !ok { offlineDisks[peerAddr] = 0 } @@ -139,13 +140,29 @@ func getDisksInfo(disks []StorageAPI, endpoints Endpoints) (disksInfo []DiskInfo onlineDisks[peerAddr]++ } + // Iterate over the passed endpoints arguments and check + // if there are still disks missing from the offline/online lists + // and update them accordingly. + missingOfflineDisks := make(map[string]int) + for _, zone := range globalEndpoints { + for _, endpoint := range zone.Endpoints { + if _, ok := offlineDisks[endpoint.Host]; !ok { + missingOfflineDisks[endpoint.Host]++ + } + } + } + for missingDisk, n := range missingOfflineDisks { + onlineDisks[missingDisk] = 0 + offlineDisks[missingDisk] = n + } + // Success. return disksInfo, onlineDisks, offlineDisks } // Get an aggregated storage info across all disks. -func getStorageInfo(disks []StorageAPI, endpoints Endpoints) StorageInfo { - disksInfo, onlineDisks, offlineDisks := getDisksInfo(disks, endpoints) +func getStorageInfo(disks []StorageAPI) StorageInfo { + disksInfo, onlineDisks, offlineDisks := getDisksInfo(disks) // Sort so that the first element is the smallest. sort.Sort(byDiskTotal(disksInfo)) @@ -183,9 +200,9 @@ func (xl xlObjects) StorageInfo(ctx context.Context, local bool) StorageInfo { disks := xl.getDisks() if local { var localDisks []StorageAPI - for i, disk := range disks { + for _, disk := range disks { if disk != nil { - if xl.endpoints[i].IsLocal && disk.Hostname() == "" { + if disk.IsLocal() { // Append this local disk since local flag is true localDisks = append(localDisks, disk) } @@ -193,7 +210,7 @@ func (xl xlObjects) StorageInfo(ctx context.Context, local bool) StorageInfo { } disks = localDisks } - return getStorageInfo(disks, xl.endpoints) + return getStorageInfo(disks) } // GetMetrics - is not implemented and shouldn't be called.