From e6ca64ddd5e73c7252a9d62e3a0a234cde6eec2f Mon Sep 17 00:00:00 2001 From: Gerrit Date: Wed, 19 May 2021 08:26:37 +0200 Subject: [PATCH 1/4] Cleanup empty directories. --- cmd/internal/sync/syncer.go | 40 +++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/cmd/internal/sync/syncer.go b/cmd/internal/sync/syncer.go index 32b0e0a..791b3dd 100644 --- a/cmd/internal/sync/syncer.go +++ b/cmd/internal/sync/syncer.go @@ -2,6 +2,8 @@ package sync import ( "context" + "io/ioutil" + // nolint "crypto/md5" "fmt" @@ -90,6 +92,11 @@ func (s *Syncer) Sync(rootPath string, entitiesToSync api.CacheEntities) error { } } + err = cleanEmptyDirs(s.fs, rootPath) + if err != nil { + return errors.Wrap(err, "error cleaning up empty directories") + } + return nil } @@ -308,3 +315,36 @@ func (s *Syncer) printSyncPlan(remove api.CacheEntities, keep []api.CacheEntity, } table.Render() } + +func cleanEmptyDirs(fs afero.Fs, rootPath string) error { + err := afero.Walk(fs, rootPath, func(path string, info os.FileInfo, innerErr error) error { + if innerErr != nil { + return errors.Wrap(innerErr, "error while walking through cache root") + } + + if !info.IsDir() { + return nil + } + + files, err := ioutil.ReadDir(path) + if err != nil { + return err + } + + if len(files) != 0 { + return nil + } + + err = os.Remove(path) + if err != nil { + return err + } + + return nil + }) + if err != nil { + return err + } + + return nil +} From 023322d7db854bbbbd52a4af01a70059a9d60468 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Wed, 19 May 2021 08:55:53 +0200 Subject: [PATCH 2/4] Next attempt. --- cmd/internal/sync/syncer.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/cmd/internal/sync/syncer.go b/cmd/internal/sync/syncer.go index 791b3dd..4bae098 100644 --- a/cmd/internal/sync/syncer.go +++ b/cmd/internal/sync/syncer.go @@ -102,22 +102,22 @@ func (s *Syncer) Sync(rootPath string, entitiesToSync api.CacheEntities) error { func currentFileIndex(fs afero.Fs, rootPath string) (api.CacheEntities, error) { var result api.CacheEntities - err := afero.Walk(fs, rootPath, func(p string, info os.FileInfo, innerErr error) error { + err := afero.Walk(fs, rootPath, func(path string, info os.FileInfo, innerErr error) error { if innerErr != nil { - return errors.Wrap(innerErr, "error while walking through cache root") + return errors.Wrap(innerErr, fmt.Sprintf("error while walking through root path %s", rootPath)) } if info.IsDir() { return nil } - if strings.HasSuffix(p, ".md5") { + if strings.HasSuffix(path, ".md5") { return nil } result = append(result, api.LocalFile{ Name: info.Name(), - SubPath: p[len(rootPath)+1:], + SubPath: path[len(rootPath)+1:], Size: info.Size(), }) @@ -317,11 +317,7 @@ func (s *Syncer) printSyncPlan(remove api.CacheEntities, keep []api.CacheEntity, } func cleanEmptyDirs(fs afero.Fs, rootPath string) error { - err := afero.Walk(fs, rootPath, func(path string, info os.FileInfo, innerErr error) error { - if innerErr != nil { - return errors.Wrap(innerErr, "error while walking through cache root") - } - + err := afero.Walk(fs, rootPath, func(path string, info os.FileInfo, _ error) error { if !info.IsDir() { return nil } From 43705184956a1f599250f7333f39ec1656fa417a Mon Sep 17 00:00:00 2001 From: Gerrit Date: Wed, 19 May 2021 11:45:06 +0200 Subject: [PATCH 3/4] Fix with test. --- cmd/internal/sync/syncer.go | 42 +++++++--- cmd/internal/sync/syncer_test.go | 134 ++++++++++++++++++++++++++++++- 2 files changed, 166 insertions(+), 10 deletions(-) diff --git a/cmd/internal/sync/syncer.go b/cmd/internal/sync/syncer.go index 4bae098..537ab0b 100644 --- a/cmd/internal/sync/syncer.go +++ b/cmd/internal/sync/syncer.go @@ -2,7 +2,6 @@ package sync import ( "context" - "io/ioutil" // nolint "crypto/md5" @@ -317,30 +316,55 @@ func (s *Syncer) printSyncPlan(remove api.CacheEntities, keep []api.CacheEntity, } func cleanEmptyDirs(fs afero.Fs, rootPath string) error { - err := afero.Walk(fs, rootPath, func(path string, info os.FileInfo, _ error) error { + files, err := afero.ReadDir(fs, rootPath) + if err != nil { + return err + } + + for _, info := range files { if !info.IsDir() { - return nil + continue } - files, err := ioutil.ReadDir(path) + err = recurseCleanEmptyDirs(fs, path.Join(rootPath, info.Name())) if err != nil { return err } + } - if len(files) != 0 { - return nil + return nil +} + +func recurseCleanEmptyDirs(fs afero.Fs, p string) error { + files, err := afero.ReadDir(fs, p) + if err != nil { + return err + } + + for _, info := range files { + if !info.IsDir() { + continue } - err = os.Remove(path) + nested := path.Join(p, info.Name()) + err = recurseCleanEmptyDirs(fs, nested) if err != nil { return err } + } - return nil - }) + // re-read files because directories could delete themselves in first loop + files, err = afero.ReadDir(fs, p) if err != nil { return err } + if len(files) == 0 { + err = fs.Remove(p) + if err != nil { + return err + } + } + return nil } diff --git a/cmd/internal/sync/syncer_test.go b/cmd/internal/sync/syncer_test.go index cf0180c..bf93b16 100644 --- a/cmd/internal/sync/syncer_test.go +++ b/cmd/internal/sync/syncer_test.go @@ -22,6 +22,7 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" "github.com/metal-stack/metal-image-cache-sync/pkg/api" "github.com/spf13/afero" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap/zaptest" ) @@ -94,7 +95,7 @@ func Test_currentFileIndex(t *testing.T) { } func createTestFile(t *testing.T, fs afero.Fs, p string) { - require.Nil(t, fs.MkdirAll(path.Base(p), 0755)) + createTestDir(t, fs, path.Base(p)) f, err := fs.Create(p) require.Nil(t, err) defer f.Close() @@ -102,6 +103,10 @@ func createTestFile(t *testing.T, fs afero.Fs, p string) { require.Nil(t, err) } +func createTestDir(t *testing.T, fs afero.Fs, p string) { + require.Nil(t, fs.MkdirAll(p, 0755)) +} + func dlLoggingSvc(data []byte) (*s3.S3, *[]string, *[]string) { var m sync.Mutex names := []string{} @@ -346,3 +351,130 @@ func TestSyncer_defineImageDiff(t *testing.T) { func strPtr(s string) *string { return &s } + +func Test_cleanEmptyDirs(t *testing.T) { + tests := []struct { + name string + fsModFunc func(t *testing.T, fs afero.Fs) + fsCheckFunc func(t *testing.T, fs afero.Fs) + wantErr error + }{ + { + name: "no directory contents, nothing happens", + fsModFunc: nil, + wantErr: nil, + }, + { + name: "flat deletion", + fsModFunc: func(t *testing.T, fs afero.Fs) { + createTestDir(t, fs, cacheRoot+"/ubuntu") + }, + fsCheckFunc: func(t *testing.T, fs afero.Fs) { + exists, err := afero.Exists(fs, cacheRoot+"/ubuntu") + assert.NoError(t, err) + assert.False(t, exists, "dir still exists") + }, + wantErr: nil, + }, + { + name: "recursive deletion 1", + fsModFunc: func(t *testing.T, fs afero.Fs) { + createTestDir(t, fs, cacheRoot+"/ubuntu/20.10/20201027") + }, + fsCheckFunc: func(t *testing.T, fs afero.Fs) { + exists, err := afero.Exists(fs, cacheRoot+"/ubuntu/20.10/20201027") + assert.NoError(t, err) + assert.False(t, exists, "dir still exists") + + exists, err = afero.Exists(fs, cacheRoot+"/ubuntu/20.10") + assert.NoError(t, err) + assert.False(t, exists, "dir still exists") + + exists, err = afero.Exists(fs, cacheRoot+"/ubuntu") + assert.NoError(t, err) + assert.False(t, exists, "dir still exists") + }, + wantErr: nil, + }, + { + name: "recursive deletion 2", + fsModFunc: func(t *testing.T, fs afero.Fs) { + createTestFile(t, fs, cacheRoot+"/ubuntu/20.04/20201028/img.tar.lz4") + createTestDir(t, fs, cacheRoot+"/ubuntu/20.10/20201027") + }, + fsCheckFunc: func(t *testing.T, fs afero.Fs) { + exists, err := afero.Exists(fs, cacheRoot+"/ubuntu/20.10/20201027") + assert.NoError(t, err) + assert.False(t, exists, "dir still exists") + + exists, err = afero.Exists(fs, cacheRoot+"/ubuntu/20.10") + assert.NoError(t, err) + assert.False(t, exists, "dir still exists") + + exists, err = afero.Exists(fs, cacheRoot+"/ubuntu") + assert.NoError(t, err) + assert.True(t, exists, "dir was deleted") + }, + wantErr: nil, + }, + { + name: "kind of realistic scenario", + + fsModFunc: func(t *testing.T, fs afero.Fs) { + createTestDir(t, fs, cacheRoot+"/boot/metal-hammer/releases/download/v0.8.0") + createTestFile(t, fs, cacheRoot+"/boot/metal-hammer/pull-requests/pr-title/metal-hammer-initrd.img.lz4") + createTestFile(t, fs, cacheRoot+"/boot/metal-hammer/pull-requests/pr-title/metal-hammer-initrd.img.lz4.md5") + createTestFile(t, fs, cacheRoot+"/ubuntu/20.10/20201026/img.tar.lz4") + createTestFile(t, fs, cacheRoot+"/ubuntu/20.10/20201026/img.tar.lz4.md5") + createTestDir(t, fs, cacheRoot+"/firewall/2.0/20210131") + createTestDir(t, fs, cacheRoot+"/firewall/2.0/20210207") + createTestFile(t, fs, cacheRoot+"/firewall/2.0/20210304/img.tar.lz4") + createTestFile(t, fs, cacheRoot+"/firewall/2.0/20210304/img.tar.lz4.md5") + }, + fsCheckFunc: func(t *testing.T, fs afero.Fs) { + for _, subPath := range []string{ + "/boot/metal-hammer/releases", + "/firewall/2.0.20210131", + "/firewall/2.0.20210207", + } { + exists, err := afero.Exists(fs, cacheRoot+subPath) + assert.NoError(t, err) + assert.False(t, exists, "dir still exists") + } + + for _, subPath := range []string{ + "/boot/metal-hammer/pull-requests/pr-title/metal-hammer-initrd.img.lz4", + "/boot/metal-hammer/pull-requests/pr-title/metal-hammer-initrd.img.lz4.md5", + "/ubuntu/20.10/20201026/img.tar.lz4", + "/ubuntu/20.10/20201026/img.tar.lz4.md5", + "/firewall/2.0/20210304/img.tar.lz4", + "/firewall/2.0/20210304/img.tar.lz4.md5", + } { + exists, err := afero.Exists(fs, cacheRoot+subPath) + assert.NoError(t, err) + assert.True(t, exists, "dir was deleted") + } + }, + wantErr: nil, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + fs := afero.NewMemMapFs() + require.Nil(t, fs.MkdirAll(cacheRoot, 0755)) + if tt.fsModFunc != nil { + tt.fsModFunc(t, fs) + } + + err := cleanEmptyDirs(fs, cacheRoot) + if diff := cmp.Diff(err, tt.wantErr); diff != "" { + t.Errorf("cleanEmptyDirs() diff = %v", diff) + } + + if tt.fsCheckFunc != nil { + tt.fsCheckFunc(t, fs) + } + }) + } +} From ef0345b0c1356baaf7d43167ceb47cb176ea3e59 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Wed, 19 May 2021 11:59:11 +0200 Subject: [PATCH 4/4] Revert renaming. --- cmd/internal/sync/syncer.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/internal/sync/syncer.go b/cmd/internal/sync/syncer.go index 537ab0b..7a79f7a 100644 --- a/cmd/internal/sync/syncer.go +++ b/cmd/internal/sync/syncer.go @@ -101,7 +101,7 @@ func (s *Syncer) Sync(rootPath string, entitiesToSync api.CacheEntities) error { func currentFileIndex(fs afero.Fs, rootPath string) (api.CacheEntities, error) { var result api.CacheEntities - err := afero.Walk(fs, rootPath, func(path string, info os.FileInfo, innerErr error) error { + err := afero.Walk(fs, rootPath, func(p string, info os.FileInfo, innerErr error) error { if innerErr != nil { return errors.Wrap(innerErr, fmt.Sprintf("error while walking through root path %s", rootPath)) } @@ -110,13 +110,13 @@ func currentFileIndex(fs afero.Fs, rootPath string) (api.CacheEntities, error) { return nil } - if strings.HasSuffix(path, ".md5") { + if strings.HasSuffix(p, ".md5") { return nil } result = append(result, api.LocalFile{ Name: info.Name(), - SubPath: path[len(rootPath)+1:], + SubPath: p[len(rootPath)+1:], Size: info.Size(), })