mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-10-19 12:34:29 +02:00
fix(shell): error on missing volume id in fsck, mergeVolumes, vacuum (#9158)
* fix(shell): error on missing volume id in fsck, mergeVolumes, vacuum Three shell commands silently report success when -volumeId / -fromVolumeId / -toVolumeId names a volume the master doesn't know about: typos, already-deleted volumes, and stale scripts all look identical to a clean no-op, which is what made the confusion in #9116 take as long as it did to diagnose. - volume.fsck: filter at the per-datanode loop drops unknown ids and findExtraChunksInVolumeServers ends with totalOrphanChunkCount==0, printing "no orphan data". - fs.mergeVolumes: createMergePlan iterates only known volumes, so an unknown -fromVolumeId produces an empty plan and we print just the "max volume size: N MB" header (indistinguishable from "nothing to merge"). - volume.vacuum: the master's VacuumVolume RPC silently iterates matching volumes; a missing id returns success having done nothing. Validate the requested ids against the current topology up front and return an explicit "volume(s) not found on master: [X Y]" error. Also drop a stale duplicate `if err != nil` in volume.fsck.Do left over from a prior refactor. Surfaces #9116 follow-up from madalee-com. * address review: propagate reloadVolumesInfo error; dedupe vacuum missing ids - fs.mergeVolumes: c.reloadVolumesInfo's return was ignored. If the master is unreachable or VolumeList fails, c.volumes stays empty and the new validation block reports "fromVolumeId X not found on master" — masking the real connection/RPC failure. Return the wrapped error instead. - volume.vacuum: "volume.vacuum -volumeId 5,5,5" on a missing volume 5 listed [5 5 5] in the error. Collect missing ids in a set so each missing id appears once. * address review: reject fromVolumeId/toVolumeId values that overflow uint32 flag.Uint produces a uint (64-bit on amd64), and the existing cast to needle.VolumeId silently truncates to uint32. A typo like `-fromVolumeId=4294967297` would wrap to volume 1 and slip past every other validation, so the merge would run against a completely different volume than the operator intended. Bail out with an explicit error when the raw flag value exceeds the uint32 range, before the cast.
This commit is contained in:
1 parent
6bdd775963
commit
08a7502b2c
3 files changed
+87
-3
No files matched your search
@@ -70,10 +70,38 @@ func (c *commandFsMergeVolumes) Do(args []string, commandEnv *CommandEnv, writer
|
||||
dir = strings.TrimRight(dir, "/")
|
||||
}
|
||||
|
||||
// flag.Uint is a 64-bit uint on amd64 but needle.VolumeId is uint32, so a
|
||||
// value that overflows (e.g. 4294967297) silently wraps to a valid id
|
||||
// like 1. Reject instead of wrapping.
|
||||
const maxVolumeID = uint(^uint32(0))
|
||||
if *fromVolumeArg > maxVolumeID {
|
||||
return fmt.Errorf("fromVolumeId %d exceeds max volume id %d", *fromVolumeArg, maxVolumeID)
|
||||
}
|
||||
if *toVolumeArg > maxVolumeID {
|
||||
return fmt.Errorf("toVolumeId %d exceeds max volume id %d", *toVolumeArg, maxVolumeID)
|
||||
}
|
||||
|
||||
fromVolumeId := needle.VolumeId(*fromVolumeArg)
|
||||
toVolumeId := needle.VolumeId(*toVolumeArg)
|
||||
|
||||
c.reloadVolumesInfo(commandEnv.MasterClient)
|
||||
if err = c.reloadVolumesInfo(commandEnv.MasterClient); err != nil {
|
||||
return fmt.Errorf("reload volumes info: %w", err)
|
||||
}
|
||||
|
||||
// Reject unknown ids before createMergePlan silently produces an empty plan
|
||||
// and we print just the "max volume size" header. That output is
|
||||
// indistinguishable from a legitimate "nothing to merge" and hides typos,
|
||||
// already-deleted volumes, and stale scripts.
|
||||
if fromVolumeId != 0 {
|
||||
if _, err := c.getVolumeInfoById(fromVolumeId); err != nil {
|
||||
return fmt.Errorf("fromVolumeId %d not found on master", fromVolumeId)
|
||||
}
|
||||
}
|
||||
if toVolumeId != 0 {
|
||||
if _, err := c.getVolumeInfoById(toVolumeId); err != nil {
|
||||
return fmt.Errorf("toVolumeId %d not found on master", toVolumeId)
|
||||
}
|
||||
}
|
||||
|
||||
if fromVolumeId != 0 && toVolumeId != 0 {
|
||||
if fromVolumeId == toVolumeId {
|
||||
|
||||
@@ -14,6 +14,7 @@ import (
|
||||
"os"
|
||||
"path"
|
||||
"path/filepath"
|
||||
"sort"
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync"
|
||||
@@ -161,8 +162,27 @@ func (c *commandVolumeFsck) Do(args []string, commandEnv *CommandEnv, writer io.
|
||||
return fmt.Errorf("failed to collect all volume locations: %w", err)
|
||||
}
|
||||
|
||||
if err != nil {
|
||||
return fmt.Errorf("read filer buckets path: %w", err)
|
||||
// If the operator specified -volumeId, reject unknown ids up front instead
|
||||
// of silently filtering them out and reporting "no orphan data". A silent
|
||||
// success on a missing volume looks identical to a clean volume and hides
|
||||
// typos, already-deleted volumes, and stale scripts.
|
||||
if len(c.volumeIds) > 0 {
|
||||
known := make(map[uint32]bool)
|
||||
for _, vidMap := range dataNodeVolumeIdToVInfo {
|
||||
for vid := range vidMap {
|
||||
known[vid] = true
|
||||
}
|
||||
}
|
||||
var missing []uint32
|
||||
for vid := range c.volumeIds {
|
||||
if !known[vid] {
|
||||
missing = append(missing, vid)
|
||||
}
|
||||
}
|
||||
if len(missing) > 0 {
|
||||
sort.Slice(missing, func(i, j int) bool { return missing[i] < missing[j] })
|
||||
return fmt.Errorf("volume(s) not found on master: %v", missing)
|
||||
}
|
||||
}
|
||||
|
||||
var collectCutoffFromAtNs int64 = 0
|
||||
|
||||
@@ -5,6 +5,7 @@ import (
|
||||
"flag"
|
||||
"fmt"
|
||||
"io"
|
||||
"sort"
|
||||
"strconv"
|
||||
"strings"
|
||||
|
||||
@@ -62,6 +63,41 @@ func (c *commandVacuum) Do(args []string, commandEnv *CommandEnv, writer io.Writ
|
||||
volumeIdInts = append(volumeIdInts, 0)
|
||||
}
|
||||
|
||||
// Reject unknown ids up front. The master's VacuumVolume RPC silently
|
||||
// iterates matching volumes, so a typo or an already-deleted volume just
|
||||
// returns success — making this command look like it worked when nothing
|
||||
// happened.
|
||||
if *volumeIds != "" {
|
||||
topo, _, err := collectTopologyInfo(commandEnv, 0)
|
||||
if err != nil {
|
||||
return fmt.Errorf("collect topology: %w", err)
|
||||
}
|
||||
known := make(map[uint32]bool)
|
||||
eachDataNode(topo, func(_ DataCenterId, _ RackId, dn *master_pb.DataNodeInfo) {
|
||||
for _, disk := range dn.DiskInfos {
|
||||
for _, vi := range disk.VolumeInfos {
|
||||
known[vi.Id] = true
|
||||
}
|
||||
}
|
||||
})
|
||||
// Dedupe via a set so "volume.vacuum -volumeId 5,5,5" on a missing
|
||||
// volume 5 reports [5] once instead of [5 5 5].
|
||||
missingSet := make(map[uint32]bool)
|
||||
for _, vid := range volumeIdInts {
|
||||
if !known[vid] {
|
||||
missingSet[vid] = true
|
||||
}
|
||||
}
|
||||
if len(missingSet) > 0 {
|
||||
missing := make([]uint32, 0, len(missingSet))
|
||||
for vid := range missingSet {
|
||||
missing = append(missing, vid)
|
||||
}
|
||||
sort.Slice(missing, func(i, j int) bool { return missing[i] < missing[j] })
|
||||
return fmt.Errorf("volume(s) not found on master: %v", missing)
|
||||
}
|
||||
}
|
||||
|
||||
for _, volumeId := range volumeIdInts {
|
||||
err = commandEnv.MasterClient.WithClient(false, func(client master_pb.SeaweedClient) error {
|
||||
_, err = client.VacuumVolume(context.Background(), &master_pb.VacuumVolumeRequest{
|
||||
|
||||
Reference in new issue
Block a user