Merge pull request #8758 from heww/issue-8681

refactor(quota,middleware): skip overflow error when subtract resources

src/common/quota/manager.go

@@ -110,7 +110,8 @@ func (m *Manager) getUsageForUpdate(o orm.Ormer) (*models.QuotaUsage, error) {
 }
 
 func (m *Manager) updateUsage(o orm.Ormer, resources types.ResourceList,
-	calculate func(types.ResourceList, types.ResourceList) types.ResourceList) error {
+	calculate func(types.ResourceList, types.ResourceList) types.ResourceList,
+	skipOverflow bool) error {
 
 	quota, err := m.getQuotaForUpdate(o)
 	if err != nil {
@@ -137,7 +138,7 @@ func (m *Manager) updateUsage(o orm.Ormer, resources types.ResourceList,
 		return fmt.Errorf("quota usage is negative for resource(s): %s", prettyPrintResourceNames(negativeUsed))
 	}
 
-	if err := isSafe(hardLimits, used, newUsed); err != nil {
+	if err := isSafe(hardLimits, used, newUsed, skipOverflow); err != nil {
 		return err
 	}
 
@@ -255,14 +256,14 @@ func (m *Manager) EnsureQuota(usages types.ResourceList) error {
 // AddResources add resources to usage
 func (m *Manager) AddResources(resources types.ResourceList) error {
 	return dao.WithTransaction(func(o orm.Ormer) error {
-		return m.updateUsage(o, resources, types.Add)
+		return m.updateUsage(o, resources, types.Add, false)
 	})
 }
 
 // SubtractResources subtract resources from usage
 func (m *Manager) SubtractResources(resources types.ResourceList) error {
 	return dao.WithTransaction(func(o orm.Ormer) error {
-		return m.updateUsage(o, resources, types.Subtract)
+		return m.updateUsage(o, resources, types.Subtract, true)
 	})
 }
 

src/common/quota/util.go

@@ -21,7 +21,7 @@ import (
 	"github.com/goharbor/harbor/src/pkg/types"
 )
 
-func isSafe(hardLimits types.ResourceList, currentUsed types.ResourceList, newUsed types.ResourceList) error {
+func isSafe(hardLimits types.ResourceList, currentUsed types.ResourceList, newUsed types.ResourceList, skipOverflow bool) error {
 	var errs Errors
 
 	for resource, value := range newUsed {
@@ -35,7 +35,7 @@ func isSafe(hardLimits types.ResourceList, currentUsed types.ResourceList, newUs
 			continue
 		}
 
-		if value > hardLimit {
+		if value > hardLimit && !skipOverflow {
 			errs = errs.Add(NewResourceOverflowError(resource, hardLimit, currentUsed[resource], value))
 		}
 	}

src/common/quota/util_test.go

@@ -22,9 +22,10 @@ import (
 
 func Test_isSafe(t *testing.T) {
 	type args struct {
-		hardLimits  types.ResourceList
+		hardLimits   types.ResourceList
-		currentUsed types.ResourceList
+		currentUsed  types.ResourceList
-		newUsed     types.ResourceList
+		newUsed      types.ResourceList
+		skipOverflow bool
 	}
 	tests := []struct {
 		name    string
@@ -37,6 +38,7 @@ func Test_isSafe(t *testing.T) {
 				types.ResourceList{types.ResourceStorage: types.UNLIMITED},
 				types.ResourceList{types.ResourceStorage: 1000},
 				types.ResourceList{types.ResourceStorage: 1000},
+				false,
 			},
 			false,
 		},
@@ -46,6 +48,7 @@ func Test_isSafe(t *testing.T) {
 				types.ResourceList{types.ResourceStorage: 100},
 				types.ResourceList{types.ResourceStorage: 10},
 				types.ResourceList{types.ResourceStorage: 1},
+				false,
 			},
 			false,
 		},
@@ -55,22 +58,34 @@ func Test_isSafe(t *testing.T) {
 				types.ResourceList{types.ResourceStorage: 100},
 				types.ResourceList{types.ResourceStorage: 0},
 				types.ResourceList{types.ResourceStorage: 200},
+				false,
 			},
 			true,
 		},
+		{
+			"skip overflow",
+			args{
+				types.ResourceList{types.ResourceStorage: 100},
+				types.ResourceList{types.ResourceStorage: 0},
+				types.ResourceList{types.ResourceStorage: 200},
+				true,
+			},
+			false,
+		},
 		{
 			"hard limit not found",
 			args{
 				types.ResourceList{types.ResourceStorage: 100},
 				types.ResourceList{types.ResourceCount: 0},
 				types.ResourceList{types.ResourceCount: 1},
+				false,
 			},
 			true,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if err := isSafe(tt.args.hardLimits, tt.args.currentUsed, tt.args.newUsed); (err != nil) != tt.wantErr {
+			if err := isSafe(tt.args.hardLimits, tt.args.currentUsed, tt.args.newUsed, tt.args.skipOverflow); (err != nil) != tt.wantErr {
 				t.Errorf("isSafe() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})

src/core/middlewares/interceptor/quota/quota.go

@@ -16,7 +16,9 @@ package quota
 
 import (
 	"fmt"
+	"math/rand"
 	"net/http"
+	"time"
 
 	"github.com/goharbor/harbor/src/common/utils/log"
 	"github.com/goharbor/harbor/src/common/utils/redis"
@@ -24,6 +26,10 @@ import (
 	"github.com/goharbor/harbor/src/pkg/types"
 )
 
+func init() {
+	rand.Seed(time.Now().UnixNano())
+}
+
 // New ....
 func New(opts ...Option) interceptor.Interceptor {
 	options := newOptions(opts...)
@@ -59,7 +65,7 @@ func (qi *quotaInterceptor) HandleRequest(req *http.Request) (err error) {
 		return
 	}
 
-	err = qi.reserve()
+	err = qi.doTry()
 	if err != nil {
 		log.Errorf("Failed to %s resources, error: %v", qi.opts.Action, err)
 	}
@@ -80,14 +86,18 @@ func (qi *quotaInterceptor) HandleResponse(w http.ResponseWriter, req *http.Requ
 
 	switch sr.Status() {
 	case opts.StatusCode:
+		if err := qi.doConfirm(); err != nil {
+			log.Errorf("Failed to confirm for resource, error: %v", err)
+		}
+
 		if opts.OnFulfilled != nil {
 			if err := opts.OnFulfilled(w, req); err != nil {
 				log.Errorf("Failed to handle on fulfilled, error: %v", err)
 			}
 		}
 	default:
-		if err := qi.unreserve(); err != nil {
+		if err := qi.doCancel(); err != nil {
-			log.Errorf("Failed to %s resources, error: %v", opts.Action, err)
+			log.Errorf("Failed to cancel for resource, error: %v", err)
 		}
 
 		if opts.OnRejected != nil {
@@ -148,42 +158,65 @@ func (qi *quotaInterceptor) computeResources(req *http.Request) error {
 	return nil
 }
 
-func (qi *quotaInterceptor) reserve() error {
+func (qi *quotaInterceptor) doTry() error {
 	if !qi.opts.EnforceResources() {
-		// Do nothing in reserve resources when quota interceptor not enforce resources
+		// Do nothing in try stage when quota interceptor not enforce resources
 		return nil
 	}
 
-	if len(qi.resources) == 0 {
+	// Add resources in try stage when it is add action
-		return nil
+	// And do nothing in confirm stage for add action
-	}
+	if len(qi.resources) != 0 && qi.opts.Action == AddAction {
-
-	switch qi.opts.Action {
-	case AddAction:
-		return qi.opts.Manager.AddResources(qi.resources)
-	case SubtractAction:
-		return qi.opts.Manager.SubtractResources(qi.resources)
-	}
-
-	return nil
-}
-
-func (qi *quotaInterceptor) unreserve() error {
-	if !qi.opts.EnforceResources() {
-		// Do nothing in unreserve resources when quota interceptor not enforce resources
-		return nil
-	}
-
-	if len(qi.resources) == 0 {
-		return nil
-	}
-
-	switch qi.opts.Action {
-	case AddAction:
-		return qi.opts.Manager.SubtractResources(qi.resources)
-	case SubtractAction:
 		return qi.opts.Manager.AddResources(qi.resources)
 	}
 
 	return nil
 }
+
+func (qi *quotaInterceptor) doConfirm() error {
+	if !qi.opts.EnforceResources() {
+		// Do nothing in confirm stage when quota interceptor not enforce resources
+		return nil
+	}
+
+	// Subtract resources in confirm stage when it is subtract action
+	// And do nothing in try stage for subtract action
+	if len(qi.resources) != 0 && qi.opts.Action == SubtractAction {
+		return retry(3, 100*time.Millisecond, func() error {
+			return qi.opts.Manager.SubtractResources(qi.resources)
+		})
+	}
+
+	return nil
+}
+
+func (qi *quotaInterceptor) doCancel() error {
+	if !qi.opts.EnforceResources() {
+		// Do nothing in cancel stage when quota interceptor not enforce resources
+		return nil
+	}
+
+	// Subtract resources back when process failed for add action
+	if len(qi.resources) != 0 && qi.opts.Action == AddAction {
+		return retry(3, 100*time.Millisecond, func() error {
+			return qi.opts.Manager.SubtractResources(qi.resources)
+		})
+	}
+
+	return nil
+}
+
+func retry(attempts int, sleep time.Duration, f func() error) error {
+	if err := f(); err != nil {
+		if attempts--; attempts > 0 {
+			r := time.Duration(rand.Int63n(int64(sleep)))
+			sleep = sleep + r/2
+
+			time.Sleep(sleep)
+			return retry(attempts, 2*sleep, f)
+		}
+		return err
+	}
+
+	return nil
+}