master: name the unlabeled disk layout plainly in assign errors (#11290)

* master: name the unlabeled disk layout plainly in assign errors When no volume server serves the layout an assign targets, the error named the empty disk type as "hdd" (HardDriveType is the empty string), sending operators looking for servers labeled hdd when the actual mismatch is labeled (e.g. -disk=ssd) servers versus unlabeled clients. - describe the layout as "default (unlabeled)" when the disk type is empty, keep %q naming for labeled types - log the unserved-layout condition once per option instead of letting every failing write repeat an unactionable line Observed in production: volume servers started with -disk=ssd while CSI mounts assign with the unlabeled layout; the per-write error stream pointed at a nonexistent hdd fleet. * master: bound and expire the unserved-layout warning dedupe The dedupe map retained every distinct option key permanently. Option keys embed request-derived fields (collection, disk type), so repeated assignments with distinct options would grow master memory without bound, and a retained key suppressed the warning if the same option went unserved again after the topology recovered. Remember last-warned timestamps instead, expiring after an hour, with a hard cap that resets the set when a client-driven key flood fills it. * master: silence per-retry unserved-layout log and name explicit hdd Addresses Devin Review comments on #11290. - The unserved-layout branch already rate-limits its warning via assignUnservedLayoutWarning.Do, but the common epilogue still logged lastErr at V(0) on every retry, so the flood the dedup was meant to stop continued. Skip the epilogue log when the unserved-layout branch owns the logging; the error is still returned to the client. - describeDiskLayout took the canonicalized option.DiskType, but ToDiskType folds both "" and "hdd" into HardDriveType, so an explicit disk=hdd request was mislabeled "default (unlabeled)". Pass the original request disk type instead: only an empty request is the unlabeled default; an explicit hdd is named "hdd". Adds TestAssignFailsFastNamesExplicitHdd covering the explicit-hdd wording. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com>
2026-09-20 05:20:49 +02:00 · 2026-09-13 11:56:32 -07:00
parent 99d2479528
commit c462fffce6
2 changed files with 139 additions and 8 deletions
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"strings"
+	"sync"
 	"time"

 	"github.com/seaweedfs/seaweedfs/weed/glog"
@@ -106,11 +107,12 @@ func (ms *MasterServer) Assign(ctx context.Context, req *master_pb.AssignRequest
 	vl.SetLastGrowCount(req.WritableVolumeCount)

 	var (
-		lastErr           error
-		maxTimeout        = time.Second * 10
-		startTime         = time.Now()
-		initiatedGrow     bool
-		repickedAfterGrow bool
+		lastErr              error
+		maxTimeout           = time.Second * 10
+		startTime            = time.Now()
+		initiatedGrow        bool
+		repickedAfterGrow    bool
+		unservedLayoutLogged bool
 	)

 	for time.Now().Sub(startTime) < maxTimeout {
@@ -150,7 +152,12 @@ func (ms *MasterServer) Assign(ctx context.Context, req *master_pb.AssignRequest
 							// wrap above, so followers and growth-disabled
 							// masters name the unserved medium too — the
 							// initiator block is skipped for both.
-							lastErr = fmt.Errorf("%s and no volume server carries disk type %q for %s", err.Error(), option.DiskType.ReadableString(), option.String())
+							// The empty disk type is the legacy unlabeled
+							// layout; naming it "hdd" here sends operators
+							// looking for servers that were never labeled.
+							lastErr = fmt.Errorf("%s and no volume server carries the %s disk layout for %s", err.Error(), describeDiskLayout(req.DiskType), option.String())
+							assignUnservedLayoutWarning.Do(option.String(), lastErr)
+							unservedLayoutLogged = true
 						}
 						break // surface the real error, not a retryable shed
 					}
@@ -213,8 +220,68 @@ func (ms *MasterServer) Assign(ctx context.Context, req *master_pb.AssignRequest
 	if initiatedGrow && vl.HasGrowRequest() && ms.Topo.AvailableSpaceFor(option) > 0 {
 		return nil, status.Errorf(codes.ResourceExhausted, "no writable volumes for %s, volume growth in progress", option.String())
 	}
-	if lastErr != nil {
+	if lastErr != nil && !unservedLayoutLogged {
+		// The unserved-layout branch already logged this once per option via
+		// assignUnservedLayoutWarning; repeating it here would flood the log
+		// on every retry of a state a retry cannot change.
 		glog.V(0).Infof("assign %v %v: %v", req, option.String(), lastErr)
 	}
 	return nil, lastErr
 }
+
+// describeDiskLayout names the disk layout an assign targets. The empty disk
+// type is the legacy unlabeled layout on servers that were never started with
+// -disk; naming it "hdd" sends operators looking for servers that were never
+// labeled.
+//
+// Pass the original request disk type, not the canonicalized option.DiskType:
+// ToDiskType folds both "" and "hdd" into HardDriveType, so only the request
+// string can tell an unlabeled request from an explicit hdd one.
+func describeDiskLayout(reqDiskType string) string {
+	if reqDiskType == "" {
+		return "default (unlabeled)"
+	}
+	return fmt.Sprintf("%q", strings.ToLower(reqDiskType))
+}
+
+// assignUnservedLayoutWarning logs a repeated assign failure at most once per
+// option per interval instead of once per write attempt: a layout no volume
+// server serves is a state a retry cannot change, so repeating it only buries
+// the actionable first warning.
+//
+// The remembered set is bounded and expires: option keys embed
+// request-derived fields (collection, disk type), so an unbounded, permanent
+// dedupe map would let a client grow master memory at will and would also
+// suppress the warning if the same option goes unserved again after the
+// topology recovers.
+const (
+	unservedLayoutWarnInterval = time.Hour
+	unservedLayoutWarnMaxKeys  = 1024
+)
+
+type unservedLayoutWarning struct {
+	mu   sync.Mutex
+	now  func() time.Time
+	last map[string]time.Time
+}
+
+var assignUnservedLayoutWarning = &unservedLayoutWarning{
+	now:  time.Now,
+	last: make(map[string]time.Time),
+}
+
+func (w *unservedLayoutWarning) Do(optionKey string, lastErr error) {
+	w.mu.Lock()
+	defer w.mu.Unlock()
+	now := w.now()
+	if t, ok := w.last[optionKey]; ok && now.Sub(t) < unservedLayoutWarnInterval {
+		return
+	}
+	if len(w.last) >= unservedLayoutWarnMaxKeys {
+		// Client-driven keys must never grow the map without bound; a full
+		// reset trades a burst of repeated warnings for bounded memory.
+		w.last = make(map[string]time.Time)
+	}
+	w.last[optionKey] = now
+	glog.Warningf("assign requests for %s will keep failing until a volume server registers that disk layout or clients change their assignment disk type: %v", optionKey, lastErr)
+}
@@ -2,6 +2,7 @@ package weed_server

 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"

@@ -268,8 +269,71 @@ func TestAssignFailsFastWhenDiskTypeUnserved(t *testing.T) {
 				assert.NotEqual(t, codes.ResourceExhausted, st.Code())
 			}
 			assert.Contains(t, err.Error(), topology.NoWritableVolumes)
-			assert.Contains(t, err.Error(), `no volume server carries disk type "hdd"`)
+			assert.Contains(t, err.Error(), `no volume server carries the default (unlabeled) disk layout`)
 			assert.Less(t, elapsed, 2*time.Second)
 		})
 	}
 }
+
+// An explicit disk=hdd request is not the unlabeled default: the error must
+// name "hdd" so an operator reading it connects it to their hdd configuration.
+// ToDiskType folds both "" and "hdd" into HardDriveType, so the formatter must
+// look at the original request string, not the canonicalized option.DiskType.
+func TestAssignFailsFastNamesExplicitHdd(t *testing.T) {
+	ms := newLeaderMaster()
+	// ssd capacity registered, but the request explicitly asks for hdd.
+	ms.Topo.GetOrCreateDataCenter("dc1").GetOrCreateRack("rack1").
+		GetOrCreateDataNode("127.0.0.1", 8080, 18080, "127.0.0.1", "dn1", map[string]uint32{"ssd": 1})
+
+	req := &master_pb.AssignRequest{Count: 1, Replication: "000", Collection: "fresh", DiskType: "hdd"}
+
+	start := time.Now()
+	resp, err := ms.Assign(context.Background(), req)
+	elapsed := time.Since(start)
+
+	require.Error(t, err)
+	require.Nil(t, resp)
+	assert.Contains(t, err.Error(), topology.NoWritableVolumes)
+	assert.Contains(t, err.Error(), `no volume server carries the "hdd" disk layout`)
+	assert.NotContains(t, err.Error(), "default (unlabeled)")
+	assert.Less(t, elapsed, 2*time.Second)
+}
+
+func TestUnservedLayoutWarningBoundedAndExpiring(t *testing.T) {
+	w := &unservedLayoutWarning{
+		now:  func() time.Time { return time.Unix(0, 0) },
+		last: make(map[string]time.Time),
+	}
+
+	// Dedupes within the interval.
+	w.Do("opt", fmt.Errorf("e1"))
+	w.Do("opt", fmt.Errorf("e2"))
+	if len(w.last) != 1 {
+		t.Fatalf("same option logged twice: %v", w.last)
+	}
+
+	// Re-warns after the interval expires.
+	base := time.Unix(0, 0)
+	w.now = func() time.Time { return base.Add(unservedLayoutWarnInterval) }
+	w.Do("opt", fmt.Errorf("e3"))
+	if len(w.last) != 1 || !w.last["opt"].Equal(base.Add(unservedLayoutWarnInterval)) {
+		t.Fatalf("expected refreshed timestamp after interval, got %v", w.last)
+	}
+
+	// Hard cap: a client-driven key flood cannot grow the map without bound.
+	w.now = func() time.Time { return base.Add(2 * unservedLayoutWarnInterval) }
+	flood := &unservedLayoutWarning{
+		now:  w.now,
+		last: make(map[string]time.Time),
+	}
+	for i := 0; i < unservedLayoutWarnMaxKeys; i++ {
+		flood.Do(fmt.Sprintf("opt-%d", i), fmt.Errorf("e"))
+	}
+	if len(flood.last) != unservedLayoutWarnMaxKeys {
+		t.Fatalf("expected exactly cap entries, got %d", len(flood.last))
+	}
+	flood.Do("opt-flood", fmt.Errorf("e"))
+	if len(flood.last) != 1 {
+		t.Fatalf("expected full reset at cap, got %d entries", len(flood.last))
+	}
+}