reject changing pools when there are pending decommissions in-progress (#14102)

do not allow mutation to pool command line when there are
unfinished decommissions in place, disallow such scenarios
to avoid user mistakes.

also add testcases to cover all relevant scenarios.

cmd/erasure-server-pool-decom.go

@@ -277,62 +277,27 @@ func (p poolMeta) IsSuspended(idx int) bool {
 	return p.Pools[idx].Decommission != nil
 }
 
-func (p *poolMeta) load(ctx context.Context, pool *erasureSets, pools []*erasureSets, validate bool) (bool, error) {
-	data, err := readConfig(ctx, pool, poolMetaName)
-	if err != nil {
-		if errors.Is(err, errConfigNotFound) || isErrObjectNotFound(err) {
-			return true, nil
-		}
-		return false, err
-	}
-	if len(data) == 0 {
-		return true, nil
-	}
-	if len(data) <= 4 {
-		return false, fmt.Errorf("poolMeta: no data")
-	}
-	// Read header
-	switch binary.LittleEndian.Uint16(data[0:2]) {
-	case poolMetaFormat:
-	default:
-		return false, fmt.Errorf("poolMeta: unknown format: %d", binary.LittleEndian.Uint16(data[0:2]))
-	}
-	switch binary.LittleEndian.Uint16(data[2:4]) {
-	case poolMetaVersion:
-	default:
-		return false, fmt.Errorf("poolMeta: unknown version: %d", binary.LittleEndian.Uint16(data[2:4]))
-	}
-
-	// OK, parse data.
-	if _, err = p.UnmarshalMsg(data[4:]); err != nil {
-		return false, err
-	}
-
-	switch p.Version {
-	case poolMetaVersionV1:
-	default:
-		return false, fmt.Errorf("unexpected pool meta version: %d", p.Version)
-	}
-
-	if !validate {
-		// No further validation requested.
-		return false, nil
-	}
-
+func (p *poolMeta) validate(pools []*erasureSets) (bool, error) {
 	type poolInfo struct {
-		position  int
-		completed bool
+		position     int
+		completed    bool
+		decomStarted bool // started but not finished yet
 	}
 
 	rememberedPools := make(map[string]poolInfo)
 	for idx, pool := range p.Pools {
 		complete := false
-		if pool.Decommission != nil && pool.Decommission.Complete {
-			complete = true
+		decomStarted := false
+		if pool.Decommission != nil {
+			if pool.Decommission.Complete {
+				complete = true
+			}
+			decomStarted = true
 		}
 		rememberedPools[pool.CmdLine] = poolInfo{
-			position:  idx,
-			completed: complete,
+			position:     idx,
+			completed:    complete,
+			decomStarted: decomStarted,
 		}
 	}
 
@@ -373,7 +338,56 @@ func (p *poolMeta) load(ctx context.Context, pool *erasureSets, pools []*erasure
 		}
 	}
 
-	return len(rememberedPools) != len(specifiedPools), nil
+	update := len(rememberedPools) != len(specifiedPools)
+	if update {
+		for k, pi := range rememberedPools {
+			if pi.decomStarted && !pi.completed {
+				return false, fmt.Errorf("pool(%s) = %s is being decommissioned, No changes should be made to the command line arguments. Please complete the decommission in progress", humanize.Ordinal(pi.position+1), k)
+			}
+		}
+	}
+	return update, nil
+}
+
+func (p *poolMeta) load(ctx context.Context, pool *erasureSets, pools []*erasureSets) error {
+	data, err := readConfig(ctx, pool, poolMetaName)
+	if err != nil {
+		if errors.Is(err, errConfigNotFound) || isErrObjectNotFound(err) {
+			return nil
+		}
+		return err
+	}
+	if len(data) == 0 {
+		// Seems to be empty create a new poolMeta object.
+		return nil
+	}
+	if len(data) <= 4 {
+		return fmt.Errorf("poolMeta: no data")
+	}
+	// Read header
+	switch binary.LittleEndian.Uint16(data[0:2]) {
+	case poolMetaFormat:
+	default:
+		return fmt.Errorf("poolMeta: unknown format: %d", binary.LittleEndian.Uint16(data[0:2]))
+	}
+	switch binary.LittleEndian.Uint16(data[2:4]) {
+	case poolMetaVersion:
+	default:
+		return fmt.Errorf("poolMeta: unknown version: %d", binary.LittleEndian.Uint16(data[2:4]))
+	}
+
+	// OK, parse data.
+	if _, err = p.UnmarshalMsg(data[4:]); err != nil {
+		return err
+	}
+
+	switch p.Version {
+	case poolMetaVersionV1:
+	default:
+		return fmt.Errorf("unexpected pool meta version: %d", p.Version)
+	}
+
+	return nil
 }
 
 func (p *poolMeta) CountItem(idx int, size int64, failed bool) {
@@ -441,7 +455,11 @@ const (
 func (z *erasureServerPools) Init(ctx context.Context) error {
 	meta := poolMeta{}
 
-	update, err := meta.load(ctx, z.serverPools[0], z.serverPools, true)
+	if err := meta.load(ctx, z.serverPools[0], z.serverPools); err != nil {
+		return err
+	}
+
+	update, err := meta.validate(z.serverPools)
 	if err != nil {
 		return err
 	}
@@ -836,7 +854,7 @@ func (z *erasureServerPools) Status(ctx context.Context, idx int) (PoolStatus, e
 func (z *erasureServerPools) ReloadPoolMeta(ctx context.Context) (err error) {
 	meta := poolMeta{}
 
-	if _, err = meta.load(ctx, z.serverPools[0], z.serverPools, false); err != nil {
+	if err = meta.load(ctx, z.serverPools[0], z.serverPools); err != nil {
 		return err
 	}
 

cmd/erasure-server-pool-decom_test.go

@@ -0,0 +1,196 @@
+// Copyright (c) 2015-2022 MinIO, Inc.
+//
+// This file is part of MinIO Object Storage stack
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package cmd
+
+import (
+	"context"
+	"testing"
+)
+
+func prepareErasurePools() (ObjectLayer, []string, error) {
+	nDisks := 32
+	fsDirs, err := getRandomDisks(nDisks)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	pools := mustGetPoolEndpoints(fsDirs[:16]...)
+	pools = append(pools, mustGetPoolEndpoints(fsDirs[16:]...)...)
+
+	// Everything is fine, should return nil
+	objLayer, err := newErasureServerPools(context.Background(), pools)
+	if err != nil {
+		return nil, nil, err
+	}
+	return objLayer, fsDirs, nil
+}
+
+func TestPoolMetaValidate(t *testing.T) {
+	objLayer1, fsDirs, err := prepareErasurePools()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer removeRoots(fsDirs)
+
+	meta := objLayer1.(*erasureServerPools).poolMeta
+	pools := objLayer1.(*erasureServerPools).serverPools
+
+	objLayer2, fsDirs, err := prepareErasurePools()
+	if err != nil {
+		t.Fatalf("Initialization of object layer failed for Erasure setup: %s", err)
+	}
+	defer removeRoots(fsDirs)
+
+	newPools := objLayer2.(*erasureServerPools).serverPools
+	reducedPools := pools[1:]
+	orderChangePools := []*erasureSets{
+		pools[1],
+		pools[0],
+	}
+
+	var nmeta1 poolMeta
+	nmeta1.Version = poolMetaVersion
+	nmeta1.Pools = append(nmeta1.Pools, meta.Pools...)
+	for i, pool := range nmeta1.Pools {
+		if i == 0 {
+			nmeta1.Pools[i] = PoolStatus{
+				CmdLine:    pool.CmdLine,
+				ID:         i,
+				LastUpdate: UTCNow(),
+				Decommission: &PoolDecommissionInfo{
+					Complete: true,
+				},
+			}
+		}
+	}
+
+	var nmeta2 poolMeta
+	nmeta2.Version = poolMetaVersion
+	nmeta2.Pools = append(nmeta2.Pools, meta.Pools...)
+	for i, pool := range nmeta2.Pools {
+		if i == 0 {
+			nmeta2.Pools[i] = PoolStatus{
+				CmdLine:    pool.CmdLine,
+				ID:         i,
+				LastUpdate: UTCNow(),
+				Decommission: &PoolDecommissionInfo{
+					Complete: false,
+				},
+			}
+		}
+	}
+
+	testCases := []struct {
+		meta           poolMeta
+		pools          []*erasureSets
+		expectedUpdate bool
+		expectedErr    bool
+		name           string
+	}{
+		{
+			meta:           meta,
+			pools:          pools,
+			name:           "Correct",
+			expectedErr:    false,
+			expectedUpdate: false,
+		},
+		{
+			meta:           meta,
+			pools:          newPools,
+			name:           "Invalid-Commandline",
+			expectedErr:    true,
+			expectedUpdate: false,
+		},
+		{
+			meta:           meta,
+			pools:          reducedPools,
+			name:           "Invalid-Reduced",
+			expectedErr:    true,
+			expectedUpdate: false,
+		},
+		{
+			meta:           meta,
+			pools:          orderChangePools,
+			name:           "Invalid-Orderchange",
+			expectedErr:    true,
+			expectedUpdate: false,
+		},
+		{
+			meta:           nmeta1,
+			pools:          pools,
+			name:           "Invalid-Completed-Pool-Not-Removed",
+			expectedErr:    true,
+			expectedUpdate: false,
+		},
+		{
+			meta:           nmeta2,
+			pools:          pools,
+			name:           "Correct-Decom-Pending",
+			expectedErr:    false,
+			expectedUpdate: false,
+		},
+		{
+			meta:           nmeta2,
+			pools:          reducedPools,
+			name:           "Invalid-Decom-Pending-Pool-Removal",
+			expectedErr:    true,
+			expectedUpdate: false,
+		},
+		{
+			meta:           nmeta1,
+			pools:          reducedPools,
+			name:           "Correct-Decom-Pool-Removed",
+			expectedErr:    false,
+			expectedUpdate: true,
+		},
+		{
+			meta:           poolMeta{}, // no-pool info available fresh setup.
+			pools:          pools,
+			name:           "Correct-Fresh-Setup",
+			expectedErr:    false,
+			expectedUpdate: true,
+		},
+		{
+			meta:           nmeta2,
+			pools:          orderChangePools,
+			name:           "Invalid-Orderchange-Decom",
+			expectedErr:    true,
+			expectedUpdate: false,
+		},
+	}
+
+	t.Parallel()
+	for _, testCase := range testCases {
+		testCase := testCase
+		t.Run(testCase.name, func(t *testing.T) {
+			update, err := testCase.meta.validate(testCase.pools)
+			if testCase.expectedErr {
+				t.Log(err)
+			}
+			if err != nil && !testCase.expectedErr {
+				t.Errorf("Expected success, but found %s", err)
+			}
+			if err == nil && testCase.expectedErr {
+				t.Error("Expected error, but got `nil`")
+			}
+			if update != testCase.expectedUpdate {
+				t.Errorf("Expected %t, got %t", testCase.expectedUpdate, update)
+			}
+		})
+	}
+}

cmd/test-utils_test.go

@@ -2157,6 +2157,7 @@ func mustGetPoolEndpoints(args ...string) EndpointServerPools {
 		SetCount:     setCount,
 		DrivesPerSet: drivesPerSet,
 		Endpoints:    endpoints,
+		CmdLine:      strings.Join(args, " "),
 	}}
 }