From 831e3f0e5755ddec211445bdbb606574f55ad71f Mon Sep 17 00:00:00 2001 From: Ciprian Hacman Date: Sun, 24 May 2020 08:38:46 +0300 Subject: [PATCH 1/6] Remove all versions of a file form the S3 bucket --- util/pkg/vfs/s3fs.go | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/util/pkg/vfs/s3fs.go b/util/pkg/vfs/s3fs.go index 416b18da043c6..c16459e49f831 100644 --- a/util/pkg/vfs/s3fs.go +++ b/util/pkg/vfs/s3fs.go @@ -92,15 +92,37 @@ func (p *S3Path) Remove() error { klog.V(8).Infof("removing file %s", p) - request := &s3.DeleteObjectInput{} - request.Bucket = aws.String(p.bucket) - request.Key = aws.String(p.key) + request := &s3.ListObjectVersionsInput{ + Bucket: aws.String(p.bucket), + Prefix: aws.String(p.key), + } - _, err = client.DeleteObject(request) + response, err := client.ListObjectVersions(request) if err != nil { - // TODO: Check for not-exists, return os.NotExist + return fmt.Errorf("error listing versions %s: %v", p, err) + } + + // Sometimes S3 will return paginated results if there are too many versions for an object. + // This is unlikely with current use cases, but a warning should be triggered in case it ever occurs. + if aws.BoolValue(response.IsTruncated) { + klog.Warningf("too many versions for %s", p) + } - return fmt.Errorf("error deleting %s: %v", p, err) + for _, version := range response.Versions { + klog.V(8).Infof("removing file %s version %q", p, aws.StringValue(version.VersionId)) + + request := &s3.DeleteObjectInput{ + Bucket: aws.String(p.bucket), + Key: aws.String(p.key), + VersionId: version.VersionId, + } + + _, err = client.DeleteObject(request) + if err != nil { + // TODO: Check for not-exists, return os.NotExist + + return fmt.Errorf("error deleting %s version %q: %v", p, aws.StringValue(version.VersionId), err) + } } return nil From 56af880c5338b513c3e7e2722d5a43604c501300 Mon Sep 17 00:00:00 2001 From: Ciprian Hacman Date: Sun, 24 May 2020 10:11:56 +0300 Subject: [PATCH 2/6] Remove TODO that was not addressed for a long time --- util/pkg/vfs/s3fs.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/util/pkg/vfs/s3fs.go b/util/pkg/vfs/s3fs.go index c16459e49f831..db8c886404907 100644 --- a/util/pkg/vfs/s3fs.go +++ b/util/pkg/vfs/s3fs.go @@ -119,8 +119,6 @@ func (p *S3Path) Remove() error { _, err = client.DeleteObject(request) if err != nil { - // TODO: Check for not-exists, return os.NotExist - return fmt.Errorf("error deleting %s version %q: %v", p, aws.StringValue(version.VersionId), err) } } From 1a38a3feaa8f188a3411b3bac26689fc9daaeeea Mon Sep 17 00:00:00 2001 From: Ciprian Hacman Date: Sun, 24 May 2020 11:42:18 +0300 Subject: [PATCH 3/6] Return os.ErrNotExist when no versions are found --- util/pkg/vfs/s3fs.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/util/pkg/vfs/s3fs.go b/util/pkg/vfs/s3fs.go index db8c886404907..0cecf63b71b70 100644 --- a/util/pkg/vfs/s3fs.go +++ b/util/pkg/vfs/s3fs.go @@ -102,6 +102,10 @@ func (p *S3Path) Remove() error { return fmt.Errorf("error listing versions %s: %v", p, err) } + if len(response.Versions) == 0 { + return os.ErrNotExist + } + // Sometimes S3 will return paginated results if there are too many versions for an object. // This is unlikely with current use cases, but a warning should be triggered in case it ever occurs. if aws.BoolValue(response.IsTruncated) { @@ -113,7 +117,7 @@ func (p *S3Path) Remove() error { request := &s3.DeleteObjectInput{ Bucket: aws.String(p.bucket), - Key: aws.String(p.key), + Key: version.Key, VersionId: version.VersionId, } From a48ccfa06c2f9b5f1b1318e0b813c6bed6c32ae5 Mon Sep 17 00:00:00 2001 From: Ciprian Hacman Date: Sun, 24 May 2020 15:20:20 +0300 Subject: [PATCH 4/6] Return warning instead of error to hide issues during cluster teardown --- util/pkg/vfs/s3fs.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/util/pkg/vfs/s3fs.go b/util/pkg/vfs/s3fs.go index 0cecf63b71b70..67fc13a487848 100644 --- a/util/pkg/vfs/s3fs.go +++ b/util/pkg/vfs/s3fs.go @@ -103,7 +103,9 @@ func (p *S3Path) Remove() error { } if len(response.Versions) == 0 { - return os.ErrNotExist + // TODO: Check why cluster teardown fails when files are not found and replace warning with error + // return os.ErrNotExist + klog.Warningf("error removing file (not found): %s", p) } // Sometimes S3 will return paginated results if there are too many versions for an object. @@ -123,7 +125,7 @@ func (p *S3Path) Remove() error { _, err = client.DeleteObject(request) if err != nil { - return fmt.Errorf("error deleting %s version %q: %v", p, aws.StringValue(version.VersionId), err) + return fmt.Errorf("error removing %s version %q: %v", p, aws.StringValue(version.VersionId), err) } } From b5651228752575477929e42f29ec853952de07bc Mon Sep 17 00:00:00 2001 From: Ciprian Hacman Date: Sun, 24 May 2020 17:42:02 +0300 Subject: [PATCH 5/6] Remove delete markers also from S3 bucket --- util/pkg/vfs/s3fs.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/util/pkg/vfs/s3fs.go b/util/pkg/vfs/s3fs.go index 67fc13a487848..bd5a8531c3baa 100644 --- a/util/pkg/vfs/s3fs.go +++ b/util/pkg/vfs/s3fs.go @@ -105,7 +105,7 @@ func (p *S3Path) Remove() error { if len(response.Versions) == 0 { // TODO: Check why cluster teardown fails when files are not found and replace warning with error // return os.ErrNotExist - klog.Warningf("error removing file (not found): %s", p) + klog.Warningf("error removing file/marker (not found): %s", p) } // Sometimes S3 will return paginated results if there are too many versions for an object. @@ -125,7 +125,22 @@ func (p *S3Path) Remove() error { _, err = client.DeleteObject(request) if err != nil { - return fmt.Errorf("error removing %s version %q: %v", p, aws.StringValue(version.VersionId), err) + return fmt.Errorf("error removing file %s version %q: %v", p, aws.StringValue(version.VersionId), err) + } + } + + for _, version := range response.DeleteMarkers { + klog.V(8).Infof("removing marker %s version %q", p, aws.StringValue(version.VersionId)) + + request := &s3.DeleteObjectInput{ + Bucket: aws.String(p.bucket), + Key: version.Key, + VersionId: version.VersionId, + } + + _, err = client.DeleteObject(request) + if err != nil { + return fmt.Errorf("error removing marker %s version %q: %v", p, aws.StringValue(version.VersionId), err) } } From 9675692b8484ec882b8f3b366af3b4873595f615 Mon Sep 17 00:00:00 2001 From: Ciprian Hacman Date: Mon, 25 May 2020 07:46:32 +0300 Subject: [PATCH 6/6] Implement RemoveAll() for S3 paths --- upup/models/vfs.go | 4 +++ util/pkg/vfs/fs.go | 4 +++ util/pkg/vfs/gsfs.go | 4 +++ util/pkg/vfs/k8sfs.go | 4 +++ util/pkg/vfs/memfs.go | 4 +++ util/pkg/vfs/ossfs.go | 4 +++ util/pkg/vfs/s3fs.go | 64 ++++++++++++++++++++++++++++------------- util/pkg/vfs/sshfs.go | 4 +++ util/pkg/vfs/swiftfs.go | 4 +++ util/pkg/vfs/vfs.go | 3 ++ 10 files changed, 79 insertions(+), 20 deletions(-) diff --git a/upup/models/vfs.go b/upup/models/vfs.go index a96c08edd616a..8f570e8363622 100644 --- a/upup/models/vfs.go +++ b/upup/models/vfs.go @@ -143,3 +143,7 @@ func (p *AssetPath) String() string { func (p *AssetPath) Remove() error { return ReadOnlyError } + +func (p *AssetPath) RemoveAll() error { + return p.Remove() +} diff --git a/util/pkg/vfs/fs.go b/util/pkg/vfs/fs.go index f45fe8a50ba95..dcba0d5df7e4c 100644 --- a/util/pkg/vfs/fs.go +++ b/util/pkg/vfs/fs.go @@ -187,6 +187,10 @@ func (p *FSPath) Remove() error { return os.Remove(p.location) } +func (p *FSPath) RemoveAll() error { + return p.Remove() +} + func (p *FSPath) PreferredHash() (*hashing.Hash, error) { return p.Hash(hashing.HashAlgorithmSHA256) } diff --git a/util/pkg/vfs/gsfs.go b/util/pkg/vfs/gsfs.go index f23b1c8d1a485..8b85e9e4d5e9b 100644 --- a/util/pkg/vfs/gsfs.go +++ b/util/pkg/vfs/gsfs.go @@ -132,6 +132,10 @@ func (p *GSPath) Remove() error { } } +func (p *GSPath) RemoveAll() error { + return p.Remove() +} + func (p *GSPath) Join(relativePath ...string) Path { args := []string{p.key} args = append(args, relativePath...) diff --git a/util/pkg/vfs/k8sfs.go b/util/pkg/vfs/k8sfs.go index 374eb1f40feb8..db540ece9d17f 100644 --- a/util/pkg/vfs/k8sfs.go +++ b/util/pkg/vfs/k8sfs.go @@ -68,6 +68,10 @@ func (p *KubernetesPath) Remove() error { return fmt.Errorf("KubernetesPath::Remove not supported") } +func (p *KubernetesPath) RemoveAll() error { + return p.Remove() +} + func (p *KubernetesPath) Join(relativePath ...string) Path { args := []string{p.key} args = append(args, relativePath...) diff --git a/util/pkg/vfs/memfs.go b/util/pkg/vfs/memfs.go index bcc2f5b7fd2d3..f79fb43fab5f4 100644 --- a/util/pkg/vfs/memfs.go +++ b/util/pkg/vfs/memfs.go @@ -169,3 +169,7 @@ func (p *MemFSPath) Remove() error { p.contents = nil return nil } + +func (p *MemFSPath) RemoveAll() error { + return p.Remove() +} diff --git a/util/pkg/vfs/ossfs.go b/util/pkg/vfs/ossfs.go index bb860ac5a6f5c..e0ff4f5edad2d 100644 --- a/util/pkg/vfs/ossfs.go +++ b/util/pkg/vfs/ossfs.go @@ -217,6 +217,10 @@ func (p *OSSPath) Remove() error { } } +func (p *OSSPath) RemoveAll() error { + return p.Remove() +} + func (p *OSSPath) Base() string { return path.Base(p.key) } diff --git a/util/pkg/vfs/s3fs.go b/util/pkg/vfs/s3fs.go index bd5a8531c3baa..d68ff2566856e 100644 --- a/util/pkg/vfs/s3fs.go +++ b/util/pkg/vfs/s3fs.go @@ -92,6 +92,28 @@ func (p *S3Path) Remove() error { klog.V(8).Infof("removing file %s", p) + request := &s3.DeleteObjectInput{} + request.Bucket = aws.String(p.bucket) + request.Key = aws.String(p.key) + + _, err = client.DeleteObject(request) + if err != nil { + // TODO: Check for not-exists, return os.NotExist + + return fmt.Errorf("error deleting %s: %v", p, err) + } + + return nil +} + +func (p *S3Path) RemoveAll() error { + client, err := p.client() + if err != nil { + return err + } + + klog.V(8).Infof("removing file %s", p) + request := &s3.ListObjectVersionsInput{ Bucket: aws.String(p.bucket), Prefix: aws.String(p.key), @@ -102,45 +124,47 @@ func (p *S3Path) Remove() error { return fmt.Errorf("error listing versions %s: %v", p, err) } - if len(response.Versions) == 0 { - // TODO: Check why cluster teardown fails when files are not found and replace warning with error - // return os.ErrNotExist - klog.Warningf("error removing file/marker (not found): %s", p) + if len(response.Versions) == 0 && len(response.DeleteMarkers) == 0 { + return os.ErrNotExist } - // Sometimes S3 will return paginated results if there are too many versions for an object. - // This is unlikely with current use cases, but a warning should be triggered in case it ever occurs. + // Sometimes S3 will return paginated results if there are too many versions and markers for an object. + // This happens at about entries 1000, so it is unlikely with current use cases. if aws.BoolValue(response.IsTruncated) { klog.Warningf("too many versions for %s", p) } + objects := []*s3.ObjectIdentifier{} for _, version := range response.Versions { klog.V(8).Infof("removing file %s version %q", p, aws.StringValue(version.VersionId)) - - request := &s3.DeleteObjectInput{ - Bucket: aws.String(p.bucket), + file := s3.ObjectIdentifier{ Key: version.Key, VersionId: version.VersionId, } - - _, err = client.DeleteObject(request) - if err != nil { - return fmt.Errorf("error removing file %s version %q: %v", p, aws.StringValue(version.VersionId), err) - } + objects = append(objects, &file) } - for _, version := range response.DeleteMarkers { klog.V(8).Infof("removing marker %s version %q", p, aws.StringValue(version.VersionId)) - - request := &s3.DeleteObjectInput{ - Bucket: aws.String(p.bucket), + marker := s3.ObjectIdentifier{ Key: version.Key, VersionId: version.VersionId, } + objects = append(objects, &marker) + } + + if len(objects) > 0 { + klog.V(8).Infof("removing %d file/marker versions\n", len(objects)) + + request := &s3.DeleteObjectsInput{ + Bucket: aws.String(p.bucket), + Delete: &s3.Delete{ + Objects: objects, + }, + } - _, err = client.DeleteObject(request) + _, err = client.DeleteObjects(request) if err != nil { - return fmt.Errorf("error removing marker %s version %q: %v", p, aws.StringValue(version.VersionId), err) + return fmt.Errorf("error removing %d file/marker versions: %v", len(objects), err) } } diff --git a/util/pkg/vfs/sshfs.go b/util/pkg/vfs/sshfs.go index 90ef61ab93c34..21acfd516ae67 100644 --- a/util/pkg/vfs/sshfs.go +++ b/util/pkg/vfs/sshfs.go @@ -111,6 +111,10 @@ func (p *SSHPath) Remove() error { return nil } +func (p *SSHPath) RemoveAll() error { + return p.Remove() +} + func (p *SSHPath) Join(relativePath ...string) Path { args := []string{p.path} args = append(args, relativePath...) diff --git a/util/pkg/vfs/swiftfs.go b/util/pkg/vfs/swiftfs.go index 7dc6e7544f2b0..4c73b27753103 100644 --- a/util/pkg/vfs/swiftfs.go +++ b/util/pkg/vfs/swiftfs.go @@ -291,6 +291,10 @@ func (p *SwiftPath) Remove() error { } } +func (p *SwiftPath) RemoveAll() error { + return p.Remove() +} + func (p *SwiftPath) Join(relativePath ...string) Path { args := []string{p.key} args = append(args, relativePath...) diff --git a/util/pkg/vfs/vfs.go b/util/pkg/vfs/vfs.go index cdcd74fe1e802..c0e32c9de981d 100644 --- a/util/pkg/vfs/vfs.go +++ b/util/pkg/vfs/vfs.go @@ -59,6 +59,9 @@ type Path interface { // Remove deletes the file Remove() error + // RemoveAll completely deletes the file (with all its versions and markers) + RemoveAll() error + // Base returns the base name (last element) Base() string