scheduler integration benchmark improvements #107677

pohly · 2022-01-20T20:50:32Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

I was running the test/integration/scheduler_perf and encountered various issues in it and some of the scheduler code. See the individual commit messages for details.

Special notes for your reviewer:

I still have problems running all benchmarks in a single "go test" invocation as discussed in https://kubernetes.slack.com/archives/C09TP78DV/p1642623117030500 but this PR helps a bit.

Does this PR introduce a user-facing change?

NONE

alculquicondor · 2022-01-21T14:56:07Z

/assign @sanposhiho
for a first pass for sig-scheduling

pohly · 2022-01-21T15:41:10Z

/retest

pkg/scheduler/framework/plugins/interpodaffinity/plugin.go

alculquicondor · 2022-01-24T16:02:21Z

/approve

pohly · 2022-01-25T19:38:06Z

I've rebased due to a conflict.

Can I get this reviewed? @alculquicondor, perhaps you can start for SIG Scheduling?

alculquicondor · 2022-01-25T20:15:44Z

I already reviewed and only left 1 comment.

But lgtm anyways

/lgtm

pohly · 2022-01-28T07:51:49Z

/retest

oomichi · 2022-02-03T00:31:27Z

./test part seems good for me.

/lgtm
/approve

sttts · 2022-02-04T13:35:26Z

pkg/controlplane/controller.go

-	if err := c.EndpointReconciler.RemoveEndpoints(kubernetesServiceName, c.PublicIP, endpointPorts); err == nil {
-		klog.Error("Found stale data, removed previous endpoints on kubernetes service, apiserver didn't exit successfully previously")
-	} else if !storage.IsNotFound(err) {
+	if err := c.EndpointReconciler.RemoveEndpoints(kubernetesServiceName, c.PublicIP, endpointPorts); err != nil && !storage.IsNotFound(err) {


@aojea wasn't that your code?

Yeah, why do we need to change it?
#105531

See the commit message: be4bf5c

@aojea : does that explain it?

heh, I understand now, this thing bit me twice this week
The test is using the masterCount reconciler, not the lease reconciler. The lease reconciler is the default ...
The integration framework should use the lease reconciler ... I think I will write something to try to deprecate the masterCount reconciler, it is flaky :(

kubernetes/cmd/kube-apiserver/app/options/options.go

Line 114 in 76cdb57

EndpointReconcilerType: string(reconcilers.LeaseEndpointReconcilerType),

ok, found the inconsistency

diff --git a/pkg/controlplane/instance.go b/pkg/controlplane/instance.go index 424cd0ce54b..6426038e3b2 100644 --- a/pkg/controlplane/instance.go +++ b/pkg/controlplane/instance.go @@ -274,9 +274,9 @@ func (c *Config) createEndpointReconciler() reconcilers.EndpointReconciler { klog.Infof("Using reconciler: %v", c.ExtraConfig.EndpointReconcilerType) switch c.ExtraConfig.EndpointReconcilerType { // there are numerous test dependencies that depend on a default controller - case "", reconcilers.MasterCountReconcilerType: + case reconcilers.MasterCountReconcilerType: return c.createMasterCountReconciler() - case reconcilers.LeaseEndpointReconcilerType: + case "", reconcilers.LeaseEndpointReconcilerType: return c.createLeaseReconciler() case reconcilers.NoneEndpointReconcilerType: return c.createNoneReconciler()

I think we should keep the code and deprecate the master-count
#107952

As @aojea is going to address the false error message, I took this commit out of this PR.

The PR was closed without a conclusion, but the issue about the error message is still open: #107953

sttts · 2022-02-04T13:37:04Z

test/integration/framework/controlplane_utils.go

+	// line. k8s.io/component-base/logs called klog.InitFlags for us.
+	v := flag.Lookup("v").Value
+	v.Set("4")
+}


why? This is affecting everybody who imports. Before it was just effective if you created an apiserver.

The "why" is that the previous approach didn't work for -v0 on the command line, which is relevant for benchmarks.

I was assuming that setting -v4 as default would be desirable for all integration tests. If that is not the case, then startAPIServerOrDie must allow controlling whether it raises the verbosity.

Would controlplane.Config be a suitable place for that?

Would controlplane.Config be a suitable place for that?

Hmm, probably not.

Simpler than an explicit parameter would be a global variable in controlplane_utils.go:

// MinVerbosity determines the minimum klog verbosity when running tests that involve the apiserver. // This overrides the -v value from the command line, i.e. -v0 has no effect when MinVerbosity is 4 (the default). // Tests can opt out of this by setting MinVerbosity to zero before starting the control plane or // choose some different minimum verbosity. var MinVerbosity = 4

@dims: I think you added that code originally. Any thoughts?

I replaced the init function with the global variable.

GetNamespaceLabelsSnapshot has a fallback when it gets errors when looking up a namespace, therefore reporting the error is more informational than a real error. In particular, not finding the namespace is normal when running test/integration/scheduler_perf and happens so frequently that there is a lot of output on stderr: E0120 12:19:09.204768 95305 plugin.go:138] "getting namespace, assuming empty set of namespace labels" err="namespace \"namespace-1\" not found" namespace="namespace-1"

When running an integration test that measures performance, like for example test/integration/scheduler_perf, running etcd with debug level output is undesirable because it creates additional load on the system and isn't realistic. The default is still "debug", but ETCD_LOGLEVEL=warn can be used to override that.

The output redirection in the info message did not match the actual invocation (stdout and stderr swapped).

"-bench=PerfScheduling/Preemption/500Nodes" ran both the PerfScheduling/Preemption/500Nodes and the PerfScheduling/PreemptionPVs/500Nodes benchmark. This can be avoided by choosing names where none is the prefix of another.

Occasionally, writing as JSON failed because a NaN float couldn't be encoded. The extended log message helps understand where that comes from, for example: F0120 20:24:45.515745 511835 scheduler_perf_test.go:540] BenchmarkPerfScheduling: unable to write measured data {Version:v1 DataItems:[{Data:map[Average:35.714285714285715 Perc50:2 Perc90:36 Perc95:412 Perc99:412] Unit:pods/s Labels:map[Metric:SchedulingThroughput Name:BenchmarkPerfScheduling/PreemptionPVs/500Nodes/namespace-2]} {Data:map[Average:27.863967530999993 Perc50:13.925925925925926 Perc90:30.06711409395973 Perc95:31.85682326621924 Perc99:704] Unit:ms Labels:map[Metric:scheduler_e2e_scheduling_duration_seconds Name:BenchmarkPerfScheduling/PreemptionPVs/500Nodes/namespace-2]} {Data:map[Average:11915.651577744 Perc50:15168.796680497926 Perc90:19417.759336099585 Perc95:19948.87966804979 Perc99:20373.77593360996] Unit:ms Labels:map[Metric:scheduler_pod_scheduling_duration_seconds Name:BenchmarkPerfScheduling/PreemptionPVs/500Nodes/namespace-2]} {Data:map[Average:1.1865832049999983 Perc50:0.7636363636363637 Perc90:2.891903719912473 Perc95:3.066958424507659 Perc99:5.333333333333334] Unit:ms Labels:map[Metric:scheduler_framework_extension_point_duration_seconds Name:BenchmarkPerfScheduling/PreemptionPVs/500Nodes/namespace-2 extension_point:Filter]} {Data:map[Average:NaN Perc50:NaN Perc90:NaN Perc95:NaN Perc99:NaN] Unit:ms Labels:map[Metric:scheduler_framework_extension_point_duration_seconds Name:BenchmarkPerfScheduling/PreemptionPVs/500Nodes/namespace-2 extension_point:Score]}]}: json: unsupported value: NaN

pohly · 2022-02-07T08:18:49Z

@sttts : can you take another look?

pohly · 2022-02-08T21:31:32Z

/retest

sttts · 2022-02-11T11:27:19Z

Change looks good now from apiserver point of view.

@alculquicondor @oomichi can you re-lgtm?

test/integration/scheduler_perf/util.go

test/integration/framework/controlplane_utils.go

This provides a mechanism for overriding the forced increase of the klog verbosity to 4 when starting the apiserver and uses that for the scheduler_perf benchmark. Other tests run as before. A global variable was used because adding an explicit parameter to several helper functions would have caused a lot of code churn (test -> integration/util.StartApiserver -> integration/framework.RunAnAPIServerUsingServer -> integration/framework.startAPIServerOrDie).

alculquicondor · 2022-02-11T17:08:27Z

/lgtm

pohly · 2022-02-12T15:52:26Z

/retest

sttts · 2022-02-14T08:20:55Z

/approve

k8s-ci-robot · 2022-02-14T08:21:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, oomichi, pohly, sttts

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:

~~hack/OWNERS~~ [sttts]
~~pkg/scheduler/OWNERS~~ [alculquicondor]
~~test/OWNERS~~ [oomichi,sttts]

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 review from chendave and dchen1107 January 20, 2022 20:50

k8s-ci-robot assigned sanposhiho Jan 21, 2022

pohly force-pushed the scheduler-integration-benchmark branch from cb5302b to 1a57e1a Compare January 21, 2022 16:42

sanposhiho mentioned this pull request Jan 22, 2022

Create namespace for Pod not to occur error log of namespace not-found #107700

Merged

sanposhiho reviewed Jan 22, 2022

View reviewed changes

pkg/scheduler/framework/plugins/interpodaffinity/plugin.go Show resolved Hide resolved

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 22, 2022

pohly mentioned this pull request Jan 23, 2022

KEP-3077: contextual logging kubernetes/enhancements#3078

Merged

pohly force-pushed the scheduler-integration-benchmark branch from b4d4076 to 07bdfc7 Compare January 25, 2022 19:33

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2022

k8s-ci-robot assigned alculquicondor Jan 25, 2022

k8s-ci-robot assigned oomichi Feb 3, 2022

sttts reviewed Feb 4, 2022

View reviewed changes

pohly mentioned this pull request Feb 4, 2022

apiserver: use endpoint lease reconciler as default #107952

Closed

aojea mentioned this pull request Feb 4, 2022

apiserver logs error "Unable to remove endpoints from kubernetes" at start #107953

Closed

pohly added 5 commits February 7, 2022 08:59

test: fix info message for etcd startup

535d885

The output redirection in the info message did not match the actual invocation (stdout and stderr swapped).

scheduler_perf: avoid ambiguous test names

8d44b81

"-bench=PerfScheduling/Preemption/500Nodes" ran both the PerfScheduling/Preemption/500Nodes and the PerfScheduling/PreemptionPVs/500Nodes benchmark. This can be avoided by choosing names where none is the prefix of another.

pohly force-pushed the scheduler-integration-benchmark branch from 0d7bcb3 to 26dac27 Compare February 7, 2022 08:16

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2022

alculquicondor reviewed Feb 11, 2022

View reviewed changes

test/integration/scheduler_perf/util.go Show resolved Hide resolved

pohly force-pushed the scheduler-integration-benchmark branch from 26dac27 to 770b2b4 Compare February 11, 2022 15:19

alculquicondor reviewed Feb 11, 2022

View reviewed changes

test/integration/framework/controlplane_utils.go Outdated Show resolved Hide resolved

pohly force-pushed the scheduler-integration-benchmark branch from 770b2b4 to e1e84c8 Compare February 11, 2022 15:58

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2022

k8s-ci-robot merged commit 21c0f6f into kubernetes:master Feb 14, 2022

k8s-ci-robot added this to the v1.24 milestone Feb 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler integration benchmark improvements #107677

scheduler integration benchmark improvements #107677

pohly commented Jan 20, 2022

alculquicondor commented Jan 21, 2022 •

edited

pohly commented Jan 21, 2022

alculquicondor commented Jan 24, 2022

pohly commented Jan 25, 2022

alculquicondor commented Jan 25, 2022

pohly commented Jan 28, 2022

oomichi commented Feb 3, 2022

sttts Feb 4, 2022

aojea Feb 4, 2022

pohly Feb 4, 2022

pohly Feb 4, 2022

aojea Feb 4, 2022

aojea Feb 4, 2022

aojea Feb 4, 2022 •

edited

aojea Feb 4, 2022

pohly Feb 7, 2022

pohly Feb 11, 2022

sttts Feb 4, 2022

pohly Feb 4, 2022

pohly Feb 4, 2022

pohly Feb 4, 2022 •

edited

pohly Feb 7, 2022 •

edited

pohly commented Feb 7, 2022

pohly commented Feb 8, 2022

sttts commented Feb 11, 2022

alculquicondor commented Feb 11, 2022

pohly commented Feb 12, 2022

sttts commented Feb 14, 2022

k8s-ci-robot commented Feb 14, 2022

scheduler integration benchmark improvements #107677

scheduler integration benchmark improvements #107677

Conversation

pohly commented Jan 20, 2022

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

alculquicondor commented Jan 21, 2022 • edited

pohly commented Jan 21, 2022

alculquicondor commented Jan 24, 2022

pohly commented Jan 25, 2022

alculquicondor commented Jan 25, 2022

pohly commented Jan 28, 2022

oomichi commented Feb 3, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea Feb 4, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly Feb 4, 2022 • edited

Choose a reason for hiding this comment

pohly Feb 7, 2022 • edited

Choose a reason for hiding this comment

pohly commented Feb 7, 2022

pohly commented Feb 8, 2022

sttts commented Feb 11, 2022

alculquicondor commented Feb 11, 2022

pohly commented Feb 12, 2022

sttts commented Feb 14, 2022

k8s-ci-robot commented Feb 14, 2022

alculquicondor commented Jan 21, 2022 •

edited

aojea Feb 4, 2022 •

edited

pohly Feb 4, 2022 •

edited

pohly Feb 7, 2022 •

edited