diff --git a/pkg/skaffold/deploy/resources/deployment.go b/pkg/skaffold/deploy/resources/deployment.go index a06bf95553a..790310362e8 100644 --- a/pkg/skaffold/deploy/resources/deployment.go +++ b/pkg/skaffold/deploy/resources/deployment.go @@ -26,7 +26,9 @@ import ( ) const ( - DeploymentType = "deployment" + DeploymentType = "deployment" + KubectlKilled = "Killed" + KubectlConnection = "KubectlConnection" ) type Deployment struct { @@ -45,8 +47,9 @@ func (d *Deployment) CheckStatus(ctx context.Context, runCtx *runcontext.RunCont kubeCtl := kubectl.NewFromRunContext(runCtx) b, err := kubeCtl.RunOut(ctx, "rollout", "status", "deployment", d.name, "--namespace", d.namespace, "--watch=false") if err != nil { - d.UpdateStatus("", err.Error(), err) - if !strings.Contains(err.Error(), "Unable to connect to the server") { + reason, details := parseKubectlError(err.Error()) + d.UpdateStatus(details, reason, err) + if reason != KubectlConnection { d.checkComplete() } return @@ -64,6 +67,17 @@ func (d *Deployment) Deadline() time.Duration { } func (d *Deployment) WithError(err error) *Deployment { - d.UpdateStatus("", "", err) + d.UpdateStatus("", err.Error(), err) return d } + +func parseKubectlError(errMsg string) (string, string) { + errMsg = strings.TrimSuffix(errMsg, "\n") + if strings.Contains(errMsg, "Unable to connect to the server") { + return KubectlConnection, errMsg + } + if strings.Contains(errMsg, "signal: killed") { + return KubectlKilled, "kubectl killed due to timeout" + } + return errMsg, errMsg +} diff --git a/pkg/skaffold/deploy/resources/deployment_test.go b/pkg/skaffold/deploy/resources/deployment_test.go index ef37951685d..6e54aefb5d9 100644 --- a/pkg/skaffold/deploy/resources/deployment_test.go +++ b/pkg/skaffold/deploy/resources/deployment_test.go @@ -61,7 +61,7 @@ func TestDeploymentCheckStatus(t *testing.T) { description: "rollout kubectl client connection error", command: testutil.NewFakeCmd(t). WithRunOutErr(rolloutCmd, "", fmt.Errorf("Unable to connect to the server")), - expectedReason: "Unable to connect to the server", + expectedReason: "KubectlConnection", }, } diff --git a/pkg/skaffold/deploy/resources/resource.go b/pkg/skaffold/deploy/resources/resource.go index e8831e517a9..48cb54cd6b7 100644 --- a/pkg/skaffold/deploy/resources/resource.go +++ b/pkg/skaffold/deploy/resources/resource.go @@ -19,11 +19,16 @@ package resources import ( "fmt" "io" + "strings" "time" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" ) +const ( + TabHeader = " -" +) + type ResourceObj struct { name string namespace string @@ -43,8 +48,8 @@ func (r *ResourceObj) UpdateStatus(msg string, reason string, err error) { newStatus := Status{details: msg, reason: reason, err: err} if !r.status.Equal(&newStatus) { r.status.err = err - r.status.details = msg - r.status.reason = reason + r.status.details = strings.TrimSuffix(msg, "\n") + r.status.reason = strings.TrimSuffix(reason, "\n") r.status.lastUpdated = time.Now().UnixNano() } } @@ -74,7 +79,7 @@ func (r *ResourceObj) ReportSinceLastUpdated(out io.Writer) { return } r.status.lastReported = time.Now().UnixNano() - color.Default.Fprintln(out, fmt.Sprintf("%s %s", r.String(), r.status.String())) + color.Default.Fprintln(out, fmt.Sprintf("%s %s %s", TabHeader, r.String(), r.status.String())) } type Status struct { @@ -91,13 +96,18 @@ func (rs *Status) Error() error { } func (rs *Status) Equal(other *Status) bool { - return rs.reason == other.reason && rs.err == other.err - + if rs.reason == other.reason { + return true + } + if rs.err != nil && other.err != nil { + return rs.err.Error() == other.err.Error() + } + return false } func (rs *Status) String() string { if rs.err != nil { - return fmt.Sprintf("is pending due to %s", rs.err.Error()) + return fmt.Sprintf("is pending due to %s", strings.TrimSuffix(rs.err.Error(), "\n")) } return fmt.Sprintf("is pending due to %s", rs.details) } diff --git a/pkg/skaffold/deploy/resources/resource_test.go b/pkg/skaffold/deploy/resources/resource_test.go index 40903c90e1e..19efff56031 100644 --- a/pkg/skaffold/deploy/resources/resource_test.go +++ b/pkg/skaffold/deploy/resources/resource_test.go @@ -53,12 +53,13 @@ func TestReportSinceLastUpdated(t *testing.T) { }{ { description: "updating an error status", + message: "cannot pull image", err: fmt.Errorf("cannot pull image"), - expected: "deployment/test is pending due to cannot pull image\n", + expected: " - deployment/test is pending due to cannot pull image\n", }, { description: "updating a non error status", message: "is waiting for container", - expected: "deployment/test is pending due to is waiting for container\n", + expected: " - deployment/test is pending due to is waiting for container\n", }, } for _, test := range tests { @@ -81,7 +82,7 @@ func TestReportSinceLastUpdatedMultipleTimes(t *testing.T) { { description: "report first time should write to out", times: 1, - expected: "deployment/test is pending due to cannot pull image\n", + expected: " - deployment/test is pending due to cannot pull image\n", }, { description: "report 2nd time should not write to out", times: 2, @@ -117,24 +118,24 @@ func TestUpdateStatus(t *testing.T) { description: "updateTimestamp should not change for same statuses", old: Status{details: "same", reason: "same", err: nil}, new: Status{details: "same", reason: "same", err: nil}, - }, { - description: "updateTimestamp should not change for same statuses if details change", - old: Status{details: "same", reason: "same", err: nil}, - new: Status{details: "another", reason: "same", err: nil}, }, { description: "updateTimestamp should change if reason change", old: Status{details: "same", reason: "same", err: nil}, new: Status{details: "same", reason: "another", err: nil}, expectChange: true, }, { - description: "updateTimestamp should change if error change", + description: "updateTimestamp should not change if reason son", + old: Status{details: "same", reason: "same", err: nil}, + new: Status{details: "same", reason: "same", err: fmt.Errorf("see this error")}, + }, { + description: "updateTimestamp should change if reason and err change", old: Status{details: "same", reason: "same", err: nil}, - new: Status{details: "same", reason: "same", err: fmt.Errorf("see this error")}, + new: Status{details: "same", reason: "another", err: fmt.Errorf("see this error")}, expectChange: true, }, { - description: "updateTimestamp should change if both reason and error change", + description: "updateTimestamp should change if both reason and details change", old: Status{details: "same", reason: "same", err: nil}, - new: Status{reason: "error", err: fmt.Errorf("cannot pull image")}, + new: Status{details: "error", reason: "error", err: nil}, expectChange: true, }, } diff --git a/pkg/skaffold/deploy/status_check.go b/pkg/skaffold/deploy/status_check.go index c11dd71457a..e49649558f3 100644 --- a/pkg/skaffold/deploy/status_check.go +++ b/pkg/skaffold/deploy/status_check.go @@ -57,9 +57,7 @@ type Checker struct { labeller *DefaultLabeller client kubernetes.Interface numDeps int - numPods int processedDeps int32 - processedPods int32 } func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx *runcontext.RunContext, out io.Writer) error { @@ -105,7 +103,7 @@ func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx * // Retrieve pending resource states go func() { - checker.printResourceStatus(rs) + checker.printResourceStatus(rs, deadline) }() // Wait for all resource status to be fetched @@ -158,11 +156,13 @@ func (c *Checker) CheckResourceStatus(r Resource) { } // Print resource statuses until all status check are completed or context is cancelled. -func (c *Checker) printResourceStatus(rs []Resource) { +func (c *Checker) printResourceStatus(rs []Resource, deadline int32) { + timeoutContext, cancel := context.WithTimeout(c.context, time.Duration(deadline)*time.Second) + defer cancel() for { allResourcesCheckComplete := true select { - case <-c.context.Done(): + case <-timeoutContext.Done(): return case <-time.After(reportStatusTime): for _, r := range rs { @@ -201,11 +201,11 @@ func (c *Checker) printStatusCheckSummary(r Resource) { fmt.Sprintf("%s failed %s. Error: %s.", r.String(), waitingMsg, - strings.TrimRight(err.Error(), "\n"), + strings.TrimSuffix(err.Error(), "\n"), ), ) } else { - color.Default.Fprintln(c.out, fmt.Sprintf("%s is ready. %s", r.String(), waitingMsg)) + color.Default.Fprintln(c.out, fmt.Sprintf("%s %s is ready. %s", resources.TabHeader, r.String(), waitingMsg)) } } diff --git a/pkg/skaffold/deploy/status_check_test.go b/pkg/skaffold/deploy/status_check_test.go index 1671bba77b8..2d2d13e07f6 100644 --- a/pkg/skaffold/deploy/status_check_test.go +++ b/pkg/skaffold/deploy/status_check_test.go @@ -29,7 +29,6 @@ import ( fakekubeclientset "k8s.io/client-go/kubernetes/fake" utilpointer "k8s.io/utils/pointer" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/testutil" ) @@ -42,7 +41,7 @@ func TestFetchDeployments(t *testing.T) { shouldErr bool }{ { - description: "multiple deployments in same namespace", + description: "multiple resources.Deployments in same namespace", deps: []*appsv1.Deployment{ { ObjectMeta: metav1.ObjectMeta{ @@ -71,7 +70,7 @@ func TestFetchDeployments(t *testing.T) { resources.NewDeployment("dep2", "test", 20*time.Second), }, }, { - description: "command flag deadline is less than deployment spec.", + description: "command flag deadline is less than resources.Deployment spec.", deps: []*appsv1.Deployment{ { ObjectMeta: metav1.ObjectMeta{ @@ -88,7 +87,7 @@ func TestFetchDeployments(t *testing.T) { expected: []*resources.Deployment{resources.NewDeployment("dep1", "test", 200*time.Second)}, }, { - description: "multiple deployments with 1 no progress deadline set", + description: "multiple resources.Deployments with 1 no progress deadline set", deps: []*appsv1.Deployment{ { ObjectMeta: metav1.ObjectMeta{ @@ -116,11 +115,11 @@ func TestFetchDeployments(t *testing.T) { }, }, { - description: "no deployments", + description: "no resources.Deployments", expected: []*resources.Deployment{}, }, { - description: "multiple deployments in different namespaces", + description: "multiple resources.Deployments in different namespaces", deps: []*appsv1.Deployment{ { ObjectMeta: metav1.ObjectMeta{ @@ -148,7 +147,7 @@ func TestFetchDeployments(t *testing.T) { }, }, { - description: "deployment in correct namespace but not deployed by skaffold", + description: "resources.Deployment in correct namespace but not deployed by skaffold", deps: []*appsv1.Deployment{ { ObjectMeta: metav1.ObjectMeta{ @@ -164,7 +163,7 @@ func TestFetchDeployments(t *testing.T) { expected: []*resources.Deployment{}, }, { - description: "deployment in correct namespace deployed by skaffold but different run", + description: "resources.Deployment in correct namespace deployed by skaffold but different run", deps: []*appsv1.Deployment{ { ObjectMeta: metav1.ObjectMeta{ @@ -234,37 +233,3 @@ func TestIsSkaffoldDeployInError(t *testing.T) { }) } } - -func TestCheckResourceStatus(t *testing.T) { - rolloutCmd := "kubectl --context kubecontext --namespace test rollout status deployment dep --watch=false" - var tests = []struct { - description string - command util.Command - expected string - shouldErr bool - }{ - { - description: "some output", - command: testutil.NewFakeCmd(t). - WithRunOut(rolloutCmd, "Waiting for replicas to be available"), - expected: "Waiting for replicas to be available", - }, - { - description: "no output", - command: testutil.NewFakeCmd(t). - WithRunOut(rolloutCmd, ""), - }, - { - description: "rollout status error", - command: testutil.NewFakeCmd(t). - WithRunOutErr(rolloutCmd, "", fmt.Errorf("error")), - shouldErr: true, - }, - } - - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - - }) - } -} diff --git a/pkg/skaffold/runner/deploy.go b/pkg/skaffold/runner/deploy.go index fe6a557be21..1e0a995144c 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, out) if err != nil { - color.Default.Fprintln(out, err.Error()) + return err } - return err + color.Default.Fprintln(out, "Deployments stabilized in Xs", time.Since(start)) } return nil }