Skip to content

Commit 7b2fe2d

Browse files
alex-aizmanalex-aizman
committed
space/cleanup: fix nil-deref on EC content types
* when we walk a given bucket it is supposted to be initialized (fix) - which is equivalent to `bck.Props != nil` * `rmLeftovers` fix: stat before removal ---- * unit test: cover EC slice/metafile keep-path * integration test: add TestECStoreCleanup 1) orphan one slice 2) age all EC content 3) assert the orphan is removed Fixes #354 Signed-off-by: Alex Aizman <[email protected]>
1 parent 42242b3 commit 7b2fe2d

3 files changed

Lines changed: 140 additions & 4 deletions

File tree

‎ais/test/ec_test.go‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2882,3 +2882,103 @@ func TestECBckEncodeRecover(t *testing.T) {
28822882
})
28832883
}
28842884
}
2885+
2886+
func ecOrphanSlice(t *testing.T, parts map[string]ecSliceMD, mainObjPath string, age time.Duration) (string, string) {
2887+
t.Helper()
2888+
2889+
var orphan, orphanMeta string
2890+
old := time.Now().Add(-age)
2891+
2892+
for fqn := range parts {
2893+
if fqn == mainObjPath {
2894+
continue
2895+
}
2896+
2897+
ct, err := core.NewCTFromFQN(fqn, nil)
2898+
tassert.CheckFatal(t, err)
2899+
2900+
switch ct.ContentType() {
2901+
case fs.ECSliceCT:
2902+
if orphan == "" {
2903+
orphan = fqn
2904+
orphanMeta = ct.GenFQN(fs.ECMetaCT)
2905+
}
2906+
case fs.ECMetaCT:
2907+
default:
2908+
continue
2909+
}
2910+
tassert.CheckFatal(t, os.Chtimes(fqn, old, old))
2911+
}
2912+
2913+
tassert.Fatalf(t, orphan != "", "no EC slice among %d parts of %s", len(parts), mainObjPath)
2914+
2915+
tlog.Logfln("Orphaning slice %s [removing %s]", orphan, orphanMeta)
2916+
tassert.CheckFatal(t, cos.RemoveFile(orphanMeta))
2917+
return orphan, orphanMeta
2918+
}
2919+
2920+
// run global store cleanup over EC content and assert both directions:
2921+
// - the orphaned slice (no metafile, aged past dont_cleanup_time) is removed;
2922+
// - every other slice, metafile, and the full replica survive untouched.
2923+
func TestECStoreCleanup(t *testing.T) {
2924+
tools.CheckSkip(t, &tools.SkipTestArgs{MinTargets: 5, RequiredDeployment: tools.ClusterTypeLocal})
2925+
2926+
var (
2927+
proxyURL = tools.RandomProxyURL()
2928+
baseParams = tools.BaseAPIParams(proxyURL)
2929+
bck = cmn.Bck{Name: testBucketName + "-ec-cleanup", Provider: apc.AIS}
2930+
o = &ecOptions{minTargets: 5, dataCnt: 2, parityCnt: 2, objSizeLimit: ecObjLimit}
2931+
)
2932+
o.init(t, proxyURL)
2933+
initMountpaths(t, proxyURL)
2934+
newLocalBckWithProps(t, baseParams, bck, defaultECBckProps(o), o)
2935+
2936+
const objName = "store-cleanup"
2937+
objPath := ecTestDir + objName
2938+
2939+
// 2 (full replica + its metafile) + (dataCnt+parityCnt)*2
2940+
before, mainObjPath := createECFile(t, baseParams, bck, objName, o)
2941+
2942+
config := tools.GetClusterConfig(t)
2943+
orphan, orphanMeta := ecOrphanSlice(t, before, mainObjPath, config.Space.DontCleanupTime.D()+time.Minute)
2944+
2945+
tlog.Logln("Starting global store cleanup...")
2946+
xargs := xact.ArgsMsg{Kind: apc.ActStoreCleanup}
2947+
xid, err := api.StartXaction(baseParams, &xargs, "")
2948+
tassert.CheckFatal(t, err)
2949+
2950+
xargs.ID = xid
2951+
xargs.Timeout = tools.RebalanceTimeout
2952+
_, err = api.WaitForXactionIC(baseParams, &xargs)
2953+
tassert.CheckFatal(t, err)
2954+
2955+
// on timing: rmAnyBatch(flagRmMisplacedEC) will not fire for a single
2956+
// orphan (len(misplaced.ec) < config.Space.BatchSize).
2957+
after, _ := ecGetAllSlices(t, bck, objPath)
2958+
2959+
// 1. positive: the orphan must be gone
2960+
_, ok := after[orphan]
2961+
tassert.Errorf(t, !ok, "orphaned slice %q survived store cleanup", orphan)
2962+
2963+
// 2. negative
2964+
tassert.Errorf(t, len(after) == len(before)-2,
2965+
"expected %d EC parts after store cleanup, got %d (before %d)",
2966+
len(before)-2, len(after), len(before))
2967+
2968+
for fqn, md := range before {
2969+
if fqn == orphan || fqn == orphanMeta {
2970+
continue
2971+
}
2972+
got, ok := after[fqn]
2973+
if !ok {
2974+
tassert.Errorf(t, false, "EC content %q removed by store cleanup", fqn)
2975+
continue
2976+
}
2977+
tassert.Errorf(t, got.size == md.size,
2978+
"EC content %q size changed: before %d, after %d", fqn, md.size, got.size)
2979+
}
2980+
2981+
// 3. 2d:2p tolerates one removed slice
2982+
_, err = api.GetObject(baseParams, bck, objPath, nil)
2983+
tassert.CheckFatal(t, err)
2984+
}

‎space/cleanup.go‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -468,9 +468,8 @@ func (j *clnJ) jogBcks(bcks []cmn.Bck) {
468468
var (
469469
err error
470470
bck = bcks[i]
471-
b = meta.CloneBck(&bck)
471+
b = (*meta.Bck)(&bck)
472472
)
473-
j.bck = bck
474473
err = b.Init(bowner)
475474
if err != nil {
476475
if cmn.IsErrBckNotFound(err) || cmn.IsErrRemoteBckNotFound(err) {
@@ -488,6 +487,9 @@ func (j *clnJ) jogBcks(bcks []cmn.Bck) {
488487
}
489488
continue
490489
}
490+
debug.Assert(apc.IsProvider(bck.Provider), "expecting normalized provider, got: ", bck.Provider)
491+
debug.Assert(bck.Props != nil)
492+
j.bck = bck
491493
j._jogBck()
492494
if xcln.IsAborted() || j.done() {
493495
return
@@ -1187,9 +1189,13 @@ func (j *clnJ) rmLeftovers(specifier int) {
11871189
if cos.Stat(metaFQN) == nil {
11881190
continue
11891191
}
1190-
if os.Remove(ct.FQN()) == nil {
1192+
fqn := ct.FQN()
1193+
finfo, ers := os.Lstat(fqn)
1194+
if os.Remove(fqn) == nil {
11911195
nfiles++
1192-
nbytes += ct.Lsize()
1196+
if ers == nil {
1197+
nbytes += finfo.Size()
1198+
}
11931199

11941200
j._throttle(nfiles)
11951201
if j.done() {

‎space/cleanup_test.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1004,6 +1004,36 @@ var _ = Describe("AIStore content cleanup tests", func() {
10041004
})
10051005
})
10061006

1007+
Describe("EC cleanup", func() {
1008+
It("should preserve a valid slice and metafile pair", func() {
1009+
// `CloneBck` is a shallow copy - `mbck.Props` is the same *Bprops the mock BMD holds.
1010+
// Scoped to this unit test: BeforeEach (above) rebuilds the BMD every time.
1011+
mbck := meta.CloneBck(&bck)
1012+
Expect(mbck.Init(core.T.Bowner())).NotTo(HaveOccurred())
1013+
mbck.Props.EC.Enabled = true
1014+
1015+
lom := core.AllocLOM("ec-object")
1016+
defer core.FreeLOM(lom)
1017+
Expect(lom.InitCmnBck(&bck)).NotTo(HaveOccurred())
1018+
1019+
slice := core.NewCTFromLOM(lom, fs.ECSliceCT)
1020+
metafile := slice.Clone(fs.ECMetaCT)
1021+
1022+
createTestFile(slice.FQN(), 1024)
1023+
createTestFile(metafile.FQN(), 256)
1024+
1025+
old := now.Add(-3 * time.Hour)
1026+
Expect(os.Chtimes(slice.FQN(), old, old)).To(Succeed())
1027+
Expect(os.Chtimes(metafile.FQN(), old, old)).To(Succeed())
1028+
1029+
// Args.Buckets remains empty: discover a Props-less bucket
1030+
// through AllMpathBcks, as in automatic OOS cleanup.
1031+
space.RunCleanup(ini)
1032+
1033+
Expect(slice.FQN()).To(BeAnExistingFile())
1034+
Expect(metafile.FQN()).To(BeAnExistingFile())
1035+
})
1036+
})
10071037
})
10081038

10091039
//

0 commit comments

Comments
 (0)