Error handling updates in quota management usage #367

z103cb · 2023-05-15T13:52:24Z

Small fixes around error handling inside the QM management initialisation path.

Improved error handling.

asm582

Thanks for PR, can you please address the logger issue and resolve the merge issue ?

pkg/controller/quota/quotaforestmanager/qm_lib_backend_with_quotasubt_mgr.go

Removed un-used code Fixed other linter warnings.

pkg/controller/queuejob/queuejob_controller_ex.go

pkg/controller/quota/quotaforestmanager/qm_lib_backend_with_quotasubt_mgr.go

in QM trees and requests

Added rules to update the deployment crds.

asm582 · 2023-05-17T17:02:38Z

I am ok to approve this PR, a single test has failed due to resource jitter issues

KPostOffice · 2023-05-17T17:28:31Z

fixes: #335

deployment/mcad-controller/crds/ibm.com_quotasubtree-v1.yaml

More error handling updates

Minor cleanup in the Fits function.

z103cb · 2023-05-23T12:23:36Z

The failures of the build are related to #388, #392

Logging improvements Additional test cases in quotaforest manager

z103cb · 2023-05-26T11:01:45Z

Makefile

@@ -115,9 +115,9 @@ endif

 run-test:
 	$(info Running unit tests...)
-	hack/make-rules/test.sh $(WHAT) $(TESTS)
+	go test -v -coverprofile cover.out -race -parallel 8  ./pkg/...


@asm582, @KPostOffice, @dmatch01, @metalcycling let me know if you are OK with the verbose output from testing and with the exclusion of the ./cmd package.

z103cb · 2023-05-26T11:05:21Z

pkg/controller/queuejob/queuejob_controller_ex.go

@@ -1235,8 +1241,11 @@ func (qjm *XController) ScheduleNext() {
 		// HeadOfLine logic
 		HOLStartTime := time.Now()
 		forwarded := false
+		fowardingLoopCount := 1


@asm582, @dmatch01, @KPostOffice and @metalcycling please take a very close look at the changes starting from this line.

z103cb · 2023-05-26T11:08:51Z

pkg/controller/queuejob/queuejob_controller_ex.go

 				if fits {
 					// aw is ready to go!
 					apiQueueJob, e := qjm.queueJobLister.AppWrappers(qj.Namespace).Get(qj.Name)
 					// apiQueueJob's ControllerFirstTimestamp is only microsecond level instead of nanosecond level
 					if e != nil {
 						klog.Errorf("[ScheduleNext] Unable to get AW %s from API cache &aw=%p Version=%s Status=%+v err=%#v", qj.Name, qj, qj.ResourceVersion, qj.Status, err)
+						if qjm.quotaManager != nil && quotaFits {


@asm582, @dmatch01, @KPostOffice and @metalcycling I believe that this one of the causes for client already allocated set of errors.

cc: @tardieu

quota is not released when AW is deleted by clients, which is a different issue.

z103cb · 2023-05-26T11:11:00Z

pkg/controller/queuejob/queuejob_controller_ex.go

+			} else if schedulingTimeExpired {
+				// stop trying to dispatch after HeadOfLineHoldingTime
+				// release quota if allocated
+				if qjm.quotaManager != nil && quotaFits {


@asm582, @KPostOffice, @metalcycling in your testing have you seen this condition happen ?

@z103cb We have not seen this condition happen, who sets HeadOfLineHoldingTime ? is this part of the helm command?

@asm582 the HeadofLineHoldingTime is a command line option, defined at:
fs.IntVar(&s.HeadOfLineHoldingTime, "headoflineholdingtime", s.HeadOfLineHoldingTime, "Number of seconds a job can stay at the Head Of Line without being bumped. Default is 0.")

AFAIK, the helm chart does not define an override

z103cb · 2023-05-26T11:12:13Z

pkg/controller/queuejob/queuejob_controller_ex.go

 				break
 			} else { // Try to dispatch again after one second
+				if qjm.quotaManager != nil && quotaFits {


@asm582, @metalcycling have you seen this happen in your testing ?

I think retries could be avoided here explicitly, undispatched AW should be picked up in the next dispatch cycle.

@asm582 can you be more specific I really don't understand your comment.

z103cb · 2023-05-26T11:16:33Z

pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager.go

@@ -39,6 +40,9 @@ type Manager struct {
 	consumerInfos map[string]*ConsumerInfo
 	// mode of the quota mnager; initially in maintenance mode
 	mode Mode
+
+	// mutex to
+	mutex sync.RWMutex


@asm582, @metalcycling and @dmatch01 I believe a single mutex would be sufficient for now and MCAD use of the library. The entire quota library forest is not thread safe, we'll have to figure out how to make it so, and perhaps simplify it.

would like to understand more about the threading issue, ideally, the whole dispatching should be single-threaded.

@asm582 take a look at #392

z103cb · 2023-05-26T11:18:09Z

pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager_test.go

+	}
+	// Execute tests in parallel
+	for _, tc := range tests {
+		tc := tc // capture range variable


This is necessary to allow for all the tests to be executed.

asm582

Please address the review

asm582 · 2023-05-26T12:48:18Z

pkg/controller/queuejob/queuejob_controller_ex.go

+			} else if schedulingTimeExpired {
+				// stop trying to dispatch after HeadOfLineHoldingTime
+				// release quota if allocated
+				if qjm.quotaManager != nil && quotaFits {


@z103cb We have not seen this condition happen, who sets HeadOfLineHoldingTime ? is this part of the helm command?

asm582 · 2023-05-26T12:52:33Z

pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2022 The Multi-Cluster App Dispatcher Authors.
+Copyright 2022, 2203 The Multi-Cluster App Dispatcher Authors.


Nit: fix date

asm582 · 2023-05-26T12:54:36Z

pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager.go

@@ -39,6 +40,9 @@ type Manager struct {
 	consumerInfos map[string]*ConsumerInfo
 	// mode of the quota mnager; initially in maintenance mode
 	mode Mode
+
+	// mutex to
+	mutex sync.RWMutex


would like to understand more about the threading issue, ideally, the whole dispatching should be single-threaded.

asm582 · 2023-05-26T13:06:32Z

pkg/controller/queuejob/queuejob_controller_ex.go

 				if fits {
 					// aw is ready to go!
 					apiQueueJob, e := qjm.queueJobLister.AppWrappers(qj.Namespace).Get(qj.Name)
 					// apiQueueJob's ControllerFirstTimestamp is only microsecond level instead of nanosecond level
 					if e != nil {
 						klog.Errorf("[ScheduleNext] Unable to get AW %s from API cache &aw=%p Version=%s Status=%+v err=%#v", qj.Name, qj, qj.ResourceVersion, qj.Status, err)
+						if qjm.quotaManager != nil && quotaFits {


quota is not released when AW is deleted by clients, which is a different issue.

pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager_test.go

asm582 · 2023-05-26T13:29:27Z

pkg/controller/queuejob/queuejob_controller_ex.go

 				break
 			} else { // Try to dispatch again after one second
+				if qjm.quotaManager != nil && quotaFits {


I think retries could be avoided here explicitly, undispatched AW should be picked up in the next dispatch cycle.

asm582

We discussed offline, LGTM 🎉

asm582 · 2023-05-26T17:02:42Z

All quota management test pass, but there is still issues with MCAD resource jitter in tests. merging

z103cb added 3 commits May 15, 2023 11:40

Small error handling updates.

847b0b9

Small formatting updates

5ebd199

Improved error handling.

Updates to the gitignore.

38b5144

asm582 reviewed May 15, 2023

View reviewed changes

pkg/controller/quota/quotaforestmanager/qm_lib_backend_with_quotasubt_mgr.go Outdated Show resolved Hide resolved

Merge branch 'main' into hotfix_qm_build

f25812c

z103cb requested review from asm582, metalcycling, KPostOffice and dmatch01 May 17, 2023 06:01

z103cb added 2 commits May 17, 2023 09:05

Changes to allow for quota trees to present

3d82520

Addressed PR comment.

e287f1d

Removed un-used code Fixed other linter warnings.

z103cb changed the title ~~Error handling updates in QM management initialization~~ Error handling updates in quite management initialisation May 17, 2023

z103cb commented May 17, 2023

View reviewed changes

z103cb added 3 commits May 17, 2023 09:19

Merge branch 'hotfix_qm_build' into fix-qm-tests

d816fb5

Fixed issues with correct interpretation of quanty

8caa3ae

in QM trees and requests

Cleanup e2e script.

5ee871e

Added rules to update the deployment crds.

asm582 previously approved these changes May 17, 2023

View reviewed changes

KPostOffice reviewed May 17, 2023

View reviewed changes

deployment/mcad-controller/crds/ibm.com_quotasubtree-v1.yaml Outdated Show resolved Hide resolved

Changed regex for quota specification

37fdd3a

z103cb dismissed asm582’s stale review via 37fdd3a May 18, 2023 10:53

z103cb added 2 commits May 19, 2023 11:01

Fixed error in qm backend struct

eb3a0e9

More error handling updates

Fixed test case error

4d29d6c

Minor cleanup in the Fits function.

First pass at fixing the concurency issues

80d1e58

tardieu mentioned this pull request May 25, 2023

Quota management MVP #395

Closed

Added explicit quota release steps for errors in ScheduleNext

986ac60

Logging improvements Additional test cases in quotaforest manager

z103cb changed the title ~~Error handling updates in quite management initialisation~~ Error handling updates in quota management usage May 26, 2023

z103cb commented May 26, 2023

View reviewed changes

z103cb added 2 commits May 26, 2023 15:26

Added sleep to allow for the test namespace to be cleaned up.

cf60966

Small changes to unit test

b32182c

asm582 reviewed May 26, 2023

View reviewed changes

asm582 approved these changes May 26, 2023

View reviewed changes

asm582 merged commit 7a5b67e into main May 26, 2023

asm582 deleted the hotfix_qm_build branch July 6, 2023 00:32

Error handling updates in quota management usage #367

Error handling updates in quota management usage #367

Uh oh!

Conversation

z103cb commented May 15, 2023

Uh oh!

asm582 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asm582 commented May 17, 2023

Uh oh!

KPostOffice commented May 17, 2023

Uh oh!

Uh oh!

z103cb commented May 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asm582 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asm582 left a comment

Choose a reason for hiding this comment

Uh oh!

asm582 commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

asm582 commented May 26, 2023 •

edited

Loading