Added benchmarks for pod affinity NamespaceSelector #101329

ahg-g · 2021-04-21T16:23:54Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds two operations to scheduler perf benchmarks integration test: 1) create namespace, 2) create multiple sets of pods.

Those were necessary to create pod (anti)affinity benchmarks with NamespaceSelector

The benchmark results are in the following file: BenchmarkPerfScheduling.txt

The comparison is against the existing affinity benchmarks. The current affinity benchmarks put all existing pods in one namespace, the new ones split them across 100 namespaces and use namespace selector, the results show that there is no performance drop.

Which issue(s) this PR fixes:

Part of kubernetes/enhancements#2249 #97203

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2021-04-21T16:24:02Z

@ahg-g: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-04-21T16:30:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g

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:

~~test/integration/scheduler_perf/OWNERS~~ [ahg-g]

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

ahg-g · 2021-04-21T16:44:00Z

/cc @adtac

ahg-g · 2021-04-21T17:09:51Z

@alculquicondor @Huang-Wei this is needed for beta graduation.

alculquicondor

/sig scheduling

alculquicondor · 2021-04-21T20:59:58Z

test/integration/scheduler_perf/scheduler_perf_test.go

+			}
+		}
+		if err != nil {
+			klog.Fatalf("Creating namespace: %v", err)


better not use klog.Fatal in a test

we should return here, updated.

alculquicondor · 2021-04-21T21:00:58Z

test/integration/scheduler_perf/scheduler_perf_test.go

@@ -681,6 +802,7 @@ func createPods(namespace string, cpo *createPodsOp, clientset clientset.Interfa
 	if err != nil {
 		return err
 	}
+	klog.Infof("Creating %d pods in namespace %q", cpo.Count, namespace)


b.Info is easier to deal with on debugging tools

the logs get truncated, not sure if there is an option to prevent that.

alculquicondor · 2021-04-21T21:02:10Z

test/integration/scheduler_perf/scheduler_perf_test.go

+				b.Fatalf("op %d: %v", opIndex, err)
+			}
+			if err := nsPreparer.prepare(); err != nil {
+				b.Fatalf("op %d: %v", opIndex, err)


what if some namespaces were successfully created?

not sure I get what is the concern, this is a fatal error, so the whole the test case will fail

Is there anything else clearing the namespaces? Isn't the etcd db shared for the entire test suite?

ah, ok, I added a call to cleanup()

Huang-Wei

Some nits below.

One q: are you going to compose a baseline test to compare the results? For example, create $initNamespaces namespaces, and run workloads specifying spec.affinity...namespaces.

Huang-Wei · 2021-04-22T05:20:35Z

test/integration/scheduler_perf/config/performance-config.yaml

+      measurePods: 1000
+
+
+- name: SchedulingPreferredAffinityWithNSSelector


Duplicated with L553?

duplicates should be L627 and L553 instead of here.

yup, removed the duplicate.

test/integration/scheduler_perf/config/performance-config.yaml

Huang-Wei · 2021-04-22T05:53:04Z

test/integration/scheduler_perf/scheduler_perf_test.go

+	}
+	klog.Infof("Making %d namespaces with prefix %q and template %v", p.count, p.prefix, *base)
+
+	retries := 5


You may wrap this by reusing:

import "k8s.io/client-go/util/retry" retry.RetryOnConflict(retry.DefaultRetry, fn)

chendave · 2021-04-22T10:02:51Z

test/integration/scheduler_perf/config/performance-config.yaml

+      measurePods: 1000
+
+
+- name: SchedulingPreferredAffinityWithNSSelector


duplicates should be L627 and L553 instead of here.

test/integration/scheduler_perf/config/performance-config.yaml

chendave · 2021-04-22T10:25:21Z

test/integration/scheduler_perf/scheduler_perf_test.go

+	// Number of namespaces to create. Parameterizable through CountParam.
+	Count int
+	// Template parameter for Count.
+	CountParam string


Both "Count" and "CountParam" are semantically identical, possible to just use one instead, maybe if the "CountParam" is not set it could be parsed as "1" for the measured namespace?

This is an established pattern across all operations.

chendave · 2021-04-22T10:28:33Z

test/integration/scheduler_perf/config/performance-config.yaml

+    namespaceTemplatePath: config/namespace-with-labels.yaml
+  - opcode: createNamespaces
+    prefix: measure-ns
+    count: 1


or maybe something like this?

countParam: $measureNamespaces

the parameter is not reused across workloads, we want to explicitly use a single namespace, hence its hardcoded.

chendave · 2021-04-22T10:30:54Z

test/integration/scheduler_perf/scheduler_perf_test.go

+func (cpso createPodSetsOp) patchParams(w *workload) (realOp, error) {
+	if cpso.CountParam != "" {
+		var ok bool
+		if cpso.Count, ok = w.Params[cpso.CountParam[1:]]; !ok {


consider the case that both "cpso.CountParam" and "cpso.Count" are set in the template.

this is an established pattern in the file, CountParam takes precedence, I added a comment.

test/integration/scheduler_perf/scheduler_perf_test.go

alculquicondor · 2021-04-22T15:32:33Z

LGTM for me after squash

ahg-g · 2021-04-22T16:22:59Z

Some nits below.

One q: are you going to compose a baseline test to compare the results? For example, create $initNamespaces namespaces, and run workloads specifying spec.affinity...namespaces.

The comparison is against the existing affinity benchmarks. The current benchmarks put all existing pods in one namespace, the new ones split them across 100 namespaces and use namespace selector, I am showing that there is no performance drop.

Huang-Wei · 2021-04-22T16:26:46Z

The comparison is against the existing affinity benchmarks. The current benchmarks put all existing pods in one namespace, the new ones split them across 100 namespaces and use namespace selector, I am showing that there is no performance drop.

Sounds good.

ahg-g · 2021-04-22T16:50:06Z

commits squashed and I updated the PR description with the results.

alculquicondor · 2021-04-22T16:51:31Z

/lgtm

/hold
for others

ahg-g · 2021-04-22T18:23:15Z

/retest

ahg-g · 2021-04-22T19:00:57Z

/retest

Huang-Wei · 2021-04-23T16:35:14Z

test/integration/scheduler_perf/scheduler_perf_test.go

+	for i := 0; i < p.count; i++ {
+		n := base.DeepCopy()
+		n.Name = fmt.Sprintf("%s-%d", p.prefix, i)
+		testutils.RetryWithExponentialBackOff(func() (bool, error) {


It seems the returned error is discarded. IMO we should abort the loop to return the (timeout) error? In current logic, prepare() always return nil.

if the function returns an error, RetryWithExponentialBackOff will directly return and not retry. Ideally there should be a way to check if the error is not retry-able and only in that case return an error.

all the functions in

kubernetes/test/utils/create_resources.go

Line 56 in 2115852

func CreatePodWithRetries(c clientset.Interface, namespace string, obj *v1.Pod) error {

are actually not doing any retries on errors. It is because of that the benchmark was sometimes failing.

if the function returns an error, RetryWithExponentialBackOff will directly return and not retry

True, but the inner function doesn't return any error, right? So the only non-nil error we may get from testutils.RetryWithExponentialBackOff is timeout error, and in that case, should we abort the test?

are actually not doing any retries on errors

yes, the namings (CreatePodWithRetries and others) are confusing and I proposed #100688.

True, but the inner function doesn't return any error, right?
you mean line 1038 below? correct. I am simplifying things here and I am assuming that all errors are retry-able because we don't have a method that tells us whether the error is retry-able (in which case we would return nil) or not retry-able (in which case we would return the error)

updated to capture the error returned by RetryWithExponentialBackOff and return it.

test/integration/scheduler_perf/scheduler_perf_test.go

Huang-Wei · 2021-04-23T18:17:06Z

/lgtm

ahg-g · 2021-04-23T18:59:57Z

/retest

ahg-g · 2021-04-26T01:08:51Z

/hold cancel

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 21, 2021

ahg-g mentioned this pull request Apr 21, 2021

Add NamespaceSelector to pod affinity spec #97203

Closed

6 tasks

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 21, 2021

k8s-ci-robot requested review from chendave and SataQiu April 21, 2021 16:24

ahg-g force-pushed the ahg-nss-bench branch from a20d11c to 0e34d4e Compare April 21, 2021 16:27

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 21, 2021

k8s-ci-robot requested a review from adtac April 21, 2021 16:44

alculquicondor reviewed Apr 21, 2021

View reviewed changes

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Apr 21, 2021

ahg-g force-pushed the ahg-nss-bench branch from 20c4c78 to d06cb68 Compare April 21, 2021 21:41

Huang-Wei reviewed Apr 22, 2021

View reviewed changes

chendave reviewed Apr 22, 2021

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 22, 2021

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

k8s-ci-robot assigned alculquicondor Apr 22, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2021

ahg-g force-pushed the ahg-nss-bench branch from e20ac68 to a1b66e5 Compare April 22, 2021 16:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2021

Huang-Wei reviewed Apr 23, 2021

View reviewed changes

ahg-g force-pushed the ahg-nss-bench branch from a1b66e5 to aa1679c Compare April 23, 2021 16:41

Added benchmarks for pod affinity namespaceselector

6988653

ahg-g force-pushed the ahg-nss-bench branch from aa1679c to 6988653 Compare April 23, 2021 18:14

k8s-ci-robot assigned Huang-Wei Apr 23, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2021

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

k8s-ci-robot merged commit 3e71ecc into kubernetes:master Apr 26, 2021

k8s-ci-robot added this to the v1.22 milestone Apr 26, 2021

ahg-g mentioned this pull request May 14, 2021

Graduate pod affinity NamespaceSelector to Beta #101496

Merged

ahg-g deleted the ahg-nss-bench branch October 25, 2021 14:39

		measurePods: 1000


		- name: SchedulingPreferredAffinityWithNSSelector

Added benchmarks for pod affinity NamespaceSelector #101329

Added benchmarks for pod affinity NamespaceSelector #101329

Conversation

ahg-g commented Apr 21, 2021 • 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.:

k8s-ci-robot commented Apr 21, 2021

k8s-ci-robot commented Apr 21, 2021

ahg-g commented Apr 21, 2021

ahg-g commented Apr 21, 2021

alculquicondor left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei left a 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

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

alculquicondor commented Apr 22, 2021

ahg-g commented Apr 22, 2021

Huang-Wei commented Apr 22, 2021

ahg-g commented Apr 22, 2021

alculquicondor commented Apr 22, 2021

ahg-g commented Apr 22, 2021

ahg-g commented Apr 22, 2021

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

Huang-Wei commented Apr 23, 2021

ahg-g commented Apr 23, 2021

ahg-g commented Apr 26, 2021

ahg-g commented Apr 21, 2021 •

edited