From 2d2fdeac3d4b984257d81246198fc1d1b2b8ddac Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 7 Jul 2026 23:00:22 -0700 Subject: [PATCH] volume: keep tier-uploaded volume reporting to master after volume.tier.upload (#10259) volume: keep tier-uploaded volume reporting to master A live volume.tier.upload removes the local .dat and swaps the data backend to remote, but v.hasRemoteFile was only ever set when a volume is loaded from disk, so the running volume kept it false. The phantom .dat guard then saw fileCount>0, !HasRemoteFile, and a missing .dat, and stopped reporting the volume to the master. The volume vanished from the topology even though the upload succeeded and the data was in cloud storage. Set hasRemoteFile in LoadRemoteFile, the single point where a volume's backend becomes remote, so it is true both on disk-scan load and after an in-process tier upload. Route the backend reassignment through SwapDataBackend so it happens under dataFileAccessLock, closing the old backend and never racing the heartbeat's concurrent DataBackend read. Make the field atomic since the heartbeat now reads it concurrently with the tier-upload handler that writes it. --- weed/storage/remote_tier_integration_test.go | 57 ++++++++++++++++---- weed/storage/store_load_balancing_test.go | 4 +- weed/storage/volume.go | 2 +- weed/storage/volume_tier.go | 21 +++++--- weed/storage/volume_write.go | 2 +- 5 files changed, 63 insertions(+), 23 deletions(-) diff --git a/weed/storage/remote_tier_integration_test.go b/weed/storage/remote_tier_integration_test.go index 59744a84c..ec08130c3 100644 --- a/weed/storage/remote_tier_integration_test.go +++ b/weed/storage/remote_tier_integration_test.go @@ -147,9 +147,9 @@ func (f *localDirBackendFile) Truncate(off int64) error { return os.Truncate(filepath.Join(f.backend.root, f.key), off) } -func (f *localDirBackendFile) Close() error { return nil } -func (f *localDirBackendFile) Name() string { return f.key } -func (f *localDirBackendFile) Sync() error { return nil } +func (f *localDirBackendFile) Close() error { return nil } +func (f *localDirBackendFile) Name() string { return f.key } +func (f *localDirBackendFile) Sync() error { return nil } func (f *localDirBackendFile) GetStat() (int64, time.Time, error) { files := f.tierInfo.GetFiles() if len(files) == 0 { @@ -174,11 +174,13 @@ func registerTestBackend(t *testing.T, b *localDirBackend) { }) } -// tierUpVolume creates a real on-disk volume, writes a few needles, then -// uploads the .dat to the fake backend and rewrites the volume in remote -// mode (mirrors the production flow in volume_grpc_tier_upload.go but -// in-process). -func tierUpVolume(t *testing.T, dir string, vid needle.VolumeId, b *localDirBackend) (collection string, key string) { +// tierUpVolumeLive creates a real on-disk volume, writes a few needles, then +// uploads the .dat to the fake backend and rewrites the volume in remote mode +// (mirrors the production flow in volume_grpc_tier_upload.go but in-process). +// It returns the still-open volume, exactly as a volume server holds it after a +// live `volume.tier.upload` — no reload. Callers that want the on-disk state a +// server sees after restart use tierUpVolume, which closes it. +func tierUpVolumeLive(t *testing.T, dir string, vid needle.VolumeId, b *localDirBackend) (v *Volume, key string) { t.Helper() v, err := NewVolume(dir, dir, "", vid, NeedleMapInMemory, &super_block.ReplicaPlacement{}, &needle.TTL{}, 0, needle.GetCurrentVersion(), 0, 0) require.NoError(t, err) @@ -208,10 +210,16 @@ func tierUpVolume(t *testing.T, dir string, vid needle.VolumeId, b *localDirBack require.NoError(t, v.LoadRemoteFile()) require.NoError(t, os.Remove(v.FileName(".dat"))) - // Close the volume cleanly. Tests below reload it from disk to mirror - // what a volume server does on restart with a tiered volume. - v.Close() + return v, uploadKey +} +// tierUpVolume runs tierUpVolumeLive then closes the volume. Tests using it +// reload from disk to mirror what a volume server does on restart with a +// tiered volume. +func tierUpVolume(t *testing.T, dir string, vid needle.VolumeId, b *localDirBackend) (collection string, key string) { + t.Helper() + v, uploadKey := tierUpVolumeLive(t, dir, vid, b) + v.Close() return v.Collection, uploadKey } @@ -259,6 +267,33 @@ func TestRemoteTier_DiskScanLoadsRemoteOnlyVolume(t *testing.T) { v.Close() } +// TestRemoteTier_LiveTierUpload_StillReportsToMaster covers a live +// `volume.tier.upload`: the .dat is removed and the volume serves from remote, +// but the same in-memory Volume keeps heartbeating with no reload. The +// phantom-.dat guard must not suppress it just because .dat is gone — +// LoadRemoteFile has flipped it into remote mode, so ToVolumeInformationMessage +// must still report it to the master. If HasRemoteFile stayed false the volume +// would vanish from the topology ("volume not found"). +func TestRemoteTier_LiveTierUpload_StillReportsToMaster(t *testing.T) { + b := newLocalDirBackend(t) + registerTestBackend(t, b) + + dir := t.TempDir() + const vid = needle.VolumeId(67) + v, _ := tierUpVolumeLive(t, dir, vid, b) + defer v.Close() + // A store-owned volume always carries its DiskLocation; NewVolume leaves it + // nil, so give it one for the IsReadOnly disk-space check inside the heartbeat. + v.location = &DiskLocation{Directory: dir, DiskType: types.HddType} + + require.True(t, v.HasRemoteFile(), "a tier-uploaded volume is in remote mode even before any reload") + require.False(t, util.FileExists(v.FileName(".dat")), "tier-up should have removed the local .dat") + + _, msg := v.ToVolumeInformationMessage() + require.NotNil(t, msg, "tier-uploaded volume must still report to master") + require.NotEmpty(t, msg.RemoteStorageName, "reported volume must carry its remote backend name") +} + // TestRemoteTier_Move_KeepsRemoteObject simulates the move-on-source-after-copy // step of a balance: Destroy(onlyEmpty=false, keepRemoteData=true). The remote // object must survive — the destination's freshly-copied .vif points at it. diff --git a/weed/storage/store_load_balancing_test.go b/weed/storage/store_load_balancing_test.go index 90fa7ff7f..c4ca73178 100644 --- a/weed/storage/store_load_balancing_test.go +++ b/weed/storage/store_load_balancing_test.go @@ -102,7 +102,7 @@ func TestLocalVolumesLen(t *testing.T) { // Mark some as remote if i < tc.remoteVolumes { - vol.hasRemoteFile = true + vol.hasRemoteFile.Store(true) vol.volumeInfo.Files = []*volume_server_pb.RemoteFile{ {BackendType: "s3", BackendId: "test", Key: "test-key"}, } @@ -250,7 +250,7 @@ func createTestVolume(vid needle.VolumeId, isRemote bool) *Volume { } if isRemote { - vol.hasRemoteFile = true + vol.hasRemoteFile.Store(true) vol.volumeInfo.Files = []*volume_server_pb.RemoteFile{ {BackendType: "s3", BackendId: "test", Key: "remote-key-" + strconv.Itoa(int(vid))}, } diff --git a/weed/storage/volume.go b/weed/storage/volume.go index f7a5423f2..e2b81fb17 100644 --- a/weed/storage/volume.go +++ b/weed/storage/volume.go @@ -32,7 +32,7 @@ type Volume struct { noWriteOrDelete bool // if readonly, either noWriteOrDelete or noWriteCanDelete noWriteCanDelete bool // if readonly, either noWriteOrDelete or noWriteCanDelete noWriteLock sync.RWMutex - hasRemoteFile bool // if the volume has a remote file + hasRemoteFile atomic.Bool // if the volume is tiered: data lives in a remote backend MemoryMapMaxSizeMb uint32 super_block.SuperBlock diff --git a/weed/storage/volume_tier.go b/weed/storage/volume_tier.go index 775db67f3..2704b9e60 100644 --- a/weed/storage/volume_tier.go +++ b/weed/storage/volume_tier.go @@ -19,13 +19,15 @@ func (v *Volume) GetVolumeInfo() *volume_server_pb.VolumeInfo { func (v *Volume) maybeLoadVolumeInfo() (found bool) { var err error - v.volumeInfo, v.hasRemoteFile, found, err = volume_info.MaybeLoadVolumeInfo(v.FileName(".vif")) + var hasRemoteFile bool + v.volumeInfo, hasRemoteFile, found, err = volume_info.MaybeLoadVolumeInfo(v.FileName(".vif")) + v.hasRemoteFile.Store(hasRemoteFile) if v.volumeInfo.Version == 0 { v.volumeInfo.Version = uint32(needle.GetCurrentVersion()) } - if v.hasRemoteFile { + if hasRemoteFile { glog.V(0).Infof("volume %d is tiered to %s as %s and read only", v.Id, v.volumeInfo.Files[0].BackendName(), v.volumeInfo.Files[0].Key) } else { @@ -55,7 +57,7 @@ func (v *Volume) maybeLoadVolumeInfo() (found bool) { } func (v *Volume) HasRemoteFile() bool { - return v.hasRemoteFile + return v.hasRemoteFile.Load() } func (v *Volume) LoadRemoteFile() error { @@ -65,11 +67,14 @@ func (v *Volume) LoadRemoteFile() error { return fmt.Errorf("backend storage %s not found", tierFile.BackendName()) } - if v.DataBackend != nil { - v.DataBackend.Close() - } - - v.DataBackend = backendStorage.NewStorageFile(tierFile.Key, v.volumeInfo) + // Swap under dataFileAccessLock (via SwapDataBackend) so the heartbeat's + // concurrent DataBackend read never races this reassignment. Then mark the + // volume tiered so a later heartbeat does not treat the just-removed local + // .dat as a phantom volume and stop reporting it to the master. On disk-scan + // load this is already true; here it flips a volume that was tier-uploaded + // in-process without a reload. + v.SwapDataBackend(backendStorage.NewStorageFile(tierFile.Key, v.volumeInfo)) + v.hasRemoteFile.Store(true) return nil } diff --git a/weed/storage/volume_write.go b/weed/storage/volume_write.go index 0d632142f..9e139c757 100644 --- a/weed/storage/volume_write.go +++ b/weed/storage/volume_write.go @@ -274,7 +274,7 @@ func (v *Volume) doDeleteRequest(n *needle.Needle) (Size, error) { var offset uint64 var err error size := nv.Size - if !v.hasRemoteFile { + if !v.HasRemoteFile() { n.Data = nil n.UpdateAppendAtNs(v.lastAppendAtNs) offset, _, _, err = n.Append(v.DataBackend, v.Version())