simplify and optimize deleting multiple versions of object (#2153)

.github/workflows/jobs.yaml

@@ -29,7 +29,7 @@ jobs:
 
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
 
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -98,7 +98,7 @@ jobs:
 
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
 
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -167,7 +167,7 @@ jobs:
 
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
 
     steps:
 
@@ -209,7 +209,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -233,7 +233,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -280,7 +280,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -335,7 +335,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -373,7 +373,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -452,7 +452,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -531,7 +531,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -612,7 +612,7 @@ jobs:
     timeout-minutes: 5
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -681,7 +681,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -750,7 +750,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -819,7 +819,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -888,7 +888,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -957,7 +957,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -1031,7 +1031,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -1069,7 +1069,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -1107,7 +1107,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -1145,7 +1145,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -1183,7 +1183,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -1221,7 +1221,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -1259,7 +1259,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -1297,7 +1297,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -1343,7 +1343,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -1390,7 +1390,7 @@ jobs:
 
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
 
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}
@@ -1502,7 +1502,7 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go-version: [ 1.17.x ]
+        go-version: [ 1.18.x ]
         os: [ ubuntu-latest ]
     steps:
       - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}

Dockerfile

@@ -12,7 +12,7 @@ RUN make build-static
 
 USER node
 
-FROM golang:1.17 as golayer
+FROM golang:1.18 as golayer
 
 RUN apt-get update -y && apt-get install -y ca-certificates
 
@@ -31,7 +31,7 @@ ENV CGO_ENABLED=0
 COPY --from=uilayer /app/build /go/src/github.com/minio/console/portal-ui/build
 RUN go build --tags=kqueue,operator -ldflags "-w -s" -a -o console ./cmd/console
 
-FROM registry.access.redhat.com/ubi8/ubi-minimal:8.5
+FROM registry.access.redhat.com/ubi8/ubi-minimal:8.6
 MAINTAINER MinIO Development "dev@min.io"
 EXPOSE 9090
 

Makefile

@@ -21,7 +21,7 @@ k8sdev:
 
 getdeps:
 	@mkdir -p ${GOPATH}/bin
-	@which golangci-lint 1>/dev/null || (echo "Installing golangci-lint" && curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH)/bin v1.43.0)
+	@echo "Installing golangci-lint" && curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH)/bin v1.45.2
 
 verifiers: getdeps fmt lint
 

restapi/user_objects.go

@@ -658,79 +658,57 @@ func getDeleteMultiplePathsResponse(session *models.Principal, params objectApi.
 func deleteObjects(ctx context.Context, client MCClient, bucket string, path string, versionID string, recursive bool, allVersions bool, nonCurrentVersionsOnly bool) error {
 	// Delete All non-Current versions only.
 	if nonCurrentVersionsOnly {
-		if err := deleteNonCurrentVersions(ctx, client, bucket, path); err != nil {
+		return deleteNonCurrentVersions(ctx, client, bucket, path)
-			return err
-		}
-		return nil
 	}
 
-	if allVersions {
-		if err := deleteMultipleObjects(ctx, client, recursive, true); err != nil {
-			return err
-		}
-	}
 	if recursive {
-		if err := deleteMultipleObjects(ctx, client, recursive, false); err != nil {
+		return deleteMultipleObjects(ctx, client, recursive, allVersions)
-			return err
-		}
-	} else {
-		if err := deleteSingleObject(ctx, client, bucket, path, versionID); err != nil {
-			return err
-		}
 	}
-	return nil
+
+	return deleteSingleObject(ctx, client, bucket, path, versionID)
 }
 
 // deleteMultipleObjects uses listing before removal, it can list recursively or not,
 //   Use cases:
 //      * Remove objects recursively
 func deleteMultipleObjects(ctx context.Context, client MCClient, recursive bool, allVersions bool) error {
-	isRemoveBucket := false
-	isIncomplete := false
 	isBypass := false
-	listOpts := mc.ListOptions{Recursive: recursive, Incomplete: isIncomplete, ShowDir: mc.DirNone, WithOlderVersions: allVersions, WithDeleteMarkers: allVersions}
+	isIncomplete := false
-	// TODO: support older Versions
+	isRemoveBucket := false
-	contentCh := make(chan *mc.ClientContent, 1)
+	listOpts := mc.ListOptions{
+		Recursive:         recursive,
+		Incomplete:        isIncomplete,
+		ShowDir:           mc.DirNone,
+		WithOlderVersions: allVersions,
+		WithDeleteMarkers: allVersions,
+	}
 
-	resultCh := client.remove(ctx, isIncomplete, isRemoveBucket, isBypass, contentCh)
+	lctx, cancel := context.WithCancel(ctx)
-OUTER_LOOP:
+	defer cancel()
-	for content := range client.list(ctx, listOpts) {
+
-		if content.Err != nil {
+	contentCh := make(chan *mc.ClientContent)
-			if _, ok := content.Err.ToGoError().(mc.PathInsufficientPermission); ok {
+
-				// Ignore Permission errors.
+	go func() {
+		defer close(contentCh)
+
+		for content := range client.list(lctx, listOpts) {
+			if content.Err != nil {
 				continue
 			}
-			close(contentCh)
-			return content.Err.Cause
-		}
-		sent := false
-		for !sent {
 			select {
 			case contentCh <- content:
-				sent = true
+			case <-lctx.Done():
-			case result := <-resultCh:
+				return
-				if result.Err != nil {
-					if _, ok := result.Err.ToGoError().(mc.PathInsufficientPermission); ok {
-						// Ignore Permission errors.
-						continue
-					}
-					close(contentCh)
-					return result.Err.Cause
-				}
-				break OUTER_LOOP
 			}
 		}
-	}
+	}()
-	close(contentCh)
+
-	for result := range resultCh {
+	for result := range client.remove(ctx, isIncomplete, isRemoveBucket, isBypass, contentCh) {
 		if result.Err != nil {
-			if _, ok := result.Err.ToGoError().(mc.PathInsufficientPermission); ok {
-				// Ignore Permission errors.
-				continue
-			}
 			return result.Err.Cause
 		}
 	}
+
 	return nil
 }
 
@@ -740,17 +718,13 @@ func deleteSingleObject(ctx context.Context, client MCClient, bucket, object str
 	contentCh <- &mc.ClientContent{URL: *newClientURL(targetURL), VersionID: versionID}
 	close(contentCh)
 
-	isRemoveBucket := false
-	isIncomplete := false
 	isBypass := false
+	isIncomplete := false
+	isRemoveBucket := false
 
 	resultCh := client.remove(ctx, isIncomplete, isRemoveBucket, isBypass, contentCh)
 	for result := range resultCh {
 		if result.Err != nil {
-			if _, ok := result.Err.ToGoError().(mc.PathInsufficientPermission); ok {
-				// Ignore Permission errors.
-				continue
-			}
 			return result.Err.Cause
 		}
 	}
@@ -758,17 +732,40 @@ func deleteSingleObject(ctx context.Context, client MCClient, bucket, object str
 }
 
 func deleteNonCurrentVersions(ctx context.Context, client MCClient, bucket, path string) error {
-	// Get current object versions
+	lctx, cancel := context.WithCancel(ctx)
-	for lsObj := range client.list(ctx, mc.ListOptions{WithDeleteMarkers: true, WithOlderVersions: true, Recursive: true}) {
+	defer cancel()
-		if lsObj.Err != nil {
-			return errors.New(lsObj.Err.String())
-		}
 
-		if !lsObj.IsLatest {
+	contentCh := make(chan *mc.ClientContent)
-			err := deleteSingleObject(ctx, client, bucket, path, lsObj.VersionID)
+
-			if err != nil {
+	go func() {
-				return err
+		defer close(contentCh)
+
+		// Get current object versions
+		for lsObj := range client.list(lctx, mc.ListOptions{
+			WithDeleteMarkers: true,
+			WithOlderVersions: true,
+			Recursive:         true,
+		}) {
+			if lsObj.Err != nil {
+				continue
 			}
+
+			if lsObj.IsLatest {
+				continue
+			}
+
+			// All non-current objects proceed to purge.
+			select {
+			case contentCh <- lsObj:
+			case <-lctx.Done():
+				return
+			}
+		}
+	}()
+
+	for result := range client.remove(ctx, false, false, false, contentCh) {
+		if result.Err != nil {
+			return result.Err.Cause
 		}
 	}
 

restapi/user_objects_test.go

@@ -656,34 +656,10 @@ func Test_deleteObjects(t *testing.T) {
 			},
 			wantError: nil,
 		},
-		{
-			// Description handle error when error happens on list function
-			// while deleting multiple objects
-			test: "Error on Remove multiple objects 1",
-			args: args{
-				path:       "path/",
-				versionID:  "",
-				recursive:  true,
-				nonCurrent: false,
-				removeFunc: func(ctx context.Context, isIncomplete, isRemoveBucket, isBypass bool, contentCh <-chan *mc.ClientContent) <-chan mc.RemoveResult {
-					resultCh := make(chan mc.RemoveResult, 1)
-					resultCh <- mc.RemoveResult{Err: nil}
-					close(resultCh)
-					return resultCh
-				},
-				listFunc: func(ctx context.Context, opts mc.ListOptions) <-chan *mc.ClientContent {
-					ch := make(chan *mc.ClientContent, 1)
-					ch <- &mc.ClientContent{Err: probe.NewError(errors.New("probe error"))}
-					close(ch)
-					return ch
-				},
-			},
-			wantError: errors.New("probe error"),
-		},
 		{
 			// Description handle error when error happens on remove function
 			// while deleting multiple objects
-			test: "Error on Remove multiple objects 2",
+			test: "Error on Remove multiple objects",
 			args: args{
 				path:       "path/",
 				versionID:  "",
@@ -705,9 +681,7 @@ func Test_deleteObjects(t *testing.T) {
 			wantError: errors.New("probe error"),
 		},
 		{
-			// Description handle error when error happens on remove function
+			test: "Remove non current objects - no error",
-			// while deleting multiple objects
-			test: "Remove non current objects",
 			args: args{
 				path:       "path/",
 				versionID:  "",