dra scheduler: fall back to SSA for PodSchedulingContext updates #120534

pohly · 2023-09-08T19:24:13Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

During scheduler_perf testing, roughly 10% of the PodSchedulingContext update operations failed with a conflict error. Using SSA would avoid that, but performance measurements showed that this causes a considerable slowdown (primarily because of the slower encoding with JSON instead of protobuf, but also because server-side processing is more expensive).

Therefore a normal update is tried first and SSA only gets used when there has been a conflict. Using SSA in that case instead of giving up outright is better because it avoids another scheduling attempt.

Which issue(s) this PR fixes:

Related-to: #120502

Special notes for your reviewer:

On my machine, "stress" shows that the new test is stable:

45s: 236 runs so far, 1 failures (0.42%)

That one failure seems unrelated:

        found unexpected goroutines:
        [Goroutine 5614 in state sleep, with time.Sleep on top of the stack:
        goroutine 5614 [sleep]:
        time.Sleep(0x2aa6db101)
                /nvme/gopath/go-1.21.0/src/runtime/time.go:195 +0x125
        k8s.io/client-go/tools/events.(*eventBroadcasterImpl).attemptRecording(0xc00636a630, 0xc002f5a780)
                /nvme/gopath/src/k8s.io/kubernetes/vendor/k8s.io/client-go/tools/events/event_broadcaster.go:221 +0x77
        k8s.io/client-go/tools/events.(*eventBroadcasterImpl).recordToSink.func1()
                /nvme/gopath/src/k8s.io/kubernetes/vendor/k8s.io/client-go/tools/events/event_broadcaster.go:200 +0x3a
        created by k8s.io/client-go/tools/events.(*eventBroadcasterImpl).recordToSink in goroutine 5021
                /nvme/gopath/src/k8s.io/kubernetes/vendor/k8s.io/client-go/tools/events/event_broadcaster.go:173 +0xdb
        ]
 >
FAIL    k8s.io/kubernetes/test/integration/scheduler    10.704s

This is a known issue (#115514) and it should be fixed. I double-checked that Shutdown() is called. ~~Needs to be investigated if it occurs in practice.~~ I got rid of this goroutine leak by not starting the event recorder in the first place - it's not needed.

Does this PR introduce a user-facing change?

dra: the scheduler plugin avoids additional scheduling attempts in some cases by falling back to SSA after a conflict

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/issues/3063

pohly · 2023-09-09T07:17:45Z

found unexpected goroutines:

This annoys me 😠

Perhaps it occurs more often in this new test because the scheduler keeps trying to schedule the pod right until everything is getting shut down. Let me try whether it helps to first delete the pod, give remaining events a bit time to be delivered, and then shut down.

/hold

pohly · 2023-09-11T08:04:32Z

found unexpected goroutines:

This annoys me 😠

Perhaps it occurs more often in this new test because the scheduler keeps trying to schedule the pod right until everything is getting shut down.

Fixed by not starting the problematic event sink.

/hold cancel

pohly · 2023-09-11T11:26:26Z

test/integration/scheduler/scheduler_test.go

+//   - Triggering this particular race is harder in E2E testing
+//     and harder to verify (needs apiserver metrics and there's
+//     no standard API for those).
+func TestPodSchedulingContextSSA(t *testing.T) {


Triggering the race here isn't trivial either. I'd appreciate feedback on the current approach. In parallel I'll investigate whether error injection can be used to trigger the fallback.

I've pushed a second commit that does error injection at the http.RoundTripper level.

@aojea: you've done some work on integration testing and apiserver setup. Do my changes look okay to you?

/hold

I want to squash before merging.

It can, and I prefer the new approach. I just haven't squashed yet to simplify before/after comparisons (if anyone cares).

Scratch that - I also fixes some other issues, squashed and force-pushed as a single commit.

checking now sorry, @pohly I think that you can do something like

diff --git a/test/integration/framework/test_server.go b/test/integration/framework/test_server.go index 6b15b09b2d8..d288ff73ca8 100644 --- a/test/integration/framework/test_server.go +++ b/test/integration/framework/test_server.go @@ -52,8 +52,9 @@ AwEHoUQDQgAEH6cuzP8XuD5wal6wf9M6xDljTOPLX2i8uIp/C/ASqiIGUeeKQtX0 // TestServerSetup holds configuration information for a kube-apiserver test server. type TestServerSetup struct { - ModifyServerRunOptions func(*options.ServerRunOptions) - ModifyServerConfig func(*controlplane.Config) + ModifyServerRunOptions func(*options.ServerRunOptions) + ModifyServerConfig func(*controlplane.Config) + ModifyServerClientConfig func(*rest.Config) } type TearDownFunc func() @@ -223,6 +224,7 @@ func StartTestServer(ctx context.Context, t testing.TB, setup TestServerSetup) ( t.Fatal(err) } + setup.ModifyServerClientConfig(kubeAPIServerClientConfig) kubeAPIServerClient, err := client.NewForConfig(kubeAPIServerClientConfig) if err != nil { t.Fatal(err)

and then use it like

_, kubeConfig, tearDownFn := framework.StartTestServer(ctx, t, framework.TestServerSetup{ ModifyServerRunOptions: func(opts *options.ServerRunOptions) { // Disable ServiceAccount admission plugin as we don't have serviceaccount controller running. opts.Admission.GenericAdmission.DisablePlugins = []string{"ServiceAccount", "TaintNodesByCondition"} opts.APIEnablement.RuntimeConfig.Set("networking.k8s.io/v1alpha1=true") }, ModifyServerClientConfig: func(c *clientrest.Config) { c.Wrap(func(rt http.RoundTripper) http.RoundTripper { return roundTripperFunc(func(req *http.Request) (*http.Response, error) { authorizationHeaderValues.append(req.Header.Values("Authorization")) return rt.RoundTrip(req) }) }) }, }) defer tearDownFn()

it seams cleaner and better so we can have all the client mutations sinde the constructor.

Another options is that you build the client directly

StartTestServer returns the rest.Config, is not that much easier?

Thanks for the suggestions. I removed all changes from test/integration/scheduler/scheduler_test.go.

pohly · 2023-09-11T16:37:58Z

test/integration/util/util.go

@@ -468,17 +495,23 @@ func UpdateNodeStatus(cs clientset.Interface, node *v1.Node) error {
 func InitTestAPIServer(t *testing.T, nsPrefix string, admission admission.Interface) *TestContext {
 	_, ctx := ktesting.NewTestContext(t)
 	ctx, cancel := context.WithCancel(ctx)
-	testCtx := TestContext{Ctx: ctx}
+	testCtx := &TestContext{Ctx: ctx}


My goal was to not change the prototype of InitTestAPIServer.

Therefore the wrapper can be set after the TestContext and all servers have been created.

aojea · 2023-09-11T18:21:52Z

/assign @aojea

sanposhiho · 2023-09-12T02:34:04Z

/assign

aojea · 2023-09-12T09:18:22Z

test/integration/framework/test_server.go

+
+	// RoundTrip, if set, will be called for every HTTP request going to the apiserver.
+	// It can be used for error injection.
+	RoundTrip func(transport http.RoundTripper, req *http.Request) (*http.Response, error)


StartTestServer(ctx context.Context, t testing.TB, setup TestServerSetup) (client.Interface, *rest.Config, TearDownFunc) {

returns a rest.Config, you can copy it and create a new clientset for that with your roundtripper

client, config, tearDownFn := framework.StartTestServer(ctx, t, framework.TestServerSetup{}) config.Wrap(func(rt http.RoundTripper) http.RoundTripper { return roundTripperFunc(func(req *http.Request) (*http.Response, error) { authorizationHeaderValues.append(req.Header.Values("Authorization")) return rt.RoundTrip(req) }) }) newClient, err := client.NewForConfig(config) if err != nil { t.Fatal(err)

I hadn't found rest.Config.Wrap. That's indeed simpler and make it possible to limit the changes to just test/integration/util/util.go. I've switched to that - please take another look.

During scheduler_perf testing, roughly 10% of the PodSchedulingContext update operations failed with a conflict error. Using SSA would avoid that, but performance measurements showed that this causes a considerable slowdown (primarily because of the slower encoding with JSON instead of protobuf, but also because server-side processing is more expensive). Therefore a normal update is tried first and SSA only gets used when there has been a conflict. Using SSA in that case instead of giving up outright is better because it avoids another scheduling attempt.

pohly · 2023-09-15T13:53:22Z

/retest

pohly · 2023-09-15T15:37:09Z

/retest

k8s-ci-robot · 2023-09-18T04:35:31Z

LGTM label has been added.

Git tree hash: 161c352c8fbf6248f634fb57cf0fbaffff45b7ac

alculquicondor · 2023-09-18T19:32:47Z

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go

@@ -187,6 +188,41 @@ func (p *podSchedulingState) publish(ctx context.Context, pod *v1.Pod, clientset
 			logger.V(5).Info("Updating PodSchedulingContext", "podSchedulingCtx", klog.KObj(schedulingCtx))
 		}
 		_, err = clientset.ResourceV1alpha2().PodSchedulingContexts(schedulingCtx.Namespace).Update(ctx, schedulingCtx, metav1.UpdateOptions{})


Have you considered a regular patch? Is that any faster than SSA?

I haven't because the guidance is to use SSA. I've shared CPU and memory profiles with @apelisse, he is looking into the performance aspect.

alculquicondor · 2023-09-18T19:47:25Z

Overall, I'm ok with this change, but I wonder if somehow we can take these updates out of the critical path. Do they need to happen in the scheduling cycle?
I'm not saying that they should, but worth considering the implications, if you can.

pohly · 2023-09-19T05:51:59Z

#120502 captures the discussion I had with @Huang-Wei about doing these updates in the background. The gist is that it would be possible, but it implies extending the scheduler framework in a non-trivial way because right now the plugin cannot handle errors when they occur outside of the normal callback functions - it's not clear yet how to do that.

We agreed to first do this PR and then come back to the topic if it turns out to be a problem in practice (= making it more important) and/or when there's time for further investigations.

pohly · 2023-09-28T05:48:28Z

/hold cancel

The hold was for squashing, which was done.

I chatted again with @apelisse about whether it would make sense to wait for SSA performance enhancements and he said that those will take more time. Let's move ahead with this PR as an interim solution.

@alculquicondor : okay for approval?

alculquicondor · 2023-09-28T16:43:48Z

Was there any conclusion about moving these calls out of Reserve?

pohly · 2023-09-28T17:41:01Z

The conclusion was "would be nice, but not absolutely required for beta" (but I intend to work on it anyway) and "not something that needs to be solved in this PR".

alculquicondor · 2023-09-28T18:23:29Z

I need to look at the KEP, but I think whatever we can do to avoid impacting workloads that don't use DRA should be high in the priority list or even be a blocker for beta.

Huang-Wei

/lgtm
/approve

k8s-ci-robot · 2023-10-23T17:24:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/scheduler/OWNERS~~ [Huang-Wei]
~~test/OWNERS~~ [pohly]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested a review from bart0sh September 8, 2023 19:29

k8s-ci-robot added the area/test label Sep 8, 2023

k8s-ci-robot requested a review from denkensk September 8, 2023 19:29

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2023

pohly mentioned this pull request Sep 9, 2023

[discussion] DRA plugin raises UPDATE API request in scheduling cycle #120502

Closed

pohly force-pushed the dra-scheduler-ssa-as-fallback branch from a3727c7 to 7b2ab3d Compare September 9, 2023 18:50

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2023

pohly commented Sep 11, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2023

bart0sh added this to Triage in SIG Node PR Triage Sep 11, 2023

pohly commented Sep 11, 2023

View reviewed changes

k8s-ci-robot assigned aojea Sep 11, 2023

pohly force-pushed the dra-scheduler-ssa-as-fallback branch from 4472576 to 5a9802d Compare September 11, 2023 18:40

k8s-ci-robot assigned sanposhiho Sep 12, 2023

aojea reviewed Sep 12, 2023

View reviewed changes

bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Sep 15, 2023

pohly force-pushed the dra-scheduler-ssa-as-fallback branch from 4f2471f to 7cac1dc Compare September 15, 2023 13:05

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2023

k8s-ci-robot requested review from aojea and sanposhiho September 15, 2023 13:05

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2023

alculquicondor reviewed Sep 18, 2023

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2023

pacoxu moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Oct 16, 2023

Huang-Wei approved these changes Oct 23, 2023

View reviewed changes

k8s-ci-robot assigned Huang-Wei Oct 23, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2023

k8s-ci-robot merged commit 5a4e792 into kubernetes:master Oct 23, 2023
18 checks passed

SIG Node PR Triage automation moved this from Needs Approver to Done Oct 23, 2023

k8s-ci-robot added this to the v1.29 milestone Oct 23, 2023

pohly mentioned this pull request Nov 6, 2023

dynamic resource allocation kubernetes/enhancements#3063

Open

34 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dra scheduler: fall back to SSA for PodSchedulingContext updates #120534

dra scheduler: fall back to SSA for PodSchedulingContext updates #120534

pohly commented Sep 8, 2023 •

edited

pohly commented Sep 9, 2023

pohly commented Sep 11, 2023

pohly Sep 11, 2023

pohly Sep 11, 2023

pohly Sep 11, 2023

pohly Sep 11, 2023

aojea Sep 12, 2023 •

edited

pohly Sep 12, 2023

pohly Sep 11, 2023

aojea commented Sep 11, 2023

sanposhiho commented Sep 12, 2023

aojea Sep 12, 2023

pohly Sep 12, 2023

pohly commented Sep 15, 2023

pohly commented Sep 15, 2023

k8s-ci-robot commented Sep 18, 2023

alculquicondor Sep 18, 2023

pohly Sep 19, 2023

alculquicondor commented Sep 18, 2023

pohly commented Sep 19, 2023

pohly commented Sep 28, 2023 •

edited

alculquicondor commented Sep 28, 2023

pohly commented Sep 28, 2023

alculquicondor commented Sep 28, 2023

Huang-Wei left a comment

k8s-ci-robot commented Oct 23, 2023

dra scheduler: fall back to SSA for PodSchedulingContext updates #120534

dra scheduler: fall back to SSA for PodSchedulingContext updates #120534

Conversation

pohly commented Sep 8, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

pohly commented Sep 9, 2023

pohly commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea Sep 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Sep 11, 2023

sanposhiho commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Sep 15, 2023

pohly commented Sep 15, 2023

k8s-ci-robot commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Sep 18, 2023

pohly commented Sep 19, 2023

pohly commented Sep 28, 2023 • edited

alculquicondor commented Sep 28, 2023

pohly commented Sep 28, 2023

alculquicondor commented Sep 28, 2023

Huang-Wei left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 23, 2023

pohly commented Sep 8, 2023 •

edited

aojea Sep 12, 2023 •

edited

pohly commented Sep 28, 2023 •

edited