From 2e0ccfbd0f0f681c82aa71fcd571d558e9d75d02 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 12 Nov 2026 19:46:00 -0800 Subject: [PATCH] shell: restore the tier download source when its readonly mark fails VolumeMarkReadonly blocks local writes and deletes before its second master notification, so a failed notification returns an error with the source already frozen. Restore it whenever the status was read. Co-Authored-By: Claude Opus 5.5 --- weed/shell/command_volume_tier_download.go | 14 +++++++--- .../command_volume_tier_download_test.go | 27 +++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/weed/shell/command_volume_tier_download.go b/weed/shell/command_volume_tier_download.go index cb268d30b6..b30d87506e 100644 --- a/weed/shell/command_volume_tier_download.go +++ b/weed/shell/command_volume_tier_download.go @@ -190,10 +190,12 @@ func downloadAndReplicateFromRemoteTier(grpcDialOption grpc.DialOption, writer i } restore, err := freezeTierDownloadSource(grpcDialOption, vid, source) + if restore != nil { + defer restore(writer) + } if err != nil { return fmt.Errorf("block deletes on volume %d at %s: %v", vid, source, err) } - defer restore(writer) // Copy over all remote-backed replicas but one, re-reading the topology // so a replica that was offline or created meanwhile is replaced too. The @@ -245,18 +247,22 @@ func downloadAndReplicateFromRemoteTier(grpcDialOption grpc.DialOption, writer i // freezeTierDownloadSource blocks deletes on the copy source: a delete applied // after VolumeCopy read the source index would be lost on the replaced replica. -// restore brings back the writable mark, or reloads the persisted state. +// restore brings back the writable mark, or reloads the persisted state. It is +// also returned when the mark fails, since the server may have applied it. func freezeTierDownloadSource(grpcDialOption grpc.DialOption, vid needle.VolumeId, source pb.ServerAddress) (restore func(io.Writer), err error) { - var wasWritable bool + var wasWritable, statusRead bool err = operation.WithVolumeServerClient(false, source, grpcDialOption, func(client volume_server_pb.VolumeServerClient) error { resp, statusErr := client.VolumeStatus(context.Background(), &volume_server_pb.VolumeStatusRequest{VolumeId: uint32(vid)}) if statusErr != nil { return statusErr } - wasWritable = !resp.IsReadOnly + wasWritable, statusRead = !resp.IsReadOnly, true _, markErr := client.VolumeMarkReadonly(context.Background(), &volume_server_pb.VolumeMarkReadonlyRequest{VolumeId: uint32(vid)}) return markErr }) + if !statusRead { + return nil, err + } restore = func(writer io.Writer) { restoreErr := operation.WithVolumeServerClient(false, source, grpcDialOption, func(client volume_server_pb.VolumeServerClient) error { if wasWritable { diff --git a/weed/shell/command_volume_tier_download_test.go b/weed/shell/command_volume_tier_download_test.go index a359679295..9f042644df 100644 --- a/weed/shell/command_volume_tier_download_test.go +++ b/weed/shell/command_volume_tier_download_test.go @@ -44,6 +44,8 @@ type tierDownloadCluster struct { placement uint32 // staleTwin lists a remote-backed twin of the volume on another disk staleTwin map[pb.ServerAddress]bool + // markFails applies the readonly mark, then fails as a lost master notification does + markFails map[pb.ServerAddress]bool } type tierDownloadTestServer struct { @@ -104,6 +106,9 @@ func (s *tierDownloadTestServer) VolumeMarkReadonly(_ context.Context, req *volu c.mu.Lock() defer c.mu.Unlock() c.frozen[s.addr] = !req.CanDelete + if c.markFails[s.addr] { + return nil, fmt.Errorf("notify master: leader changed") + } return &volume_server_pb.VolumeMarkReadonlyResponse{}, nil } @@ -165,6 +170,7 @@ func newTierDownloadCluster(t *testing.T, states ...string) *tierDownloadCluster writable: make(map[pb.ServerAddress]bool), remounts: make(map[pb.ServerAddress]int), staleTwin: make(map[pb.ServerAddress]bool), + markFails: make(map[pb.ServerAddress]bool), } for _, state := range states { lis, err := net.Listen("tcp", "127.0.0.1:0") @@ -385,3 +391,24 @@ func TestTierDownloadKeepsRemoteObjectForStaleTwinOnSource(t *testing.T) { t.Errorf("remote object deleted while a twin on %s still references it", c.servers[0]) } } + +func TestTierDownloadRestoresSourceWhenMarkFailsAfterApplying(t *testing.T) { + for _, writable := range []bool{false, true} { + c := newTierDownloadCluster(t, tierReplicaLocal, tierReplicaRemote, tierReplicaRemote) + source := c.servers[0] + c.writable[source] = writable + c.markFails[source] = true + if err := c.run(t); err == nil { + t.Fatalf("writable=%v: tier download should fail when the readonly mark fails", writable) + } + if c.frozen[source] { + t.Errorf("writable=%v: source still blocks deletes after the failed mark", writable) + } + if c.writable[source] != writable { + t.Errorf("writable=%v: source writable state changed to %v", writable, c.writable[source]) + } + if !c.objectExists { + t.Errorf("writable=%v: remote object deleted although no replica was replaced", writable) + } + } +}