From 66b217ca4a2df9a6397e6fc0e30156609016ce40 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 4 Sep 2019 16:52:29 -0700 Subject: [PATCH 1/7] add polling status after ever half second --- .../leeroy-app/kubernetes/deployment.yaml | 2 +- pkg/skaffold/deploy/resources/resource.go | 135 ++++++++++++++++ .../deploy/resources/resource_test.go | 153 ++++++++++++++++++ pkg/skaffold/deploy/status_check.go | 96 +++++++++-- pkg/skaffold/deploy/status_check_test.go | 101 ++++++++++-- pkg/skaffold/runner/deploy.go | 8 +- pkg/skaffold/runner/deploy_test.go | 5 +- pkg/skaffold/util/util.go | 4 + 8 files changed, 470 insertions(+), 34 deletions(-) create mode 100644 pkg/skaffold/deploy/resources/resource.go create mode 100644 pkg/skaffold/deploy/resources/resource_test.go diff --git a/examples/microservices/leeroy-app/kubernetes/deployment.yaml b/examples/microservices/leeroy-app/kubernetes/deployment.yaml index 56ef7152fef..dfba0a5920c 100644 --- a/examples/microservices/leeroy-app/kubernetes/deployment.yaml +++ b/examples/microservices/leeroy-app/kubernetes/deployment.yaml @@ -19,7 +19,7 @@ metadata: labels: app: leeroy-app spec: - replicas: 1 + replicas: 4 selector: matchLabels: app: leeroy-app diff --git a/pkg/skaffold/deploy/resources/resource.go b/pkg/skaffold/deploy/resources/resource.go new file mode 100644 index 00000000000..89ce2482f45 --- /dev/null +++ b/pkg/skaffold/deploy/resources/resource.go @@ -0,0 +1,135 @@ +/* +Copyright 2019 The Skaffold Authors + +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 resources + +import ( + "fmt" + "io" + + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" +) + +const ( + TabHeader = " -" + DeploymentType = "deployment" +) + +type ResourceObj struct { + name string + namespace string + rType string + status Status +} + +func (r *ResourceObj) String() string { + return fmt.Sprintf("%s:%s/%s", r.namespace, r.rType, r.name) +} + +func (r *ResourceObj) Type() string { + return r.rType +} + +func (r *ResourceObj) UpdateStatus(msg string, reason string, err error) { + newStatus := Status{details: msg, reason: reason, err: err} + if !r.status.Equals(&newStatus) { + r.status.err = err + r.status.details = util.Trim(msg) + r.status.reason = util.Trim(reason) + r.status.updated = true + } +} + +func (r *ResourceObj) Status() *Status { + return &r.status +} + +func (r *ResourceObj) Namespace() string { + return r.namespace +} + +func (r *ResourceObj) Name() string { + return r.name +} + +func (r *ResourceObj) MarkCheckComplete() *ResourceObj { + r.status.completed = true + return r +} + +func (r *ResourceObj) IsStatusCheckComplete() bool { + return r.status.completed +} + +func (r *ResourceObj) ReportSinceLastUpdated(out io.Writer) { + if !r.status.updated { + return + } + r.status.updated = false + color.Default.Fprintln(out, fmt.Sprintf("%s %s %s", TabHeader, r.String(), r.status.String())) +} + +type Status struct { + err error + reason string + details string + updated bool + completed bool +} + +func (rs *Status) Error() error { + return rs.err +} + +func (rs *Status) Equals(other *Status) bool { + return util.Trim(rs.reason) == util.Trim(other.reason) +} + +func (rs *Status) String() string { + if rs.err != nil { + return fmt.Sprintf("%s", util.Trim(rs.err.Error())) + } + return fmt.Sprintf("%s", rs.details) +} + +func NewResource(name string, ns string) *ResourceObj { + return &ResourceObj{ + name: name, + namespace: ns, + rType: DeploymentType, + } +} + +func (r *ResourceObj) WithStatus(status Status) *ResourceObj { + r.status = status + return r +} + +// For testing, mimics when a ResourceObj status is updated. +func (r *ResourceObj) WithUpdatedStatus(status Status) *ResourceObj { + r.status = status + r.status.updated = true + return r +} + +func NewStatus(msg string, reason string, err error) Status { + return Status{ + details: msg, + reason: reason, + err: err, + } +} diff --git a/pkg/skaffold/deploy/resources/resource_test.go b/pkg/skaffold/deploy/resources/resource_test.go new file mode 100644 index 00000000000..9cf70011cf3 --- /dev/null +++ b/pkg/skaffold/deploy/resources/resource_test.go @@ -0,0 +1,153 @@ +/* +Copyright 2019 The Skaffold Authors + +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 resources + +import ( + "bytes" + "fmt" + "testing" + + "github.com/GoogleContainerTools/skaffold/testutil" +) + +func TestUpdateTimestamp(t *testing.T) { + dep := NewResource("test", "test-ns") + + // Check updated bool is false + testutil.CheckDeepEqual(t, false, dep.status.updated) + + // Update the status + dep.UpdateStatus("success", "success", nil) + + // Check the updated bool is true + testutil.CheckDeepEqual(t, true, dep.status.updated) +} + +func TestReportSinceLastUpdated(t *testing.T) { + var tests = []struct { + description string + message string + err error + expected string + }{ + { + description: "updating an error status", + message: "cannot pull image", + err: fmt.Errorf("cannot pull image"), + expected: " - test-ns:deployment/test cannot pull image\n", + }, + { + description: "updating a non error status", + message: "is waiting for container", + expected: " - test-ns:deployment/test is waiting for container\n", + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + dep := NewResource("test", "test-ns") + out := new(bytes.Buffer) + dep.UpdateStatus(test.message, test.message, test.err) + dep.ReportSinceLastUpdated(out) + t.CheckDeepEqual(test.expected, out.String()) + }) + } +} + +func TestReportSinceLastUpdatedMultipleTimes(t *testing.T) { + var tests = []struct { + description string + times int + expected string + }{ + { + description: "report first time should write to out", + times: 1, + expected: " - test-ns:deployment/test cannot pull image\n", + }, + { + description: "report 2nd time should not write to out", + times: 2, + expected: "", + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + dep := NewResource("test", "test-ns") + dep.UpdateStatus("cannot pull image", "err", nil) + var out *bytes.Buffer + for i := 0; i < test.times; i++ { + out = new(bytes.Buffer) + dep.ReportSinceLastUpdated(out) + // Check reported timestamp is set to false + t.CheckDeepEqual(false, dep.status.updated) + } + t.CheckDeepEqual(test.expected, out.String()) + }) + } +} + +func TestUpdateStatus(t *testing.T) { + var tests = []struct { + description string + old Status + new Status + expectChange bool + }{ + { + description: "updated should be false for same statuses", + old: Status{details: "Waiting for 0/1 replicas to be available...", reason: "Waiting for 0/1 replicas to be available...", err: nil}, + new: Status{details: "Waiting for 0/1 replicas to be available...", reason: "Waiting for 0/1 replicas to be available...", err: nil}, + expectChange: false, + }, + { + description: "updated should be true if reason changes", + old: Status{details: "same", reason: "same", err: nil}, + new: Status{details: "same", reason: "another", err: nil}, + expectChange: true, + }, + { + description: "updated should be false if reason is same", + old: Status{details: "same", reason: "same", err: nil}, + new: Status{details: "same", reason: "same", err: fmt.Errorf("see this error")}, + }, + { + description: "updated should be true if reason and err change", + old: Status{details: "same", reason: "same", err: nil}, + new: Status{details: "same", reason: "another", err: fmt.Errorf("see this error")}, + expectChange: true, + }, + { + description: "updated should be true if both reason and details change", + old: Status{details: "same", reason: "same", err: nil}, + new: Status{details: "error", reason: "error", err: nil}, + expectChange: true, + }, + { + description: "updated should be false if both reason has a new line", + old: Status{details: "same", reason: "same\n", err: nil}, + new: Status{details: "same", reason: "same", err: nil}, + expectChange: false, + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + dep := NewResource("test", "test-ns").WithStatus(test.old) + dep.UpdateStatus(test.new.details, test.new.reason, test.new.err) + t.CheckDeepEqual(test.expectChange, dep.status.updated) + }) + } +} diff --git a/pkg/skaffold/deploy/status_check.go b/pkg/skaffold/deploy/status_check.go index 9d23cc4be09..7dbd8a2154f 100644 --- a/pkg/skaffold/deploy/status_check.go +++ b/pkg/skaffold/deploy/status_check.go @@ -19,10 +19,12 @@ package deploy import ( "context" "fmt" + "io" "strings" "sync" "time" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/resources" "github.com/pkg/errors" "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -39,11 +41,24 @@ var ( // Poll period for checking set to 100 milliseconds defaultPollPeriodInMilliseconds = 100 + // report resource status for pending resources 0.5 second. + reportStatusTime = 500 * time.Millisecond + // For testing executeRolloutStatus = getRollOutStatus ) -func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx *runcontext.RunContext) error { +type Checker struct { + context context.Context + runCtx *runcontext.RunContext + out io.Writer + labeller *DefaultLabeller + client *kubectl.CLI + numDeps int + processedDeps int32 +} + +func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx *runcontext.RunContext, out io.Writer) error { client, err := pkgkubernetes.Client() if err != nil { return errors.Wrap(err, "getting kubernetes client") @@ -51,7 +66,7 @@ func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx * deadline := getDeadline(runCtx.Cfg.Deploy.StatusCheckDeadlineSeconds) - dMap, err := getDeployments(client, runCtx.Opts.Namespace, defaultLabeller, deadline) + rsMap, err := getDeployments(client, runCtx.Opts.Namespace, defaultLabeller, deadline) if err != nil { return errors.Wrap(err, "could not fetch deployments") } @@ -61,20 +76,36 @@ func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx * syncMap := &sync.Map{} kubeCtl := kubectl.NewFromRunContext(runCtx) - for dName, deadlineDuration := range dMap { + checker := &Checker{ + context: ctx, + runCtx: runCtx, + labeller: defaultLabeller, + client: kubectl.NewFromRunContext(runCtx), + out: out, + numDeps: len(rsMap), + } + + rs := make([]*resources.ResourceObj, 0, len(rsMap)) + for r, deadlineDuration := range rsMap { + rs = append(rs, &r) wg.Add(1) - go func(dName string, deadlineDuration time.Duration) { + go func(r *resources.ResourceObj, deadlineDuration time.Duration) { defer wg.Done() - pollDeploymentRolloutStatus(ctx, kubeCtl, dName, deadlineDuration, syncMap) - }(dName, deadlineDuration) + pollDeploymentRolloutStatus(ctx, kubeCtl, r, deadlineDuration, syncMap) + }(&r, deadlineDuration) } + // Retrieve pending resource states + go func() { + checker.printResourceStatus(rs, deadline) + }() + // Wait for all deployment status to be fetched wg.Wait() return getSkaffoldDeployStatus(syncMap) } -func getDeployments(client kubernetes.Interface, ns string, l *DefaultLabeller, deadlineDuration time.Duration) (map[string]time.Duration, error) { +func getDeployments(client kubernetes.Interface, ns string, l *DefaultLabeller, deadlineDuration time.Duration) (map[resources.ResourceObj]time.Duration, error) { deps, err := client.AppsV1().Deployments(ns).List(metav1.ListOptions{ LabelSelector: l.RunIDKeyValueString(), }) @@ -82,7 +113,7 @@ func getDeployments(client kubernetes.Interface, ns string, l *DefaultLabeller, return nil, errors.Wrap(err, "could not fetch deployments") } - depMap := map[string]time.Duration{} + depMap := map[resources.ResourceObj]time.Duration{} for _, d := range deps.Items { var deadline time.Duration @@ -91,33 +122,39 @@ func getDeployments(client kubernetes.Interface, ns string, l *DefaultLabeller, } else { deadline = time.Duration(*d.Spec.ProgressDeadlineSeconds) * time.Second } - depMap[d.Name] = deadline + r := resources.NewResource(d.Name, d.Namespace) + depMap[*r] = deadline } return depMap, nil } -func pollDeploymentRolloutStatus(ctx context.Context, k *kubectl.CLI, dName string, deadline time.Duration, syncMap *sync.Map) { +func pollDeploymentRolloutStatus(ctx context.Context, k *kubectl.CLI, r *resources.ResourceObj, deadline time.Duration, syncMap *sync.Map) { pollDuration := time.Duration(defaultPollPeriodInMilliseconds) * time.Millisecond // Add poll duration to account for one last attempt after progressDeadlineSeconds. timeoutContext, cancel := context.WithTimeout(ctx, deadline+pollDuration) - logrus.Debugf("checking rollout status %s", dName) + logrus.Debugf("checking rollout status %s", r.Name()) defer cancel() for { select { case <-timeoutContext.Done(): - syncMap.Store(dName, errors.Wrap(timeoutContext.Err(), fmt.Sprintf("deployment rollout status could not be fetched within %v", deadline))) + syncMap.Store(r.Name(), errors.Wrap(timeoutContext.Err(), fmt.Sprintf("deployment rollout status could not be fetched within %v", deadline))) return case <-time.After(pollDuration): - status, err := executeRolloutStatus(timeoutContext, k, dName) + status, err := executeRolloutStatus(timeoutContext, k, r.Name()) if err != nil { - syncMap.Store(dName, err) + syncMap.Store(r.Name(), err) + r.UpdateStatus("", err.Error(), err) + r.MarkCheckComplete() return } if strings.Contains(status, "successfully rolled out") { - syncMap.Store(dName, nil) + syncMap.Store(r.Name(), nil) + r.UpdateStatus(status, status, nil) + r.MarkCheckComplete() return } + r.UpdateStatus(status, status, nil) } } } @@ -148,3 +185,32 @@ func getDeadline(d int) time.Duration { } return defaultStatusCheckDeadline } + +// Print resource statuses until all status check are completed or context is cancelled. +func (c *Checker) printResourceStatus(rs []*resources.ResourceObj, deadline time.Duration) { + timeoutContext, cancel := context.WithTimeout(c.context, deadline) + defer cancel() + for { + var allResourcesCheckComplete bool + select { + case <-timeoutContext.Done(): + return + case <-time.After(reportStatusTime): + allResourcesCheckComplete = printStatus(rs, c.out) + } + if allResourcesCheckComplete { + return + } + } +} + +func printStatus(rs []*resources.ResourceObj, out io.Writer) bool { + allResourcesCheckComplete := true + for _, r := range rs { + if !r.IsStatusCheckComplete() { + allResourcesCheckComplete = false + r.ReportSinceLastUpdated(out) + } + } + return allResourcesCheckComplete +} diff --git a/pkg/skaffold/deploy/status_check_test.go b/pkg/skaffold/deploy/status_check_test.go index 353cb19c24a..c4740a47cd3 100644 --- a/pkg/skaffold/deploy/status_check_test.go +++ b/pkg/skaffold/deploy/status_check_test.go @@ -17,6 +17,7 @@ limitations under the License. package deploy import ( + "bytes" "context" "errors" "fmt" @@ -24,6 +25,7 @@ import ( "testing" "time" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/resources" appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -40,7 +42,7 @@ func TestGetDeployments(t *testing.T) { tests := []struct { description string deps []*appsv1.Deployment - expected map[string]time.Duration + expected map[resources.ResourceObj]time.Duration shouldErr bool }{ { @@ -68,8 +70,12 @@ func TestGetDeployments(t *testing.T) { Spec: appsv1.DeploymentSpec{ProgressDeadlineSeconds: utilpointer.Int32Ptr(20)}, }, }, - expected: map[string]time.Duration{"dep1": time.Duration(10) * time.Second, "dep2": time.Duration(20) * time.Second}, - }, { + expected: map[resources.ResourceObj]time.Duration{ + *resources.NewResource("dep1", "test"): time.Duration(10) * time.Second, + *resources.NewResource("dep2", "test"): time.Duration(20) * time.Second, + }, + }, + { description: "command flag deadline is less than deployment spec.", deps: []*appsv1.Deployment{ { @@ -84,8 +90,11 @@ func TestGetDeployments(t *testing.T) { Spec: appsv1.DeploymentSpec{ProgressDeadlineSeconds: utilpointer.Int32Ptr(300)}, }, }, - expected: map[string]time.Duration{"dep1": time.Duration(200) * time.Second}, - }, { + expected: map[resources.ResourceObj]time.Duration{ + *resources.NewResource("dep1", "test"): time.Duration(200) * time.Second, + }, + }, + { description: "multiple deployments with no progress deadline set", deps: []*appsv1.Deployment{ { @@ -108,12 +117,14 @@ func TestGetDeployments(t *testing.T) { }, }, }, - expected: map[string]time.Duration{"dep1": time.Duration(100) * time.Second, - "dep2": time.Duration(200) * time.Second}, + expected: map[resources.ResourceObj]time.Duration{ + *resources.NewResource("dep1", "test"): time.Duration(100) * time.Second, + *resources.NewResource("dep2", "test"): time.Duration(200) * time.Second, + }, }, { description: "no deployments", - expected: map[string]time.Duration{}, + expected: map[resources.ResourceObj]time.Duration{}, }, { description: "multiple deployments in different namespaces", @@ -139,7 +150,9 @@ func TestGetDeployments(t *testing.T) { Spec: appsv1.DeploymentSpec{ProgressDeadlineSeconds: utilpointer.Int32Ptr(100)}, }, }, - expected: map[string]time.Duration{"dep1": time.Duration(100) * time.Second}, + expected: map[resources.ResourceObj]time.Duration{ + *resources.NewResource("dep1", "test"): time.Duration(100) * time.Second, + }, }, { description: "deployment in correct namespace but not deployed by skaffold", @@ -155,7 +168,7 @@ func TestGetDeployments(t *testing.T) { Spec: appsv1.DeploymentSpec{ProgressDeadlineSeconds: utilpointer.Int32Ptr(100)}, }, }, - expected: map[string]time.Duration{}, + expected: map[resources.ResourceObj]time.Duration{}, }, { description: "deployment in correct namespace deployed by skaffold but different run", @@ -171,7 +184,7 @@ func TestGetDeployments(t *testing.T) { Spec: appsv1.DeploymentSpec{ProgressDeadlineSeconds: utilpointer.Int32Ptr(100)}, }, }, - expected: map[string]time.Duration{}, + expected: map[resources.ResourceObj]time.Duration{}, }, } @@ -183,7 +196,7 @@ func TestGetDeployments(t *testing.T) { } client := fakekubeclientset.NewSimpleClientset(objs...) actual, err := getDeployments(client, "test", labeller, time.Duration(200)*time.Second) - t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, actual) + t.CheckErrorAndDeepEqual(test.shouldErr, err, &test.expected, &actual) }) } } @@ -228,7 +241,8 @@ func TestPollDeploymentRolloutStatus(t *testing.T) { actual := &sync.Map{} cli := &kubectl.CLI{KubeContext: testKubeContext, Namespace: "test"} - pollDeploymentRolloutStatus(context.Background(), cli, "dep", time.Duration(test.duration)*time.Millisecond, actual) + r := resources.NewResource("dep", "test") + pollDeploymentRolloutStatus(context.Background(), cli, r, time.Duration(test.duration)*time.Millisecond, actual) if _, ok := actual.Load("dep"); !ok { t.Error("expected result for deployment dep. But found none") } @@ -334,3 +348,64 @@ func TestGetRollOutStatus(t *testing.T) { }) } } + +func TestPrintStatus(t *testing.T) { + tests := []struct { + description string + rs []*resources.ResourceObj + expectedOut string + expected bool + }{ + { + description: "single resource successful marked complete - skip print", + rs: []*resources.ResourceObj{ + resources.NewResource("r1", "test").WithUpdatedStatus( + resources.NewStatus("success", "ready", nil), + ).MarkCheckComplete(), + }, + expected: true, + }, + { + description: "single resource in error marked complete -skip print", + rs: []*resources.ResourceObj{ + resources.NewResource("r1", "test").WithUpdatedStatus( + resources.NewStatus("", "error", fmt.Errorf("error")), + ).MarkCheckComplete(), + }, + expected: true, + }, + { + description: "multiple resources 1 not complete", + rs: []*resources.ResourceObj{ + resources.NewResource("r1", "test").WithUpdatedStatus( + resources.NewStatus("succes", "ready", nil), + ).MarkCheckComplete(), + resources.NewResource("r2", "test").WithUpdatedStatus( + resources.NewStatus("pending", "pending", nil), + ), + }, + expectedOut: " - test:deployment/r2 pending\n", + }, + { + description: "multiple resources 1 not complete and in error", + rs: []*resources.ResourceObj{ + resources.NewResource("r1", "test").WithUpdatedStatus( + resources.NewStatus("succes", "ready", nil), + ).MarkCheckComplete(), + resources.NewResource("r2", "test").WithUpdatedStatus( + resources.NewStatus("", "", fmt.Errorf("context deadline expired")), + ), + }, + expectedOut: " - test:deployment/r2 context deadline expired\n", + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + out := new(bytes.Buffer) + actual := printStatus(test.rs, out) + t.CheckDeepEqual(test.expectedOut, out.String()) + t.CheckDeepEqual(test.expected, actual) + }) + } +} diff --git a/pkg/skaffold/runner/deploy.go b/pkg/skaffold/runner/deploy.go index 69584dfbe42..6f31e2fbf73 100644 --- a/pkg/skaffold/runner/deploy.go +++ b/pkg/skaffold/runner/deploy.go @@ -19,6 +19,7 @@ package runner import ( "context" "io" + "time" "github.com/pkg/errors" @@ -47,12 +48,13 @@ func (r *SkaffoldRunner) Deploy(ctx context.Context, out io.Writer, artifacts [] func (r *SkaffoldRunner) performStatusCheck(ctx context.Context, out io.Writer) error { // Check if we need to perform deploy status if r.runCtx.Opts.StatusCheck { + start := time.Now() color.Default.Fprintln(out, "Waiting for deployments to stabilize") - err := statusCheck(ctx, r.defaultLabeller, r.runCtx) + err := statusCheck(ctx, r.defaultLabeller, r.runCtx, out) if err != nil { - color.Default.Fprintln(out, err.Error()) + return err } - return err + color.Default.Fprintln(out, "Deployments stabilized in", time.Since(start)) } return nil } diff --git a/pkg/skaffold/runner/deploy_test.go b/pkg/skaffold/runner/deploy_test.go index f5802b0f498..038bb860ae9 100644 --- a/pkg/skaffold/runner/deploy_test.go +++ b/pkg/skaffold/runner/deploy_test.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "errors" + "io" "io/ioutil" "strings" "testing" @@ -59,7 +60,7 @@ func TestDeploy(t *testing.T) { }, } - dummyStatusCheck := func(ctx context.Context, l *deploy.DefaultLabeller, runCtx *runcontext.RunContext) error { + dummyStatusCheck := func(context.Context, *deploy.DefaultLabeller, *runcontext.RunContext, io.Writer) error { return nil } for _, test := range tests { @@ -109,7 +110,7 @@ func TestDeployNamespace(t *testing.T) { }, } - dummyStatusCheck := func(ctx context.Context, l *deploy.DefaultLabeller, runCtx *runcontext.RunContext) error { + dummyStatusCheck := func(context.Context, *deploy.DefaultLabeller, *runcontext.RunContext, io.Writer) error { return nil } for _, test := range tests { diff --git a/pkg/skaffold/util/util.go b/pkg/skaffold/util/util.go index ee96cc83374..0501f648771 100644 --- a/pkg/skaffold/util/util.go +++ b/pkg/skaffold/util/util.go @@ -321,3 +321,7 @@ func IsHiddenFile(filename string) bool { func hasHiddenPrefix(s string) bool { return strings.HasPrefix(s, hiddenPrefix) } + +func Trim(msg string) string { + return strings.TrimSuffix(msg, "\n") +} From 69f66a6d6847db7b6ea080da587f91d3477fb204 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 4 Sep 2019 16:58:07 -0700 Subject: [PATCH 2/7] revert accidental change --- examples/microservices/leeroy-app/kubernetes/deployment.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/microservices/leeroy-app/kubernetes/deployment.yaml b/examples/microservices/leeroy-app/kubernetes/deployment.yaml index dfba0a5920c..56ef7152fef 100644 --- a/examples/microservices/leeroy-app/kubernetes/deployment.yaml +++ b/examples/microservices/leeroy-app/kubernetes/deployment.yaml @@ -19,7 +19,7 @@ metadata: labels: app: leeroy-app spec: - replicas: 4 + replicas: 1 selector: matchLabels: app: leeroy-app From d6bab7a41a141cc715dc290d519ed61a7f2159bf Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 5 Sep 2019 08:43:12 -0700 Subject: [PATCH 3/7] fix linter --- pkg/skaffold/deploy/resources/resource.go | 4 ++-- pkg/skaffold/deploy/status_check.go | 13 ++++++------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/skaffold/deploy/resources/resource.go b/pkg/skaffold/deploy/resources/resource.go index 89ce2482f45..e9e3dbf6320 100644 --- a/pkg/skaffold/deploy/resources/resource.go +++ b/pkg/skaffold/deploy/resources/resource.go @@ -101,9 +101,9 @@ func (rs *Status) Equals(other *Status) bool { func (rs *Status) String() string { if rs.err != nil { - return fmt.Sprintf("%s", util.Trim(rs.err.Error())) + return util.Trim(rs.err.Error()) } - return fmt.Sprintf("%s", rs.details) + return rs.details } func NewResource(name string, ns string) *ResourceObj { diff --git a/pkg/skaffold/deploy/status_check.go b/pkg/skaffold/deploy/status_check.go index 7dbd8a2154f..896b51dee44 100644 --- a/pkg/skaffold/deploy/status_check.go +++ b/pkg/skaffold/deploy/status_check.go @@ -49,13 +49,12 @@ var ( ) type Checker struct { - context context.Context - runCtx *runcontext.RunContext - out io.Writer - labeller *DefaultLabeller - client *kubectl.CLI - numDeps int - processedDeps int32 + context context.Context + runCtx *runcontext.RunContext + out io.Writer + labeller *DefaultLabeller + client *kubectl.CLI + numDeps int } func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx *runcontext.RunContext, out io.Writer) error { From 268ab6378341314968b55b3b0edc21f001068854 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 5 Sep 2019 10:42:32 -0700 Subject: [PATCH 4/7] remove unused structs from the Checker --- pkg/skaffold/deploy/status_check.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/pkg/skaffold/deploy/status_check.go b/pkg/skaffold/deploy/status_check.go index 896b51dee44..20fd3f40bba 100644 --- a/pkg/skaffold/deploy/status_check.go +++ b/pkg/skaffold/deploy/status_check.go @@ -49,12 +49,9 @@ var ( ) type Checker struct { - context context.Context - runCtx *runcontext.RunContext - out io.Writer - labeller *DefaultLabeller - client *kubectl.CLI - numDeps int + context context.Context + out io.Writer + numDeps int } func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx *runcontext.RunContext, out io.Writer) error { @@ -76,12 +73,9 @@ func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx * kubeCtl := kubectl.NewFromRunContext(runCtx) checker := &Checker{ - context: ctx, - runCtx: runCtx, - labeller: defaultLabeller, - client: kubectl.NewFromRunContext(runCtx), - out: out, - numDeps: len(rsMap), + context: ctx, + out: out, + numDeps: len(rsMap), } rs := make([]*resources.ResourceObj, 0, len(rsMap)) From 4f6da358735659290a9a3c1636cf8a860c36d47f Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 11 Sep 2019 16:59:24 -0700 Subject: [PATCH 5/7] minimal diff --- pkg/skaffold/deploy/resource/deployment.go | 13 +- .../deploy/resource/deployment_test.go | 84 ++++++++++ pkg/skaffold/deploy/resource/status.go | 15 +- pkg/skaffold/deploy/resource/status_test.go | 67 ++++++++ pkg/skaffold/deploy/resources/resource.go | 65 -------- .../deploy/resources/resource_test.go | 153 ------------------ pkg/skaffold/deploy/status_check_test.go | 45 +++++- pkg/skaffold/util/util.go | 4 - 8 files changed, 220 insertions(+), 226 deletions(-) create mode 100644 pkg/skaffold/deploy/resource/deployment_test.go create mode 100644 pkg/skaffold/deploy/resource/status_test.go delete mode 100644 pkg/skaffold/deploy/resources/resource.go delete mode 100644 pkg/skaffold/deploy/resources/resource_test.go diff --git a/pkg/skaffold/deploy/resource/deployment.go b/pkg/skaffold/deploy/resource/deployment.go index 7f848d119d8..5aee07a3b1c 100644 --- a/pkg/skaffold/deploy/resource/deployment.go +++ b/pkg/skaffold/deploy/resource/deployment.go @@ -50,7 +50,18 @@ func (d *Deployment) Status() Status { } func (d *Deployment) UpdateStatus(details string, err error) { - d.status = newStatus(details, err) + updated := newStatus(details, err) + if !d.status.Equal(updated) { + d.status = updated + } +} + +func (d *Deployment) ReportSinceLastUpdated() string { + if d.status.reported { + return "" + } + d.status.reported = true + return fmt.Sprintf("%s %s", d, d.status) } func NewDeployment(name string, ns string, deadline time.Duration) *Deployment { diff --git a/pkg/skaffold/deploy/resource/deployment_test.go b/pkg/skaffold/deploy/resource/deployment_test.go new file mode 100644 index 00000000000..cddfff19e04 --- /dev/null +++ b/pkg/skaffold/deploy/resource/deployment_test.go @@ -0,0 +1,84 @@ +/* +Copyright 2019 The Skaffold Authors + +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 resource + +import ( + "testing" + + "github.com/GoogleContainerTools/skaffold/testutil" + "github.com/pkg/errors" +) + +func TestReportSinceLastUpdated(t *testing.T) { + var tests = []struct { + description string + message string + err error + expected string + }{ + { + description: "updating an error status", + message: "cannot pull image", + err: errors.New("cannot pull image"), + expected: "test-ns:deployment/test cannot pull image\n", + }, + { + description: "updating a non error status", + message: "is waiting for container", + expected: "test-ns:deployment/test is waiting for container\n", + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + dep := NewDeployment("test", "test-ns", 1) + dep.UpdateStatus(test.message, test.err) + // Check reported is set to true. + t.CheckDeepEqual(true, dep.status.reported) + t.CheckDeepEqual(test.expected, dep.ReportSinceLastUpdated()) + }) + } +} + +func TestReportSinceLastUpdatedMultipleTimes(t *testing.T) { + var tests = []struct { + description string + times int + expected string + }{ + { + description: "report first time should return status", + times: 1, + expected: "test-ns:deployment/test cannot pull image\n", + }, + { + description: "report 2nd time should not return", + times: 2, + expected: "", + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + dep := NewDeployment("test", "test-ns", 1) + dep.UpdateStatus("cannot pull image", nil) + var actual string + for i := 0; i < test.times; i++ { + actual = dep.ReportSinceLastUpdated() + } + t.CheckDeepEqual(test.expected, actual) + }) + } +} diff --git a/pkg/skaffold/deploy/resource/status.go b/pkg/skaffold/deploy/resource/status.go index 5dc1fce6500..de1dff84b19 100644 --- a/pkg/skaffold/deploy/resource/status.go +++ b/pkg/skaffold/deploy/resource/status.go @@ -17,8 +17,9 @@ limitations under the License. package resource type Status struct { - err error - details string + err error + details string + reported bool } func (rs Status) Error() error { @@ -32,6 +33,16 @@ func (rs Status) String() string { return rs.details } +func (rs Status) Equal(other Status) bool { + if rs.details != other.details { + return false + } + if rs.err != nil && other.err != nil { + return rs.err.Error() == rs.err.Error() + } + return rs.err == other.err +} + func newStatus(msg string, err error) Status { return Status{ details: msg, diff --git a/pkg/skaffold/deploy/resource/status_test.go b/pkg/skaffold/deploy/resource/status_test.go new file mode 100644 index 00000000000..7d58a121eec --- /dev/null +++ b/pkg/skaffold/deploy/resource/status_test.go @@ -0,0 +1,67 @@ +/* +Copyright 2019 The Skaffold Authors + +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 resource + +import ( + "errors" + "fmt" + "testing" + + "github.com/GoogleContainerTools/skaffold/testutil" +) + +func TestEqual(t *testing.T) { + var tests = []struct { + description string + old Status + new Status + expected bool + }{ + { + description: "status should be same for same details and error", + old: Status{details: "Waiting for 0/1 replicas to be available...", err: nil}, + new: Status{details: "Waiting for 0/1 replicas to be available...", err: nil}, + expected: true, + }, + { + description: "status should be new if error messages are same", + old: Status{details: "same", err: errors.New("same error")}, + new: Status{details: "same", err: errors.New("same error")}, + expected: true, + }, + { + description: "status should be new if error is different", + old: Status{details: "same", err: nil}, + new: Status{details: "same", err: fmt.Errorf("see this error")}, + }, + { + description: "status should be new if details and err are different", + old: Status{details: "same", err: nil}, + new: Status{details: "same", err: fmt.Errorf("see this error")}, + }, + { + description: "status should be new if details change", + old: Status{details: "same", err: nil}, + new: Status{details: "error", err: nil}, + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + t.CheckDeepEqual(test.expected, test.old.Equal(test.new)) + }) + } +} diff --git a/pkg/skaffold/deploy/resources/resource.go b/pkg/skaffold/deploy/resources/resource.go deleted file mode 100644 index d8d0c026041..00000000000 --- a/pkg/skaffold/deploy/resources/resource.go +++ /dev/null @@ -1,65 +0,0 @@ -/* -Copyright 2019 The Skaffold Authors - -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 resources - -import ( - "fmt" - "io" - - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" -) - -func (r *ResourceObj) ReportSinceLastUpdated(out io.Writer) { - if !r.status.updated { - return - } - r.status.updated = false - color.Default.Fprintln(out, fmt.Sprintf("%s %s %s", TabHeader, r.String(), r.status.String())) -} - -type Status struct { - err error - reason string - details string - updated bool - completed bool -} - -func (rs *Status) Error() error { - return rs.err -} - -func (rs *Status) Equals(other *Status) bool { - return util.Trim(rs.reason) == util.Trim(other.reason) -} - -func (rs *Status) String() string { - if rs.err != nil { - return util.Trim(rs.err.Error()) - } - return rs.details -} - - -func NewStatus(msg string, reason string, err error) Status { - return Status{ - details: msg, - reason: reason, - err: err, - } -} diff --git a/pkg/skaffold/deploy/resources/resource_test.go b/pkg/skaffold/deploy/resources/resource_test.go deleted file mode 100644 index 9cf70011cf3..00000000000 --- a/pkg/skaffold/deploy/resources/resource_test.go +++ /dev/null @@ -1,153 +0,0 @@ -/* -Copyright 2019 The Skaffold Authors - -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 resources - -import ( - "bytes" - "fmt" - "testing" - - "github.com/GoogleContainerTools/skaffold/testutil" -) - -func TestUpdateTimestamp(t *testing.T) { - dep := NewResource("test", "test-ns") - - // Check updated bool is false - testutil.CheckDeepEqual(t, false, dep.status.updated) - - // Update the status - dep.UpdateStatus("success", "success", nil) - - // Check the updated bool is true - testutil.CheckDeepEqual(t, true, dep.status.updated) -} - -func TestReportSinceLastUpdated(t *testing.T) { - var tests = []struct { - description string - message string - err error - expected string - }{ - { - description: "updating an error status", - message: "cannot pull image", - err: fmt.Errorf("cannot pull image"), - expected: " - test-ns:deployment/test cannot pull image\n", - }, - { - description: "updating a non error status", - message: "is waiting for container", - expected: " - test-ns:deployment/test is waiting for container\n", - }, - } - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - dep := NewResource("test", "test-ns") - out := new(bytes.Buffer) - dep.UpdateStatus(test.message, test.message, test.err) - dep.ReportSinceLastUpdated(out) - t.CheckDeepEqual(test.expected, out.String()) - }) - } -} - -func TestReportSinceLastUpdatedMultipleTimes(t *testing.T) { - var tests = []struct { - description string - times int - expected string - }{ - { - description: "report first time should write to out", - times: 1, - expected: " - test-ns:deployment/test cannot pull image\n", - }, - { - description: "report 2nd time should not write to out", - times: 2, - expected: "", - }, - } - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - dep := NewResource("test", "test-ns") - dep.UpdateStatus("cannot pull image", "err", nil) - var out *bytes.Buffer - for i := 0; i < test.times; i++ { - out = new(bytes.Buffer) - dep.ReportSinceLastUpdated(out) - // Check reported timestamp is set to false - t.CheckDeepEqual(false, dep.status.updated) - } - t.CheckDeepEqual(test.expected, out.String()) - }) - } -} - -func TestUpdateStatus(t *testing.T) { - var tests = []struct { - description string - old Status - new Status - expectChange bool - }{ - { - description: "updated should be false for same statuses", - old: Status{details: "Waiting for 0/1 replicas to be available...", reason: "Waiting for 0/1 replicas to be available...", err: nil}, - new: Status{details: "Waiting for 0/1 replicas to be available...", reason: "Waiting for 0/1 replicas to be available...", err: nil}, - expectChange: false, - }, - { - description: "updated should be true if reason changes", - old: Status{details: "same", reason: "same", err: nil}, - new: Status{details: "same", reason: "another", err: nil}, - expectChange: true, - }, - { - description: "updated should be false if reason is same", - old: Status{details: "same", reason: "same", err: nil}, - new: Status{details: "same", reason: "same", err: fmt.Errorf("see this error")}, - }, - { - description: "updated should be true if reason and err change", - old: Status{details: "same", reason: "same", err: nil}, - new: Status{details: "same", reason: "another", err: fmt.Errorf("see this error")}, - expectChange: true, - }, - { - description: "updated should be true if both reason and details change", - old: Status{details: "same", reason: "same", err: nil}, - new: Status{details: "error", reason: "error", err: nil}, - expectChange: true, - }, - { - description: "updated should be false if both reason has a new line", - old: Status{details: "same", reason: "same\n", err: nil}, - new: Status{details: "same", reason: "same", err: nil}, - expectChange: false, - }, - } - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - dep := NewResource("test", "test-ns").WithStatus(test.old) - dep.UpdateStatus(test.new.details, test.new.reason, test.new.err) - t.CheckDeepEqual(test.expectChange, dep.status.updated) - }) - } -} diff --git a/pkg/skaffold/deploy/status_check_test.go b/pkg/skaffold/deploy/status_check_test.go index 9eab80781c8..8425a7c0485 100644 --- a/pkg/skaffold/deploy/status_check_test.go +++ b/pkg/skaffold/deploy/status_check_test.go @@ -17,6 +17,7 @@ limitations under the License. package deploy import ( + "bytes" "context" "errors" "testing" @@ -149,7 +150,7 @@ func TestGetDeployments(t *testing.T) { }, }, expected: []*resource.Deployment{ - resource.NewDeployment("dep2", "test", time.Duration(100)*time.Second), + resource.NewDeployment("dep1", "test", time.Duration(100)*time.Second), }, }, { @@ -345,3 +346,45 @@ func TestGetRollOutStatus(t *testing.T) { }) } } + +func TestPrintSummaryStatus(t *testing.T) { + tests := []struct { + description string + pending int32 + err error + expected string + }{ + { + description: "no deployment left and current is in success", + pending: 0, + err: nil, + expected: " - test:deployment/dep is ready.\n", + }, + { + description: "no deployment left and current is in error", + pending: 0, + err: errors.New("context deadline expired"), + expected: " - test:deployment/dep failed. Error: context deadline expired.\n", + }, + { + description: "more than 1 deployment left and current is in success", + pending: 4, + err: nil, + expected: " - test:deployment/dep is ready. [4/10 deployment(s) still pending]\n", + }, + { + description: "more than 1 deployment left and current is in error", + pending: 8, + err: errors.New("context deadline expired"), + expected: " - test:deployment/dep failed. [8/10 deployment(s) still pending] Error: context deadline expired.\n", + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + out := new(bytes.Buffer) + printStatusCheckSummary(resource.NewDeployment("dep", "test", 0), int(test.pending), 10, test.err, out) + t.CheckDeepEqual(test.expected, out.String()) + }) + } +} diff --git a/pkg/skaffold/util/util.go b/pkg/skaffold/util/util.go index 0501f648771..ee96cc83374 100644 --- a/pkg/skaffold/util/util.go +++ b/pkg/skaffold/util/util.go @@ -321,7 +321,3 @@ func IsHiddenFile(filename string) bool { func hasHiddenPrefix(s string) bool { return strings.HasPrefix(s, hiddenPrefix) } - -func Trim(msg string) string { - return strings.TrimSuffix(msg, "\n") -} From ee038d0ec3dd6811aff061b4117bb3ae4b6a9c78 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 11 Sep 2019 18:06:01 -0700 Subject: [PATCH 6/7] fix tests --- pkg/skaffold/deploy/resource/deployment_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/skaffold/deploy/resource/deployment_test.go b/pkg/skaffold/deploy/resource/deployment_test.go index cddfff19e04..b40c4253c51 100644 --- a/pkg/skaffold/deploy/resource/deployment_test.go +++ b/pkg/skaffold/deploy/resource/deployment_test.go @@ -34,21 +34,21 @@ func TestReportSinceLastUpdated(t *testing.T) { description: "updating an error status", message: "cannot pull image", err: errors.New("cannot pull image"), - expected: "test-ns:deployment/test cannot pull image\n", + expected: "test-ns:deployment/test cannot pull image", }, { description: "updating a non error status", message: "is waiting for container", - expected: "test-ns:deployment/test is waiting for container\n", + expected: "test-ns:deployment/test is waiting for container", }, } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { dep := NewDeployment("test", "test-ns", 1) dep.UpdateStatus(test.message, test.err) + t.CheckDeepEqual(test.expected, dep.ReportSinceLastUpdated()) // Check reported is set to true. t.CheckDeepEqual(true, dep.status.reported) - t.CheckDeepEqual(test.expected, dep.ReportSinceLastUpdated()) }) } } @@ -62,7 +62,7 @@ func TestReportSinceLastUpdatedMultipleTimes(t *testing.T) { { description: "report first time should return status", times: 1, - expected: "test-ns:deployment/test cannot pull image\n", + expected: "test-ns:deployment/test cannot pull image", }, { description: "report 2nd time should not return", From 4ec3f8af34126c8b9b1b1f664cfe9dbfe05c8843 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 11 Sep 2019 21:36:27 -0700 Subject: [PATCH 7/7] fix linter issue --- pkg/skaffold/deploy/resource/status.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/skaffold/deploy/resource/status.go b/pkg/skaffold/deploy/resource/status.go index de1dff84b19..f048923a894 100644 --- a/pkg/skaffold/deploy/resource/status.go +++ b/pkg/skaffold/deploy/resource/status.go @@ -38,7 +38,7 @@ func (rs Status) Equal(other Status) bool { return false } if rs.err != nil && other.err != nil { - return rs.err.Error() == rs.err.Error() + return rs.err.Error() == other.err.Error() } return rs.err == other.err }