Merge pull request #7558 from priyawadhwa/monitor-1

Add support for specifying pr when running mkcmp

cmd/performance/mkcmp/cmd/mkcmp.go

@@ -35,7 +35,11 @@ var rootCmd = &cobra.Command{
 		return validateArgs(args)
 	},
 	RunE: func(cmd *cobra.Command, args []string) error {
-		return perf.CompareMinikubeStart(context.Background(), os.Stdout, args)
+		binaries, err := retrieveBinaries(args)
+		if err != nil {
+			return err
+		}
+		return perf.CompareMinikubeStart(context.Background(), os.Stdout, binaries)
 	},
 }
 
@@ -46,6 +50,18 @@ func validateArgs(args []string) error {
 	return nil
 }
 
+func retrieveBinaries(args []string) ([]*perf.Binary, error) {
+	binaries := []*perf.Binary{}
+	for _, a := range args {
+		binary, err := perf.NewBinary(a)
+		if err != nil {
+			return nil, err
+		}
+		binaries = append(binaries, binary)
+	}
+	return binaries, nil
+}
+
 // Execute runs the mkcmp command
 func Execute() {
 	if err := rootCmd.Execute(); err != nil {

pkg/minikube/perf/binary.go

@@ -0,0 +1,108 @@
+/*
+Copyright 2020 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 perf
+
+import (
+	"fmt"
+	"io"
+	"net/http"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+
+	"github.com/pkg/errors"
+	"k8s.io/minikube/pkg/minikube/constants"
+)
+
+type Binary struct {
+	path string
+	pr   int
+}
+
+const (
+	prPrefix = "pr://"
+)
+
+// NewBinary returns a new binary type
+func NewBinary(b string) (*Binary, error) {
+	// If it doesn't have the prefix, assume a path
+	if !strings.HasPrefix(b, prPrefix) {
+		return &Binary{
+			path: b,
+		}, nil
+	}
+	return newBinaryFromPR(b)
+}
+
+// Name returns the name of the binary
+func (b *Binary) Name() string {
+	if b.pr != 0 {
+		return fmt.Sprintf("Minikube (PR %d)", b.pr)
+	}
+	return filepath.Base(b.path)
+}
+
+// newBinaryFromPR downloads the minikube binary built for the pr by Jenkins from GCS
+func newBinaryFromPR(pr string) (*Binary, error) {
+	pr = strings.TrimPrefix(pr, prPrefix)
+	// try to convert to int
+	i, err := strconv.Atoi(pr)
+	if err != nil {
+		return nil, errors.Wrapf(err, "converting %s to an integer", pr)
+	}
+
+	b := &Binary{
+		path: localMinikubePath(i),
+		pr:   i,
+	}
+
+	if err := downloadBinary(remoteMinikubeURL(i), b.path); err != nil {
+		return nil, errors.Wrapf(err, "downloading minikube")
+	}
+
+	return b, nil
+}
+
+func remoteMinikubeURL(pr int) string {
+	return fmt.Sprintf("https://storage.googleapis.com/minikube-builds/%d/minikube-linux-amd64", pr)
+}
+
+func localMinikubePath(pr int) string {
+	return fmt.Sprintf("%s/minikube-binaries/%d/minikube", constants.DefaultMinipath, pr)
+}
+
+func downloadBinary(url, path string) error {
+	resp, err := http.Get(url)
+	if err != nil {
+		return err
+	}
+	defer resp.Body.Close()
+
+	if err := os.MkdirAll(filepath.Dir(path), 0777); err != nil {
+		return err
+	}
+
+	f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0777)
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+
+	_, err = io.Copy(f, resp.Body)
+	return err
+}

pkg/minikube/perf/start.go

@@ -35,21 +35,26 @@ const (
 
 var (
 	// For testing
-	collectTimeMinikubeStart = timeMinikubeStart
+	collectTimeMinikubeStart = collectTimes
 )
 
 // CompareMinikubeStart compares the time to run `minikube start` between two minikube binaries
-func CompareMinikubeStart(ctx context.Context, out io.Writer, binaries []string) error {
+func CompareMinikubeStart(ctx context.Context, out io.Writer, binaries []*Binary) error {
-	durations, err := collectTimes(ctx, binaries)
+	durations, err := collectTimeMinikubeStart(ctx, binaries)
 	if err != nil {
 		return err
 	}
 
-	fmt.Fprintf(out, "Old binary: %v\nNew binary: %v\nAverage Old: %f\nAverage New: %f\n", durations[0], durations[1], average(durations[0]), average(durations[1]))
+	for i, d := range durations {
+		fmt.Fprintf(out, "Results for %s:\n", binaries[i].Name())
+		fmt.Fprintf(out, "Times: %v\n", d)
+		fmt.Fprintf(out, "Average Time: %f\n\n", average(d))
+	}
+
 	return nil
 }
 
-func collectTimes(ctx context.Context, binaries []string) ([][]float64, error) {
+func collectTimes(ctx context.Context, binaries []*Binary) ([][]float64, error) {
 	durations := make([][]float64, len(binaries))
 	for i := range durations {
 		durations[i] = make([]float64, runs)
@@ -58,9 +63,9 @@ func collectTimes(ctx context.Context, binaries []string) ([][]float64, error) {
 	for r := 0; r < runs; r++ {
 		log.Printf("Executing run %d...", r)
 		for index, binary := range binaries {
-			duration, err := collectTimeMinikubeStart(ctx, binary)
+			duration, err := timeMinikubeStart(ctx, binary)
 			if err != nil {
-				return nil, errors.Wrapf(err, "timing run %d with %s", r, binary)
+				return nil, errors.Wrapf(err, "timing run %d with %s", r, binary.Name())
 			}
 			durations[index][r] = duration
 		}
@@ -79,12 +84,12 @@ func average(nums []float64) float64 {
 
 // timeMinikubeStart returns the time it takes to execute `minikube start`
 // It deletes the VM after `minikube start`.
-func timeMinikubeStart(ctx context.Context, binary string) (float64, error) {
+func timeMinikubeStart(ctx context.Context, binary *Binary) (float64, error) {
-	startCmd := exec.CommandContext(ctx, binary, "start")
+	startCmd := exec.CommandContext(ctx, binary.path, "start")
 	startCmd.Stdout = os.Stdout
 	startCmd.Stderr = os.Stderr
 
-	deleteCmd := exec.CommandContext(ctx, binary, "delete")
+	deleteCmd := exec.CommandContext(ctx, binary.path, "delete")
 	defer func() {
 		if err := deleteCmd.Run(); err != nil {
 			log.Printf("error deleting minikube: %v", err)

pkg/minikube/perf/start_test.go

@@ -19,86 +19,64 @@ package perf
 import (
 	"bytes"
 	"context"
-	"reflect"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
 )
 
-func mockCollectTimeMinikubeStart(durations []float64) func(ctx context.Context, binary string) (float64, error) {
+func mockCollectTimes(times [][]float64) func(ctx context.Context, binaries []*Binary) ([][]float64, error) {
-	index := 0
+	return func(ctx context.Context, binaries []*Binary) ([][]float64, error) {
-	return func(context.Context, string) (float64, error) {
+		return times, nil
-		duration := durations[index]
-		index++
-		return duration, nil
 	}
 }
 
 func TestCompareMinikubeStartOutput(t *testing.T) {
+	binaries := []*Binary{
+		{
+			path: "minikube1",
+		}, {
+			path: "minikube2",
+		},
+	}
 	tests := []struct {
 		description string
-		durations   []float64
+		times       [][]float64
 		expected    string
 	}{
 		{
 			description: "standard run",
-			durations:   []float64{4.5, 6},
+			times:       [][]float64{{4.5, 6}, {1, 2}},
-			expected:    "Old binary: [4.5]\nNew binary: [6]\nAverage Old: 4.500000\nAverage New: 6.000000\n",
+			expected: `Results for minikube1:
+Times: [4.5 6]
+Average Time: 5.250000
+
+Results for minikube2:
+Times: [1 2]
+Average Time: 1.500000
+
+`,
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
-			originalCollectTimes := collectTimeMinikubeStart
+			originalCollectTimes := collectTimes
-			collectTimeMinikubeStart = mockCollectTimeMinikubeStart(test.durations)
+			collectTimeMinikubeStart = mockCollectTimes(test.times)
 			defer func() { collectTimeMinikubeStart = originalCollectTimes }()
 
 			buf := bytes.NewBuffer([]byte{})
-			err := CompareMinikubeStart(context.Background(), buf, []string{"", ""})
+			err := CompareMinikubeStart(context.Background(), buf, binaries)
 			if err != nil {
 				t.Fatalf("error comparing minikube start: %v", err)
 			}
 
 			actual := buf.String()
-			if test.expected != actual {
+			if diff := cmp.Diff(test.expected, actual); diff != "" {
-				t.Fatalf("actual output does not match expected output\nActual: %v\nExpected: %v", actual, test.expected)
+				t.Errorf("machines mismatch (-want +got):\n%s", diff)
 			}
 		})
 	}
 }
-
-func TestCollectTimes(t *testing.T) {
-	tests := []struct {
-		description string
-		durations   []float64
-		expected    [][]float64
-	}{
-		{
-			description: "test collect time",
-			durations:   []float64{1, 2},
-			expected: [][]float64{
-				{1},
-				{2},
-			},
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.description, func(t *testing.T) {
-			originalCollectTimes := collectTimeMinikubeStart
-			collectTimeMinikubeStart = mockCollectTimeMinikubeStart(test.durations)
-			defer func() { collectTimeMinikubeStart = originalCollectTimes }()
-
-			actual, err := collectTimes(context.Background(), []string{"", ""})
-			if err != nil {
-				t.Fatalf("error collecting times: %v", err)
-			}
-
-			if !reflect.DeepEqual(actual, test.expected) {
-				t.Fatalf("actual output does not match expected output\nActual: %v\nExpected: %v", actual, test.expected)
-			}
-		})
-	}
-}
-
 func TestAverage(t *testing.T) {
 	tests := []struct {
 		description string