Add ability to build with runtime coverage instrumentation #67971

Katharine · 2018-08-28T22:22:37Z

What this PR does / why we need it:

This PR adds the ability to instrument a subset of kubernetes binaries to report code coverage information. The specific use-case is to help determine coverage of our end-to-end Conformance tests, as well as provide data that can be used to help determine where to focus. This PR focuses on making it possible to build with instrumentation; collecting and using the generated coverage data will be done in later PRs. For more details as to the intent, see the design doc (google doc; requires kubernetes-dev membership).

Specifically, this PR adds a new KUBE_BUILD_WITH_COVERAGE make variable, which when set will cause kube-apiserver, kube-controller-manager, kube-scheduler, kube-proxy and kubelet to be built with coverage instrumentation. These coverage-instrumented binaries will flush coverage information to disk every five seconds, defaulting to a temporary directory unless the KUBE_COVERAGE_FILE environment variable is set at launch, in which case it will write to that file instead.

The mechanism used to achieve coverage instrumentation is to build the targeted binaries as "unit tests" with coverage enabled, and then rigging the unit tests to just execute the binary's usual entry point. This is implemented only for the bash build system.

/sig testing

NONE

ixdy · 2018-08-28T22:41:35Z

/ok-to-test

Katharine · 2018-08-29T01:51:22Z

/retest

hh · 2018-08-29T19:00:08Z

pkg/util/coverage/coverage.go

+func InitCoverage(name string) {
+	// We read the coverage destination in from the KUBE_COVERAGE_FILE env var,
+	// or if it's empty we just use a default in /tmp
+	coverageFile = os.Getenv("KUBE_COVERAGE_FILE")


Glad to see the runtime configuration options within util.
I wonder if something like this could be use to augment client-go rest api-calls?

Use of an env var for config makes me wince a bit. We do have prior art that uses it, but I'm curious if there's a way we could do this via a command line flag instead. ie: have the dummy test add a command line flag with a default value, call this with the parsed value?

adding package scoped flags is generally pretty awful, env are not great for lots of config but it's just one simple string and only when coverage is enabled. doing it this way has the upside of not mutating any global state within the binary. this is probably the way to go imho.

Another downside to using a flag instead of an environment variable is that, because this feature is not always compiled in, you now need to be particularly careful about when you are passing the flag. By contrast, the environment variable will be silently ignored when it isn't relevant.

I am personally inclined to stick to the env var approach.

OK I'm sold

BenTheElder · 2018-08-29T20:14:08Z

pkg/util/coverage/coverage.go

+func FlushCoverage() {
+	// We're not actually going to run any tests, but we need Go to think we did so it writes
+	// coverage information to disk. To achieve this, we create a bunch of empty test suites and
+	// have it "run" them.


this is great 😂

BenTheElder · 2018-08-29T20:16:33Z

pkg/util/coverage/coverage.go

+	// originals away and set it up with this set.
+	originalArgs := os.Args
+	testArgs := []string{os.Args[0], "-test.coverprofile=" + tempCoveragePath()}
+	os.Args = testArgs


we should be able to pass testArgs to flag.CommandLine.Parse() instead I think.

BenTheElder · 2018-08-29T20:17:10Z

pkg/util/coverage/coverage.go

+	os.Args = testArgs
+
+	// This is sufficient for the unit tests to be set up.
+	flag.Parse()


yep this forwards to:
https://golang.org/src/flag/flag.go?s=32007:32019#L941

BenTheElder · 2018-08-29T21:19:34Z

/lint

k8s-ci-robot

@BenTheElder: 3 warnings.

In response to this:

/lint

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 · 2018-08-29T21:20:00Z

pkg/util/coverage/coverage.go

+limitations under the License.
+*/
+
+package coverage


Golint comments: should have a package comment, unless it's in another file for this package. More info.

pkg/util/coverage/coverage_disabled.go

pkg/util/coverage/fake_test_deps.go

Katharine · 2018-08-29T23:24:38Z

/retest

(the code hasn't even changed since this last passed...)

BenTheElder · 2018-08-30T20:30:34Z

for build/ and hack/:
/assign @cblecker @ixdy

cblecker · 2018-08-30T22:06:41Z

hack/lib/golang.sh

+          "${build_args[@]}" \
+          -tags coverage \
+          "$package"
+        kube::golang::delete_coverage_dummy_test "$package"


I'd like this to be trapped, so it can be cleaned up even if the previous command exits non-zero

cblecker · 2018-08-30T22:35:55Z

hack/lib/golang.sh

+# Argument: the name of a Kubernetes package (e.g. k8s.io/kubernetes/cmd/kube-scheduler)
+# Echos the path to a dummy test used for coverage information.
+kube::golang::path_for_coverage_dummy_test() {
+	local package=$1


nit: whitespace

cblecker · 2018-08-30T22:40:05Z

hack/lib/golang.sh

+# Echos the path to a dummy test used for coverage information.
+kube::golang::path_for_coverage_dummy_test() {
+	local package=$1
+  local path="${KUBE_GOPATH}/src/$package"


nit: can we curly bracket all these variables?

Katharine · 2018-08-31T01:26:06Z

/retest

thockin · 2018-08-31T23:13:31Z

/approve

spiffxp · 2018-08-31T23:21:41Z

/hold
for /lgtm once comments are resolved

Katharine · 2018-09-01T00:16:15Z

/retest

BenTheElder

/lgtm
despite the bash though 😬

BenTheElder

/lgtm

k8s-ci-robot · 2018-09-01T01:08:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, Katharine, spiffxp, thockin

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:

~~OWNERS~~ [thockin]

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

spiffxp · 2018-09-01T01:09:12Z

pkg/util/coverage/coverage.go

+	if coverageFile == "" {
+		coverageFile = "/tmp/k8s-" + name + ".cov"
+	}
+	fmt.Println("Dumping coverage information to " + coverageFile)


Can this be a glog.Infof?

No; glog isn't initialised at this point.

spiffxp · 2018-09-01T01:14:42Z

/hold cancel

BenTheElder · 2018-09-01T01:33:19Z

/retest

k8s-github-robot · 2018-09-01T07:25:25Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-09-01T08:32:51Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

…971-upstream-release-1.11 Automated cherry pick of #67971: Add ability to build with runtime coverage instrumentation

k8s-ci-robot requested review from ixdy and lavalamp August 28, 2018 22:23

Katharine changed the title ~~Coverage instrumentation~~ Add ability to build with runtime coverage instrumentation Aug 28, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 28, 2018

hh reviewed Aug 29, 2018

View reviewed changes

BenTheElder reviewed Aug 29, 2018

View reviewed changes

k8s-ci-robot reviewed Aug 29, 2018

View reviewed changes

Katharine added 4 commits August 29, 2018 14:48

Build relevant binaries with coverage.

aa2a7d0

Add runtime coverage support.

da4bbd4

Pass KUBE_BUILD_WITH_COVERAGE through to docker instance.

254618e

Add autogenerated BUILD files.

6afc130

Katharine force-pushed the coverage-instrumentation branch from d53ec9d to 6afc130 Compare August 29, 2018 21:48

k8s-ci-robot assigned cblecker and ixdy Aug 30, 2018

cblecker reviewed Aug 30, 2018

View reviewed changes

Improve bash formatting.

8fe6467

Be sure we delete the dummy tests.

9d499cd

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2018

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

Katharine mentioned this pull request Aug 31, 2018

REQUEST: New membership for Katharine kubernetes/org#53

Closed

6 tasks

Katharine added 2 commits August 31, 2018 17:06

Improve error behaviour of package coverage.

13d1961

Address shell-related comments.

9b790da

k8s-ci-robot assigned BenTheElder Sep 1, 2018

BenTheElder approved these changes Sep 1, 2018

View reviewed changes

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

Add owners.

c3e08be

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

Change owners.

9390847

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

BenTheElder approved these changes Sep 1, 2018

View reviewed changes

spiffxp reviewed Sep 1, 2018

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 1, 2018

k8s-github-robot merged commit 68d22a8 into kubernetes:master Sep 1, 2018

This was referenced Sep 7, 2018

Add coverage profile retrieval to log dumps #68387

Merged

Use KUBE_BUILD_WITH_COVERAGE directly #68412

Merged

This was referenced Oct 5, 2018

Automated cherry pick of #67971: Add ability to build with runtime coverage instrumentation #69481

Merged

[WIP] Backport #67971 (building with code coverage) to release-1.10 #69531

Closed

k8s-ci-robot added a commit that referenced this pull request Oct 19, 2018

Merge pull request #69481 from Katharine/automated-cherry-pick-of-#67…

fcaa65b

…971-upstream-release-1.11 Automated cherry pick of #67971: Add ability to build with runtime coverage instrumentation

spiffxp mentioned this pull request Aug 26, 2022

pkg/util/coverage: update fakeTestDeps impl #112047

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to build with runtime coverage instrumentation #67971

Add ability to build with runtime coverage instrumentation #67971

Katharine commented Aug 28, 2018

ixdy commented Aug 28, 2018

Katharine commented Aug 29, 2018

hh Aug 29, 2018

spiffxp Aug 31, 2018

BenTheElder Aug 31, 2018

Katharine Aug 31, 2018 •

edited

spiffxp Aug 31, 2018

BenTheElder Aug 29, 2018

spiffxp Aug 31, 2018

BenTheElder Aug 29, 2018

BenTheElder Aug 29, 2018

BenTheElder commented Aug 29, 2018

k8s-ci-robot left a comment

k8s-ci-robot Aug 29, 2018

Katharine commented Aug 29, 2018

BenTheElder commented Aug 30, 2018

cblecker Aug 30, 2018

cblecker Aug 30, 2018

cblecker Aug 30, 2018

Katharine commented Aug 31, 2018

thockin commented Aug 31, 2018

spiffxp commented Aug 31, 2018

Katharine commented Sep 1, 2018

BenTheElder left a comment

BenTheElder left a comment

k8s-ci-robot commented Sep 1, 2018

spiffxp Sep 1, 2018

Katharine Sep 1, 2018

spiffxp commented Sep 1, 2018

BenTheElder commented Sep 1, 2018

k8s-github-robot commented Sep 1, 2018

k8s-github-robot commented Sep 1, 2018

Add ability to build with runtime coverage instrumentation #67971

Add ability to build with runtime coverage instrumentation #67971

Conversation

Katharine commented Aug 28, 2018

ixdy commented Aug 28, 2018

Katharine commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Katharine Aug 31, 2018 • 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

BenTheElder commented Aug 29, 2018

k8s-ci-robot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Katharine commented Aug 29, 2018

BenTheElder commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Katharine commented Aug 31, 2018

thockin commented Aug 31, 2018

spiffxp commented Aug 31, 2018

Katharine commented Sep 1, 2018

BenTheElder left a comment

Choose a reason for hiding this comment

BenTheElder left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spiffxp commented Sep 1, 2018

BenTheElder commented Sep 1, 2018

k8s-github-robot commented Sep 1, 2018

k8s-github-robot commented Sep 1, 2018

Katharine Aug 31, 2018 •

edited