From c22bf2539ebcd3eae1d49c9b140bf3ebcfc87110 Mon Sep 17 00:00:00 2001 From: He Weiwei Date: Tue, 20 Aug 2019 14:41:10 +0000 Subject: [PATCH] refactor(quota,middleware): skip overflow error when subtract resources 1. Skip overflow error when subtract resources 2. Take up resources before handle request and put it back when handle failed for add action in quota interceptor 3. Free resources only after handle success for subtract action in quota interceptor Closes #8681 Signed-off-by: He Weiwei --- src/common/quota/manager.go | 9 +- src/common/quota/util.go | 4 +- src/common/quota/util_test.go | 23 ++++- .../middlewares/interceptor/quota/quota.go | 99 ++++++++++++------- 4 files changed, 92 insertions(+), 43 deletions(-) diff --git a/src/common/quota/manager.go b/src/common/quota/manager.go index a70199ed3..cfcbe4741 100644 --- a/src/common/quota/manager.go +++ b/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 } @@ -245,14 +246,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) }) } diff --git a/src/common/quota/util.go b/src/common/quota/util.go index 5f8687cc7..e57e05170 100644 --- a/src/common/quota/util.go +++ b/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)) } } diff --git a/src/common/quota/util_test.go b/src/common/quota/util_test.go index d0db1166a..44432348d 100644 --- a/src/common/quota/util_test.go +++ b/src/common/quota/util_test.go @@ -22,9 +22,10 @@ import ( func Test_isSafe(t *testing.T) { type args struct { - hardLimits types.ResourceList - currentUsed types.ResourceList - newUsed types.ResourceList + hardLimits types.ResourceList + currentUsed 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) } }) diff --git a/src/core/middlewares/interceptor/quota/quota.go b/src/core/middlewares/interceptor/quota/quota.go index 607f58dde..1ae530078 100644 --- a/src/core/middlewares/interceptor/quota/quota.go +++ b/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 { - log.Errorf("Failed to %s resources, error: %v", opts.Action, err) + if err := qi.doCancel(); err != nil { + 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 { - return nil - } - - 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: + // Add resources in try stage when it is add action + // And do nothing in confirm stage for add action + if len(qi.resources) != 0 && qi.opts.Action == AddAction { 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 +}