Move Logger from Runner into Deployer (#5990)

* Move Logger inside Deployer

* add deploy.TrackBuildArtifacts() to clarify this behavior is required of Deployer

* Synchronize watcher.Stop() channel close

* Add docs for Logger and NoopLogger

integration/dev_test.go

@@ -22,7 +22,9 @@ import (
 	"io/ioutil"
 	"net/http"
 	"os"
+	"runtime"
 	"strings"
+	"syscall"
 	"testing"
 	"time"
 
@@ -82,6 +84,58 @@ func TestDevNotification(t *testing.T) {
 	}
 }
 
+func TestDevGracefulCancel(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("graceful cancel doesn't work on windows")
+	}
+
+	tests := []struct {
+		name        string
+		dir         string
+		pods        []string
+		deployments []string
+	}{
+		{
+			name: "getting-started",
+			dir:  "examples/getting-started",
+			pods: []string{"getting-started"},
+		},
+		{
+			name:        "multi-config-microservices",
+			dir:         "examples/multi-config-microservices",
+			deployments: []string{"leeroy-app", "leeroy-web"},
+		},
+		{
+			name: "multiple deployers",
+			dir:  "testdata/deploy-multiple",
+			pods: []string{"deploy-kubectl", "deploy-kustomize"},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			ns, client := SetupNamespace(t)
+			p, _ := skaffold.Dev().InDir(test.dir).InNs(ns.Name).StartWithProcess(t)
+			client.WaitForPodsReady(test.pods...)
+			client.WaitForDeploymentsToStabilize(test.deployments...)
+
+			defer func() {
+				state, _ := p.Wait()
+
+				// We can't `recover()` from a remotely panicked process, but we can check exit code instead.
+				// Exit code 2 means the process panicked.
+				// https://github.com/golang/go/issues/24284
+				if state.ExitCode() == 2 {
+					t.Fail()
+				}
+			}()
+
+			// once deployments are stable, send a SIGINT and make sure things cleanup correctly
+			p.Signal(syscall.SIGINT)
+		})
+	}
+}
+
 func TestDevAPITriggers(t *testing.T) {
 	MarkIntegrationTest(t, CanRunWithoutGcp)
 

integration/render_test.go

@@ -30,6 +30,7 @@ import (
 
 	"github.com/GoogleContainerTools/skaffold/integration/skaffold"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/helm"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/kubectl"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
@@ -88,7 +89,7 @@ spec:
 					},
 				},
 			}}),
-		}, nil, &latestV1.KubectlDeploy{
+		}, nil, deploy.NoopComponentProvider, &latestV1.KubectlDeploy{
 			Manifests: []string{"deployment.yaml"},
 		})
 		t.RequireNoError(err)
@@ -248,7 +249,7 @@ spec:
 				Opts: config.SkaffoldOptions{
 					AddSkaffoldLabels: true,
 				},
-			}, nil, &latestV1.KubectlDeploy{
+			}, nil, deploy.NoopComponentProvider, &latestV1.KubectlDeploy{
 				Manifests: []string{"deployment.yaml"},
 			})
 			t.RequireNoError(err)
@@ -434,7 +435,7 @@ spec:
 						},
 					},
 				}}),
-			}, nil, &latestV1.HelmDeploy{
+			}, nil, deploy.NoopComponentProvider, &latestV1.HelmDeploy{
 				Releases: test.helmReleases,
 			})
 			t.RequireNoError(err)

integration/skaffold/helper.go

@@ -246,14 +246,25 @@ func (b *RunBuilder) Run(t *testing.T) error {
 	cmd := b.cmd(context.Background())
 	logrus.Infof("Running %s in %s", cmd.Args, cmd.Dir)
 
-	start := time.Now()
 	if err := cmd.Run(); err != nil {
 		return fmt.Errorf("skaffold %q: %w", b.command, err)
 	}
-	logrus.Infof("Ran %s in %v", cmd.Args, util.ShowHumanizeTime(time.Since(start)))
 	return nil
 }
 
+// StartWithProcess starts the skaffold command and returns the process id and error.
+func (b *RunBuilder) StartWithProcess(t *testing.T) (*os.Process, error) {
+	t.Helper()
+
+	cmd := b.cmd(context.Background())
+	logrus.Infof("Running %s in %s", cmd.Args, cmd.Dir)
+
+	if err := cmd.Start(); err != nil {
+		return nil, fmt.Errorf("skaffold %q: %w", b.command, err)
+	}
+	return cmd.Process, nil
+}
+
 // RunWithCombinedOutput runs the skaffold command and returns the combined standard output and error.
 func (b *RunBuilder) RunWithCombinedOutput(t *testing.T) ([]byte, error) {
 	t.Helper()

pkg/skaffold/deploy/deploy.go

@@ -21,8 +21,12 @@ import (
 	"io"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/log"
 )
 
+// NoopComponentProvider is for tests
+var NoopComponentProvider = ComponentProvider{Logger: &log.NoopProvider{}}
+
 // Deployer is the Deploy API of skaffold and responsible for deploying
 // the build results to a Kubernetes cluster
 type Deployer interface {
@@ -40,4 +44,16 @@ type Deployer interface {
 	// Render generates the Kubernetes manifests replacing the build results and
 	// writes them to the given file path
 	Render(context.Context, io.Writer, []graph.Artifact, bool, string) error
+
+	// GetLogger returns a Deployer's implementation of a Logger
+	GetLogger() log.Logger
+
+	// TrackBuildArtifacts registers build artifacts to be tracked by a Deployer
+	TrackBuildArtifacts([]graph.Artifact)
+}
+
+// ComponentProvider serves as a clean way to send three providers
+// as params to the Deployer constructors
+type ComponentProvider struct {
+	Logger log.Provider
 }

pkg/skaffold/deploy/deploy_mux.go

@@ -26,6 +26,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/instrumentation"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/manifest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/log"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 )
 
@@ -34,6 +35,14 @@ import (
 // it collects the results and returns it in bulk.
 type DeployerMux []Deployer
 
+func (m DeployerMux) GetLogger() log.Logger {
+	var loggers log.LoggerMux
+	for _, deployer := range m {
+		loggers = append(loggers, deployer.GetLogger())
+	}
+	return loggers
+}
+
 func (m DeployerMux) Deploy(ctx context.Context, w io.Writer, as []graph.Artifact) ([]string, error) {
 	seenNamespaces := util.NewStringSet()
 
@@ -95,3 +104,6 @@ func (m DeployerMux) Render(ctx context.Context, w io.Writer, as []graph.Artifac
 	allResources := strings.Join(resources, "\n---\n")
 	return manifest.Write(strings.TrimSpace(allResources), filepath, w)
 }
+
+// TrackBuildArtifacts should *only* be called on individual deployers. This is a noop.
+func (m DeployerMux) TrackBuildArtifacts(_ []graph.Artifact) {}

pkg/skaffold/deploy/deploy_mux_test.go

@@ -26,6 +26,7 @@ import (
 	"testing"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/log"
 	latestV1 "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest/v1"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 	testEvent "github.com/GoogleContainerTools/skaffold/testutil/event"
@@ -44,6 +45,12 @@ type MockDeployer struct {
 	renderErr        error
 }
 
+func (m *MockDeployer) GetLogger() log.Logger {
+	return &log.NoopLogger{}
+}
+
+func (m *MockDeployer) TrackBuildArtifacts(_ []graph.Artifact) {}
+
 func (m *MockDeployer) Dependencies() ([]string, error) {
 	return m.dependencies, m.dependenciesErr
 }

pkg/skaffold/deploy/helm/deploy.go

@@ -37,6 +37,7 @@ import (
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy"
 	deployerr "github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/error"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/kubectl"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/label"
@@ -46,6 +47,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/instrumentation"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/manifest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/log"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/output"
 	latestV1 "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest/v1"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
@@ -76,6 +78,7 @@ var (
 type Deployer struct {
 	*latestV1.HelmDeploy
 
+	logger         log.Logger
 	podSelector    *kubernetes.ImageList
 	originalImages []graph.Artifact
 
@@ -102,7 +105,7 @@ type Config interface {
 }
 
 // NewDeployer returns a configured Deployer.  Returns an error if current version of helm is less than 3.0.0.
-func NewDeployer(cfg Config, labels map[string]string, h *latestV1.HelmDeploy) (*Deployer, *kubernetes.ImageList, error) {
+func NewDeployer(cfg Config, labels map[string]string, p deploy.ComponentProvider, h *latestV1.HelmDeploy) (*Deployer, *kubernetes.ImageList, error) {
 	hv, err := binVer()
 	if err != nil {
 		return nil, nil, versionGetErr(err)
@@ -126,6 +129,7 @@ func NewDeployer(cfg Config, labels map[string]string, h *latestV1.HelmDeploy) (
 	return &Deployer{
 		HelmDeploy:     h,
 		podSelector:    podSelector,
+		logger:         p.Logger.GetKubernetesLogger(podSelector),
 		originalImages: originalImages,
 		kubeContext:    cfg.GetKubeContext(),
 		kubeConfig:     cfg.GetKubeConfig(),
@@ -139,6 +143,15 @@ func NewDeployer(cfg Config, labels map[string]string, h *latestV1.HelmDeploy) (
 	}, podSelector, nil
 }
 
+func (h *Deployer) GetLogger() log.Logger {
+	return h.logger
+}
+
+func (h *Deployer) TrackBuildArtifacts(artifacts []graph.Artifact) {
+	deployutil.AddTagsToPodSelector(artifacts, h.originalImages, h.podSelector)
+	h.logger.RegisterArtifacts(artifacts)
+}
+
 // Deploy deploys the build results to the Kubernetes cluster
 func (h *Deployer) Deploy(ctx context.Context, out io.Writer, builds []graph.Artifact) ([]string, error) {
 	ctx, endTrace := instrumentation.StartTrace(ctx, "Render", map[string]string{
@@ -190,7 +203,7 @@ func (h *Deployer) Deploy(ctx context.Context, out io.Writer, builds []graph.Art
 		namespaces = append(namespaces, ns)
 	}
 
-	deployutil.AddTagsToPodSelector(builds, h.originalImages, h.podSelector)
+	h.TrackBuildArtifacts(builds)
 	return namespaces, nil
 }
 

pkg/skaffold/deploy/helm/helm_test.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/mitchellh/go-homedir"
 
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/kubectl"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
@@ -471,7 +472,7 @@ func TestNewDeployer(t *testing.T) {
 		testutil.Run(t, test.description, func(t *testutil.T) {
 			t.Override(&util.DefaultExecCommand, testutil.CmdRunWithOutput("helm version --client", test.helmVersion))
 
-			_, _, err := NewDeployer(&helmConfig{}, nil, &testDeployConfig)
+			_, _, err := NewDeployer(&helmConfig{}, nil, deploy.NoopComponentProvider, &testDeployConfig)
 			t.CheckError(test.shouldErr, err)
 		})
 	}
@@ -1009,7 +1010,7 @@ func TestHelmDeploy(t *testing.T) {
 				namespace:  test.namespace,
 				force:      test.force,
 				configFile: "test.yaml",
-			}, nil, &test.helm)
+			}, nil, deploy.NoopComponentProvider, &test.helm)
 			t.RequireNoError(err)
 
 			if test.configure != nil {
@@ -1086,7 +1087,7 @@ func TestHelmCleanup(t *testing.T) {
 
 			deployer, _, err := NewDeployer(&helmConfig{
 				namespace: test.namespace,
-			}, nil, &test.helm)
+			}, nil, deploy.NoopComponentProvider, &test.helm)
 			t.RequireNoError(err)
 
 			deployer.Cleanup(context.Background(), ioutil.Discard)
@@ -1189,7 +1190,7 @@ func TestHelmDependencies(t *testing.T) {
 				local = tmpDir.Root()
 			}
 
-			deployer, _, err := NewDeployer(&helmConfig{}, nil, &latestV1.HelmDeploy{
+			deployer, _, err := NewDeployer(&helmConfig{}, nil, deploy.NoopComponentProvider, &latestV1.HelmDeploy{
 				Releases: []latestV1.HelmRelease{{
 					Name:                  "skaffold-helm",
 					ChartPath:             local,
@@ -1454,7 +1455,7 @@ func TestHelmRender(t *testing.T) {
 			t.Override(&util.DefaultExecCommand, test.commands)
 			deployer, _, err := NewDeployer(&helmConfig{
 				namespace: test.namespace,
-			}, nil, &test.helm)
+			}, nil, deploy.NoopComponentProvider, &test.helm)
 			t.RequireNoError(err)
 			err = deployer.Render(context.Background(), ioutil.Discard, test.builds, true, file)
 			t.CheckError(test.shouldErr, err)
@@ -1523,7 +1524,7 @@ func TestGenerateSkaffoldDebugFilter(t *testing.T) {
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
 			t.Override(&util.DefaultExecCommand, testutil.CmdRunWithOutput("helm version --client", version31))
-			h, _, err := NewDeployer(&helmConfig{}, nil, &testDeployConfig)
+			h, _, err := NewDeployer(&helmConfig{}, nil, deploy.NoopComponentProvider, &testDeployConfig)
 			t.RequireNoError(err)
 			result := h.generateSkaffoldDebugFilter(test.buildFile)
 			t.CheckDeepEqual(test.result, result)