From 3674f9d04d4b98778d61160c2df41dd55f4bda40 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 28 May 2026 15:39:31 -0700 Subject: [PATCH] fix(storage): keep EC .vif when deleting a coexisting regular volume (#9723) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(storage): keep EC .vif when deleting a coexisting regular volume A regular volume and an EC volume for the same id share .vif. When EC shards are distributed onto a server that still holds the regular volume — the encode source, or any replica the planner targets — the post-encode VolumeDelete ran removeVolumeFiles and stripped the shared .vif, leaving the freshly built EC volume without its info file. Skip the .vif in removeVolumeFiles when an EC volume for the same id exists on the disk (mounted, or a sealed .ecx on disk). The regular volume's .dat/.idx still go; the EC sidecars survive. A two-server end-to-end test encodes a volume whose source and a stub replica both also receive shards, and asserts the final on-disk layout: both .dat/.idx gone, each server holding only its assigned shards plus .ecx/.vif. Storage unit tests cover the with-EC and no-EC cases, and the Rust seaweed-volume port carries the same guard and tests. * test(storage): assert .idx is removed in the no-EC destroy case Strengthen TestDestroyRemovesVifWhenNoEc to confirm the full regular volume cleanup (.dat, .idx, .vif) when no EC volume coexists. --- seaweed-volume/src/storage/disk_location.rs | 4 +- seaweed-volume/src/storage/volume.rs | 70 +++++++- weed/storage/disk_location.go | 4 +- weed/storage/volume_destroy_ec_vif_test.go | 75 +++++++++ weed/storage/volume_write.go | 28 ++- .../ec_task_encode_files_e2e_test.go | 159 ++++++++++++++++++ 6 files changed, 329 insertions(+), 11 deletions(-) create mode 100644 weed/storage/volume_destroy_ec_vif_test.go create mode 100644 weed/worker/tasks/erasure_coding/ec_task_encode_files_e2e_test.go diff --git a/seaweed-volume/src/storage/disk_location.rs b/seaweed-volume/src/storage/disk_location.rs index a7ed3f28a..21d478b11 100644 --- a/seaweed-volume/src/storage/disk_location.rs +++ b/seaweed-volume/src/storage/disk_location.rs @@ -131,8 +131,8 @@ impl DiskLocation { volume_id = vid.0, "volume was not completed: {}, removing files", note ); - remove_volume_files(&volume_name); - remove_volume_files(&idx_name); + remove_volume_files(&volume_name, false); + remove_volume_files(&idx_name, false); continue; } diff --git a/seaweed-volume/src/storage/volume.rs b/seaweed-volume/src/storage/volume.rs index 1f29c68d0..c2957e1f2 100644 --- a/seaweed-volume/src/storage/volume.rs +++ b/seaweed-volume/src/storage/volume.rs @@ -3157,12 +3157,34 @@ impl Volume { } } + // A regular volume and an EC volume for the same id share .vif. + // When EC artefacts coexist on this disk (e.g. shards distributed onto + // a source replica before it is deleted), keep the .vif so removing the + // regular volume does not strip the EC volume's info file. + let keep_vif = self.shares_vif_with_ec_volume(); self.close(); - remove_volume_files(&self.data_file_name()); - remove_volume_files(&self.index_file_name()); + remove_volume_files(&self.data_file_name(), keep_vif); + remove_volume_files(&self.index_file_name(), keep_vif); Ok(()) } + /// Reports whether an EC volume for this id has a sealed .ecx on the same + /// disk, in which case its .vif is the same file as the regular volume's + /// and must outlive the regular volume's deletion. Mirrors the on-disk + /// portion of Go's Volume.sharesVifWithEcVolume / HasEcxFileOnDisk. + fn shares_vif_with_ec_volume(&self) -> bool { + let has_ecx = |base: &str| -> bool { + fs::metadata(format!("{}.ecx", base)) + .map(|m| !m.is_dir() && m.len() > 0) + .unwrap_or(false) + }; + if has_ecx(&volume_file_name(&self.dir_idx, &self.collection, self.id)) { + return true; + } + self.dir != self.dir_idx + && has_ecx(&volume_file_name(&self.dir, &self.collection, self.id)) + } + /// Check if an I/O error is EIO (errno 5) and record it for health monitoring. /// On success (None), clears any previously recorded EIO error. /// Matches Go's `checkReadWriteError` in volume_write.go. @@ -3231,10 +3253,13 @@ fn get_append_at_ns(last: u64) -> u64 { /// Remove all files associated with a volume. /// .dat/.idx removals log at info level so destructive calls are traceable. -pub(crate) fn remove_volume_files(base: &str) { +pub(crate) fn remove_volume_files(base: &str, keep_vif: bool) { for ext in &[ ".dat", ".idx", ".vif", ".sdx", ".cpd", ".cpx", ".note", ".rdb", ] { + if *ext == ".vif" && keep_vif { + continue; + } let path = format!("{}{}", base, ext); let size = fs::metadata(&path).map(|m| m.len()).unwrap_or(0); let existed = fs::remove_file(&path).is_ok(); @@ -4740,4 +4765,43 @@ mod tests { ".vif removed from data dir" ); } + + /// When an EC volume for the same id has a sealed .ecx on the same disk, the + /// .vif is shared with it and must survive the regular volume's deletion. + #[test] + fn test_destroy_keeps_vif_when_ec_coexists() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().to_str().unwrap(); + + let mut v = make_test_volume(dir); + let mut n = Needle { + id: NeedleId(1), + cookie: Cookie(1), + data: b"test".to_vec(), + data_size: 4, + ..Needle::default() + }; + v.write_needle(&mut n, true).unwrap(); + + let vif_path = format!("{}/1.vif", dir); + std::fs::write(&vif_path, r#"{"version":3}"#).unwrap(); + // A sealed .ecx marks a coexisting EC volume for the same id. + let ecx_path = format!("{}/1.ecx", dir); + std::fs::write(&ecx_path, b"ec-index").unwrap(); + + v.destroy(false, false).unwrap(); + + let dat_path = format!("{}/1.dat", dir); + let idx_path = format!("{}/1.idx", dir); + assert!(!std::path::Path::new(&dat_path).exists(), ".dat removed"); + assert!(!std::path::Path::new(&idx_path).exists(), ".idx removed"); + assert!( + std::path::Path::new(&vif_path).exists(), + ".vif kept: shared with the coexisting EC volume" + ); + assert!( + std::path::Path::new(&ecx_path).exists(), + ".ecx is an EC sidecar, never touched here" + ); + } } diff --git a/weed/storage/disk_location.go b/weed/storage/disk_location.go index fbdc66353..bfeff4033 100644 --- a/weed/storage/disk_location.go +++ b/weed/storage/disk_location.go @@ -209,8 +209,8 @@ func (l *DiskLocation) loadExistingVolume(dirEntry os.DirEntry, needleMapKind Ne if util.FileExists(noteFile) { note, _ := os.ReadFile(noteFile) glog.Warningf("volume %s was not completed: %s", volumeName, string(note)) - removeVolumeFiles(l.Directory + "/" + volumeName) - removeVolumeFiles(l.IdxDirectory + "/" + volumeName) + removeVolumeFiles(l.Directory+"/"+volumeName, false) + removeVolumeFiles(l.IdxDirectory+"/"+volumeName, false) return false } diff --git a/weed/storage/volume_destroy_ec_vif_test.go b/weed/storage/volume_destroy_ec_vif_test.go new file mode 100644 index 000000000..c90f68301 --- /dev/null +++ b/weed/storage/volume_destroy_ec_vif_test.go @@ -0,0 +1,75 @@ +package storage + +import ( + "os" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/storage/erasure_coding" + "github.com/seaweedfs/seaweedfs/weed/storage/needle" + "github.com/seaweedfs/seaweedfs/weed/storage/super_block" + "github.com/seaweedfs/seaweedfs/weed/storage/types" + "github.com/seaweedfs/seaweedfs/weed/util" + "github.com/stretchr/testify/require" +) + +// A regular volume and an EC volume for the same id share .vif. Deleting +// the regular volume must drop its .dat/.idx but keep the .vif so the +// coexisting EC volume's info file survives. This is the same-disk case that +// arises when EC shards are distributed onto a source/replica server before +// the original volume is deleted. +func TestDestroyKeepsVifWhenEcCoexists(t *testing.T) { + dir := t.TempDir() + v, err := NewVolume(dir, dir, "", 1, NeedleMapInMemory, &super_block.ReplicaPlacement{}, &needle.TTL{}, 0, needle.GetCurrentVersion(), 0, 0) + require.NoError(t, err) + v.location = newTestDiskLocation(dir) + _, _, _, err = v.writeNeedle2(newRandomNeedle(1), true, false) + require.NoError(t, err) + + base := VolumeFileName(dir, "", 1) + vifPath := base + ".vif" + require.NoError(t, os.WriteFile(vifPath, []byte("ec-volume-info"), 0o644)) + // An on-disk .ecx marks a coexisting EC volume for the same id. + ecxPath := erasure_coding.EcShardFileName("", dir, 1) + ".ecx" + require.NoError(t, os.WriteFile(ecxPath, []byte("ec-index"), 0o644)) + + require.NoError(t, v.Destroy(false, false)) + + assertFileExist(t, false, base+".dat") + assertFileExist(t, false, base+".idx") + assertFileExist(t, true, vifPath) // shared with the EC volume, must survive + assertFileExist(t, true, ecxPath) // EC sidecars are never touched here +} + +// With no coexisting EC volume the .vif is a plain regular-volume file and is +// removed with the rest. +func TestDestroyRemovesVifWhenNoEc(t *testing.T) { + dir := t.TempDir() + v, err := NewVolume(dir, dir, "", 1, NeedleMapInMemory, &super_block.ReplicaPlacement{}, &needle.TTL{}, 0, needle.GetCurrentVersion(), 0, 0) + require.NoError(t, err) + v.location = newTestDiskLocation(dir) + _, _, _, err = v.writeNeedle2(newRandomNeedle(1), true, false) + require.NoError(t, err) + + base := VolumeFileName(dir, "", 1) + vifPath := base + ".vif" + require.NoError(t, os.WriteFile(vifPath, []byte("regular-volume-info"), 0o644)) + + require.NoError(t, v.Destroy(false, false)) + + assertFileExist(t, false, base+".dat") + assertFileExist(t, false, base+".idx") + assertFileExist(t, false, vifPath) // removed along with the regular volume +} + +func newTestDiskLocation(dir string) *DiskLocation { + loc := &DiskLocation{ + Directory: dir, + IdxDirectory: dir, + DiskType: types.HddType, + MaxVolumeCount: 100, + MinFreeSpace: util.MinFreeSpace{Type: util.AsPercent, Percent: 1, Raw: "1"}, + } + loc.volumes = make(map[needle.VolumeId]*Volume) + loc.ecVolumes = make(map[needle.VolumeId]*erasure_coding.EcVolume) + return loc +} diff --git a/weed/storage/volume_write.go b/weed/storage/volume_write.go index 686a19d7d..24b1dbb9d 100644 --- a/weed/storage/volume_write.go +++ b/weed/storage/volume_write.go @@ -94,13 +94,31 @@ func (v *Volume) Destroy(onlyEmpty bool, keepRemoteData bool) (err error) { } } } + // A regular volume and an EC volume for the same id share .vif. When + // EC artefacts coexist on this disk (e.g. shards distributed onto a source + // replica before it is deleted), keep the .vif so removing the regular + // volume does not strip the EC volume's info file. + keepVif := v.sharesVifWithEcVolume() v.doClose() - removeVolumeFiles(v.DataFileName()) - removeVolumeFiles(v.IndexFileName()) + removeVolumeFiles(v.DataFileName(), keepVif) + removeVolumeFiles(v.IndexFileName(), keepVif) return } -func removeVolumeFiles(filename string) { +// sharesVifWithEcVolume reports whether an EC volume for this volume id lives +// on the same disk, in which case its .vif is the same file as the regular +// volume's and must outlive the regular volume's deletion. +func (v *Volume) sharesVifWithEcVolume() bool { + if v.location == nil { + return false + } + if _, found := v.location.FindEcVolume(v.Id); found { + return true + } + return v.location.HasEcxFileOnDisk(v.Collection, v.Id) +} + +func removeVolumeFiles(filename string, keepVif bool) { // .dat/.idx removals log at V(0) so destructive calls are traceable. deleteAndLog := func(ext string) { fullFilename := filename + "." + ext @@ -116,7 +134,9 @@ func removeVolumeFiles(filename string) { } deleteAndLog("dat") deleteAndLog("idx") - deleteAndLog("vif") + if !keepVif { + deleteAndLog("vif") + } // sorted index file deleteAndLog("sdx") // compaction diff --git a/weed/worker/tasks/erasure_coding/ec_task_encode_files_e2e_test.go b/weed/worker/tasks/erasure_coding/ec_task_encode_files_e2e_test.go new file mode 100644 index 000000000..f33e6713c --- /dev/null +++ b/weed/worker/tasks/erasure_coding/ec_task_encode_files_e2e_test.go @@ -0,0 +1,159 @@ +package erasure_coding + +import ( + "context" + "fmt" + "net" + "net/http" + "os" + "path/filepath" + "testing" + "time" + + "github.com/seaweedfs/seaweedfs/test/volume_server/framework" + "github.com/seaweedfs/seaweedfs/test/volume_server/matrix" + "github.com/seaweedfs/seaweedfs/weed/pb/worker_pb" + "github.com/seaweedfs/seaweedfs/weed/storage/erasure_coding" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" +) + +// End-to-end on two servers: a volume with a real replica on server A and a +// 0-byte stub replica of the same id on server B (an interrupted-encode +// leftover). After a full EC encode the cluster must end in exactly one valid +// layout — the complete shard set split across A and B, each with .ecx/.vif — +// and every wrong file must be gone: both regular .dat files (source deleted +// after verify, stub swept before distribute), no shard on the wrong server, +// and B's shared _.vif intact rather than clobbered by the +// stub's delete. +func TestEcEncodeLeavesRightFilesAndRemovesStubAndSource(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + + cluster := framework.StartMultiVolumeCluster(t, matrix.P1(), 2) + dialOption := grpc.WithTransportCredentials(insecure.NewCredentials()) + + const ( + volumeID = uint32(9490) + collection = "ec-e2e" + ) + addrA := serverAddress(cluster, 0) + addrB := serverAddress(cluster, 1) + + connA, clientA := framework.DialVolumeServer(t, cluster.VolumeGRPCAddress(0)) + defer connA.Close() + connB, clientB := framework.DialVolumeServer(t, cluster.VolumeGRPCAddress(1)) + defer connB.Close() + + // Server A: real source replica with data. + framework.AllocateVolume(t, clientA, volumeID, collection) + httpClient := framework.NewHTTPClient() + for i := 0; i < 8; i++ { + fid := framework.NewFileID(volumeID, uint64(948000+i), uint32(0x9490CA00+i)) + payload := make([]byte, 4096) + for j := range payload { + payload[j] = byte(i + 1) + } + resp := framework.UploadBytes(t, httpClient, cluster.VolumeAdminURL(0), fid, payload) + _ = framework.ReadAllAndClose(t, resp) + require.Equal(t, http.StatusCreated, resp.StatusCode) + } + + // Server B: empty stub replica of the same volume id. + framework.AllocateVolume(t, clientB, volumeID, collection) + + dataShards := int(erasure_coding.DataShardsCount) + totalShards := int(erasure_coding.DataShardsCount + erasure_coding.ParityShardsCount) + aShards := shardRange(0, dataShards) // 0..DataShardsCount-1 on A + bShards := shardRange(dataShards, totalShards) // parity range on B + + task := NewErasureCodingTask("ec-e2e", addrA, volumeID, collection, dialOption) + params := &worker_pb.TaskParams{ + VolumeId: volumeID, + Collection: collection, + Sources: []*worker_pb.TaskSource{ + {Node: addrA, VolumeId: volumeID}, + {Node: addrB, VolumeId: volumeID}, + }, + Targets: []*worker_pb.TaskTarget{ + {Node: addrA, ShardIds: aShards}, + {Node: addrB, ShardIds: bShards}, + }, + TaskParams: &worker_pb.TaskParams_ErasureCodingParams{ + ErasureCodingParams: &worker_pb.ErasureCodingTaskParams{ + DataShards: erasure_coding.DataShardsCount, + ParityShards: erasure_coding.ParityShardsCount, + WorkingDir: t.TempDir(), + }, + }, + } + + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + defer cancel() + + require.NoError(t, task.Execute(ctx, params)) + + dirA := filepath.Join(cluster.BaseDir(), "volume0") + dirB := filepath.Join(cluster.BaseDir(), "volume1") + base := fmt.Sprintf("%s_%d", collection, volumeID) + + // Both original regular volumes are gone: A's source deleted after verify, + // B's stub swept before distribute. + requireAbsent(t, dirA, base+".dat") + requireAbsent(t, dirA, base+".idx") + requireAbsent(t, dirB, base+".dat") + requireAbsent(t, dirB, base+".idx") + + // Each server holds exactly its assigned shards, plus index/info sidecars. + for _, id := range aShards { + requirePresent(t, dirA, fmt.Sprintf("%s.ec%02d", base, id)) + } + for _, id := range bShards { + requireAbsent(t, dirA, fmt.Sprintf("%s.ec%02d", base, id)) + } + requirePresent(t, dirA, base+".ecx") + requirePresent(t, dirA, base+".vif") + + for _, id := range bShards { + requirePresent(t, dirB, fmt.Sprintf("%s.ec%02d", base, id)) + } + for _, id := range aShards { + requireAbsent(t, dirB, fmt.Sprintf("%s.ec%02d", base, id)) + } + requirePresent(t, dirB, base+".ecx") + // The shared .vif must survive on B: the stub was deleted before the EC + // files landed, so deleteOriginalVolume never ran removeVolumeFiles there. + requirePresent(t, dirB, base+".vif") +} + +// serverAddress builds the SeaweedFS ip:httpPort.grpcPort address the worker's +// gRPC client decodes, from the multi-cluster's separate admin and grpc ports. +func serverAddress(c *framework.MultiVolumeCluster, index int) string { + _, grpcPort, err := net.SplitHostPort(c.VolumeGRPCAddress(index)) + if err != nil { + panic(err) + } + return c.VolumeAdminAddress(index) + "." + grpcPort +} + +func shardRange(start, end int) []uint32 { + ids := make([]uint32, 0, end-start) + for i := start; i < end; i++ { + ids = append(ids, uint32(i)) + } + return ids +} + +func requirePresent(t *testing.T, dir, name string) { + t.Helper() + _, err := os.Stat(filepath.Join(dir, name)) + require.NoError(t, err, "expected %s to be present in %s", name, dir) +} + +func requireAbsent(t *testing.T, dir, name string) { + t.Helper() + _, err := os.Stat(filepath.Join(dir, name)) + require.True(t, os.IsNotExist(err), "expected %s to be absent in %s, stat err=%v", name, dir, err) +}