Merge pull request #24850 from madhusudancs/speedup-do-nothing

Automatic merge from submit-queue Improve the speed of do-nothing build. As @thockin found out here https://github.com/kubernetes/kubernetes/issues/24518, vast majority of the do-nothing build time is spent in rebuilding the test binaries. There is no staleness check support for test binaries. This commit implements the staleness checks for test binaries and uses them while building packages. On my workstation, do-nothing hack/build-go.sh time goes from ~20 secs to ~4 secs, of which only ~1 sec is from doing test binary staleness check now (as opposed to ~17 secs it took to build the test binaries before). I did some experiments to bring this time down to <1 sec. I measured using go test -bench, but it was not very useful in this case. I believe, a vast majority of that ~1 second is being spent in fork/exec and piping the results back to the staleness check program along with the ser-deser involved, but it needs to be validated. Not a proof, but to provide some supporting evidence to this claim, running `go list -f format packages` in the shell takes about 600ms irrespective of what's in the format. Tests are TBD. I am still trying to figure out how to test this, but I would like to get early feedback cc @mikedanese @mml
2025-07-31 07:20:13 +00:00 · 2016-05-22 10:33:04 -07:00 · 2016-05-22 10:33:04 -07:00 · 0257f54a8c
commit 0257f54a8c
parent 39f0c6ba25 47efee7ce4
3 changed files with 584 additions and 1 deletions
--- a/hack/cmd/teststale/teststale.go
+++ b/hack/cmd/teststale/teststale.go
@ -0,0 +1,209 @@
+/*
+Copyright 2016 The Kubernetes Authors All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// teststale checks the staleness of a test binary. go test -c builds a test
+// binary but it does no staleness check. In other words, every time one runs
+// go test -c, it compiles the test packages and links the binary even when
+// nothing has changed. This program helps to mitigate that problem by allowing
+// to check the staleness of a given test package and its binary.
+package main
+
+import (
+	"encoding/json"
+	"flag"
+	"fmt"
+	"io"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"time"
+
+	"github.com/golang/glog"
+)
+
+const usageHelp = "" +
+	`This program checks the staleness of a given test package and its test
+binary so that one can make a decision about re-building the test binary.
+
+Usage:
+  teststale -binary=/path/to/test/binary -package=package
+
+Example:
+  teststale -binary="$HOME/gosrc/bin/e2e.test" -package="k8s.io/kubernetes/test/e2e"
+
+`
+
+var (
+	binary  = flag.String("binary", "", "filesystem path to the test binary file. Example: \"$HOME/gosrc/bin/e2e.test\"")
+	pkgPath = flag.String("package", "", "import path of the test package in the format used while importing packages. Example: \"k8s.io/kubernetes/test/e2e\"")
+)
+
+func usage() {
+	fmt.Fprintln(os.Stderr, usageHelp)
+	fmt.Fprintln(os.Stderr, "Flags:")
+	flag.PrintDefaults()
+	os.Exit(2)
+}
+
+// golist is an interface emulating the `go list` command to get package information.
+// TODO: Evaluate using `go/build` package instead. It doesn't provide staleness
+// information, but we can probably run `go list` and `go/build.Import()` concurrently
+// in goroutines and merge the results. Evaluate if that's faster.
+type golist interface {
+	pkgInfo(pkgPaths []string) ([]pkg, error)
+}
+
+// execmd implements the `golist` interface.
+type execcmd struct {
+	cmd  string
+	args []string
+	env  []string
+}
+
+func (e *execcmd) pkgInfo(pkgPaths []string) ([]pkg, error) {
+	args := append(e.args, pkgPaths...)
+	cmd := exec.Command(e.cmd, args...)
+	cmd.Env = e.env
+
+	stdout, err := cmd.StdoutPipe()
+	if err != nil {
+		return nil, fmt.Errorf("failed to obtain the metadata output stream: %v", err)
+	}
+
+	dec := json.NewDecoder(stdout)
+
+	// Start executing the command
+	if err := cmd.Start(); err != nil {
+		return nil, fmt.Errorf("command did not start: %v", err)
+	}
+
+	var pkgs []pkg
+	for {
+		var p pkg
+		if err := dec.Decode(&p); err == io.EOF {
+			break
+		} else if err != nil {
+			return nil, fmt.Errorf("failed to unmarshal metadata for package %s: %v", p.ImportPath, err)
+		}
+		pkgs = append(pkgs, p)
+	}
+
+	if err := cmd.Wait(); err != nil {
+		return nil, fmt.Errorf("command did not complete: %v", err)
+	}
+	return pkgs, nil
+}
+
+type pkg struct {
+	Dir          string
+	ImportPath   string
+	Target       string
+	Stale        bool
+	TestGoFiles  []string
+	TestImports  []string
+	XTestGoFiles []string
+	XTestImports []string
+}
+
+func (p *pkg) isNewerThan(cmd golist, buildTime time.Time) bool {
+	// If the package itself is stale, then we have to rebuild the whole thing anyway.
+	if p.Stale {
+		return true
+	}
+
+	// Test for file staleness
+	for _, f := range p.TestGoFiles {
+		if isNewerThan(filepath.Join(p.Dir, f), buildTime) {
+			glog.V(4).Infof("test Go file %s is stale", f)
+			return true
+		}
+	}
+	for _, f := range p.XTestGoFiles {
+		if isNewerThan(filepath.Join(p.Dir, f), buildTime) {
+			glog.V(4).Infof("external test Go file %s is stale", f)
+			return true
+		}
+	}
+
+	imps := []string{}
+	imps = append(imps, p.TestImports...)
+	imps = append(imps, p.XTestImports...)
+
+	// This calls `go list` the second time. This is required because the first
+	// call to `go list` checks the staleness of the package in question by
+	// looking the non-test dependencies, but it doesn't look at the test
+	// dependencies. However, it returns the list of test dependencies. This
+	// second call to `go list` checks the staleness of all the test
+	// dependencies.
+	pkgs, err := cmd.pkgInfo(imps)
+	if err != nil || len(pkgs) < 1 {
+		glog.V(4).Infof("failed to obtain metadata for packages %s: %v", imps, err)
+		return true
+	}
+
+	for _, p := range pkgs {
+		if p.Stale {
+			glog.V(4).Infof("import %q is stale", p.ImportPath)
+			return true
+		}
+	}
+
+	return false
+}
+
+func isNewerThan(filename string, buildTime time.Time) bool {
+	stat, err := os.Stat(filename)
+	if err != nil {
+		return true
+	}
+	return stat.ModTime().After(buildTime)
+}
+
+// isTestStale checks if the test binary is stale and needs to rebuilt.
+// Some of the ideas here are inspired by how Go does staleness checks.
+func isTestStale(cmd golist, binPath, pkgPath string) bool {
+	bStat, err := os.Stat(binPath)
+	if err != nil {
+		glog.V(4).Infof("Couldn't obtain the modified time of the binary %s: %v", binPath, err)
+		return true
+	}
+	buildTime := bStat.ModTime()
+
+	pkgs, err := cmd.pkgInfo([]string{pkgPath})
+	if err != nil || len(pkgs) < 1 {
+		glog.V(4).Infof("Couldn't retrieve test package information for package %s: %v", pkgPath, err)
+		return false
+	}
+
+	return pkgs[0].isNewerThan(cmd, buildTime)
+}
+
+func main() {
+	flag.Usage = usage
+	flag.Parse()
+
+	cmd := &execcmd{
+		cmd: "go",
+		args: []string{
+			"list",
+			"-json",
+		},
+		env: os.Environ(),
+	}
+	if !isTestStale(cmd, *binary, *pkgPath) {
+		os.Exit(1)
+	}
+}
--- a/hack/cmd/teststale/teststale_test.go
+++ b/hack/cmd/teststale/teststale_test.go
@ -0,0 +1,325 @@
+/*
+Copyright 2016 The Kubernetes Authors All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package main
+
+import (
+	"fmt"
+	"io/ioutil"
+	"math/rand"
+	"os"
+	"path"
+	"path/filepath"
+	"testing"
+	"time"
+)
+
+const (
+	// seed for rand.Source to generate data for files
+	seed int64 = 42
+
+	// 1K binary file
+	binLen = 1024
+
+	// Directory of the test package relative to $GOPATH
+	testImportDir = "example.com/proj/pkg"
+)
+
+var (
+	pastHour = time.Now().Add(-1 * time.Hour)
+
+	// The test package we are testing against
+	testPkg = path.Join(testImportDir, "test")
+)
+
+// fakegolist implements the `golist` interface providing fake package information for testing.
+type fakegolist struct {
+	dir       string
+	importMap map[string]pkg
+	testFiles []string
+	binfile   string
+}
+
+func newFakegolist() (*fakegolist, error) {
+	dir, err := ioutil.TempDir("", "teststale")
+	if err != nil {
+		// test can't proceed without a temp directory.
+		return nil, fmt.Errorf("failed to create a temp directory for testing: %v", err)
+	}
+
+	// Set the temp directory as the $GOPATH
+	if err := os.Setenv("GOPATH", dir); err != nil {
+		// can't proceed without pointing the $GOPATH to the temp directory.
+		return nil, fmt.Errorf("failed to set \"$GOPATH\" pointing to %q: %v", dir, err)
+	}
+
+	// Setup $GOPATH directory layout.
+	// Yeah! I am bored of repeatedly writing "if err != nil {}"!
+	if os.MkdirAll(filepath.Join(dir, "bin"), 0750) != nil ||
+		os.MkdirAll(filepath.Join(dir, "pkg", "linux_amd64"), 0750) != nil ||
+		os.MkdirAll(filepath.Join(dir, "src"), 0750) != nil {
+		return nil, fmt.Errorf("failed to setup the $GOPATH directory structure")
+	}
+
+	// Create a temp file to represent the test binary.
+	binfile, err := ioutil.TempFile("", "testbin")
+	if err != nil {
+		return nil, fmt.Errorf("failed to create the temp file to represent the test binary: %v", err)
+	}
+
+	// Could have used crypto/rand instead, but it doesn't matter.
+	rr := rand.New(rand.NewSource(42))
+	bin := make([]byte, binLen)
+	if _, err = rr.Read(bin); err != nil {
+		return nil, fmt.Errorf("couldn't read from the random source: %v", err)
+	}
+	if _, err := binfile.Write(bin); err != nil {
+		return nil, fmt.Errorf("couldn't write to the binary file %q: %v", binfile.Name(), err)
+	}
+	if err := binfile.Close(); err != nil {
+		// It is arguable whether this should be fatal.
+		return nil, fmt.Errorf("failed to close the binary file %q: %v", binfile.Name(), err)
+	}
+
+	if err := os.Chtimes(binfile.Name(), time.Now(), time.Now()); err != nil {
+		return nil, fmt.Errorf("failed to modify the mtime of the binary file %q: %v", binfile.Name(), err)
+	}
+
+	// Create test source files directory.
+	testdir := filepath.Join(dir, "src", testPkg)
+	if err := os.MkdirAll(testdir, 0750); err != nil {
+		return nil, fmt.Errorf("failed to create test source directory %q: %v", testdir, err)
+	}
+
+	fgl := &fakegolist{
+		dir: dir,
+		importMap: map[string]pkg{
+			"example.com/proj/pkg/test": {
+				Dir:        path.Join(dir, "src", testPkg),
+				ImportPath: testPkg,
+				Target:     path.Join(dir, "pkg", "linux_amd64", testImportDir, "test.a"),
+				Stale:      false,
+				TestGoFiles: []string{
+					"foo_test.go",
+					"bar_test.go",
+				},
+				TestImports: []string{
+					"example.com/proj/pkg/p1",
+					"example.com/proj/pkg/p1/c11",
+					"example.com/proj/pkg/p2",
+					"example.com/proj/cmd/p3/c12/c23",
+					"strings",
+					"testing",
+				},
+				XTestGoFiles: []string{
+					"xfoo_test.go",
+					"xbar_test.go",
+					"xbaz_test.go",
+				},
+				XTestImports: []string{
+					"example.com/proj/pkg/test",
+					"example.com/proj/pkg/p1",
+					"example.com/proj/cmd/p3/c12/c23",
+					"os",
+					"testing",
+				},
+			},
+			"example.com/proj/pkg/p1":         {Stale: false},
+			"example.com/proj/pkg/p1/c11":     {Stale: false},
+			"example.com/proj/pkg/p2":         {Stale: false},
+			"example.com/proj/cmd/p3/c12/c23": {Stale: false},
+			"strings":                         {Stale: false},
+			"testing":                         {Stale: false},
+			"os":                              {Stale: false},
+		},
+		testFiles: []string{
+			"foo_test.go",
+			"bar_test.go",
+			"xfoo_test.go",
+			"xbar_test.go",
+			"xbaz_test.go",
+		},
+		binfile: binfile.Name(),
+	}
+
+	// Create test source files.
+	for _, fn := range fgl.testFiles {
+		fp := filepath.Join(testdir, fn)
+		if _, err := os.Create(fp); err != nil {
+			return nil, fmt.Errorf("failed to create the test file %q: %v", fp, err)
+		}
+		if err := os.Chtimes(fp, time.Now(), pastHour); err != nil {
+			return nil, fmt.Errorf("failed to modify the mtime of the test file %q: %v", binfile.Name(), err)
+		}
+	}
+
+	return fgl, nil
+}
+
+func (fgl *fakegolist) pkgInfo(pkgPaths []string) ([]pkg, error) {
+	var pkgs []pkg
+	for _, path := range pkgPaths {
+		p, ok := fgl.importMap[path]
+		if !ok {
+			return nil, fmt.Errorf("package %q not found", path)
+		}
+		pkgs = append(pkgs, p)
+	}
+	return pkgs, nil
+}
+
+func (fgl *fakegolist) chMtime(filename string, mtime time.Time) error {
+	for _, fn := range fgl.testFiles {
+		if fn == filename {
+			fp := filepath.Join(fgl.dir, "src", testPkg, fn)
+			if err := os.Chtimes(fp, time.Now(), mtime); err != nil {
+				return fmt.Errorf("failed to modify the mtime of %q: %v", filename, err)
+			}
+			return nil
+		}
+	}
+	return fmt.Errorf("file %q not found", filename)
+}
+
+func (fgl *fakegolist) chStale(pkg string, stale bool) error {
+	if p, ok := fgl.importMap[pkg]; ok {
+		p.Stale = stale
+		fgl.importMap[pkg] = p
+		return nil
+	}
+	return fmt.Errorf("package %q not found", pkg)
+}
+
+func (fgl *fakegolist) cleanup() {
+	os.RemoveAll(fgl.dir)
+	os.Remove(fgl.binfile)
+}
+
+func TestIsTestStale(t *testing.T) {
+	cases := []struct {
+		fileMtime    map[string]time.Time
+		pkgStaleness map[string]bool
+		result       bool
+	}{
+		// Basic test: binary is fresh, all modifications were before the binary was built.
+		{
+			result: false,
+		},
+		// A local test file is new, hence binary must be stale.
+		{
+			fileMtime: map[string]time.Time{
+				"foo_test.go": time.Now().Add(1 * time.Hour),
+			},
+			result: true,
+		},
+		// Test package is new, so binary must be stale.
+		{
+			pkgStaleness: map[string]bool{
+				"example.com/proj/pkg/test": true,
+			},
+			result: true,
+		},
+		// Test package dependencies are new, so binary must be stale.
+		{
+			pkgStaleness: map[string]bool{
+				"example.com/proj/cmd/p3/c12/c23": true,
+				"strings":                         true,
+			},
+			result: true,
+		},
+		// External test files are new, hence binary must be stale.
+		{
+			fileMtime: map[string]time.Time{
+				"xfoo_test.go": time.Now().Add(1 * time.Hour),
+				"xbar_test.go": time.Now().Add(2 * time.Hour),
+			},
+			result: true,
+		},
+		// External test dependency is new, so binary must be stale.
+		{
+			pkgStaleness: map[string]bool{
+				"os": true,
+			},
+			result: true,
+		},
+		// Multiple source files and dependencies are new, so binary must be stale.
+		{
+			fileMtime: map[string]time.Time{
+				"foo_test.go":  time.Now().Add(1 * time.Hour),
+				"xfoo_test.go": time.Now().Add(2 * time.Hour),
+				"xbar_test.go": time.Now().Add(3 * time.Hour),
+			},
+			pkgStaleness: map[string]bool{
+				"example.com/proj/pkg/p1":         true,
+				"example.com/proj/pkg/p1/c11":     true,
+				"example.com/proj/pkg/p2":         true,
+				"example.com/proj/cmd/p3/c12/c23": true,
+				"strings":                         true,
+				"os":                              true,
+			},
+			result: true,
+		},
+		// Everything is new, so binary must be stale.
+		{
+			fileMtime: map[string]time.Time{
+				"foo_test.go":  time.Now().Add(3 * time.Hour),
+				"bar_test.go":  time.Now().Add(1 * time.Hour),
+				"xfoo_test.go": time.Now().Add(2 * time.Hour),
+				"xbar_test.go": time.Now().Add(1 * time.Hour),
+				"xbaz_test.go": time.Now().Add(2 * time.Hour),
+			},
+			pkgStaleness: map[string]bool{
+				"example.com/proj/pkg/p1":         true,
+				"example.com/proj/pkg/p1/c11":     true,
+				"example.com/proj/pkg/p2":         true,
+				"example.com/proj/cmd/p3/c12/c23": true,
+				"example.com/proj/pkg/test":       true,
+				"strings":                         true,
+				"testing":                         true,
+				"os":                              true,
+			},
+			result: true,
+		},
+	}
+
+	for _, tc := range cases {
+		fgl, err := newFakegolist()
+		if err != nil {
+			t.Fatalf("failed to setup the test: %v", err)
+		}
+		defer fgl.cleanup()
+
+		for fn, mtime := range tc.fileMtime {
+			if err := fgl.chMtime(fn, mtime); err != nil {
+				t.Fatalf("failed to change the mtime of %q: %v", fn, err)
+			}
+		}
+
+		for pkg, stale := range tc.pkgStaleness {
+			if err := fgl.chStale(pkg, stale); err != nil {
+				t.Fatalf("failed to change the staleness of %q: %v", pkg, err)
+			}
+		}
+
+		if tc.result != isTestStale(fgl, fgl.binfile, testPkg) {
+			if tc.result {
+				t.Errorf("Expected test package %q to be stale", testPkg)
+			} else {
+				t.Errorf("Expected test package %q to be not stale", testPkg)
+			}
+		}
+	}
+}
--- a/hack/lib/golang.sh
+++ b/hack/lib/golang.sh
@ -377,6 +377,25 @@ kube::golang::fallback_if_stdlib_not_installable() {
  use_go_build=true
 }

+# Builds the toolchain necessary for building kube. This needs to be
+# built only on the host platform.
+# TODO: This builds only the `teststale` binary right now. As we expand
+# this function's capabilities we need to find this a right home.
+# Ideally, not a shell script because testing shell scripts is painful.
+kube::golang::build_kube_toolchain() {
+  local targets=(
+    hack/cmd/teststale
+  )
+
+  local binaries
+  binaries=($(kube::golang::binaries_from_targets "${targets[@]}"))
+
+  kube::log::status "Building the toolchain targets:" "${binaries[@]}"
+  go install "${goflags[@]:+${goflags[@]}}" \
+        -ldflags "${goldflags}" \
+        "${binaries[@]:+${binaries[@]}}"
+}
+
 # Try and replicate the native binary placement of go install without
 # calling go install.
 kube::golang::output_filename_for_binary() {
@ -449,12 +468,39 @@ kube::golang::build_binaries_for_platform() {
  for test in "${tests[@]:+${tests[@]}}"; do
    local outfile=$(kube::golang::output_filename_for_binary "${test}" \
      "${platform}")
+
+    local testpkg="$(dirname ${test})"
+
+    # Staleness check always happens on the host machine, so we don't
+    # have to locate the `teststale` binaries for the other platforms.
+    # Since we place the host binaries in `$KUBE_GOPATH/bin`, we can
+    # assume that the binary exists there, if it exists at all.
+    # Otherwise, something has gone wrong with building the `teststale`
+    # binary and we should safely proceed building the test binaries
+    # assuming that they are stale. There is no good reason to error
+    # out.
+    if test -x "${KUBE_GOPATH}/bin/teststale" && ! "${KUBE_GOPATH}/bin/teststale" -binary "${outfile}" -package "${testpkg}"
+    then
+      continue
+    fi
+
+    # `go test -c` below directly builds the binary. It builds the packages,
+    # but it never installs them. `go test -i` only installs the dependencies
+    # of the test, but not the test package itself. So neither `go test -c`
+    # nor `go test -i` installs, for example, test/e2e.a. And without that,
+    # doing a staleness check on k8s.io/kubernetes/test/e2e package always
+    # returns true (always stale). And that's why we need to install the
+    # test package.
+    go install "${goflags[@]:+${goflags[@]}}" \
+        -ldflags "${goldflags}" \
+        "${testpkg}"
+
    mkdir -p "$(dirname ${outfile})"
    go test -c \
      "${goflags[@]:+${goflags[@]}}" \
      -ldflags "${goldflags}" \
      -o "${outfile}" \
-      "$(dirname ${test})"
+      "${testpkg}"
  done
 }

@ -549,6 +595,9 @@ kube::golang::build_binaries() {
      fi
    fi

+    # First build the toolchain before building any other targets
+    kube::golang::build_kube_toolchain
+
    if [[ "${parallel}" == "true" ]]; then
      kube::log::status "Building go targets for ${platforms[@]} in parallel (output will appear in a burst when complete):" "${targets[@]}"
      local platform