Added state machine checks for Container.Status

For my own sanitity when thinking about possible transitions and state.

PiperOrigin-RevId: 213559482
Change-Id: I25588c86cf6098be4eda01f4e7321c102ceef33c

2 files changed, 58 insertions, 11 deletions

diff --git a/runsc/container/container.go b/runsc/container/container.go
index 9bf2f4625..3be88066c 100644
--- a/runsc/container/container.go
+++ b/runsc/container/container.go
@@ -92,7 +92,7 @@ type Container struct {
 	Status Status `json:"status"`
 
 	// GoferPid is the pid of the gofer running along side the sandbox. May
-	// be 0 if the gofer has been killed or it's not being used.
+	// be 0 if the gofer has been killed.
 	GoferPid int `json:"goferPid"`
 
 	// Sandbox is the sandbox this container is running in. It will be nil
@@ -138,14 +138,13 @@ func Load(rootDir, id string) (*Container, error) {
 		// Check if the sandbox process is still running.
 		if !c.Sandbox.IsRunning() {
 			// Sandbox no longer exists, so this container definitely does not exist.
-			c.Status = Stopped
-			c.Sandbox = nil
+			c.changeStatus(Stopped)
 		} else if c.Status == Running {
 			// Container state should reflect the actual state of
 			// the application, so we don't consider gofer process
 			// here.
 			if err := c.Signal(syscall.Signal(0)); err != nil {
-				c.Status = Stopped
+				c.changeStatus(Stopped)
 			}
 		}
 	}
@@ -265,7 +264,7 @@ func Create(id string, spec *specs.Spec, conf *boot.Config, bundleDir, consoleSo
 		}
 		c.Sandbox = sb.Sandbox
 	}
-	c.Status = Created
+	c.changeStatus(Created)
 
 	// Save the metadata file.
 	if err := c.save(); err != nil {
@@ -322,7 +321,7 @@ func (c *Container) Start(conf *boot.Config) error {
 		executeHooksBestEffort(c.Spec.Hooks.Poststart, c.State())
 	}
 
-	c.Status = Running
+	c.changeStatus(Running)
 	return c.save()
 }
 
@@ -338,7 +337,7 @@ func (c *Container) Restore(spec *specs.Spec, conf *boot.Config, restoreFile str
 	if err := c.Sandbox.Restore(c.ID, spec, conf, restoreFile); err != nil {
 		return err
 	}
-	c.Status = Running
+	c.changeStatus(Running)
 	return c.save()
 }
 
@@ -447,7 +446,7 @@ func (c *Container) Pause() error {
 		if err := c.Sandbox.Pause(c.ID); err != nil {
 			return fmt.Errorf("error pausing container: %v", err)
 		}
-		c.Status = Paused
+		c.changeStatus(Paused)
 		return c.save()
 	default:
 		return fmt.Errorf("container %q not created or running, not pausing", c.ID)
@@ -463,7 +462,7 @@ func (c *Container) Resume() error {
 		if err := c.Sandbox.Resume(c.ID); err != nil {
 			return fmt.Errorf("error resuming container: %v", err)
 		}
-		c.Status = Running
+		c.changeStatus(Running)
 		return c.save()
 	default:
 		return fmt.Errorf("container %q not paused, not resuming", c.ID)
@@ -519,7 +518,7 @@ func (c *Container) Destroy() error {
 		executeHooksBestEffort(c.Spec.Hooks.Poststop, c.State())
 	}
 
-	c.Status = Stopped
+	c.changeStatus(Stopped)
 	return nil
 }
 
@@ -583,6 +582,7 @@ func (c *Container) waitForStopped() error {
 			if err := syscall.Kill(c.GoferPid, 0); err == nil {
 				return fmt.Errorf("gofer is still running")
 			}
+			c.GoferPid = 0
 		}
 		return nil
 	}
@@ -652,3 +652,47 @@ func (c *Container) createGoferProcess(spec *specs.Spec, conf *boot.Config, bund
 	c.GoferPid = cmd.Process.Pid
 	return sandEnds, nil
 }
+
+// changeStatus transitions from one status to another ensuring that the
+// transition is valid.
+func (c *Container) changeStatus(s Status) {
+	switch s {
+	case Creating:
+		// Initial state, never transitions to it.
+		panic(fmt.Sprintf("invalid state transition: %v => %v", c.Status, s))
+
+	case Created:
+		if c.Status != Creating {
+			panic(fmt.Sprintf("invalid state transition: %v => %v", c.Status, s))
+		}
+		if c.Sandbox == nil {
+			panic("sandbox cannot be nil")
+		}
+
+	case Paused:
+		if c.Status != Running {
+			panic(fmt.Sprintf("invalid state transition: %v => %v", c.Status, s))
+		}
+		if c.Sandbox == nil {
+			panic("sandbox cannot be nil")
+		}
+
+	case Running:
+		if c.Status != Created && c.Status != Paused {
+			panic(fmt.Sprintf("invalid state transition: %v => %v", c.Status, s))
+		}
+		if c.Sandbox == nil {
+			panic("sandbox cannot be nil")
+		}
+
+	case Stopped:
+		if c.Status != Created && c.Status != Running && c.Status != Stopped {
+			panic(fmt.Sprintf("invalid state transition: %v => %v", c.Status, s))
+		}
+		c.Sandbox = nil
+
+	default:
+		panic(fmt.Sprintf("invalid new state: %v", s))
+	}
+	c.Status = s
+}
diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go
index 349ea755a..d6418efb6 100644
--- a/runsc/container/multi_container_test.go
+++ b/runsc/container/multi_container_test.go
@@ -378,12 +378,15 @@ func TestMultiContainerSignal(t *testing.T) {
 			t.Errorf("failed to wait for sleep to start: %v", err)
 		}
 
+		// goferPid is reset when container is destroyed.
+		goferPid := containers[1].GoferPid
+
 		// Destroy container and ensure container's gofer process has exited.
 		if err := containers[1].Destroy(); err != nil {
 			t.Errorf("failed to destroy container: %v", err)
 		}
 		_, _, err = testutil.RetryEintr(func() (uintptr, uintptr, error) {
-			cpid, err := syscall.Wait4(containers[1].GoferPid, nil, 0, nil)
+			cpid, err := syscall.Wait4(goferPid, nil, 0, nil)
 			return uintptr(cpid), 0, err
 		})
 		if err != nil && err != syscall.ECHILD {