From e7675f2d01e285ae89af5b6dafb5962bfc16f8c2 Mon Sep 17 00:00:00 2001 From: Nick Date: Thu, 20 Aug 2020 21:10:56 -0700 Subject: [PATCH] Address additional suggestions from fio workload PR #529 (#550) Followup on recent PR #529, some suggestions and discussion after it was merged: - Express probability as float in range [0,1] - Add a unit test for DeleteContentsAtDepth - Add a comment on writeFilesAtDepth explaining depth vs branchDepth - Refactor pickRandSubdirPath for easier readability and understanding Upon some reflection, I decided to refactor pickRandSubdirPath() to gather indexes and pick randomly from them instead of the previous reservoir sampling approach. I think this is easier to understand going forward without extra explanation, doesn't have much additional memory overhead, and reduces the number of rand calls to 1. --- tests/tools/fio/workload.go | 44 +++++++++++------ tests/tools/fio/workload_test.go | 82 ++++++++++++++++++++++++++++++-- 2 files changed, 108 insertions(+), 18 deletions(-) diff --git a/tests/tools/fio/workload.go b/tests/tools/fio/workload.go index 44c0c16e2..502a93458 100644 --- a/tests/tools/fio/workload.go +++ b/tests/tools/fio/workload.go @@ -88,8 +88,11 @@ func (fr *Runner) DeleteDirAtDepth(relBasePath string, depth int) error { } // DeleteContentsAtDepth deletes some or all of the contents of a directory -// at the provided depths. -func (fr *Runner) DeleteContentsAtDepth(relBasePath string, depth, pcnt int) error { +// at the provided depths. The probability argument denotes the probability in the +// range [0.0,1.0] that a given file system entry in the directory at this depth will be +// deleted. Probability set to 0 will delete nothing. Probability set to 1 will delete +// everything. +func (fr *Runner) DeleteContentsAtDepth(relBasePath string, depth int, prob float32) error { fullBasePath := filepath.Join(fr.LocalDataDir, relBasePath) return fr.operateAtDepth(fullBasePath, depth, func(dirPath string) error { @@ -99,8 +102,7 @@ func (fr *Runner) DeleteContentsAtDepth(relBasePath string, depth, pcnt int) err } for _, fi := range fileInfoList { - const hundred = 100 - if rand.Intn(hundred) < pcnt { // nolint:gosec + if rand.Float32() < prob { // nolint:gosec path := filepath.Join(dirPath, fi.Name()) err = os.RemoveAll(path) if err != nil { @@ -152,6 +154,15 @@ func (fr *Runner) operateAtDepth(path string, depth int, f func(string) error) e return ErrNoDirFound } +// writeFilesAtDepth traverses the file system tree until it reaches a given "depth", at which +// point it uses fio to write one or more files controlled by the provided Options. +// The "branchDepth" argument gives control over whether the files will be written into +// existing directories or a new path entirely. The function will traverse existing directories +// (if any) until it reaches "branchDepth", after which it will begin creating new directories +// for the remainder of the path until "depth" is reached. +// If "branchDepth" is zero, the entire path will be newly created directories. +// If "branchDepth" is greater than or equal to "depth", the files will be written to +// a random existing directory, if one exists at that depth. func (fr *Runner) writeFilesAtDepth(fromDirPath string, depth, branchDepth int, opt Options) error { if depth <= 0 { return fr.writeFiles(fromDirPath, opt) @@ -177,24 +188,27 @@ func (fr *Runner) writeFilesAtDepth(fromDirPath string, depth, branchDepth int, } func pickRandSubdirPath(dirPath string) (subdirPath string) { - subdirCount := 0 - fileInfoList, err := ioutil.ReadDir(dirPath) if err != nil { return "" } - for _, fi := range fileInfoList { - if fi.IsDir() { - subdirCount++ + // Find all entries that are directories, record each of their fileInfoList indexes + dirIdxs := make([]int, 0, len(fileInfoList)) - // Decide if this directory will be selected - probability of - // being selected is uniform across all subdirs - if rand.Intn(subdirCount) == 0 { // nolint:gosec - subdirPath = filepath.Join(dirPath, fi.Name()) - } + for idx, fi := range fileInfoList { + if fi.IsDir() { + dirIdxs = append(dirIdxs, idx) } } - return subdirPath + if len(dirIdxs) == 0 { + return "" + } + + // Pick a random index from the list of indexes of fileInfo entries known to be directories. + randDirIdx := dirIdxs[rand.Intn(len(dirIdxs))] //nolint:gosec + randDirInfo := fileInfoList[randDirIdx] + + return filepath.Join(dirPath, randDirInfo.Name()) } diff --git a/tests/tools/fio/workload_test.go b/tests/tools/fio/workload_test.go index 012d9fd3e..6abb06e0d 100644 --- a/tests/tools/fio/workload_test.go +++ b/tests/tools/fio/workload_test.go @@ -1,7 +1,6 @@ package fio import ( - "fmt" "io/ioutil" "os" "path/filepath" @@ -113,7 +112,6 @@ func testWriteAtDepth(t *testing.T, r *Runner, depth, expFileCount int) { return err } - fmt.Println(path, info.Name()) if info.IsDir() { dirCount++ } else { @@ -220,7 +218,6 @@ func testDeleteAtDepth(t *testing.T, r *Runner, wrDepth, delDepth, expDirCount i return err } - fmt.Println(path) if info.IsDir() { dirCount++ } else { @@ -237,3 +234,82 @@ func testDeleteAtDepth(t *testing.T, r *Runner, wrDepth, delDepth, expDirCount i t.Errorf("Expected %v directories, but found %v", want, got) } } + +func TestDeleteContentsAtDepth(t *testing.T) { + for _, tc := range []struct { + prob float32 + expFileCountChecker func(t *testing.T, fileCount int) + }{ + { + prob: 0.0, + expFileCountChecker: func(t *testing.T, fileCount int) { + if fileCount != 100 { + t.Error("some files were deleted despite 0% probability") + } + }, + }, + { + prob: 1.0, + expFileCountChecker: func(t *testing.T, fileCount int) { + if fileCount != 0 { + t.Error("not all files were deleted despite 100% probability") + } + }, + }, + { + prob: 0.5, + expFileCountChecker: func(t *testing.T, fileCount int) { + // Broad check: just make sure a 50% probability deleted something. + // Extremely improbable that this causes a false failure; + // akin to 100 coin flips all landing on the same side. + if !(fileCount > 0 && fileCount < 100) { + t.Error("expected some but not all files to be deleted") + } + }, + }, + } { + testDeleteContentsAtDepth(t, tc.prob, tc.expFileCountChecker) + } +} + +func testDeleteContentsAtDepth(t *testing.T, prob float32, checker func(t *testing.T, fileCount int)) { + r, err := NewRunner() + testenv.AssertNoError(t, err) + + defer r.Cleanup() + + testSubdir := "test" + testDirAbs := filepath.Join(r.LocalDataDir, testSubdir) + + sizeB := int64(128 * 1024 * 1024) + numFiles := 100 + fioOpt := Options{}.WithSize(sizeB).WithNumFiles(numFiles) + + wrDepth := 3 + err = r.WriteFilesAtDepth(testSubdir, wrDepth, fioOpt) + testenv.AssertNoError(t, err) + + defer r.DeleteRelDir(testSubdir) + + delDepth := 3 + err = r.DeleteContentsAtDepth(testSubdir, delDepth, prob) + testenv.AssertNoError(t, err) + + fileCount := 0 + + err = filepath.Walk(testDirAbs, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + + if !info.IsDir() { + fileCount++ + } + + return nil + }) + + testenv.AssertNoError(t, err) + + checker(t, fileCount) +}