storage: Return details about the deleted items
Update Storage.RemoveAll() and Storage.RemoveAllButCurrent() to return the details about the deleted items. This helps emit useful information about garbage collection in the controllers and ignore no-op garbage collections. RemoveAll() returns the path of the deleted directory if any. RemoveAllButCurrent() returns a slice of path of all the deleted items from a resource's artifact dir. Signed-off-by: Sunny <darkowlzz@protonmail.com>
This commit is contained in:
parent
a6e7b84028
commit
b9979c9a47
|
@ -741,27 +741,28 @@ func (r *BucketReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.Bu
|
|||
// resource.
|
||||
func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *sourcev1.Bucket) error {
|
||||
if !obj.DeletionTimestamp.IsZero() {
|
||||
if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
|
||||
if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
|
||||
return &serror.Event{
|
||||
Err: fmt.Errorf("garbage collection for deleted resource failed: %s", err),
|
||||
Reason: "GarbageCollectionFailed",
|
||||
}
|
||||
} else if deleted != "" {
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected artifacts for deleted resource")
|
||||
}
|
||||
obj.Status.Artifact = nil
|
||||
// TODO(hidde): we should only push this event if we actually garbage collected something
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected artifacts for deleted resource")
|
||||
return nil
|
||||
}
|
||||
if obj.GetArtifact() != nil {
|
||||
if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
|
||||
if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
|
||||
return &serror.Event{
|
||||
Err: fmt.Errorf("garbage collection of old artifacts failed: %s", err),
|
||||
Reason: "GarbageCollectionFailed",
|
||||
}
|
||||
} else if len(deleted) > 0 {
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected old artifacts")
|
||||
}
|
||||
// TODO(hidde): we should only push this event if we actually garbage collected something
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected old artifacts")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
|
|
@ -642,27 +642,27 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj
|
|||
// resource.
|
||||
func (r *GitRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourcev1.GitRepository) error {
|
||||
if !obj.DeletionTimestamp.IsZero() {
|
||||
if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
|
||||
if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
|
||||
return &serror.Event{
|
||||
Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err),
|
||||
Reason: "GarbageCollectionFailed",
|
||||
}
|
||||
} else if deleted != "" {
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected artifacts for deleted resource")
|
||||
}
|
||||
obj.Status.Artifact = nil
|
||||
// TODO(hidde): we should only push this event if we actually garbage collected something
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected artifacts for deleted resource")
|
||||
return nil
|
||||
}
|
||||
if obj.GetArtifact() != nil {
|
||||
if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
|
||||
if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
|
||||
return &serror.Event{
|
||||
Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err),
|
||||
}
|
||||
} else if len(deleted) > 0 {
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected old artifacts")
|
||||
}
|
||||
// TODO(hidde): we should only push this event if we actually garbage collected something
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected old artifacts")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
|
|
@ -750,27 +750,28 @@ func (r *HelmChartReconciler) reconcileDelete(ctx context.Context, obj *sourcev1
|
|||
// resource.
|
||||
func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.HelmChart) error {
|
||||
if !obj.DeletionTimestamp.IsZero() {
|
||||
if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
|
||||
if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
|
||||
return &serror.Event{
|
||||
Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err),
|
||||
Reason: "GarbageCollectionFailed",
|
||||
}
|
||||
} else if deleted != "" {
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected artifacts for deleted resource")
|
||||
}
|
||||
obj.Status.Artifact = nil
|
||||
// TODO(hidde): we should only push this event if we actually garbage collected something
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected artifacts for deleted resource")
|
||||
return nil
|
||||
}
|
||||
if obj.GetArtifact() != nil {
|
||||
if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
|
||||
if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
|
||||
return &serror.Event{
|
||||
Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err),
|
||||
Reason: "GarbageCollectionFailed",
|
||||
}
|
||||
} else if len(deleted) > 0 {
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected old artifacts")
|
||||
}
|
||||
// TODO(hidde): we should only push this event if we actually garbage collected something
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected old artifacts")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
|
|
@ -499,28 +499,28 @@ func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, obj *sou
|
|||
// resource.
|
||||
func (r *HelmRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourcev1.HelmRepository) error {
|
||||
if !obj.DeletionTimestamp.IsZero() {
|
||||
if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
|
||||
if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
|
||||
return &serror.Event{
|
||||
Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err),
|
||||
Reason: "GarbageCollectionFailed",
|
||||
}
|
||||
} else if deleted != "" {
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected artifacts for deleted resource")
|
||||
}
|
||||
obj.Status.Artifact = nil
|
||||
// TODO(hidde): we should only push this event if we actually garbage collected something
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected artifacts for deleted resource")
|
||||
return nil
|
||||
}
|
||||
if obj.GetArtifact() != nil {
|
||||
if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
|
||||
if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
|
||||
return &serror.Event{
|
||||
Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err),
|
||||
Reason: "GarbageCollectionFailed",
|
||||
}
|
||||
} else if len(deleted) > 0 {
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected old artifacts")
|
||||
}
|
||||
// TODO(hidde): we should only push this event if we actually garbage collected something
|
||||
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
|
||||
"garbage collected old artifacts")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
|
|
@ -105,13 +105,20 @@ func (s *Storage) MkdirAll(artifact sourcev1.Artifact) error {
|
|||
}
|
||||
|
||||
// RemoveAll calls os.RemoveAll for the given v1beta1.Artifact base dir.
|
||||
func (s *Storage) RemoveAll(artifact sourcev1.Artifact) error {
|
||||
func (s *Storage) RemoveAll(artifact sourcev1.Artifact) (string, error) {
|
||||
var deletedDir string
|
||||
dir := filepath.Dir(s.LocalPath(artifact))
|
||||
return os.RemoveAll(dir)
|
||||
// Check if the dir exists.
|
||||
_, err := os.Stat(dir)
|
||||
if err == nil {
|
||||
deletedDir = dir
|
||||
}
|
||||
return deletedDir, os.RemoveAll(dir)
|
||||
}
|
||||
|
||||
// RemoveAllButCurrent removes all files for the given v1beta1.Artifact base dir, excluding the current one.
|
||||
func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) error {
|
||||
func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) ([]string, error) {
|
||||
deletedFiles := []string{}
|
||||
localPath := s.LocalPath(artifact)
|
||||
dir := filepath.Dir(localPath)
|
||||
var errors []string
|
||||
|
@ -124,15 +131,18 @@ func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) error {
|
|||
if path != localPath && !info.IsDir() && info.Mode()&os.ModeSymlink != os.ModeSymlink {
|
||||
if err := os.Remove(path); err != nil {
|
||||
errors = append(errors, info.Name())
|
||||
} else {
|
||||
// Collect the successfully deleted file paths.
|
||||
deletedFiles = append(deletedFiles, path)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
})
|
||||
|
||||
if len(errors) > 0 {
|
||||
return fmt.Errorf("failed to remove files: %s", strings.Join(errors, " "))
|
||||
return deletedFiles, fmt.Errorf("failed to remove files: %s", strings.Join(errors, " "))
|
||||
}
|
||||
return nil
|
||||
return deletedFiles, nil
|
||||
}
|
||||
|
||||
// ArtifactExist returns a boolean indicating whether the v1beta1.Artifact exists in storage and is a regular file.
|
||||
|
|
|
@ -28,6 +28,7 @@ import (
|
|||
"time"
|
||||
|
||||
"github.com/go-git/go-git/v5/plumbing/format/gitignore"
|
||||
. "github.com/onsi/gomega"
|
||||
|
||||
sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
|
||||
)
|
||||
|
@ -293,10 +294,96 @@ func TestStorageRemoveAllButCurrent(t *testing.T) {
|
|||
t.Fatalf("Valid path did not successfully return: %v", err)
|
||||
}
|
||||
|
||||
if err := s.RemoveAllButCurrent(sourcev1.Artifact{Path: path.Join(dir, "really", "nonexistent")}); err == nil {
|
||||
if _, err := s.RemoveAllButCurrent(sourcev1.Artifact{Path: path.Join(dir, "really", "nonexistent")}); err == nil {
|
||||
t.Fatal("Did not error while pruning non-existent path")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("collect names of deleted items", func(t *testing.T) {
|
||||
g := NewWithT(t)
|
||||
dir, err := os.MkdirTemp("", "")
|
||||
g.Expect(err).ToNot(HaveOccurred())
|
||||
t.Cleanup(func() { os.RemoveAll(dir) })
|
||||
|
||||
s, err := NewStorage(dir, "hostname", time.Minute)
|
||||
g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage")
|
||||
|
||||
artifact := sourcev1.Artifact{
|
||||
Path: path.Join("foo", "bar", "artifact1.tar.gz"),
|
||||
}
|
||||
|
||||
// Create artifact dir and artifacts.
|
||||
artifactDir := path.Join(dir, "foo", "bar")
|
||||
g.Expect(os.MkdirAll(artifactDir, 0755)).NotTo(HaveOccurred())
|
||||
current := []string{
|
||||
path.Join(artifactDir, "artifact1.tar.gz"),
|
||||
}
|
||||
wantDeleted := []string{
|
||||
path.Join(artifactDir, "file1.txt"),
|
||||
path.Join(artifactDir, "file2.txt"),
|
||||
}
|
||||
createFile := func(files []string) {
|
||||
for _, c := range files {
|
||||
f, err := os.Create(c)
|
||||
g.Expect(err).ToNot(HaveOccurred())
|
||||
g.Expect(f.Close()).ToNot(HaveOccurred())
|
||||
}
|
||||
}
|
||||
createFile(current)
|
||||
createFile(wantDeleted)
|
||||
_, err = s.Symlink(artifact, "latest.tar.gz")
|
||||
g.Expect(err).ToNot(HaveOccurred(), "failed to create symlink")
|
||||
|
||||
deleted, err := s.RemoveAllButCurrent(artifact)
|
||||
g.Expect(err).ToNot(HaveOccurred(), "failed to remove all but current")
|
||||
g.Expect(deleted).To(Equal(wantDeleted))
|
||||
})
|
||||
}
|
||||
|
||||
func TestStorageRemoveAll(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
artifactPath string
|
||||
createArtifactPath bool
|
||||
wantDeleted string
|
||||
}{
|
||||
{
|
||||
name: "delete non-existent path",
|
||||
artifactPath: path.Join("foo", "bar", "artifact1.tar.gz"),
|
||||
createArtifactPath: false,
|
||||
wantDeleted: "",
|
||||
},
|
||||
{
|
||||
name: "delete existing path",
|
||||
artifactPath: path.Join("foo", "bar", "artifact1.tar.gz"),
|
||||
createArtifactPath: true,
|
||||
wantDeleted: path.Join("foo", "bar"),
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
g := NewWithT(t)
|
||||
dir, err := os.MkdirTemp("", "")
|
||||
g.Expect(err).ToNot(HaveOccurred())
|
||||
t.Cleanup(func() { os.RemoveAll(dir) })
|
||||
|
||||
s, err := NewStorage(dir, "hostname", time.Minute)
|
||||
g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage")
|
||||
|
||||
artifact := sourcev1.Artifact{
|
||||
Path: tt.artifactPath,
|
||||
}
|
||||
|
||||
if tt.createArtifactPath {
|
||||
g.Expect(os.MkdirAll(path.Join(dir, tt.artifactPath), 0755)).ToNot(HaveOccurred())
|
||||
}
|
||||
|
||||
deleted, err := s.RemoveAll(artifact)
|
||||
g.Expect(err).ToNot(HaveOccurred())
|
||||
g.Expect(deleted).To(ContainSubstring(tt.wantDeleted), "unexpected deleted path")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestStorageCopyFromPath(t *testing.T) {
|
||||
|
|
Loading…
Reference in New Issue