From a785076ef74792a88c5eaa9b38273fdfdeabd166 Mon Sep 17 00:00:00 2001 From: Abu Kashem Date: Thu, 5 Sep 2024 11:57:47 -0400 Subject: [PATCH 1/2] apiserver: fix apf watch test the assert to verify that 'atomicReadOnlyExecuting' is zero should be executed if the apf handler panics, all apf bookkeeping must be completed before the handler returns Kubernetes-commit: 0c8632de57075191e6c4e34897fb7871034c7081 --- .../filters/priority-and-fairness_test.go | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/pkg/server/filters/priority-and-fairness_test.go b/pkg/server/filters/priority-and-fairness_test.go index ad3ac780b..55ef87fdf 100644 --- a/pkg/server/filters/priority-and-fairness_test.go +++ b/pkg/server/filters/priority-and-fairness_test.go @@ -177,11 +177,21 @@ func newApfHandlerWithFilter(t *testing.T, flowControlFilter utilflowcontrol.Int r = r.WithContext(apirequest.WithUser(r.Context(), &user.DefaultInfo{ Groups: []string{user.AllUnauthenticated}, })) - apfHandler.ServeHTTP(w, r) - postExecute() - if atomicReadOnlyExecuting != 0 { - t.Errorf("Wanted %d requests executing, got %d", 0, atomicReadOnlyExecuting) - } + func() { + // the APF handler completes its run, either normally or + // with a panic, in either case, all APF book keeping must + // be completed by now. Also, whether the request is + // executed or rejected, we expect the counter to be zero. + // TODO: all test(s) using this filter must run + // serially to each other + defer func() { + if atomicReadOnlyExecuting != 0 { + t.Errorf("Wanted %d requests executing, got %d", 0, atomicReadOnlyExecuting) + } + }() + apfHandler.ServeHTTP(w, r) + postExecute() + }() }), requestInfoFactory) return handler From 4483cc97cfea68248785f404d8c06f07ce8b55b5 Mon Sep 17 00:00:00 2001 From: Abu Kashem Date: Thu, 5 Sep 2024 12:01:36 -0400 Subject: [PATCH 2/2] apiserver: all bookkeeping must complete before apf handler returns all bookkeeping must complete before the apf handler returns, whether it panics or returns normally Kubernetes-commit: 71d9307eaeda86d6a205548ecdeb7fbf226d7d82 --- pkg/server/filters/priority-and-fairness.go | 28 +++++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/pkg/server/filters/priority-and-fairness.go b/pkg/server/filters/priority-and-fairness.go index 1e91b9a31..decd9d6ca 100644 --- a/pkg/server/filters/priority-and-fairness.go +++ b/pkg/server/filters/priority-and-fairness.go @@ -266,17 +266,23 @@ func (h *priorityAndFairnessHandler) Handle(w http.ResponseWriter, r *http.Reque select { case <-shouldStartWatchCh: - watchCtx := utilflowcontrol.WithInitializationSignal(ctx, watchInitializationSignal) - watchReq = r.WithContext(watchCtx) - h.handler.ServeHTTP(w, watchReq) - // Protect from the situation when request will not reach storage layer - // and the initialization signal will not be send. - // It has to happen before waiting on the resultCh below. - watchInitializationSignal.Signal() - // TODO: Consider finishing the request as soon as Handle call panics. - if err := <-resultCh; err != nil { - panic(err) - } + func() { + // TODO: if both goroutines panic, propagate the stack traces from both + // goroutines so they are logged properly: + defer func() { + // Protect from the situation when request will not reach storage layer + // and the initialization signal will not be send. + // It has to happen before waiting on the resultCh below. + watchInitializationSignal.Signal() + // TODO: Consider finishing the request as soon as Handle call panics. + if err := <-resultCh; err != nil { + panic(err) + } + }() + watchCtx := utilflowcontrol.WithInitializationSignal(ctx, watchInitializationSignal) + watchReq = r.WithContext(watchCtx) + h.handler.ServeHTTP(w, watchReq) + }() case err := <-resultCh: if err != nil { panic(err)