From 37b32199e3ee76f43b0c71e2046fb0ef58cfebc6 Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Mon, 13 Jan 2020 22:09:10 +0100 Subject: [PATCH] Validate XL sets on format (#8779) When formatting a set validate if a host failure will likely lead to data loss. While we don't know what config will be set in the future evaluate to our best knowledge, assuming default settings. --- cmd/endpoint.go | 23 ------------------ cmd/endpoint_test.go | 39 ------------------------------ cmd/erasure-encode_test.go | 4 ++++ cmd/format-xl.go | 49 ++++++++++++++++++++++++++++++++++++-- cmd/naughty-disk_test.go | 4 ++++ cmd/posix-diskid-check.go | 4 ++++ cmd/posix.go | 4 ++++ cmd/prepare-storage.go | 1 + cmd/server-main.go | 4 ---- cmd/storage-interface.go | 3 ++- cmd/storage-rest-client.go | 4 ++++ 11 files changed, 70 insertions(+), 69 deletions(-) diff --git a/cmd/endpoint.go b/cmd/endpoint.go index 3516597b7..0c5c440da 100644 --- a/cmd/endpoint.go +++ b/cmd/endpoint.go @@ -29,7 +29,6 @@ import ( "time" humanize "github.com/dustin/go-humanize" - "github.com/minio/cli" "github.com/minio/minio-go/v6/pkg/set" "github.com/minio/minio/cmd/config" "github.com/minio/minio/cmd/logger" @@ -459,28 +458,6 @@ func NewEndpoints(args ...string) (endpoints Endpoints, err error) { return endpoints, nil } -func checkEndpointsSubOptimal(ctx *cli.Context, setupType SetupType, endpointZones EndpointZones) (err error) { - // Validate sub optimal ordering only for distributed setup. - if setupType != DistXLSetupType { - return nil - } - var endpointOrder int - err = fmt.Errorf("Too many disk args are local, input is in sub-optimal order. Please review input args: %s", ctx.Args()) - for _, endpoints := range endpointZones { - for _, endpoint := range endpoints.Endpoints { - if endpoint.IsLocal { - endpointOrder++ - } else { - endpointOrder-- - } - if endpointOrder >= 2 { - return err - } - } - } - return nil -} - // Checks if there are any cross device mounts. func checkCrossDeviceMounts(endpoints Endpoints) (err error) { var absPaths []string diff --git a/cmd/endpoint_test.go b/cmd/endpoint_test.go index 3a2c3dacd..d828b06f3 100644 --- a/cmd/endpoint_test.go +++ b/cmd/endpoint_test.go @@ -17,7 +17,6 @@ package cmd import ( - "flag" "fmt" "net" "net/url" @@ -25,47 +24,9 @@ import ( "strings" "testing" - "github.com/minio/cli" "github.com/minio/minio-go/v6/pkg/set" ) -func TestSubOptimalEndpointInput(t *testing.T) { - args1 := []string{"http://localhost/d1", "http://localhost/d2", "http://localhost/d3", "http://localhost/d4"} - args2 := []string{"http://example.org/d1", "http://example.com/d1", "http://example.net/d1", "http://example.edu/d1"} - - tests := []struct { - setupType SetupType - ctx *cli.Context - endpoints EndpointZones - isErr bool - }{ - { - setupType: DistXLSetupType, - ctx: cli.NewContext(cli.NewApp(), flag.NewFlagSet("", flag.ContinueOnError), nil), - endpoints: mustGetZoneEndpoints(args1...), - isErr: false, - }, - { - setupType: DistXLSetupType, - ctx: cli.NewContext(cli.NewApp(), flag.NewFlagSet("", flag.ContinueOnError), nil), - endpoints: mustGetZoneEndpoints(args2...), - isErr: false, - }, - } - for i, test := range tests { - test := test - t.Run(fmt.Sprintf("Test%d", i+1), func(t *testing.T) { - err := checkEndpointsSubOptimal(test.ctx, test.setupType, test.endpoints) - if test.isErr && err == nil { - t.Error("expected err but found nil") - } - if !test.isErr && err != nil { - t.Errorf("expected err nil but found an err %s", err) - } - }) - } -} - func TestNewEndpoint(t *testing.T) { u2, _ := url.Parse("https://example.org/path") u4, _ := url.Parse("http://192.168.253.200/path") diff --git a/cmd/erasure-encode_test.go b/cmd/erasure-encode_test.go index cfab9746a..d8ce6fb2a 100644 --- a/cmd/erasure-encode_test.go +++ b/cmd/erasure-encode_test.go @@ -44,6 +44,10 @@ func (a badDisk) CreateFile(volume, path string, size int64, reader io.Reader) e return errFaultyDisk } +func (badDisk) Hostname() string { + return "" +} + const oneMiByte = 1 * humanize.MiByte var erasureEncodeTests = []struct { diff --git a/cmd/format-xl.go b/cmd/format-xl.go index 951b323c9..58f4fc0f4 100644 --- a/cmd/format-xl.go +++ b/cmd/format-xl.go @@ -19,15 +19,17 @@ package cmd import ( "bytes" "context" + "encoding/hex" "encoding/json" "fmt" "io/ioutil" "reflect" - - "encoding/hex" + "sync" humanize "github.com/dustin/go-humanize" + "github.com/minio/minio/cmd/config/storageclass" "github.com/minio/minio/cmd/logger" + "github.com/minio/minio/pkg/color" "github.com/minio/minio/pkg/sync/errgroup" sha256 "github.com/minio/sha256-simd" ) @@ -753,16 +755,43 @@ func fixFormatXLV3(storageDisks []StorageAPI, endpoints Endpoints, formats []*fo func initFormatXL(ctx context.Context, storageDisks []StorageAPI, setCount, drivesPerSet int, deploymentID string) (*formatXLV3, error) { format := newFormatXLV3(setCount, drivesPerSet) formats := make([]*formatXLV3, len(storageDisks)) + wantAtMost := ecDrivesNoConfig(drivesPerSet) + logger.Info("Formatting zone, %v set(s), %v drives per set.", setCount, drivesPerSet) for i := 0; i < setCount; i++ { + hostCount := make(map[string]int, drivesPerSet) for j := 0; j < drivesPerSet; j++ { + disk := storageDisks[i*drivesPerSet+j] newFormat := format.Clone() newFormat.XL.This = format.XL.Sets[i][j] if deploymentID != "" { newFormat.ID = deploymentID } + hostCount[disk.Hostname()]++ formats[i*drivesPerSet+j] = newFormat } + if len(hostCount) > 0 { + var once sync.Once + for host, count := range hostCount { + if count > wantAtMost { + if host == "" { + host = "local" + } + once.Do(func() { + if len(hostCount) == 1 { + return + } + logger.Info(" * Set %v:", i+1) + for j := 0; j < drivesPerSet; j++ { + disk := storageDisks[i*drivesPerSet+j] + logger.Info(" - Drive: %s", disk.String()) + } + }) + logger.Info(color.Yellow("WARNING:")+" Host %v has more than %v drives of set. "+ + "A host failure will result in data becoming unavailable.", host, wantAtMost) + } + } + } } // Save formats `format.json` across all disks. @@ -773,6 +802,22 @@ func initFormatXL(ctx context.Context, storageDisks []StorageAPI, setCount, driv return getFormatXLInQuorum(formats) } +// ecDrivesNoConfig returns the erasure coded drives in a set if no config has been set. +// It will attempt to read it from env variable and fall back to drives/2. +func ecDrivesNoConfig(drivesPerSet int) int { + ecDrives := globalStorageClass.GetParityForSC(storageclass.STANDARD) + if ecDrives == 0 { + cfg, err := storageclass.LookupConfig(nil, drivesPerSet) + if err == nil { + ecDrives = cfg.Standard.Parity + } + if ecDrives == 0 { + ecDrives = drivesPerSet / 2 + } + } + return ecDrives +} + // Make XL backend meta volumes. func makeFormatXLMetaVolumes(disk StorageAPI) error { // Attempt to create MinIO internal buckets. diff --git a/cmd/naughty-disk_test.go b/cmd/naughty-disk_test.go index f548f8143..f3aab1ddb 100644 --- a/cmd/naughty-disk_test.go +++ b/cmd/naughty-disk_test.go @@ -53,6 +53,10 @@ func (d *naughtyDisk) IsOnline() bool { return d.disk.IsOnline() } +func (*naughtyDisk) Hostname() string { + return "" +} + func (d *naughtyDisk) LastError() (err error) { return nil } diff --git a/cmd/posix-diskid-check.go b/cmd/posix-diskid-check.go index 794d27e66..59355f97d 100644 --- a/cmd/posix-diskid-check.go +++ b/cmd/posix-diskid-check.go @@ -42,6 +42,10 @@ func (p *posixDiskIDCheck) CrawlAndGetDataUsage(endCh <-chan struct{}) (DataUsag return p.storage.CrawlAndGetDataUsage(endCh) } +func (p *posixDiskIDCheck) Hostname() string { + return p.storage.Hostname() +} + func (p *posixDiskIDCheck) LastError() error { return p.storage.LastError() } diff --git a/cmd/posix.go b/cmd/posix.go index 77596ff80..a3d4b176f 100644 --- a/cmd/posix.go +++ b/cmd/posix.go @@ -309,6 +309,10 @@ func (s *posix) String() string { return s.diskPath } +func (*posix) Hostname() string { + return "" +} + func (s *posix) LastError() error { if atomic.LoadInt32(&s.ioErrCount) > maxAllowedIOError { return errFaultyDisk diff --git a/cmd/prepare-storage.go b/cmd/prepare-storage.go index a69b3526c..d124f4a53 100644 --- a/cmd/prepare-storage.go +++ b/cmd/prepare-storage.go @@ -160,6 +160,7 @@ func validateXLFormats(format *formatXLV3, formats []*formatXLV3, endpoints Endp if len(format.XL.Sets[0]) != drivesPerSet { return fmt.Errorf("Current backend format is inconsistent with input args (%s), Expected drive count per set %d, got %d", endpoints, len(format.XL.Sets[0]), drivesPerSet) } + return nil } diff --git a/cmd/server-main.go b/cmd/server-main.go index 539534483..81d56377a 100644 --- a/cmd/server-main.go +++ b/cmd/server-main.go @@ -127,10 +127,6 @@ func serverHandleCmdArgs(ctx *cli.Context) { } logger.FatalIf(err, "Invalid command line arguments") - if err = checkEndpointsSubOptimal(ctx, setupType, globalEndpoints); err != nil { - logger.Info("Optimal endpoint check failed %s", err) - } - // On macOS, if a process already listens on LOCALIPADDR:PORT, net.Listen() falls back // to IPv6 address ie minio will start listening on IPv6 address whereas another // (non-)minio process is listening on IPv4 of given port. diff --git a/cmd/storage-interface.go b/cmd/storage-interface.go index f6e85b02e..962bd8c31 100644 --- a/cmd/storage-interface.go +++ b/cmd/storage-interface.go @@ -26,7 +26,8 @@ type StorageAPI interface { String() string // Storage operations. - IsOnline() bool // Returns true if disk is online. + IsOnline() bool // Returns true if disk is online. + Hostname() string // Returns host name if remote host. LastError() error Close() error SetDiskID(id string) diff --git a/cmd/storage-rest-client.go b/cmd/storage-rest-client.go index e2166ab58..4496ad910 100644 --- a/cmd/storage-rest-client.go +++ b/cmd/storage-rest-client.go @@ -148,6 +148,10 @@ func (client *storageRESTClient) IsOnline() bool { return atomic.LoadInt32(&client.connected) == 1 } +func (client *storageRESTClient) Hostname() string { + return client.endpoint.Host +} + func (client *storageRESTClient) CrawlAndGetDataUsage(endCh <-chan struct{}) (DataUsageInfo, error) { respBody, err := client.call(storageRESTMethodCrawlAndGetDataUsage, nil, nil, -1) defer http.DrainBody(respBody)