Merge pull request #13295 from mesosphere/minion-signals

contrib/mesos/pkg/minion/server.go

@@ -21,15 +21,15 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"os/exec"
+	"os/signal"
 	"path"
 	"strings"
-	"time"
+	"syscall"
 
 	exservice "k8s.io/kubernetes/contrib/mesos/pkg/executor/service"
 	"k8s.io/kubernetes/contrib/mesos/pkg/hyperkube"
 	"k8s.io/kubernetes/contrib/mesos/pkg/minion/config"
-	"k8s.io/kubernetes/contrib/mesos/pkg/runtime"
+	"k8s.io/kubernetes/contrib/mesos/pkg/minion/tasks"
 	"k8s.io/kubernetes/pkg/api/resource"
 	client "k8s.io/kubernetes/pkg/client/unversioned"
 
@@ -39,6 +39,11 @@ import (
 	"gopkg.in/natefinch/lumberjack.v2"
 )
 
+const (
+	proxyLogFilename    = "proxy.log"
+	executorLogFilename = "executor.log"
+)
+
 type MinionServer struct {
 	// embed the executor server to be able to use its flags
 	// TODO(sttts): get rid of this mixing of the minion and the executor server with a multiflags implementation for km
@@ -48,8 +53,7 @@ type MinionServer struct {
 	hks            hyperkube.Interface
 	clientConfig   *client.Config
 	kmBinary       string
-	done           chan struct{} // closed when shutting down
-	exit           chan error    // to signal fatal errors
+	tasks          []*tasks.Task
 
 	pathOverride string // the PATH environment for the sub-processes
 	cgroupPrefix string // e.g. mesos
@@ -69,15 +73,11 @@ func NewMinionServer() *MinionServer {
 	s := &MinionServer{
 		KubeletExecutorServer: exservice.NewKubeletExecutorServer(),
 		privateMountNS:        false, // disabled until Docker supports customization of the parent mount namespace
-		done:                  make(chan struct{}),
-		exit:                  make(chan error),
-
-		cgroupPrefix:    config.DefaultCgroupPrefix,
-		logMaxSize:      config.DefaultLogMaxSize(),
-		logMaxBackups:   config.DefaultLogMaxBackups,
-		logMaxAgeInDays: config.DefaultLogMaxAgeInDays,
-
-		runProxy: true,
+		cgroupPrefix:          config.DefaultCgroupPrefix,
+		logMaxSize:            config.DefaultLogMaxSize(),
+		logMaxBackups:         config.DefaultLogMaxBackups,
+		logMaxAgeInDays:       config.DefaultLogMaxAgeInDays,
+		runProxy:              true,
 	}
 
 	// cache this for later use
@@ -141,10 +141,13 @@ func (ms *MinionServer) launchProxyServer() {
 		args = append(args, fmt.Sprintf("--hostname-override=%s", ms.KubeletExecutorServer.HostnameOverride))
 	}
 
-	ms.launchHyperkubeServer(hyperkube.CommandProxy, &args, "proxy.log")
+	ms.launchHyperkubeServer(hyperkube.CommandProxy, args, proxyLogFilename, nil)
 }
 
-func (ms *MinionServer) launchExecutorServer() {
+// launchExecutorServer returns a chan that closes upon kubelet-executor death. since the kubelet-
+// executor doesn't support failover right now, the right thing to do is to fail completely since all
+// pods will be lost upon restart and we want mesos to recover the resources from them.
+func (ms *MinionServer) launchExecutorServer() <-chan struct{} {
 	allArgs := os.Args[1:]
 
 	// filter out minion flags, leaving those for the executor
@@ -159,111 +162,65 @@ func (ms *MinionServer) launchExecutorServer() {
 	}
 
 	// run executor and quit minion server when this exits cleanly
-	err := ms.launchHyperkubeServer(hyperkube.CommandExecutor, &executorArgs, "executor.log")
-	if err != nil {
-		// just return, executor will be restarted on error
-		log.Error(err)
-		return
+	execDied := make(chan struct{})
+	decorator := func(t *tasks.Task) *tasks.Task {
+		t.Finished = func(_ bool) bool {
+			// this func implements the task.finished spec, so when the executor exits
+			// we return false to indicate that it should not be restarted. we also
+			// close execDied to signal interested listeners.
+			close(execDied)
+			return false
+		}
+		// since we only expect to die once, and there is no restart; don't delay any longer than needed
+		t.RestartDelay = 0
+		return t
 	}
-
-	log.Info("Executor exited cleanly, stopping the minion")
-	ms.exit <- nil
+	ms.launchHyperkubeServer(hyperkube.CommandExecutor, executorArgs, executorLogFilename, decorator)
+	return execDied
 }
 
-func (ms *MinionServer) launchHyperkubeServer(server string, args *[]string, logFileName string) error {
+func (ms *MinionServer) launchHyperkubeServer(server string, args []string, logFileName string, decorator func(*tasks.Task) *tasks.Task) {
 	log.V(2).Infof("Spawning hyperkube %v with args '%+v'", server, args)
 
-	// prepare parameters
-	kmArgs := []string{server}
-	for _, arg := range *args {
-		kmArgs = append(kmArgs, arg)
-	}
-
-	// create command
-	cmd := exec.Command(ms.kmBinary, kmArgs...)
-	if _, err := cmd.StdoutPipe(); err != nil {
-		// fatal error => terminate minion
-		err = fmt.Errorf("error getting stdout of %v: %v", server, err)
-		ms.exit <- err
-		return err
-	}
-	stderrLogs, err := cmd.StderrPipe()
-	if err != nil {
-		// fatal error => terminate minion
-		err = fmt.Errorf("error getting stderr of %v: %v", server, err)
-		ms.exit <- err
-		return err
+	kmArgs := append([]string{server}, args...)
+	maxSize := ms.logMaxSize.Value()
+	if maxSize > 0 {
+		// convert to MB
+		maxSize = maxSize / 1024 / 1024
+		if maxSize == 0 {
+			log.Warning("maximal log file size is rounded to 1 MB")
+			maxSize = 1
+		}
 	}
 
-	ch := make(chan struct{})
-	go func() {
-		defer func() {
-			select {
-			case <-ch:
-				log.Infof("killing %v process...", server)
-				if err = cmd.Process.Kill(); err != nil {
-					log.Errorf("failed to kill %v process: %v", server, err)
-				}
-			default:
-			}
-		}()
-
-		maxSize := ms.logMaxSize.Value()
-		if maxSize > 0 {
-			// convert to MB
-			maxSize = maxSize / 1024 / 1024
-			if maxSize == 0 {
-				log.Warning("maximal log file size is rounded to 1 MB")
-				maxSize = 1
-			}
-		}
-		writer := &lumberjack.Logger{
+	writerFunc := func() io.WriteCloser {
+		return &lumberjack.Logger{
 			Filename:   logFileName,
 			MaxSize:    int(maxSize),
 			MaxBackups: ms.logMaxBackups,
 			MaxAge:     ms.logMaxAgeInDays,
 		}
-		defer writer.Close()
-
-		log.V(2).Infof("Starting logging for %v: max log file size %d MB, keeping %d backups, for %d days", server, maxSize, ms.logMaxBackups, ms.logMaxAgeInDays)
-
-		<-ch
-		written, err := io.Copy(writer, stderrLogs)
-		if err != nil {
-			log.Errorf("error writing data to %v: %v", logFileName, err)
-		}
-
-		log.Infof("wrote %d bytes to %v", written, logFileName)
-	}()
+	}
 
 	// use given environment, but add /usr/sbin to the path for the iptables binary used in kube-proxy
+	var kmEnv []string
 	if ms.pathOverride != "" {
 		env := os.Environ()
-		cmd.Env = make([]string, 0, len(env))
+		kmEnv = make([]string, 0, len(env))
 		for _, e := range env {
 			if !strings.HasPrefix(e, "PATH=") {
-				cmd.Env = append(cmd.Env, e)
+				kmEnv = append(kmEnv, e)
 			}
 		}
-		cmd.Env = append(cmd.Env, "PATH="+ms.pathOverride)
+		kmEnv = append(kmEnv, "PATH="+ms.pathOverride)
 	}
 
-	// if the server fails to start then we exit the executor, otherwise
-	// wait for the proxy process to end (and release resources after).
-	if err := cmd.Start(); err != nil {
-		// fatal error => terminate minion
-		err = fmt.Errorf("error starting %v: %v", server, err)
-		ms.exit <- err
-		return err
-	}
-	close(ch)
-	if err := cmd.Wait(); err != nil {
-		log.Errorf("%v exited with error: %v", server, err)
-		err = fmt.Errorf("%v exited with error: %v", server, err)
-		return err
+	t := tasks.New(server, ms.kmBinary, kmArgs, kmEnv, writerFunc)
+	if decorator != nil {
+		t = decorator(t)
 	}
-
-	return nil
+	go t.Start()
+	ms.tasks = append(ms.tasks, t)
 }
 
 // runs the main kubelet loop, closing the kubeletFinished chan when the loop exits.
@@ -295,15 +252,52 @@ func (ms *MinionServer) Run(hks hyperkube.Interface, _ []string) error {
 	cgroupLogger("using cgroup-root %q", ms.cgroupRoot)
 
 	// run subprocesses until ms.done is closed on return of this function
-	defer close(ms.done)
 	if ms.runProxy {
-		go runtime.Until(ms.launchProxyServer, 5*time.Second, ms.done)
+		ms.launchProxyServer()
 	}
-	go runtime.Until(ms.launchExecutorServer, 5*time.Second, ms.done)
 
-	// wait until minion exit is requested
-	// don't close ms.exit here to avoid panics of go routines writing an error to it
-	return <-ms.exit
+	// abort closes when the kubelet-executor dies
+	abort := ms.launchExecutorServer()
+	shouldQuit := termSignalListener(abort)
+	te := tasks.MergeOutput(ms.tasks, shouldQuit)
+
+	// TODO(jdef) do something fun here, such as reporting task completion to the apiserver
+
+	<-te.Close().Done() // we don't listen for any specific events yet; wait for all tasks to finish
+	return nil
+}
+
+// termSignalListener returns a signal chan that closes when either (a) the process receives a termination
+// signal: SIGTERM, SIGINT, or SIGHUP; or (b) the abort chan closes.
+func termSignalListener(abort <-chan struct{}) <-chan struct{} {
+	shouldQuit := make(chan struct{})
+	sigCh := make(chan os.Signal, 1)
+	signal.Notify(sigCh)
+
+	go func() {
+		defer close(shouldQuit)
+		for {
+			select {
+			case <-abort:
+				log.Infof("executor died, aborting")
+				return
+			case s, ok := <-sigCh:
+				if !ok {
+					return
+				}
+				switch s {
+				case os.Interrupt, os.Signal(syscall.SIGTERM), os.Signal(syscall.SIGINT), os.Signal(syscall.SIGHUP):
+					log.Infof("received signal %q, aborting", s)
+					return
+				case os.Signal(syscall.SIGCHLD): // who cares?
+				default:
+					log.Errorf("unexpected signal: %T %#v", s, s)
+				}
+
+			}
+		}
+	}()
+	return shouldQuit
 }
 
 func (ms *MinionServer) AddExecutorFlags(fs *pflag.FlagSet) {

contrib/mesos/pkg/minion/tasks/doc.go

+/*
+Copyright 2015 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 tasks provides an API for supervising system processes as Task's.
+// It provides stronger guarantees with respect to process lifecycle than a
+// standalone kubelet running static pods.
+package tasks

contrib/mesos/pkg/minion/tasks/events.go

+/*
+Copyright 2015 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 tasks
+
+type Events interface {
+	// Close stops delivery of events in the completion and errors channels; callers must close this when they intend to no longer read from completion() or errors()
+	Close() Events
+
+	// Completion reports Completion events as they happen
+	Completion() <-chan *Completion
+
+	// Done returns a signal chan that closes when all tasks have completed and there are no more events to deliver
+	Done() <-chan struct{}
+}
+
+type eventsImpl struct {
+	tc             chan *Completion
+	stopForwarding chan struct{}
+	done           <-chan struct{}
+}
+
+func newEventsImpl(tcin <-chan *Completion, done <-chan struct{}) *eventsImpl {
+	ei := &eventsImpl{
+		tc:             make(chan *Completion),
+		stopForwarding: make(chan struct{}),
+		done:           done,
+	}
+	go func() {
+		defer close(ei.tc)
+		forwardCompletionUntil(tcin, ei.tc, ei.stopForwarding, done, nil)
+	}()
+	return ei
+}
+
+func (e *eventsImpl) Close() Events                  { close(e.stopForwarding); return e }
+func (e *eventsImpl) Completion() <-chan *Completion { return e.tc }
+func (e *eventsImpl) Done() <-chan struct{}          { return e.done }
+
+// forwardCompletionUntil is a generic pipe that forwards objects between channels.
+// if discard is closed, objects are silently dropped.
+// if tap != nil then it's invoked for each object as it's read from tin, but before it's written to tch.
+// returns when either reading from tin completes (no more objects, and is closed), or else
+// abort is closed, which ever happens first.
+func forwardCompletionUntil(tin <-chan *Completion, tch chan<- *Completion, discard <-chan struct{}, abort <-chan struct{}, tap func(*Completion, bool)) {
+	var tc *Completion
+	var ok bool
+forwardLoop:
+	for {
+		select {
+		case tc, ok = <-tin:
+			if !ok {
+				return
+			}
+			if tap != nil {
+				tap(tc, false)
+			}
+			select {
+			case <-abort:
+				break forwardLoop
+			case <-discard:
+			case tch <- tc:
+			}
+		case <-abort:
+			// best effort
+			select {
+			case tc, ok = <-tin:
+				if ok {
+					if tap != nil {
+						tap(tc, true)
+					}
+					break forwardLoop
+				}
+			default:
+			}
+			return
+		}
+	}
+	// best effort
+	select {
+	case tch <- tc:
+	case <-discard:
+	default:
+	}
+}

contrib/mesos/pkg/minion/tasks/task_test.go

+/*
+Copyright 2015 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 tasks
+
+import (
+	"bytes"
+	"errors"
+	"fmt"
+	"io"
+	"sync"
+	"syscall"
+	"testing"
+
+	log "github.com/golang/glog"
+)
+
+type badWriteCloser struct {
+	err error
+}
+
+func (b *badWriteCloser) Write(_ []byte) (int, error) { return 0, b.err }
+func (b *badWriteCloser) Close() error                { return b.err }
+
+type discardCloser int
+
+func (d discardCloser) Write(b []byte) (int, error) { return len(b), nil }
+func (d discardCloser) Close() error                { return nil }
+
+var devNull = func() io.WriteCloser { return discardCloser(0) }
+
+type fakeExitError uint32
+
+func (f fakeExitError) Sys() interface{} { return syscall.WaitStatus(f << 8) }
+func (f fakeExitError) Error() string    { return fmt.Sprintf("fake-exit-error: %d", f) }
+
+type fakeProcess struct {
+	done chan struct{}
+	pid  int
+	err  error
+}
+
+func (f *fakeProcess) Wait() error {
+	<-f.done
+	return f.err
+}
+func (f *fakeProcess) Kill() (int, error) {
+	close(f.done)
+	return f.pid, f.err
+}
+func (f *fakeProcess) exit(code int) {
+	f.err = fakeExitError(code)
+	close(f.done)
+}
+
+func newFakeProcess() *fakeProcess {
+	return &fakeProcess{
+		done: make(chan struct{}),
+	}
+}
+
+func TestBadLogger(t *testing.T) {
+	err := errors.New("qux")
+	fp := newFakeProcess()
+	tt := New("foo", "bar", nil, nil, func() io.WriteCloser {
+		defer func() {
+			fp.pid = 123 // sanity check
+			fp.Kill()    // this causes Wait() to return
+		}()
+		return &badWriteCloser{err}
+	})
+
+	tt.RestartDelay = 0 // don't slow the test down for no good reason
+
+	finishCalled := make(chan struct{})
+	tt.Finished = func(ok bool) bool {
+		log.Infof("tt.Finished: ok %t", ok)
+		if ok {
+			close(finishCalled)
+		}
+		return false // never respawn, this causes t.done to close
+	}
+
+	// abuse eventsImpl: we're not going to listen on the task completion or event chans,
+	// and we don't want to block the state machine, so discard all task events as they happen
+	ei := newEventsImpl(tt.completedCh, tt.done)
+	ei.Close()
+
+	go tt.run(func(_ *Task) taskStateFn {
+		log.Infof("tt initialized")
+		tt.initLogging(bytes.NewBuffer(([]byte)("unlogged bytes")))
+		tt.cmd = fp
+		return taskRunning
+	})
+
+	// if the logger fails the task will be killed
+	// badWriteLogger generates an error immediately and results in a task kill
+	<-finishCalled
+	<-tt.done
+
+	// this should never data race since the state machine is dead at this point
+	if fp.pid != 123 {
+		t.Fatalf("incorrect pid, expected 123 not %d", fp.pid)
+	}
+
+	// TODO(jdef) would be nice to check for a specific error that indicates the logger died
+}
+
+func TestMergeOutput(t *testing.T) {
+	var tasksStarted, tasksDone sync.WaitGroup
+	tasksDone.Add(2)
+	tasksStarted.Add(2)
+
+	t1 := New("foo", "", nil, nil, devNull)
+	t1exited := make(chan struct{})
+	t1.RestartDelay = 0 // don't slow the test down for no good reason
+	t1.Finished = func(ok bool) bool {
+		// we expect each of these cases to happen exactly once
+		if !ok {
+			tasksDone.Done()
+		} else {
+			close(t1exited)
+		}
+		return ok
+	}
+	go t1.run(func(t *Task) taskStateFn {
+		defer tasksStarted.Done()
+		t.initLogging(bytes.NewBuffer([]byte{}))
+		t.cmd = newFakeProcess()
+		return taskRunning
+	})
+
+	t2 := New("bar", "", nil, nil, devNull)
+	t2exited := make(chan struct{})
+	t2.RestartDelay = 0 // don't slow the test down for no good reason
+	t2.Finished = func(ok bool) bool {
+		// we expect each of these cases to happen exactly once
+		if !ok {
+			tasksDone.Done()
+		} else {
+			close(t2exited)
+		}
+		return ok
+	}
+	go t2.run(func(t *Task) taskStateFn {
+		defer tasksStarted.Done()
+		t.initLogging(bytes.NewBuffer([]byte{}))
+		t.cmd = newFakeProcess()
+		return taskRunning
+	})
+
+	shouldQuit := make(chan struct{})
+	te := MergeOutput([]*Task{t1, t2}, shouldQuit)
+
+	tasksStarted.Wait()
+	tasksStarted.Add(2) // recycle the barrier
+
+	// kill each task once, let it restart; make sure that we get the completion status?
+	t1.cmd.(*fakeProcess).exit(1)
+	t2.cmd.(*fakeProcess).exit(2)
+
+	codes := map[int]struct{}{}
+	for i := 0; i < 2; i++ {
+		switch tc := <-te.Completion(); tc.code {
+		case 1, 2:
+			codes[tc.code] = struct{}{}
+		default:
+			if tc.err != nil {
+				t.Errorf("unexpected task completion error: %v", tc.err)
+			} else {
+				t.Errorf("unexpected task completion code: %d", tc.code)
+			}
+		}
+	}
+
+	te.Close() // we're not going to read any other completion or error events
+
+	if len(codes) != 2 {
+		t.Fatalf("expected each task process to exit once")
+	}
+
+	// each task invokes Finished() once
+	<-t1exited
+	<-t2exited
+
+	log.Infoln("each task process has completed one round")
+	tasksStarted.Wait() // tasks will auto-restart their exited procs
+
+	// assert that the tasks are not dead; TODO(jdef) not sure that these checks are useful
+	select {
+	case <-t1.done:
+		t.Fatalf("t1 is unexpectedly dead")
+	default:
+	}
+	select {
+	case <-t2.done:
+		t.Fatalf("t2 is unexpectedly dead")
+	default:
+	}
+
+	log.Infoln("firing quit signal")
+	close(shouldQuit) // fire shouldQuit, and everything should terminate gracefully
+
+	log.Infoln("waiting for tasks to die")
+	tasksDone.Wait() // our tasks should die
+
+	log.Infoln("waiting for merge to complete")
+	<-te.Done() // wait for the merge to complete
+}