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") +}