feat(master): drain pending size before marking volume readonly (#9036)

* feat(master): drain pending size before marking volume readonly When vacuum, volume move, or EC encoding marks a volume readonly, in-flight assigned bytes may still be pending. This adds a drain step: immediately remove from writable list (stop new assigns), then wait for pending to decay below 4MB or 30s timeout. - Add volumeSizeTracking struct consolidating effectiveSize, reportedSize, and compactRevision into a single map - Add GetPendingSize, waitForPendingDrain, DrainAndRemoveFromWritable, DrainAndSetVolumeReadOnly to VolumeLayout - UpdateVolumeSize detects compaction via compactRevision change and resets effectiveSize instead of decaying - Wire drain into vacuum (topology_vacuum.go) and volume mark readonly (master_grpc_server_volume.go) * fix: use 2MB pending size drain threshold * fix: check crowded state on initial UpdateVolumeSize registration * fix: respect context cancellation in drain, relax test timing - DrainAndSetVolumeReadOnly now accepts context.Context and returns early on cancellation (for gRPC handler timeout/cancel) - waitForPendingDrain uses select on ctx.Done instead of time.Sleep - Increase concurrent heartbeat test timeout from 10s to 15s for CI * fix: use time-based dedup so decay runs even when reported size is unchanged The value-based dedup (same reportedSize + compactRevision = skip) prevented decay from running when pending bytes existed but no writes had landed on disk yet. The reported size stayed the same across heartbeats, so the excess never decayed. Fix: dedup replicas within the same heartbeat cycle using a 2-second time window instead of comparing values. This allows decay to run once per heartbeat cycle even when the reported size is unchanged. Also confirmed finding 1 (draining re-add race) is a false positive: - Vacuum: ensureCorrectWritables only runs for ReadOnly-changed volumes - Move/EC: readonlyVolumes flag prevents re-adding during drain * fix: make VolumeMarkReadonly non-blocking to fix EC integration test timeout The DrainAndSetVolumeReadOnly call in VolumeMarkReadonly gRPC blocked up to 30s waiting for pending bytes to decay. In integration tests (and real clusters during EC encoding), this caused timeouts because multiple volumes are marked readonly sequentially and heartbeats may not arrive fast enough to decay pending within the drain window. Fix: VolumeMarkReadonly now calls SetVolumeReadOnly immediately (stops new assigns) and only logs a warning if pending bytes remain. The drain wait is kept only for vacuum (DrainAndRemoveFromWritable) which runs inside the master's own goroutine pool. Remove DrainAndSetVolumeReadOnly as it's no longer used. * fix: relax test timing, rename test, add post-condition assert * test: add vacuum integration tests with CI workflow Full-cluster integration test for vacuum, modeled on the EC integration tests. Starts a real master + 2 volume servers, uploads data, deletes entries to create garbage, runs volume.vacuum via shell command, and verifies garbage cleanup and data integrity. Test flow: 1. Start cluster (master + 2 volume servers) 2. Upload 10 files to create volume with data 3. Delete 5 files to create ~50% garbage 4. Verify garbage ratio > 10% 5. Run volume.vacuum command 6. Verify garbage cleaned up 7. Verify remaining 5 files are still accessible CI workflow runs on push/PR to master with 15-minute timeout. Log collection on failure via artifact upload. * fix: use 500KB files and delete 75% to exceed vacuum garbage threshold * fix: add shell lock before vacuum command, fix compilation error * fix: strengthen vacuum integration test assertions - waitForServer: use net.DialTimeout instead of grpc.NewClient for real TCP readiness check - verify_garbage_before_vacuum: t.Fatal instead of warning when no garbage detected - verify_cleanup_after_vacuum: t.Fatal if no server reported the volume or cleanup wasn't verified - verify_remaining_data: read actual file contents via HTTP and compare byte-for-byte against original uploaded payloads * fix: use http.Client with timeout and close body before retry
2026-09-28 18:55:49 +00:00 · 2026-04-11 18:29:11 -07:00
parent 10b0bdce02
commit b37bbf541a
8 changed files with 903 additions and 46 deletions
@@ -0,0 +1,402 @@
+package vacuum
+
+import (
+	"bytes"
+	"context"
+	"fmt"
+	"io"
+	"net"
+	"net/http"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/seaweedfs/seaweedfs/weed/operation"
+	"github.com/seaweedfs/seaweedfs/weed/pb"
+	"github.com/seaweedfs/seaweedfs/weed/pb/volume_server_pb"
+	"github.com/seaweedfs/seaweedfs/weed/shell"
+	"github.com/seaweedfs/seaweedfs/weed/storage/needle"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc"
+)
+
+type TestCluster struct {
+	masterCmd     *exec.Cmd
+	volumeServers []*exec.Cmd
+}
+
+func (c *TestCluster) Stop() {
+	for _, cmd := range c.volumeServers {
+		if cmd != nil && cmd.Process != nil {
+			cmd.Process.Kill()
+			cmd.Wait()
+		}
+	}
+	if c.masterCmd != nil && c.masterCmd.Process != nil {
+		c.masterCmd.Process.Kill()
+		c.masterCmd.Wait()
+	}
+}
+
+func startCluster(ctx context.Context, dataDir string) (*TestCluster, error) {
+	weedBinary := findWeedBinary()
+	if weedBinary == "" {
+		return nil, fmt.Errorf("weed binary not found - build with 'cd weed && go build' first")
+	}
+
+	cluster := &TestCluster{}
+
+	masterDir := filepath.Join(dataDir, "master")
+	os.MkdirAll(masterDir, 0755)
+
+	// Empty security.toml to disable JWT in tests
+	os.WriteFile(filepath.Join(dataDir, "security.toml"), []byte("# test\n"), 0644)
+
+	// Start master
+	masterCmd := exec.CommandContext(ctx, weedBinary, "master",
+		"-port", "9333",
+		"-mdir", masterDir,
+		"-volumeSizeLimitMB", "10",
+		"-ip", "127.0.0.1",
+	)
+	masterCmd.Dir = dataDir
+	masterLog, _ := os.Create(filepath.Join(masterDir, "master.log"))
+	masterCmd.Stdout = masterLog
+	masterCmd.Stderr = masterLog
+	if err := masterCmd.Start(); err != nil {
+		return nil, fmt.Errorf("start master: %v", err)
+	}
+	cluster.masterCmd = masterCmd
+	time.Sleep(2 * time.Second)
+
+	// Start 2 volume servers (enough for vacuum testing)
+	for i := 0; i < 2; i++ {
+		volumeDir := filepath.Join(dataDir, fmt.Sprintf("volume%d", i))
+		os.MkdirAll(volumeDir, 0755)
+
+		port := fmt.Sprintf("808%d", i)
+		volumeCmd := exec.CommandContext(ctx, weedBinary, "volume",
+			"-port", port,
+			"-dir", volumeDir,
+			"-max", "10",
+			"-master", "127.0.0.1:9333",
+			"-ip", "127.0.0.1",
+		)
+		volumeCmd.Dir = dataDir
+		volumeLog, _ := os.Create(filepath.Join(volumeDir, "volume.log"))
+		volumeCmd.Stdout = volumeLog
+		volumeCmd.Stderr = volumeLog
+		if err := volumeCmd.Start(); err != nil {
+			cluster.Stop()
+			return nil, fmt.Errorf("start volume server %d: %v", i, err)
+		}
+		cluster.volumeServers = append(cluster.volumeServers, volumeCmd)
+	}
+
+	time.Sleep(5 * time.Second)
+	return cluster, nil
+}
+
+func findWeedBinary() string {
+	candidates := []string{
+		"../../weed/weed",
+		"../weed/weed",
+		"./weed",
+	}
+	for _, c := range candidates {
+		if _, err := os.Stat(c); err == nil {
+			if abs, err := filepath.Abs(c); err == nil {
+				return abs
+			}
+			return c
+		}
+	}
+	if path, err := exec.LookPath("weed"); err == nil {
+		return path
+	}
+	return ""
+}
+
+func waitForServer(address string, timeout time.Duration) error {
+	start := time.Now()
+	for time.Since(start) < timeout {
+		if conn, err := net.DialTimeout("tcp", address, 1*time.Second); err == nil {
+			conn.Close()
+			return nil
+		}
+		time.Sleep(500 * time.Millisecond)
+	}
+	return fmt.Errorf("timeout waiting for server %s", address)
+}
+
+func uploadData(masterAddr, collection string, data []byte) (string, needle.VolumeId, error) {
+	assignResult, err := operation.Assign(context.Background(), func(ctx context.Context) pb.ServerAddress {
+		return pb.ServerAddress(masterAddr)
+	}, grpc.WithInsecure(), &operation.VolumeAssignRequest{
+		Count:      1,
+		Collection: collection,
+	})
+	if err != nil {
+		return "", 0, fmt.Errorf("assign: %v", err)
+	}
+
+	uploader, err := operation.NewUploader()
+	if err != nil {
+		return "", 0, fmt.Errorf("new uploader: %v", err)
+	}
+
+	uploadResult, err, _ := uploader.Upload(context.Background(), bytes.NewReader(data), &operation.UploadOption{
+		UploadUrl: "http://" + assignResult.Url + "/" + assignResult.Fid,
+		Filename:  "testfile.txt",
+		MimeType:  "text/plain",
+	})
+	if err != nil {
+		return "", 0, fmt.Errorf("upload: %v", err)
+	}
+	if uploadResult.Error != "" {
+		return "", 0, fmt.Errorf("upload error: %s", uploadResult.Error)
+	}
+
+	fid, err := needle.ParseFileIdFromString(assignResult.Fid)
+	if err != nil {
+		return "", 0, err
+	}
+	return assignResult.Fid, fid.VolumeId, nil
+}
+
+func deleteFile(masterAddr string, fid string) error {
+	results := operation.DeleteFileIds(func(ctx context.Context) pb.ServerAddress {
+		return pb.ServerAddress(masterAddr)
+	}, false, grpc.WithInsecure(), []string{fid})
+	for _, r := range results {
+		if r.Error != "" {
+			return fmt.Errorf("delete %s: %s", fid, r.Error)
+		}
+	}
+	return nil
+}
+
+func getGarbageRatio(volumeServerAddr string, volumeId uint32) (float64, error) {
+	var ratio float64
+	err := operation.WithVolumeServerClient(false, pb.ServerAddress(volumeServerAddr), grpc.WithInsecure(),
+		func(client volume_server_pb.VolumeServerClient) error {
+			resp, err := client.VacuumVolumeCheck(context.Background(), &volume_server_pb.VacuumVolumeCheckRequest{
+				VolumeId: volumeId,
+			})
+			if err != nil {
+				return err
+			}
+			ratio = resp.GarbageRatio
+			return nil
+		})
+	return ratio, err
+}
+
+// TestVacuumIntegration tests the full vacuum flow:
+// upload data → delete some → verify garbage → vacuum → verify cleanup
+func TestVacuumIntegration(t *testing.T) {
+	if testing.Short() {
+		t.Skip("Skipping integration test in short mode")
+	}
+
+	testDir := t.TempDir()
+
+	ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second)
+	defer cancel()
+
+	cluster, err := startCluster(ctx, testDir)
+	require.NoError(t, err)
+	defer cluster.Stop()
+
+	require.NoError(t, waitForServer("127.0.0.1:9333", 30*time.Second))
+	require.NoError(t, waitForServer("127.0.0.1:8080", 30*time.Second))
+	require.NoError(t, waitForServer("127.0.0.1:8081", 30*time.Second))
+
+	masterAddr := "127.0.0.1:9333"
+	collection := "vactest"
+
+	// Upload files large enough that deleting most creates significant garbage.
+	// With volumeSizeLimitMB=10, we need several MB of garbage to exceed the
+	// 10% threshold passed to vacuum.
+	const fileSize = 500 * 1024 // 500 KB per file
+	const totalFiles = 16
+	const filesToDelete = 12 // delete 75% → ~6 MB garbage out of ~8 MB
+
+	var fids []string
+	var payloads [][]byte
+	var volumeId needle.VolumeId
+	for i := 0; i < totalFiles; i++ {
+		data := bytes.Repeat([]byte{byte('A' + i%26)}, fileSize)
+		fid, vid, err := uploadData(masterAddr, collection, data)
+		require.NoError(t, err, "upload %d", i)
+		fids = append(fids, fid)
+		payloads = append(payloads, data)
+		volumeId = vid
+	}
+	t.Logf("Uploaded %d files (%d KB each) to volume %d", totalFiles, fileSize/1024, volumeId)
+
+	// Wait for heartbeat to report sizes
+	time.Sleep(6 * time.Second)
+
+	// Delete most files to create garbage well above the threshold
+	for i := 0; i < filesToDelete; i++ {
+		err := deleteFile(masterAddr, fids[i])
+		require.NoError(t, err, "delete %s", fids[i])
+	}
+	t.Logf("Deleted %d of %d files to create garbage", filesToDelete, totalFiles)
+
+	// Wait for heartbeat to report deletions
+	time.Sleep(6 * time.Second)
+
+	// Verify garbage exists
+	t.Run("verify_garbage_before_vacuum", func(t *testing.T) {
+		for _, addr := range []string{"127.0.0.1:8080", "127.0.0.1:8081"} {
+			ratio, err := getGarbageRatio(addr, uint32(volumeId))
+			if err != nil {
+				continue
+			}
+			t.Logf("Garbage ratio on %s: %.2f%%", addr, ratio*100)
+			if ratio > 0.1 {
+				return // sufficient garbage found
+			}
+		}
+		t.Fatal("No server reported garbage > 10% — test data setup failed")
+	})
+
+	// Execute vacuum via shell command
+	t.Run("run_vacuum", func(t *testing.T) {
+		options := &shell.ShellOptions{
+			Masters:        stringPtr(masterAddr),
+			GrpcDialOption: grpc.WithInsecure(),
+			FilerGroup:     stringPtr("default"),
+		}
+		commandEnv := shell.NewCommandEnv(options)
+
+		shellCtx, shellCancel := context.WithTimeout(context.Background(), 60*time.Second)
+		defer shellCancel()
+		go commandEnv.MasterClient.KeepConnectedToMaster(shellCtx)
+		commandEnv.MasterClient.WaitUntilConnected(shellCtx)
+		time.Sleep(2 * time.Second)
+
+		// Acquire lock (required by shell commands)
+		locked, unlock := tryLock(t, commandEnv, 30*time.Second)
+		require.True(t, locked, "could not acquire shell lock")
+		defer unlock()
+
+		// Find and execute vacuum command
+		var output bytes.Buffer
+		var found bool
+		var err error
+		for _, cmd := range shell.Commands {
+			if cmd.Name() == "volume.vacuum" {
+				err = cmd.Do(
+					[]string{"-garbageThreshold", "0.1", "-collection", collection},
+					commandEnv, &output,
+				)
+				found = true
+				break
+			}
+		}
+		require.True(t, found, "volume.vacuum command not found")
+		t.Logf("Vacuum output: %s", output.String())
+		require.NoError(t, err, "vacuum command failed")
+		t.Log("Vacuum completed successfully")
+	})
+
+	// Wait for vacuum effects to settle
+	time.Sleep(6 * time.Second)
+
+	// Verify garbage was cleaned
+	t.Run("verify_cleanup_after_vacuum", func(t *testing.T) {
+		var volumeFound, cleanupVerified bool
+		for _, addr := range []string{"127.0.0.1:8080", "127.0.0.1:8081"} {
+			ratio, err := getGarbageRatio(addr, uint32(volumeId))
+			if err != nil {
+				continue
+			}
+			volumeFound = true
+			t.Logf("Garbage ratio after vacuum on %s: %.2f%%", addr, ratio*100)
+			if ratio < 0.05 {
+				cleanupVerified = true
+			}
+		}
+		if !volumeFound {
+			t.Fatal("No server reported volume after vacuum")
+		}
+		if !cleanupVerified {
+			t.Fatal("Garbage was not cleaned up after vacuum")
+		}
+	})
+
+	// Verify remaining files are still readable with correct contents
+	t.Run("verify_remaining_data", func(t *testing.T) {
+		for i := filesToDelete; i < totalFiles; i++ {
+			fid := fids[i]
+			expected := payloads[i]
+
+			// Read file via HTTP from volume server
+			client := &http.Client{Timeout: 5 * time.Second}
+			url := fmt.Sprintf("http://127.0.0.1:8080/%s", fid)
+			resp, err := client.Get(url)
+			if err != nil || resp.StatusCode == http.StatusNotFound {
+				if resp != nil {
+					resp.Body.Close()
+				}
+				url = fmt.Sprintf("http://127.0.0.1:8081/%s", fid)
+				resp, err = client.Get(url)
+			}
+			require.NoError(t, err, "read fid %s", fid)
+			body, err := io.ReadAll(resp.Body)
+			resp.Body.Close()
+			require.NoError(t, err, "read body of fid %s", fid)
+			require.Equal(t, http.StatusOK, resp.StatusCode, "fid %s returned %d", fid, resp.StatusCode)
+			require.Equal(t, len(expected), len(body), "fid %s size mismatch", fid)
+			require.True(t, bytes.Equal(expected, body), "fid %s content mismatch", fid)
+			t.Logf("File %s verified (%d bytes)", fid, len(body))
+		}
+	})
+}
+
+func stringPtr(s string) *string {
+	return &s
+}
+
+func tryLock(t *testing.T, commandEnv *shell.CommandEnv, timeout time.Duration) (locked bool, unlock func()) {
+	t.Helper()
+	type result struct {
+		err error
+	}
+	done := make(chan result, 1)
+	go func() {
+		for _, cmd := range shell.Commands {
+			if cmd.Name() == "lock" {
+				var out bytes.Buffer
+				done <- result{err: cmd.Do([]string{}, commandEnv, &out)}
+				return
+			}
+		}
+		done <- result{err: fmt.Errorf("lock command not found")}
+	}()
+
+	select {
+	case res := <-done:
+		if res.err != nil {
+			t.Logf("lock failed: %v", res.err)
+			return false, nil
+		}
+		return true, func() {
+			for _, cmd := range shell.Commands {
+				if cmd.Name() == "unlock" {
+					var out bytes.Buffer
+					cmd.Do([]string{}, commandEnv, &out)
+					return
+				}
+			}
+		}
+	case <-time.After(timeout):
+		t.Log("lock timed out")
+		return false, nil
+	}
+}