add DestroyIfUnpaused; use it in evictor to avoid ill-timed evictions

Author: tylerharter

src/lambda/lambda.go

@@ -840,7 +840,7 @@ func (linst *LambdaInstance) TrySendError(req *Invocation, statusCode int, msg s
 
 	var err error
 	if sb != nil {
-		_, err = req.w.Write([]byte(msg+"\nSandbox State:"+sb.DebugString()+"\n"))
+		_, err = req.w.Write([]byte(msg+"\nSandbox State: "+sb.DebugString()+"\n"))
 	} else {
 		_, err = req.w.Write([]byte(msg+"\n"))
 	}

src/lambda/packagePuller.go

@@ -9,7 +9,6 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
-	"strconv"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -301,14 +300,8 @@ func (pp *PackagePuller) sandboxInstall(p *Package) (err error) {
 	}
 
 	if resp.StatusCode != http.StatusOK {
-		// did we run out of memory?
-		if stat, err := sb.Status(sandbox.StatusMemFailures); err == nil {
-			if b, err := strconv.ParseBool(stat); err == nil && b {
-				return fmt.Errorf("ran out of memory while installing %s", p.name)
-			}
-		}
-
-		return fmt.Errorf("install lambda returned status %d, body '%s'", resp.StatusCode, string(body))
+		return fmt.Errorf("install lambda returned status %d, Body: '%s', Installer Sandbox State: %s",
+			resp.StatusCode, string(body), sb.DebugString())
 	}
 
 	if err := json.Unmarshal(body, &p.meta); err != nil {

src/sandbox/api.go

@@ -47,6 +47,12 @@ type Sandbox interface {
 	// error messages
 	Destroy(reason string)
 
+	// The Sandbox may choose to ignore the Destroy request if the
+	// state is unpaused.  Some simple Sandbox implementations may
+	// also respond to this by always killing the sandbox,
+	// regardless of current state.
+	DestroyIfPaused(reason string)
+
 	// Make processes in the container non-schedulable
 	Pause() error
 
@@ -59,9 +65,6 @@ type Sandbox interface {
 	// Lookup metadata that Sandbox was initialized with (static over time)
 	Meta() *SandboxMeta
 
-	// Lookup a particular stat (changes over time)
-	Status(SandboxStatus) (string, error)
-
 	// Get output of the runtime; if any
 	GetRuntimeLog() string
 
@@ -114,6 +117,7 @@ type SandboxEventType int
 const (
 	EvCreate    SandboxEventType = iota
 	EvDestroy                    = iota
+	EvDestroyIgnored             = iota
 	EvPause                      = iota
 	EvUnpause                    = iota
 	EvFork                       = iota
@@ -124,9 +128,3 @@ type SandboxEvent struct {
 	EvType SandboxEventType
 	SB     Sandbox
 }
-
-type SandboxStatus int
-
-const (
-	StatusMemFailures SandboxStatus = iota // boolean
-)

src/sandbox/cgroups.go

@@ -298,6 +298,26 @@ func (cg *Cgroup) WriteString(resource string, val string) {
 	}
 }
 
+func (cg *Cgroup) TryReadIntKV(resource string, key string) (int64, error) {
+	raw, err := ioutil.ReadFile(cg.ResourcePath(resource))
+	if err != nil {
+		return 0, err
+	}
+	body := string(raw)
+	lines := strings.Split(body, "\n")
+	for i := 0; i <= len(lines); i++ {
+		parts := strings.Split(lines[i], " ")
+		if len(parts) == 2 && parts[0] == key {
+			val, err := strconv.ParseInt(strings.TrimSpace(string(parts[1])), 10, 64)
+			if err != nil {
+				return 0, err
+			}
+			return val, nil
+		}
+	}
+	return 0, fmt.Errorf("could not find key '%s' in file: %s", key, body)
+}
+
 func (cg *Cgroup) TryReadInt(resource string) (int64, error) {
 	raw, err := ioutil.ReadFile(cg.ResourcePath(resource))
 	if err != nil {

src/sandbox/docker.go

@@ -192,6 +192,10 @@ func (c *DockerContainer) Destroy(reason string) {
 	}
 }
 
+func (c *DockerContainer) DestroyIfPaused(reason string) {
+	c.Destroy(reason) // we're allowed to implement this by uncondationally destroying
+}
+
 // frees all resources associated with the lambda
 func (c *DockerContainer) internalDestroy() error {
 	c.Unpause()
@@ -353,10 +357,6 @@ func waitForServerPipeReady(hostDir string) error {
 	}
 }
 
-func (c *DockerContainer) Status(key SandboxStatus) (string, error) {
-	return "", STATUS_UNSUPPORTED
-}
-
 func (c *DockerContainer) Meta() *SandboxMeta {
 	return c.meta
 }

src/sandbox/evictors.go

@@ -80,27 +80,17 @@ func (evictor *SOCKEvictor) Event(evType SandboxEventType, sb Sandbox) {
 
 // move Sandbox to a given queue, removing from previous (if necessary).
 // a move to nil is just a delete.
-//
-// a Sandbox cannot be moved from evicting to another queue (only to nil);
-// requests attempting to do so are quietly ignored
 func (evictor *SOCKEvictor) move(sb Sandbox, target *list.List) {
 	// remove from previous queue if necessary
 	prev := evictor.stateMap[sb.ID()]
 	if prev != nil {
-		// you cannot move off evicting to a live queue
-		if prev.List == evictor.evicting && target != nil {
-			return
-		}
-
 		prev.List.Remove(prev.Element)
 	}
 
 	// add to new queue
 	if target != nil {
-		if target != nil {
-			element := target.PushBack(sb)
-			evictor.stateMap[sb.ID()] = &ListLocation{target, element}
-		}
+		element := target.PushBack(sb)
+		evictor.stateMap[sb.ID()] = &ListLocation{target, element}
 	} else {
 		delete(evictor.stateMap, sb.ID())
 	}
@@ -154,7 +144,7 @@ func (evictor *SOCKEvictor) updateState() {
 			prio += 2
 		case EvChildExit:
 			prio -= 2
-		case EvDestroy:
+		case EvDestroy, EvDestroyIgnored:
 		default:
 			evictor.printf("Unknown event: %v", event.EvType)
 		}
@@ -174,6 +164,7 @@ func (evictor *SOCKEvictor) updateState() {
 			if prio >= len(evictor.prioQueues) {
 				prio = len(evictor.prioQueues) - 1
 			}
+
 			evictor.move(sb, evictor.prioQueues[prio])
 		}
 
@@ -183,20 +174,24 @@ func (evictor *SOCKEvictor) updateState() {
 
 // evict whatever SB is at the front of the queue, assumes
 // queue is not empty
-func (evictor *SOCKEvictor) evictFront(queue *list.List) {
+func (evictor *SOCKEvictor) evictFront(queue *list.List, force bool) {
 	front := queue.Front()
 	sb := front.Value.(Sandbox)
 
 	evictor.printf("Evict Sandbox %v", sb.ID())
+	evictor.move(sb, evictor.evicting)
 
 	// destroy async (we'll know when it's done, because
 	// we'll see a evDestroy event later on our chan)
 	go func() {
 		t := common.T0("evict")
-		sb.Destroy("evicted")
+		if force {
+			sb.Destroy("forced eviction")
+		} else {
+			sb.DestroyIfPaused("idle eviction")
+		}
 		t.T1()
 	}()
-	evictor.move(sb, evictor.evicting)
 }
 
 // POLICY: how should we select a victim?
@@ -224,7 +219,7 @@ func (evictor *SOCKEvictor) doEvictions() {
 
 	// try evicting the desired number, starting with the paused queue
 	for evictCount > 0 && evictor.prioQueues[0].Len() > 0 {
-		evictor.evictFront(evictor.prioQueues[0])
+		evictor.evictFront(evictor.prioQueues[0], false)
 		evictCount -= 1
 	}
 
@@ -237,12 +232,13 @@ func (evictor *SOCKEvictor) doEvictions() {
 	if freeSandboxes <= 0 && evictor.evicting.Len() == 0 {
 		evictor.printf("WARNING!  Critically low on memory, so evicting an active Sandbox")
 		if evictor.prioQueues[1].Len() > 0 {
-			evictor.evictFront(evictor.prioQueues[1])
+			evictor.evictFront(evictor.prioQueues[1], true)
 		}
 	}
 
-	// we never evict from prioQueues[2], because have descendents
-	// with lower priority that should be evicted first
+	// we never evict from prioQueues[2+], because those have
+	// descendents with lower priority that should be evicted
+	// first
 }
 
 func (evictor *SOCKEvictor) Run() {

src/sandbox/safeSandbox.go

@@ -3,9 +3,9 @@ package sandbox
 // this layer can wrap any sandbox, and provides several (mostly) safety features:
 // 1. it prevents concurrent calls to Sandbox functions that modify the Sandbox
 // 2. it automatically destroys unhealthy sandboxes (it is considered unhealthy after returnning any error)
-// 3. calls on a destroyed sandbox just return a DEAD_SANDBOX error (no harm is done)
+// 3. calls on a destroyed sandbox just return a dead sandbox error (no harm is done)
 // 4. suppresses Pause calls to already paused Sandboxes, and similar for Unpause calls.
-// 5. it traces all calls
+// 5. it asyncronously notifies event listeners of state changes
 
 import (
 	"fmt"
@@ -87,13 +87,36 @@ func (sb *safeSandbox) Destroy(reason string) {
 	}
 
 	sb.Sandbox.Destroy(reason)
-	// TODO: allow message to be passed in (so we can blame cache eviction, for example)
-	sb.dead = SandboxDeadError(fmt.Sprintf("Sandbox previously killed exlicitly by Destroy(reason=%s) call", reason))
+	state := "unpaused"
+	if sb.paused {
+		state = "paused"
+	}
+	sb.dead = SandboxDeadError(fmt.Sprintf("Sandbox previously killed exlicitly by Destroy(reason=%s) call while %s", reason, state))
 
 	// let anybody interested know this died
 	sb.event(EvDestroy)
 }
 
+func (sb *safeSandbox) DestroyIfPaused(reason string) {
+	sb.printf("DestroyIfPaused()")
+	t := common.T0("DestroyIfPaused()")
+	defer t.T1()
+	sb.Mutex.Lock()
+	defer sb.Mutex.Unlock()
+
+	if sb.dead != nil {
+		return
+	}
+
+	if sb.paused {
+		sb.Sandbox.Destroy(reason)
+		sb.dead = SandboxDeadError(fmt.Sprintf("Sandbox previously killed exlicitly by DestroyIfPaused(reason=%s) call", reason))
+		sb.event(EvDestroy)
+	} else {
+		sb.event(EvDestroyIgnored)
+	}
+}
+
 func (sb *safeSandbox) Pause() (err error) {
 	sb.printf("Pause()")
 	t := common.T0("Pause()")
@@ -111,8 +134,8 @@ func (sb *safeSandbox) Pause() (err error) {
 		sb.destroyOnErr("Pause", err)
 		return err
 	}
-
 	sb.event(EvPause)
+
 	sb.paused = true
 	return nil
 }
@@ -130,12 +153,16 @@ func (sb *safeSandbox) Unpause() (err error) {
 		return nil
 	}
 
+	// we want watchers (e.g., the evictor) to overestimate when
+	// we're active so that we're less likely to be evicted at a
+	// bad time, so the Unpause event is sent before the actual op
+	// and the Pause event is sent after the actual op
+	sb.event(EvUnpause)
 	if err := sb.Sandbox.Unpause(); err != nil {
 		sb.destroyOnErr("Unpause", err)
 		return err
 	}
 
-	sb.event(EvUnpause)
 	sb.paused = false
 	return nil
 }
@@ -197,30 +224,12 @@ func (sb *safeSandbox) childExit(child Sandbox) {
 	}
 }
 
-func (sb *safeSandbox) Status(key SandboxStatus) (stat string, err error) {
-	sb.printf("Status(%d)", key)
-	t := common.T0("Status()")
-	defer t.T1()
-	sb.Mutex.Lock()
-	defer sb.Mutex.Unlock()
-
-	if sb.dead != nil {
-		return "", sb.dead
-	}
-
-	stat, err = sb.Sandbox.Status(key)
-	if err != nil && err != STATUS_UNSUPPORTED {
-		sb.destroyOnErr("Status", err)
-	}
-	return stat, err
-}
-
 func (sb *safeSandbox) DebugString() string {
 	sb.Mutex.Lock()
 	defer sb.Mutex.Unlock()
 
 	if sb.dead != nil {
-		return fmt.Sprintf("SANDBOX %s: DEAD (%s)\n", sb.Sandbox.ID(), sb.dead.Error())
+		return fmt.Sprintf("SANDBOX %s is DEAD: %s\n", sb.Sandbox.ID(), sb.dead.Error())
 	}
 
 	return sb.Sandbox.DebugString()

src/sandbox/sock.go

@@ -4,7 +4,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"log"
-    "sync/atomic"
+	"sync/atomic"
 	"net"
 	"net/http"
 	"net/http/httputil"
@@ -294,11 +294,15 @@ func (container *SOCKContainer) Destroy(reason string) {
 	container.decCgRefCount()
 }
 
+func (c *SOCKContainer) DestroyIfPaused(reason string) {
+	c.Destroy(reason) // we're allowed to implement this by uncondationally destroying
+}
+
 // when the count goes to zero, it means (a) this container and (b)
 // all it's descendants are destroyed. Thus, it's safe to release it's
 // cgroups, and return the memory allocation to the memPool
 func (container *SOCKContainer) decCgRefCount() {
-    newCount := atomic.AddInt32(&container.cgRefCount, -1)
+	newCount := atomic.AddInt32(&container.cgRefCount, -1)
 
 	container.printf("CG ref count decremented to %d", newCount)
 	if newCount < 0 {
@@ -429,17 +433,6 @@ func (container *SOCKContainer) fork(dst Sandbox) (err error) {
 	return nil
 }
 
-func (container *SOCKContainer) Status(key SandboxStatus) (string, error) {
-	switch key {
-	case StatusMemFailures:
-		// TODO should we only count max memory or also out of bounds?
-		status := container.cg.MemoryEvents()
-		return strconv.FormatBool(status["max"] > 0), nil
-	default:
-		return "", STATUS_UNSUPPORTED
-	}
-}
-
 func (container *SOCKContainer) Meta() *SandboxMeta {
 	return container.meta
 }
@@ -490,8 +483,11 @@ func (container *SOCKContainer) DebugString() string {
 	s += fmt.Sprintf("MEMORY USED: %d of %d MB\n",
 		container.cg.getMemUsageMB(), container.cg.getMemLimitMB())
 
-	s += fmt.Sprintf("MEMORY FAILURES: %d\n",
-		container.cg.ReadInt("memory.failcnt"))
+	if kills, err := container.cg.TryReadIntKV("memory.events", "oom_kill"); err == nil {
+		s += fmt.Sprintf("OOM KILLS: %d\n", kills)
+	} else {
+		s += fmt.Sprintf("OOM KILLS: could not read because %d\n", err.Error())
+	}
 
 	return s
 }