New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance tests and fix for IPAM controller. #61143

Merged
merged 1 commit into from Mar 27, 2018

Conversation

Projects
None yet
7 participants
@satyasm
Contributor

satyasm commented Mar 13, 2018

Tests the four modes of allocations. Can be run using
./test-performance.sh under tests/integration/ipamperf
directory. See ./test-performance.sh -h for supported flags.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
Please see the implementation notes comment block in cloud.go for core details of how
the mocking works. README.md has details on how the tests can be run on the
command line.

Release note:

Performance test framework and basic tests for the IPAM controller, to simulate behavior
of the four supported modes under lightly loaded and loaded conditions, where load is 
defined as the number of operations to perform as against the configured kubernetes
API server QPS.
@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 13, 2018

/assign @bowei

@MrHohn

This comment has been minimized.

Member

MrHohn commented Mar 14, 2018

/ok-to-test

@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 14, 2018

/retest

@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 14, 2018

/cc @shyamjvs in case you want to compare with the previous server mock. The differences are primarily in cloud.go (just about everything else is the same).

@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 14, 2018

/retest

clusterCIDR *net.IPNet
subnetMaskSize int
cidrSet *cidrset.CidrSet
lock sync.Mutex

This comment has been minimized.

@bowei

bowei Mar 20, 2018

Member

does lock protect just instances or more? document and separate in a separate block if so

...
cidrSet *cidrset.CidrSet

lock          sync.Mutex
instances map[meta.Key]*baseInstance
}
func runTest(t *testing.T, apiURL string, config *Config, clusterCIDR, serviceCIDR *net.IPNet, subnetMaskSize int) (*Results, error) {
glog.Infof("Running test %s", t.Name())

This comment has been minimized.

@bowei

bowei Mar 20, 2018

Member

is t.Helper() appropriate here?

defer shutdownFunc()
o := NewObserver(clientSet, config.NumNodes)
err = o.StartObserving()

This comment has been minimized.

@bowei

bowei Mar 20, 2018

Member

if err := .oStartObserving(); err != nil { ...

t.Fatalf("Could not start test observer: %v", err)
}
err = createNodes(apiURL, config)

This comment has been minimized.

@bowei

bowei Mar 20, 2018

Member

if err := ...; err != nil {

apiURL, masterShutdown := util.StartApiserver()
defer masterShutdown()
_, clusterCIDR, _ := net.ParseCIDR("10.96.0.0/11")

This comment has been minimized.

@bowei

bowei Mar 20, 2018

Member

Can you document why these were chosen?

set -o pipefail
TEST_ARGS=""
RUN_PATTERN="."

This comment has been minimized.

@bowei

bowei Mar 20, 2018

Member

Isn't it ".*"

TEST_ARGS=""
RUN_PATTERN="."
function ::usage() {

This comment has been minimized.

@bowei

bowei Mar 20, 2018

Member

why ::usage() vs cleanup() below?

go test -c -o "perf.test"
kube::log::status "performance test (benchmark) start"
"./perf.test" -test.bench=. -test.run=xxxx -test.cpuprofile=prof.out -test.short=false

This comment has been minimized.

@bowei

bowei Mar 20, 2018

Member

Why does "./perf.test" need to be quoted?

test.run=xxxx?

prof.out as a argument to the script so people know about it?

kube::log::status "performance test (IPAM) start"
set -x
go test -test.run=${RUN_PATTERN} -test.timeout=60m -test.short=false -v -args ${TEST_ARGS}
set +x

This comment has been minimized.

@bowei

bowei Mar 20, 2018

Member

Is this for testing (but not for final)

GenerateName: "sample-node-",
},
Spec: v1.NodeSpec{
// TODO: investigate why this is needed.

This comment has been minimized.

@bowei

bowei Mar 20, 2018

Member

is this a TODO copied from somewhere else?

@bowei

This comment has been minimized.

Member

bowei commented Mar 22, 2018

Updates?

@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 22, 2018

working on it. thanks.

Performance tests and fix for IPAM controller.
Tests the four modes of allocations. Can be run using
./test-performance.sh under tests/integration/ipamperf
directory. See ./test-performance.sh -h for supported flags.
@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 22, 2018

update with all the changes you recommended. thanks.

@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 22, 2018

/retest

3 similar comments
@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 22, 2018

/retest

@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 22, 2018

/retest

@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 23, 2018

/retest

@bowei

This comment has been minimized.

Member

bowei commented Mar 26, 2018

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 26, 2018

@bowei

This comment has been minimized.

Member

bowei commented Mar 26, 2018

/approve no-issue

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 26, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, satyasm

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:

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

@fejta-bot

This comment has been minimized.

fejta-bot commented Mar 27, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 27, 2018

/retest

1 similar comment
@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 27, 2018

/retest

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 27, 2018

Automatic merge from submit-queue (batch tested with PRs 61402, 61143, 61427, 60592). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 2bc231e into kubernetes:master Mar 27, 2018

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation satyasm authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@mindprince

This comment has been minimized.

Member

mindprince commented Mar 28, 2018

Can we revert this PR till the test flakiness is fixed?

These are currently the flakiest tests: https://k8s-testgrid.appspot.com/presubmits-kubernetes-blocking#pull-kubernetes-integration&sort-by-flakiness=

@satyasm

This comment has been minimized.

Contributor

satyasm commented Mar 28, 2018

@mindprince this PR #61863 might help. It skips this testing for short tests, which should avoid for every PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment