Integration test for Docker and perf stats #2489

iwankgb · 2020-04-10T09:33:17Z

Integration tests for Docker container and perf stats.

.github/workflows/test.yml

build/integration.sh

googlebot · 2020-04-14T18:07:22Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-04-14T18:08:28Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

iwankgb · 2020-04-14T18:14:31Z

@dashpole, you will find a proposal for extending integration tests in this PR. What changes is that integration tests are being run for "normal" build of cAdvisor (i.e. with no tags) and for a custom one (with libpfm support). I was trying to make each of the test runs independent but it seems to be impossible to use list of objects in build matrix configuration so I decided to settle with shell script based approach.
I have a question about pull-cadvisor-e2e tests: as far as I understand integrations tests are executed using test-integration make target; is this target expected to run on any distribution? To be able to execute perf-related tests I need to install an additional package and it's a distro-specific operation...

dashpole · 2020-04-14T19:11:36Z

build/integration-in-docker.sh

+CADVISOR_ARGS="$CADVISOR_ARGS" /usr/local/bin/runner.sh build/integration.sh"
+}
+
+run_tests "-race" "sudo"


Is there a reason why we want to do perf tests separately? Are they expected to fail in some scenarios?

Just read the comment above. I think this is a reasonable solution, although I think we should consider having a separate target. That way users who just want to run the "distro agnostic" tests can do so without having errors mixed in.

dashpole · 2020-04-14T19:12:21Z

integration/tests/api/perf_test.go

+
+	assert.Nil(t, err)
+	assert.Len(t, info.Stats, 1)
+	assert.Greater(t, len(info.Stats[0].PerfStats), 0, "Length of info.Stats[0].PerfStats is not greater tahn zero")


s/tahn/than

dashpole · 2020-04-14T19:13:31Z

integration/tests/api/perf_test.go

+	assert.Greater(t, len(info.Stats[0].PerfStats), 0, "Length of info.Stats[0].PerfStats is not greater tahn zero")
+	for k, stat := range info.Stats[0].PerfStats {
+		//Everything beyond name is non-deterministic unfortunately.
+		assert.Contains(t, []string{"context-switches", "cpu-migrations-custom"}, stat.Name, "Wrong metric name for key %d: %#v", k, stat)


Are we guaranteed to have more than 0 of either of these events? Even that check can sometimes catch problems...

In waitForContainerInfo() we wait for v1.ContainerInfo to be available. Perf stats are part of this structure and I would consider it a bug if they were not available.

dashpole · 2020-04-14T19:14:22Z

cc @dims
in case you want to take a look

dims · 2020-04-14T19:40:31Z

+1 to "perf tests separately" please!

looks reasonable otherwise :)

Signed-off-by: Maciej "Iwan" Iwanowski <[email protected]>

iwankgb · 2020-04-15T09:25:51Z

@dashpole @dims - is such approach acceptable? Changes are:

there are shell files that should be sourced before running non-standard integration tests suite
same make target is used in both cases
paths to the shell files are part of build matrix
documentation is provided.

Eventually it would make sense to execute unit tests in Docker too, I think. There are some unit tests against libpfm4 part of code that can be executed without installing the library. What is you opinion?

dims · 2020-04-15T10:42:31Z

@iwankgb works for me!

dashpole

lgtm

iwankgb force-pushed the perf_integration_tests branch 4 times, most recently from 8d0b455 to 1fc64df Compare April 14, 2020 17:46

iwankgb commented Apr 14, 2020

View reviewed changes

.github/workflows/test.yml Show resolved Hide resolved

iwankgb commented Apr 14, 2020

View reviewed changes

build/integration.sh Outdated Show resolved Hide resolved

iwankgb commented Apr 14, 2020

View reviewed changes

build/integration.sh Show resolved Hide resolved

iwankgb force-pushed the perf_integration_tests branch from 9af4e46 to 478f27b Compare April 14, 2020 18:07

iwankgb force-pushed the perf_integration_tests branch from 478f27b to 4dc7481 Compare April 14, 2020 18:08

iwankgb changed the title ~~WIP: Integration test for Docker and perf stats~~ Integration test for Docker and perf stats Apr 14, 2020

dashpole reviewed Apr 14, 2020

View reviewed changes

dashpole self-assigned this Apr 14, 2020

Maciej "Iwan" Iwanowski added 6 commits April 15, 2020 08:45

Integration test for Docker and perf stats

764622f

Signed-off-by: Maciej "Iwan" Iwanowski <[email protected]>

Fixed typo

9945e35

Signed-off-by: Maciej "Iwan" Iwanowski <[email protected]>

Using environment variables to create build matrix

5a0d353

Signed-off-by: Maciej "Iwan" Iwanowski <[email protected]>

Adding missing boilerplate comments

456582c

Signed-off-by: Maciej "Iwan" Iwanowski <[email protected]>

Adding build config to unit tests too

d991727

Signed-off-by: Maciej "Iwan" Iwanowski <[email protected]>

We no longer need to append entries to logfile

396c32a

Signed-off-by: Maciej "Iwan" Iwanowski <[email protected]>

iwankgb force-pushed the perf_integration_tests branch 2 times, most recently from 4cd5cf6 to 396c32a Compare April 15, 2020 08:31

Maciej "Iwan" Iwanowski added 3 commits April 15, 2020 10:51

Extending integration tests documentation

5cfb4c2

Signed-off-by: Maciej "Iwan" Iwanowski <[email protected]>

No environment configuration for unit tests

96150b3

Signed-off-by: Maciej "Iwan" Iwanowski <[email protected]>

New line at the end of file

4a2441a

Signed-off-by: Maciej "Iwan" Iwanowski <[email protected]>

dashpole approved these changes Apr 15, 2020

View reviewed changes

dashpole merged commit ba6fa06 into google:master Apr 15, 2020

Integration test for Docker and perf stats #2489

Integration test for Docker and perf stats #2489

Uh oh!

Conversation

iwankgb commented Apr 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

googlebot commented Apr 14, 2020

Uh oh!

googlebot commented Apr 14, 2020

Uh oh!

iwankgb commented Apr 14, 2020

Uh oh!

dashpole Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

dashpole Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

dashpole Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

dashpole Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

iwankgb Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

dashpole commented Apr 14, 2020

Uh oh!

dims commented Apr 14, 2020

Uh oh!

iwankgb commented Apr 15, 2020

Uh oh!

dims commented Apr 15, 2020

Uh oh!

dashpole left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants