Merge pull request #61 from drone-runners/b7

Using runner-go 1.8.0; Updated podwatcher logging

engine/compiler/compiler.go

@@ -9,14 +9,13 @@ import (
 	"fmt"
 	"strings"
 
-	"k8s.io/apimachinery/pkg/util/validation"
-
 	"github.com/drone-runners/drone-runner-kube/engine"
 	"github.com/drone-runners/drone-runner-kube/engine/policy"
 	"github.com/drone-runners/drone-runner-kube/engine/resource"
 	"github.com/drone-runners/drone-runner-kube/internal/docker/image"
 
 	"github.com/drone/runner-go/clone"
+	"github.com/drone/runner-go/container"
 	"github.com/drone/runner-go/environ"
 	"github.com/drone/runner-go/environ/provider"
 	"github.com/drone/runner-go/labels"
@@ -28,6 +27,7 @@ import (
 
 	"github.com/dchest/uniuri"
 	"github.com/gosimple/slug"
+	"k8s.io/apimachinery/pkg/util/validation"
 )
 
 // random generator function
@@ -149,7 +149,7 @@ func (c *Compiler) Compile(ctx context.Context, args runtime.CompilerArgs) runti
 
 	// reset the workspace path if attempting to mount
 	// volumes that are internal use only.
-	if isRestrictedVolume(workspace) {
+	if container.IsRestrictedVolume(workspace) {
 		workspace = "/drone/src"
 	}
 
@@ -664,7 +664,7 @@ func (c *Compiler) isPrivileged(step *resource.Step) bool {
 	// privileged-by-default mode is disabled if the
 	// pipeline step mounts a restricted volume.
 	for _, mount := range step.Volumes {
-		if isRestrictedVolume(mount.MountPath) {
+		if container.IsRestrictedVolume(mount.MountPath) {
 			return false
 		}
 	}

engine/compiler/step.go

@@ -12,6 +12,7 @@ import (
 	"github.com/drone-runners/drone-runner-kube/internal/docker/image"
 	"github.com/drone-runners/drone-runner-kube/internal/encoder"
 
+	"github.com/drone/runner-go/environ"
 	"github.com/drone/runner-go/pipeline/runtime"
 )
 
@@ -27,7 +28,7 @@ func createStep(spec *resource.Pipeline, src *resource.Step) *engine.Step {
 		Entrypoint:   src.Entrypoint,
 		Detach:       src.Detach,
 		DependsOn:    src.DependsOn,
-		Envs:         convertStaticEnv(src.Environment),
+		Envs:         environ.Combine(convertStaticEnv(src.Environment), environ.StepName(src.Name)),
 		IgnoreStderr: false,
 		IgnoreStdout: false,
 		Privileged:   src.Privileged,

engine/compiler/util.go

@@ -5,7 +5,6 @@
 package compiler
 
 import (
-	"path/filepath"
 	"strings"
 
 	"github.com/drone-runners/drone-runner-kube/engine"
@@ -148,33 +147,6 @@ func convertPullPolicy(s string) engine.PullPolicy {
 	}
 }
 
-// helper function returns true if mounting the volume
-// is restricted for un-trusted containers.
-func isRestrictedVolume(path string) bool {
-	path, _ = filepath.Abs(path)
-	path = strings.ToLower(path)
-	switch {
-	case path == "/":
-	case path == "/var":
-	case path == "/etc":
-	case strings.Contains(path, "/var/run"):
-	case strings.Contains(path, "/proc"):
-	case strings.Contains(path, "/mount"):
-	case strings.Contains(path, "/bin"):
-	case strings.Contains(path, "/usr/local/bin"):
-	case strings.Contains(path, "/usr/local/sbin"):
-	case strings.Contains(path, "/usr/bin"):
-	case strings.Contains(path, "/mnt"):
-	case strings.Contains(path, "/media"):
-	case strings.Contains(path, "/sys"):
-	case strings.Contains(path, "/dev"):
-	case strings.Contains(path, "/etc/docker"):
-	default:
-		return false
-	}
-	return true
-}
-
 // helper function returns true if the environment variable
 // is restricted for internal-use only.
 func isRestrictedVariable(env map[string]*manifest.Variable) bool {

engine/engine_impl.go

@@ -187,6 +187,11 @@ func (k *Kubernetes) Destroy(ctx context.Context, specv runtime.Spec) error {
 				WithField("namespace", spec.PodSpec.Namespace).
 				Error("failed to wait for removal of pod")
 		}
+
+		logger.FromContext(ctx).
+			WithField("pod", spec.PodSpec.Name).
+			WithField("namespace", spec.PodSpec.Namespace).
+			Debug("PodWatcher terminated")
 	}
 
 	return nil
@@ -211,8 +216,13 @@ func (k *Kubernetes) Run(ctx context.Context, specv runtime.Spec, stepv runtime.
 			PodNamespace: podNamespace,
 			PodName:      podId,
 			Clientset:    k.client,
-			Period:       time.Minute,
+			Period:       20 * time.Second,
 		})
+
+		logger.FromContext(ctx).
+			WithField("pod", podId).
+			WithField("step", stepName).
+			Debug("PodWatcher started")
 	}
 
 	watcher.AddContainer(step.ID, step.Placeholder)
@@ -235,24 +245,6 @@ func (k *Kubernetes) Run(ctx context.Context, specv runtime.Spec, stepv runtime.
 	}
 
 	err = k.tail(ctx, spec, step, output)
-	// this feature flag retries fetching the logs if it fails on
-	// the first attempt. This is meant to help triage the following
-	// issue:
-	//
-	//    https://discourse.drone.io/t/kubernetes-runner-intermittently-fails-steps/7372
-	//
-	// BEGIN: FEATURE FLAG
-	if err != nil {
-		if os.Getenv("DRONE_FEATURE_FLAG_RETRY_LOGS") == "true" {
-			<-time.After(time.Second * 5)
-			err = k.tail(ctx, spec, step, output)
-		}
-		if err != nil {
-			<-time.After(time.Second * 5)
-			err = k.tail(ctx, spec, step, output)
-		}
-	}
-	// END: FEATURE FAG
 	if err != nil {
 		return nil, err
 	}

engine/podwatcher/watcher.go

@@ -7,6 +7,7 @@ package podwatcher
 import (
 	"context"
 	"errors"
+	"fmt"
 	"sync"
 	"time"
 
@@ -22,9 +23,9 @@ var (
 	// ErrPodTerminated is an error that container wait functions return when the pod is already terminated.
 	ErrPodTerminated = errors.New("pod is terminated")
 
-	// ErrFailedContainer is returned when placeholder container terminated abnormally.
+	// MessageFailedContainer is returned as error when placeholder container terminates abnormally.
 	// The correct container image failed to load. Usually happens when image doesn't exist.
-	ErrFailedContainer = errors.New("container failed to start (invalid image?)")
+	MessageFailedContainer = "container failed to start"
 )
 
 // PodWatcher is used to monitor status of a Kubernetes pod and containers inside of it.
@@ -78,10 +79,6 @@ func (pw *PodWatcher) Start(ctx context.Context, cw ContainerWatcher) {
 	pw.containerRegCh = make(chan containerInfo)
 	pw.clientCh = make(chan *waitClient) // a channel for accepting new wait clients
 
-	logrus.
-		WithField("pod", podName).
-		Debug("PodWatcher: Started")
-
 	errDone := make(chan error)
 
 	wg := &sync.WaitGroup{}
@@ -159,9 +156,7 @@ func (pw *PodWatcher) Start(ctx context.Context, cw ContainerWatcher) {
 
 	go func() {
 		wg.Wait()
-		logrus.
-			WithField("pod", podName).
-			Debug("PodWatcher: Terminated")
+		//...
 	}()
 }
 
@@ -202,7 +197,7 @@ func (pw *PodWatcher) updateContainers(containers []containerInfo) {
 				WithField("image", c.image).
 				WithField("state", c.state).
 				WithField("stateInfo", c.stateInfo).
-				Trace("PodWatcher: Container state changed")
+				Debug("PodWatcher: Container state changed")
 
 			pw.notifyClientsContainerChange(c)
 		}
@@ -219,7 +214,7 @@ func _tryResolveWaitClient(cl *waitClient, c *containerInfo) bool {
 
 	if image.Match(c.image, c.placeholder) {
 		if c.state == stateTerminated && c.exitCode != 0 {
-			cl.resolveCh <- ErrFailedContainer
+			cl.resolveCh <- fmt.Errorf("%s: exitCode=%d reason=%s", MessageFailedContainer, c.exitCode, c.stateInfo)
 			return true
 		} else {
 			return false

engine/podwatcher/watcher_test.go

@@ -6,6 +6,8 @@ package podwatcher
 
 import (
 	"context"
+	"errors"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -186,7 +188,7 @@ func TestPodWatcher(t *testing.T) {
 			containers: []string{"A"},
 			steps: []step{
 				{op: opContAdd, containerId: "A"},
-				{op: opWaitContainer, containerId: "A", state: stateRunning, expected: ErrFailedContainer},
+				{op: opWaitContainer, containerId: "A", state: stateRunning, expected: errors.New(MessageFailedContainer)},
 				{op: opContSetStatePlaceholder, containerId: "A", state: stateTerminated, exitCode: 2},
 			},
 		},
@@ -302,7 +304,7 @@ func TestPodWatcher(t *testing.T) {
 							err := pw.WaitContainerStart(containerId)
 							if err != nil && expected == nil {
 								t.Errorf("test %q, step=%d failed: expected no error but got %v", testName, stepIdx, err)
-							} else if expected != nil && err != expected {
+							} else if expected != nil && (err == nil || !strings.HasPrefix(err.Error(), expected.Error())) {
 								t.Errorf("test %q, step=%d failed: expected error %v but got %v", testName, stepIdx, expected, err)
 							}
 						}(test.name, s.containerId, stepIdx, s.expected)

go.mod

@@ -12,7 +12,7 @@ require (
 	github.com/docker/go-units v0.4.0
 	github.com/drone/drone-go v1.6.0
 	github.com/drone/envsubst v1.0.2
-	github.com/drone/runner-go v1.7.0
+	github.com/drone/runner-go v1.8.0
 	github.com/drone/signal v1.0.0
 	github.com/ghodss/yaml v1.0.0
 	github.com/golang/mock v1.3.1