master: deleting a read-only volume no longer drifts the active count (#12285)

* master: undo the stored record when a delete report arrives

Deleting a volume decremented the active count from the delete report's
ReadOnly flag, which the volume server never sets, so a volume the master
had already stopped counting as writable was uncounted a second time.
Every volume.move does this to its source, and the node, rack, DC and
topology counts drifted negative; the full-list path already used the
stored record. Undo exactly what was counted for the record held.

* volume: say whether a deleted volume was read-only

Explicit deletes, unmounts, collection deletes and the departures named
in a delta heartbeat all left ReadOnly unset, so an older master undid
the writable count for a volume it was no longer counting. Carry the
flags, as the Rust server already does.

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

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,
 	}
 }
 

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) {

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)

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)