diff --git a/weed/storage/store.go b/weed/storage/store.go index 1c395c69e6..4f6a7cc72c 100644 --- a/weed/storage/store.go +++ b/weed/storage/store.go @@ -287,14 +287,17 @@ func (s *Store) DeleteCollection(collection string) (e error) { // volume grown and destroyed between two of them was never reported at // all, so nothing else would ever tell the master its slot came free. for _, v := range deleted { + readOnly, _, readOnlyCanDelete, _ := v.ReadOnlyReasons() s.DeletedVolumesChan <- &master_pb.VolumeShortInformationMessage{ - Id: uint32(v.Id), - Collection: v.Collection, - ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), - Version: uint32(v.Version()), - Ttl: v.Ttl.ToUint32(), - DiskType: string(location.DiskType), - DiskId: v.diskId, + Id: uint32(v.Id), + Collection: v.Collection, + ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), + Version: uint32(v.Version()), + Ttl: v.Ttl.ToUint32(), + DiskType: string(location.DiskType), + DiskId: v.diskId, + ReadOnly: readOnly, + ReadOnlyCanDelete: readOnlyCanDelete, } } if err != nil { @@ -1176,14 +1179,17 @@ func (s *Store) UnmountVolume(i needle.VolumeId) error { if !found { continue } + readOnly, _, readOnlyCanDelete, _ := v.ReadOnlyReasons() message := master_pb.VolumeShortInformationMessage{ - Id: uint32(v.Id), - Collection: v.Collection, - ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), - Version: uint32(v.Version()), - Ttl: v.Ttl.ToUint32(), - DiskType: string(location.DiskType), - DiskId: v.diskId, + Id: uint32(v.Id), + Collection: v.Collection, + ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), + Version: uint32(v.Version()), + Ttl: v.Ttl.ToUint32(), + DiskType: string(location.DiskType), + DiskId: v.diskId, + ReadOnly: readOnly, + ReadOnlyCanDelete: readOnlyCanDelete, } if err := location.UnloadVolume(i); err != nil { if err == ErrVolumeNotFound { @@ -1279,14 +1285,17 @@ func (s *Store) DeleteVolume(i needle.VolumeId, onlyEmpty bool, onlyGarbage bool if !found { continue } + readOnly, _, readOnlyCanDelete, _ := v.ReadOnlyReasons() message := master_pb.VolumeShortInformationMessage{ - Id: uint32(v.Id), - Collection: v.Collection, - ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), - Version: uint32(v.Version()), - Ttl: v.Ttl.ToUint32(), - DiskType: string(location.DiskType), - DiskId: v.diskId, + Id: uint32(v.Id), + Collection: v.Collection, + ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), + Version: uint32(v.Version()), + Ttl: v.Ttl.ToUint32(), + DiskType: string(location.DiskType), + DiskId: v.diskId, + ReadOnly: readOnly, + ReadOnlyCanDelete: readOnlyCanDelete, } err := location.DeleteVolume(i, onlyEmpty, onlyGarbage, keepRemoteData) if err == nil { @@ -1356,14 +1365,17 @@ func (s *Store) deleteVolumeGuarded(i needle.VolumeId, onlyEmpty bool, onlyGarba var deletedMessages []*master_pb.VolumeShortInformationMessage for _, location := range lockedLocations { v := location.volumes[i] + readOnly, _, readOnlyCanDelete, _ := v.ReadOnlyReasons() message := &master_pb.VolumeShortInformationMessage{ - Id: uint32(v.Id), - Collection: v.Collection, - ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), - Version: uint32(v.Version()), - Ttl: v.Ttl.ToUint32(), - DiskType: string(location.DiskType), - DiskId: v.diskId, + Id: uint32(v.Id), + Collection: v.Collection, + ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), + Version: uint32(v.Version()), + Ttl: v.Ttl.ToUint32(), + DiskType: string(location.DiskType), + DiskId: v.diskId, + ReadOnly: readOnly, + ReadOnlyCanDelete: readOnlyCanDelete, } if err := v.destroyLocked(onlyEmpty, onlyGarbage, keepRemoteData); err != nil { // A real failure on one disk must not be masked by another copy's diff --git a/weed/storage/store_volume_report.go b/weed/storage/store_volume_report.go index d74ef72abd..5d9787b9ac 100644 --- a/weed/storage/store_volume_report.go +++ b/weed/storage/store_volume_report.go @@ -26,11 +26,13 @@ type volumeReportKey struct { // internVolumeString warns against, would have the next volume make a second // copy. type reportedIdentity struct { - collection string - diskType string - replicaPlacement uint32 - version uint32 - ttl uint32 + collection string + diskType string + replicaPlacement uint32 + version uint32 + ttl uint32 + readOnly bool + readOnlyCanDelete bool } // reportedVolume is what the master was told about one volume copy: the hash @@ -123,24 +125,28 @@ func (s *volumeReportState) record(m *master_pb.VolumeInformationMessage, hash u func identityOf(m *master_pb.VolumeInformationMessage) unique.Handle[reportedIdentity] { return unique.Make(reportedIdentity{ - collection: m.Collection, - diskType: m.DiskType, - replicaPlacement: m.ReplicaPlacement, - version: m.Version, - ttl: m.Ttl, + collection: m.Collection, + diskType: m.DiskType, + replicaPlacement: m.ReplicaPlacement, + version: m.Version, + ttl: m.Ttl, + readOnly: m.ReadOnly, + readOnlyCanDelete: m.ReadOnlyCanDelete, }) } func (held reportedVolume) toShortInformation(key volumeReportKey) *master_pb.VolumeShortInformationMessage { identity := held.identity.Value() return &master_pb.VolumeShortInformationMessage{ - Id: key.volumeId, - Collection: identity.collection, - ReplicaPlacement: identity.replicaPlacement, - Version: identity.version, - Ttl: identity.ttl, - DiskType: identity.diskType, - DiskId: key.diskId, + Id: key.volumeId, + Collection: identity.collection, + ReplicaPlacement: identity.replicaPlacement, + Version: identity.version, + Ttl: identity.ttl, + DiskType: identity.diskType, + DiskId: key.diskId, + ReadOnly: identity.readOnly, + ReadOnlyCanDelete: identity.readOnlyCanDelete, } } diff --git a/weed/storage/store_volume_report_test.go b/weed/storage/store_volume_report_test.go index 6725c33844..b1918e524f 100644 --- a/weed/storage/store_volume_report_test.go +++ b/weed/storage/store_volume_report_test.go @@ -178,6 +178,34 @@ func TestDepartedVolumeIsNamedInTheDelta(t *testing.T) { } } +// The master undoes what it counted for the volume, so a delete has to say +// whether the volume was still counted as writable. +func TestDeleteReportsCarryTheReadOnlyState(t *testing.T) { + store := reportingStore(t, 1, 2) + store.ResetVolumeReporting() + store.AcceptVolumeChanges() + store.CollectHeartbeat() + for _, vid := range []needle.VolumeId{1, 2} { + if err := store.MarkVolumeReadonly(vid, false, false); err != nil { + t.Fatal(err) + } + } + store.CollectHeartbeat() + + store.Locations[0].UnloadVolume(needle.VolumeId(1)) + heartbeat := store.CollectHeartbeat() + if len(heartbeat.DeletedVolumes) != 1 || !heartbeat.DeletedVolumes[0].ReadOnly { + t.Fatalf("departed volume 1 not reported read-only: %v", heartbeat.DeletedVolumes) + } + + if err := store.DeleteVolume(needle.VolumeId(2), false, false, false); err != nil { + t.Fatal(err) + } + if m := <-store.DeletedVolumesChan; m.Id != 2 || !m.ReadOnly { + t.Fatalf("deleted volume 2 not reported read-only: %v", m) + } +} + // A full list is already the whole truth; naming departures beside it would // tell the master to remove what the list already excludes. func TestFullListCarriesNoDepartures(t *testing.T) { diff --git a/weed/topology/data_node.go b/weed/topology/data_node.go index 444adf7646..5e90dff418 100644 --- a/weed/topology/data_node.go +++ b/weed/topology/data_node.go @@ -187,7 +187,9 @@ func (dn *DataNode) DeltaUpdateVolumes(newVolumes, deletedVolumes []storage.Volu for _, v := range deletedVolumes { disk := dn.getOrCreateDisk(v.DiskType) - _, err := disk.GetVolumesById(v.Id) + // Undo what was counted for the stored record: the delete report does + // not carry the read-only state the master has since learned. + held, err := disk.GetVolumesById(v.Id) if err != nil { continue } @@ -195,10 +197,10 @@ func (dn *DataNode) DeltaUpdateVolumes(newVolumes, deletedVolumes []storage.Volu deltaDiskUsage := &DiskUsageCounts{} deltaDiskUsage.volumeCount = -1 - if v.IsRemote() { + if held.IsRemote() { deltaDiskUsage.remoteVolumeCount = -1 } - if !v.ReadOnly { + if !held.ReadOnly { deltaDiskUsage.activeVolumeCount = -1 } disk.UpAdjustDiskUsageDelta(types.ToDiskType(v.DiskType), deltaDiskUsage) diff --git a/weed/topology/topology_test.go b/weed/topology/topology_test.go index 5bf0202c25..681d9f282d 100644 --- a/weed/topology/topology_test.go +++ b/weed/topology/topology_test.go @@ -471,6 +471,37 @@ func TestVolumeReadOnlyStatusChange(t *testing.T) { assert(t, "activeVolumeCount after writable again", int(usageCounts.activeVolumeCount), 1) } +func TestDeletingReadOnlyVolumeKeepsActiveCount(t *testing.T) { + topo := NewTopology("weedfs", sequence.NewMemorySequencer(), 32*1024, 5, false) + dn := topo.GetOrCreateDataCenter("dc1").GetOrCreateRack("rack1"). + GetOrCreateDataNode("127.0.0.1", 34534, 0, "127.0.0.1", "", map[string]uint32{"": 25}) + + v := storage.VolumeInfo{ + Id: needle.VolumeId(1), + Version: needle.GetCurrentVersion(), + ReplicaPlacement: &super_block.ReplicaPlacement{}, + Ttl: needle.EMPTY_TTL, + } + dn.UpdateVolumes([]storage.VolumeInfo{v}) + topo.RegisterVolumeLayout(v, dn) + v.ReadOnly = true + dn.UpdateVolumes([]storage.VolumeInfo{v}) + assert(t, "activeVolumeCount after read-only", int(topo.diskUsages.usages[types.HardDriveType].activeVolumeCount), 0) + + // The server's delete report does not say the volume was read-only. + topo.IncrementalSyncDataNodeRegistration(nil, []*master_pb.VolumeShortInformationMessage{ + {Id: 1, Version: uint32(needle.GetCurrentVersion())}, + }, dn) + + for name, usage := range map[string]*DiskUsageCounts{ + "topology": topo.diskUsages.usages[types.HardDriveType], + "node": dn.diskUsages.usages[types.HardDriveType], + } { + assert(t, name+" volumeCount after delete", int(usage.volumeCount), 0) + assert(t, name+" activeVolumeCount after delete", int(usage.activeVolumeCount), 0) + } +} + func TestVolumeReadOnlyAndRemoteStatusChange(t *testing.T) { topo := NewTopology("weedfs", sequence.NewMemorySequencer(), 32*1024, 5, false)