From 24b8646ec366643758cbe1484c285b45bbb2625f Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 3 Sep 2026 18:39:45 -0700 Subject: [PATCH] volume: let evacuation proceed on a server in maintenance mode (#11145) Maintenance mode exists to fence a volume server so it can be evacuated without taking new writes (#7977), but the gate added in #8115 also rejected the RPCs evacuation issues against the source: VolumeMarkReadonly (the first step of every move, and the failure reported in #11066), VolumeDelete (the last step), and VolumeEcShardsDelete (the last step for EC shards). volumeServer.evacuate, volume.move and ec.balance therefore all failed on exactly the server they were meant to drain. Those three RPCs only remove data or restrict the server further, the same class as DeleteCollection and the unmount RPCs that were never gated, so they are exempted from the maintenance check in both the Go and Rust volume servers. Everything that adds data or reopens the server for writes (AllocateVolume, WriteNeedleBlob, BatchDelete, VolumeCopy, ReceiveFile, EC generate/copy/rebuild, vacuum, tiering, VolumeMarkWritable) stays blocked. A side effect is that scrub can now fence broken volumes readonly on a server already in maintenance. Fixes #11066 Generated with [Devin](https://devin.ai) Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- seaweed-volume/src/server/grpc_server.rs | 21 ++-- .../grpc/admin_lifecycle_test.go | 24 ++-- .../grpc/admin_readonly_collection_test.go | 16 ++- .../volume_server/grpc/erasure_coding_test.go | 47 +++++--- test/volume_server/grpc/scrub_query_test.go | 21 +++- weed/server/volume_grpc_admin.go | 14 +-- weed/server/volume_grpc_erasure_coding.go | 5 +- weed/server/volume_grpc_maintenance_test.go | 111 ++++++++++++++++++ 8 files changed, 198 insertions(+), 61 deletions(-) create mode 100644 weed/server/volume_grpc_maintenance_test.go diff --git a/seaweed-volume/src/server/grpc_server.rs b/seaweed-volume/src/server/grpc_server.rs index f6f546ff4..051b47119 100644 --- a/seaweed-volume/src/server/grpc_server.rs +++ b/seaweed-volume/src/server/grpc_server.rs @@ -297,18 +297,18 @@ impl VolumeGrpcService { } /// Shared helper matching Go's `makeVolumeReadonly(ctx, v, canDelete, persist)`. - /// 1. Check maintenance mode - /// 2. Notify master (readonly=true) - /// 3. Mark local volume readonly - /// 4. Notify master again (cover heartbeat race) + /// Not gated on maintenance mode: marking a volume readonly only restricts a + /// server that is already meant to be read-only, and it is the first step of + /// moving a volume off a server under evacuation (issue #11066). + /// 1. Notify master (readonly=true) + /// 2. Mark local volume readonly + /// 3. Notify master again (cover heartbeat race) async fn make_volume_readonly( &self, vid: VolumeId, can_delete: bool, persist: bool, ) -> Result<(), Status> { - self.state.check_maintenance()?; - let info = { let store = self.state.store.read().unwrap(); let (loc_idx, vol) = store @@ -1020,12 +1020,14 @@ impl VolumeServer for VolumeGrpcService { )) } + /// Allowed in maintenance mode: it removes data from the server rather than + /// adding any, and evacuating a server in maintenance mode ends each move by + /// deleting the source copy (issue #11066). async fn volume_delete( &self, request: Request, ) -> Result, Status> { self.check_grpc_admin_auth(&request)?; - self.state.check_maintenance()?; let req = request.into_inner(); let vid = VolumeId(req.volume_id); let mut store = self.state.store.write().unwrap(); @@ -1059,7 +1061,7 @@ impl VolumeServer for VolumeGrpcService { self.check_grpc_admin_auth(&request)?; let req = request.into_inner(); let vid = VolumeId(req.volume_id); - // Go: volume lookup (L239-241) happens before maintenance check (L166 in makeVolumeReadonly) + // Go: VolumeMarkReadonly looks the volume up before calling makeVolumeReadonly { let store = self.state.store.read().unwrap(); store @@ -3014,12 +3016,13 @@ impl VolumeServer for VolumeGrpcService { )) } + /// Allowed in maintenance mode: like `volume_delete` it only removes data, and + /// evacuating EC shards off a server in maintenance mode ends here (issue #11066). async fn volume_ec_shards_delete( &self, request: Request, ) -> Result, Status> { self.check_grpc_admin_auth(&request)?; - self.state.check_maintenance()?; let req = request.into_inner(); let vid = VolumeId(req.volume_id); diff --git a/test/volume_server/grpc/admin_lifecycle_test.go b/test/volume_server/grpc/admin_lifecycle_test.go index 7ec5d64ef..10aaba34b 100644 --- a/test/volume_server/grpc/admin_lifecycle_test.go +++ b/test/volume_server/grpc/admin_lifecycle_test.go @@ -179,7 +179,9 @@ func TestAllocateDuplicateAndMountUnmountMissingVariants(t *testing.T) { } } -func TestMaintenanceModeRejectsVolumeDelete(t *testing.T) { +// Evacuating a server in maintenance mode ends each move by deleting the source +// volume, so VolumeDelete must stay available in that mode (issue #11066). +func TestMaintenanceModeAllowsVolumeDelete(t *testing.T) { if testing.Short() { t.Skip("skipping integration test in short mode") } @@ -194,22 +196,14 @@ func TestMaintenanceModeRejectsVolumeDelete(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - stateResp, err := client.GetState(ctx, &volume_server_pb.GetStateRequest{}) - if err != nil { - t.Fatalf("GetState failed: %v", err) - } - _, err = client.SetState(ctx, &volume_server_pb.SetStateRequest{ - State: &volume_server_pb.VolumeServerState{Maintenance: true, Version: stateResp.GetState().GetVersion()}, - }) - if err != nil { - t.Fatalf("SetState maintenance=true failed: %v", err) + framework.EnableMaintenanceMode(t, ctx, client) + + if _, err := client.VolumeDelete(ctx, &volume_server_pb.VolumeDeleteRequest{VolumeId: volumeID, OnlyEmpty: true}); err != nil { + t.Fatalf("VolumeDelete should succeed in maintenance mode, got: %v", err) } - _, err = client.VolumeDelete(ctx, &volume_server_pb.VolumeDeleteRequest{VolumeId: volumeID, OnlyEmpty: true}) + _, err := client.VolumeStatus(ctx, &volume_server_pb.VolumeStatusRequest{VolumeId: volumeID}) if err == nil { - t.Fatalf("VolumeDelete should fail when maintenance mode is enabled") - } - if !strings.Contains(err.Error(), "maintenance mode") { - t.Fatalf("expected maintenance mode error, got: %v", err) + t.Fatalf("VolumeStatus should fail once the volume is deleted") } } diff --git a/test/volume_server/grpc/admin_readonly_collection_test.go b/test/volume_server/grpc/admin_readonly_collection_test.go index 5f9679f2e..16760209c 100644 --- a/test/volume_server/grpc/admin_readonly_collection_test.go +++ b/test/volume_server/grpc/admin_readonly_collection_test.go @@ -117,19 +117,27 @@ func TestVolumeMarkReadonlyWritableErrorPaths(t *testing.T) { // enter maintenance mode framework.EnableMaintenanceMode(t, ctx, grpcClient) - // existing volume in maintenance mode should return "maintenance mode" error + // marking readonly only restricts the server further and is the first step + // of evacuating it, so maintenance mode lets it through (issue #11066) _, err = grpcClient.VolumeMarkReadonly(ctx, &volume_server_pb.VolumeMarkReadonlyRequest{VolumeId: volumeID, Persist: true}) - if err == nil || !strings.Contains(err.Error(), "maintenance mode") { - t.Fatalf("VolumeMarkReadonly maintenance error mismatch: %v", err) + if err != nil { + t.Fatalf("VolumeMarkReadonly should succeed in maintenance mode, got: %v", err) + } + statusResp, err := grpcClient.VolumeStatus(ctx, &volume_server_pb.VolumeStatusRequest{VolumeId: volumeID}) + if err != nil { + t.Fatalf("VolumeStatus after readonly in maintenance failed: %v", err) + } + if !statusResp.GetIsReadOnly() { + t.Fatalf("VolumeStatus expected readonly=true after VolumeMarkReadonly in maintenance mode") } + // reopening a volume for writes is still refused on a read-only server _, err = grpcClient.VolumeMarkWritable(ctx, &volume_server_pb.VolumeMarkWritableRequest{VolumeId: volumeID}) if err == nil || !strings.Contains(err.Error(), "maintenance mode") { t.Fatalf("VolumeMarkWritable maintenance error mismatch: %v", err) } // non-existent volume in maintenance mode should still return "not found" - // (volume lookup happens before maintenance check) _, err = grpcClient.VolumeMarkReadonly(ctx, &volume_server_pb.VolumeMarkReadonlyRequest{VolumeId: 98773, Persist: true}) if err == nil || !strings.Contains(err.Error(), "not found") { t.Fatalf("VolumeMarkReadonly missing-volume in maintenance error mismatch: %v", err) diff --git a/test/volume_server/grpc/erasure_coding_test.go b/test/volume_server/grpc/erasure_coding_test.go index af75b6f50..7fd20be77 100644 --- a/test/volume_server/grpc/erasure_coding_test.go +++ b/test/volume_server/grpc/erasure_coding_test.go @@ -27,24 +27,27 @@ func TestEcMaintenanceModeRejections(t *testing.T) { conn, grpcClient := framework.DialVolumeServer(t, clusterHarness.VolumeGRPCAddress()) defer conn.Close() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + // Encode a volume before entering maintenance mode so the shard delete + // below has real shards to remove. + const ecVolumeID = uint32(1) + framework.AllocateVolume(t, grpcClient, ecVolumeID, "") + httpClient := framework.NewHTTPClient() + uploadResp := framework.UploadBytes(t, httpClient, clusterHarness.VolumeAdminURL(), framework.NewFileID(ecVolumeID, 990001, 0x11223344), []byte("ec-maintenance-content")) + _ = framework.ReadAllAndClose(t, uploadResp) + if uploadResp.StatusCode != http.StatusCreated { + t.Fatalf("upload expected 201, got %d", uploadResp.StatusCode) + } + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - stateResp, err := grpcClient.GetState(ctx, &volume_server_pb.GetStateRequest{}) - if err != nil { - t.Fatalf("GetState failed: %v", err) - } - _, err = grpcClient.SetState(ctx, &volume_server_pb.SetStateRequest{ - State: &volume_server_pb.VolumeServerState{ - Maintenance: true, - Version: stateResp.GetState().GetVersion(), - }, - }) - if err != nil { - t.Fatalf("SetState maintenance=true failed: %v", err) + if _, err := grpcClient.VolumeEcShardsGenerate(ctx, &volume_server_pb.VolumeEcShardsGenerateRequest{VolumeId: ecVolumeID, Collection: ""}); err != nil { + t.Fatalf("VolumeEcShardsGenerate before maintenance failed: %v", err) } - _, err = grpcClient.VolumeEcShardsGenerate(ctx, &volume_server_pb.VolumeEcShardsGenerateRequest{VolumeId: 1, Collection: ""}) + framework.EnableMaintenanceMode(t, ctx, grpcClient) + + _, err := grpcClient.VolumeEcShardsGenerate(ctx, &volume_server_pb.VolumeEcShardsGenerateRequest{VolumeId: 1, Collection: ""}) if err == nil || !strings.Contains(err.Error(), "maintenance mode") { t.Fatalf("VolumeEcShardsGenerate maintenance error mismatch: %v", err) } @@ -59,13 +62,23 @@ func TestEcMaintenanceModeRejections(t *testing.T) { t.Fatalf("VolumeEcShardsCopy maintenance error mismatch: %v", err) } + // Deleting shards removes data rather than adding it, and is how evacuating + // EC shards off a server in maintenance mode finishes (issue #11066). _, err = grpcClient.VolumeEcShardsDelete(ctx, &volume_server_pb.VolumeEcShardsDeleteRequest{ - VolumeId: 1, + VolumeId: ecVolumeID, Collection: "", ShardIds: []uint32{0}, }) - if err == nil || !strings.Contains(err.Error(), "maintenance mode") { - t.Fatalf("VolumeEcShardsDelete maintenance error mismatch: %v", err) + if err != nil { + t.Fatalf("VolumeEcShardsDelete should succeed in maintenance mode, got: %v", err) + } + _, err = grpcClient.VolumeEcShardsMount(ctx, &volume_server_pb.VolumeEcShardsMountRequest{ + VolumeId: ecVolumeID, + Collection: "", + ShardIds: []uint32{0}, + }) + if err == nil { + t.Fatalf("VolumeEcShardsMount should fail once shard 0 has been deleted") } _, err = grpcClient.VolumeEcBlobDelete(ctx, &volume_server_pb.VolumeEcBlobDeleteRequest{ diff --git a/test/volume_server/grpc/scrub_query_test.go b/test/volume_server/grpc/scrub_query_test.go index a4a776df2..fda4ce29d 100644 --- a/test/volume_server/grpc/scrub_query_test.go +++ b/test/volume_server/grpc/scrub_query_test.go @@ -520,14 +520,25 @@ func TestScrubVolumeMarkBrokenReadonlyInMaintenanceMode(t *testing.T) { framework.EnableMaintenanceMode(t, ctx, grpcClient) - // scrub with the flag in maintenance mode: makeVolumeReadonly should fail - // and ScrubVolume should propagate the error - _, err := grpcClient.ScrubVolume(ctx, &volume_server_pb.ScrubVolumeRequest{ + // marking a volume readonly only restricts a server that is already + // read-only, so maintenance mode does not block fencing broken volumes + resp, err := grpcClient.ScrubVolume(ctx, &volume_server_pb.ScrubVolumeRequest{ VolumeIds: []uint32{volumeID}, Mode: volume_server_pb.VolumeScrubMode_INDEX, MarkBrokenVolumesReadonly: true, }) - if err == nil || !strings.Contains(err.Error(), "maintenance mode") { - t.Fatalf("ScrubVolume with MarkBrokenVolumesReadonly in maintenance mode error mismatch: %v", err) + if err != nil { + t.Fatalf("ScrubVolume with MarkBrokenVolumesReadonly in maintenance mode failed: %v", err) + } + if len(resp.GetBrokenVolumeIds()) == 0 { + t.Fatalf("expected broken volume after corruption") + } + + statusResp, err := grpcClient.VolumeStatus(ctx, &volume_server_pb.VolumeStatusRequest{VolumeId: volumeID}) + if err != nil { + t.Fatalf("VolumeStatus after scrub in maintenance mode failed: %v", err) + } + if !statusResp.GetIsReadOnly() { + t.Fatalf("broken volume should be read-only after MarkBrokenVolumesReadonly scrub in maintenance mode") } } diff --git a/weed/server/volume_grpc_admin.go b/weed/server/volume_grpc_admin.go index 2ee9a4f9a..a4b6b2241 100644 --- a/weed/server/volume_grpc_admin.go +++ b/weed/server/volume_grpc_admin.go @@ -186,6 +186,9 @@ func (vs *VolumeServer) VolumeConsolidateIndex(ctx context.Context, req *volume_ } +// VolumeDelete is allowed in maintenance mode: it removes data from the server +// rather than adding any, and evacuating a server in maintenance mode ends each +// move by deleting the source copy (issue #11066). func (vs *VolumeServer) VolumeDelete(ctx context.Context, req *volume_server_pb.VolumeDeleteRequest) (*volume_server_pb.VolumeDeleteResponse, error) { resp := &volume_server_pb.VolumeDeleteResponse{} @@ -193,10 +196,6 @@ func (vs *VolumeServer) VolumeDelete(ctx context.Context, req *volume_server_pb. return resp, err } - if err := vs.CheckMaintenanceMode(); err != nil { - return resp, err - } - err := vs.store.DeleteVolume(needle.VolumeId(req.VolumeId), req.OnlyEmpty, req.KeepRemoteData) if err != nil { @@ -271,11 +270,10 @@ func (vs *VolumeServer) VolumeConfigure(ctx context.Context, req *volume_server_ } +// makeVolumeReadonly is not gated on maintenance mode: marking a volume readonly +// only restricts a server that is already meant to be read-only, and it is the +// first step of moving a volume off a server under evacuation (issue #11066). func (vs *VolumeServer) makeVolumeReadonly(ctx context.Context, v *storage.Volume, canDelete bool, persist bool) error { - if err := vs.CheckMaintenanceMode(); err != nil { - return err - } - // step 1: stop master from redirecting traffic here if err := vs.notifyMasterVolumeReadonly(ctx, v, true); err != nil { return err diff --git a/weed/server/volume_grpc_erasure_coding.go b/weed/server/volume_grpc_erasure_coding.go index 7d268f641..5a0a12fe0 100644 --- a/weed/server/volume_grpc_erasure_coding.go +++ b/weed/server/volume_grpc_erasure_coding.go @@ -464,13 +464,12 @@ func (vs *VolumeServer) VolumeEcShardsCopy(ctx context.Context, req *volume_serv // VolumeEcShardsDelete local delete the .ecx and some ec data slices if not needed // the shard should not be mounted before calling this. +// Allowed in maintenance mode: like VolumeDelete it only removes data, and +// evacuating EC shards off a server in maintenance mode ends here (issue #11066). func (vs *VolumeServer) VolumeEcShardsDelete(ctx context.Context, req *volume_server_pb.VolumeEcShardsDeleteRequest) (*volume_server_pb.VolumeEcShardsDeleteResponse, error) { if err := vs.checkGrpcAdminAuth(ctx); err != nil { return nil, err } - if err := vs.CheckMaintenanceMode(); err != nil { - return nil, err - } bName := erasure_coding.EcShardBaseFileName(req.Collection, int(req.VolumeId)) diff --git a/weed/server/volume_grpc_maintenance_test.go b/weed/server/volume_grpc_maintenance_test.go new file mode 100644 index 000000000..2f1f33093 --- /dev/null +++ b/weed/server/volume_grpc_maintenance_test.go @@ -0,0 +1,111 @@ +package weed_server + +import ( + "context" + "os" + "testing" + + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" + + "github.com/seaweedfs/seaweedfs/weed/pb/master_pb" + "github.com/seaweedfs/seaweedfs/weed/pb/volume_server_pb" + "github.com/seaweedfs/seaweedfs/weed/storage" + "github.com/seaweedfs/seaweedfs/weed/storage/erasure_coding" + "github.com/seaweedfs/seaweedfs/weed/storage/needle" + "github.com/seaweedfs/seaweedfs/weed/storage/types" + "github.com/seaweedfs/seaweedfs/weed/util" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newMaintenanceModeServer builds a volume server whose store holds volume vid +// in collection and has maintenance mode switched on. +func newMaintenanceModeServer(t *testing.T, vid needle.VolumeId, collection string) (*VolumeServer, string) { + t.Helper() + dir := t.TempDir() + store := newTraversalTestStore(dir) + t.Cleanup(store.Close) + require.NoError(t, store.AddVolume(vid, collection, storage.NeedleMapInMemory, "000", "", 0, needle.GetCurrentVersion(), 0, types.HardDriveType, 0)) + require.NoError(t, store.State.Update(&volume_server_pb.VolumeServerState{Maintenance: true})) + + vs := &VolumeServer{ + store: store, + grpcDialOption: grpc.WithTransportCredentials(insecure.NewCredentials()), + } + require.True(t, vs.MaintenanceMode()) + return vs, dir +} + +type fakeReadonlyAcceptingMaster struct { + master_pb.UnimplementedSeaweedServer +} + +func (s *fakeReadonlyAcceptingMaster) VolumeMarkReadonly(context.Context, *master_pb.VolumeMarkReadonlyRequest) (*master_pb.VolumeMarkReadonlyResponse, error) { + return &master_pb.VolumeMarkReadonlyResponse{}, nil +} + +// Evacuating a server in maintenance mode marks each volume readonly on the +// source, copies it, then deletes the source (issue #11066). Those source-side +// RPCs remove data or restrict the server further, so maintenance mode must +// let them through — otherwise the mode defeats the evacuation it exists for. +func TestMaintenanceModeAllowsVolumeMarkReadonly(t *testing.T) { + vid := needle.VolumeId(1) + vs, _ := newMaintenanceModeServer(t, vid, "") + vs.setCurrentMaster(startFakeMasterServerForLeaderLookup(t, &fakeReadonlyAcceptingMaster{})) + + _, err := vs.VolumeMarkReadonly(context.Background(), &volume_server_pb.VolumeMarkReadonlyRequest{VolumeId: uint32(vid)}) + require.NoError(t, err) + assert.True(t, vs.store.GetVolume(vid).IsReadOnly()) +} + +func TestMaintenanceModeAllowsVolumeDelete(t *testing.T) { + vid := needle.VolumeId(2) + vs, _ := newMaintenanceModeServer(t, vid, "") + + _, err := vs.VolumeDelete(context.Background(), &volume_server_pb.VolumeDeleteRequest{VolumeId: uint32(vid)}) + require.NoError(t, err) + assert.Nil(t, vs.store.GetVolume(vid), "volume should be gone from the store") +} + +func TestMaintenanceModeAllowsVolumeEcShardsDelete(t *testing.T) { + const collection = "ec-maint" + vid := needle.VolumeId(3) + vs, dir := newMaintenanceModeServer(t, needle.VolumeId(99), "") + + base := erasure_coding.EcShardFileName(collection, dir, int(vid)) + require.NoError(t, os.WriteFile(base+".ecx", make([]byte, 16), 0o644)) + for _, id := range []int{0, 1} { + require.NoError(t, os.WriteFile(base+erasure_coding.ToExt(id), []byte("s"), 0o644)) + } + + _, err := vs.VolumeEcShardsDelete(context.Background(), &volume_server_pb.VolumeEcShardsDeleteRequest{ + VolumeId: uint32(vid), + Collection: collection, + ShardIds: []uint32{0, 1}, + }) + require.NoError(t, err) + assert.False(t, util.FileExists(base+erasure_coding.ToExt(0))) + assert.False(t, util.FileExists(base+erasure_coding.ToExt(1))) +} + +// Maintenance mode keeps rejecting RPCs that add data to the server or reopen +// it for writes; only the removal/restriction path above is exempt. +func TestMaintenanceModeStillBlocksWrites(t *testing.T) { + vid := needle.VolumeId(4) + vs, _ := newMaintenanceModeServer(t, vid, "") + wantErr := vs.CheckMaintenanceMode().Error() + ctx := context.Background() + + _, err := vs.AllocateVolume(ctx, &volume_server_pb.AllocateVolumeRequest{VolumeId: 5, Replication: "000", DiskType: string(types.HardDriveType)}) + assert.EqualError(t, err, wantErr, "AllocateVolume") + + _, err = vs.WriteNeedleBlob(ctx, &volume_server_pb.WriteNeedleBlobRequest{VolumeId: uint32(vid), NeedleId: 1, Size: 1, NeedleBlob: []byte{0}}) + assert.EqualError(t, err, wantErr, "WriteNeedleBlob") + + _, err = vs.BatchDelete(ctx, &volume_server_pb.BatchDeleteRequest{FileIds: []string{"4,01637037d6"}}) + assert.EqualError(t, err, wantErr, "BatchDelete") + + _, err = vs.VolumeMarkWritable(ctx, &volume_server_pb.VolumeMarkWritableRequest{VolumeId: uint32(vid)}) + assert.EqualError(t, err, wantErr, "VolumeMarkWritable") +}