runc/lifecycle: use error to express missing processes

We modelled the renamed StatProcess function on the os.Stat function
from the standard library. This means that we can safely say that if the
error is nil then there will always be a process returned. The
IsNotExist function was added to check for the missing process error case.

This check was then added to the shell command so that the user does not
see an internal error message with internal container names.

* Re-ordered and re-grouped some imports so that bpm imports are at the
  bottom.

* Renamed job to process where necessary.

* Use the state of a process instead of its PID to see if it has failed.

* Do not try to start a job if there is an error trying to fetch
  existing process state.

[finishes #156173439]

Signed-off-by: Christopher Brown <cbrown@pivotal.io>

Author: goonzoid

CHANGELOG.md

@@ -1,6 +1,7 @@
 # v0.6.0
 
 * change ownership of `/etc/profile.d/bpm.sh` to `vcap` group
+* improved consistency of error messages
 
 # v0.5.0
 

src/bpm/commands/pid.go

@@ -20,6 +20,9 @@ import (
 	"fmt"
 
 	"github.com/spf13/cobra"
+
+	"bpm/models"
+	"bpm/runc/lifecycle"
 )
 
 func init() {
@@ -43,20 +46,14 @@ func pidForJob(cmd *cobra.Command, _ []string) error {
 	cmd.SilenceUsage = true
 
 	runcLifecycle := newRuncLifecycle()
-	job, err := runcLifecycle.GetProcess(bpmCfg)
-	if err != nil {
-		return fmt.Errorf("failed to get job: %s", err.Error())
-	}
-
-	if job == nil {
-		return errors.New("job is not running")
-	}
-
-	if job.Pid <= 0 {
-		return errors.New("no pid for job")
+	process, err := runcLifecycle.StatProcess(bpmCfg)
+	if lifecycle.IsNotExist(err) || process.Status == models.ProcessStateFailed {
+		return errors.New("process is not running or could not be found")
+	} else if err != nil {
+		return fmt.Errorf("failed to get job: %s", err)
 	}
 
-	fmt.Fprintf(cmd.OutOrStdout(), "%d\n", job.Pid)
+	fmt.Fprintf(cmd.OutOrStdout(), "%d\n", process.Pid)
 
 	return nil
 }

src/bpm/commands/shell.go

@@ -16,9 +16,14 @@
 package commands
 
 import (
+	"errors"
+	"fmt"
 	"os"
 
 	"github.com/spf13/cobra"
+
+	"bpm/models"
+	"bpm/runc/lifecycle"
 )
 
 func init() {
@@ -42,5 +47,13 @@ func shell(cmd *cobra.Command, _ []string) error {
 	cmd.SilenceUsage = true
 
 	runcLifecycle := newRuncLifecycle()
+
+	process, err := runcLifecycle.StatProcess(bpmCfg)
+	if lifecycle.IsNotExist(err) || process.Status == models.ProcessStateFailed {
+		return errors.New("process is not running or could not be found")
+	} else if err != nil {
+		return fmt.Errorf("failed to get process: %s", err)
+	}
+
 	return runcLifecycle.OpenShell(bpmCfg, os.Stdin, cmd.OutOrStdout(), cmd.OutOrStderr())
 }

src/bpm/commands/start.go

@@ -18,6 +18,7 @@ package commands
 import (
 	"bpm/config"
 	"bpm/models"
+	"bpm/runc/lifecycle"
 	"fmt"
 
 	"github.com/spf13/cobra"
@@ -72,14 +73,15 @@ func start(cmd *cobra.Command, _ []string) error {
 	}
 
 	runcLifecycle := newRuncLifecycle()
-	job, err := runcLifecycle.GetProcess(bpmCfg)
-	if err != nil {
+	process, err := runcLifecycle.StatProcess(bpmCfg)
+	if err != nil && !lifecycle.IsNotExist(err) {
 		logger.Error("failed-getting-job", err)
+		return err
 	}
 
 	var state string
-	if job != nil {
-		state = job.Status
+	if process != nil {
+		state = process.Status
 	}
 
 	switch state {

src/bpm/commands/stop.go

@@ -20,6 +20,8 @@ import (
 	"time"
 
 	"github.com/spf13/cobra"
+
+	"bpm/runc/lifecycle"
 )
 
 const DefaultStopTimeout = 20 * time.Second
@@ -61,19 +63,16 @@ func stop(cmd *cobra.Command, _ []string) error {
 	defer logger.Info("complete")
 
 	runcLifecycle := newRuncLifecycle()
-	job, err := runcLifecycle.GetProcess(bpmCfg)
-	if err != nil {
-		logger.Error("failed-to-get-job", err)
-		return fmt.Errorf("failed to get job-process status: %s", err)
-	}
 
-	if job == nil {
+	if _, err := runcLifecycle.StatProcess(bpmCfg); lifecycle.IsNotExist(err) {
 		logger.Info("job-already-stopped")
 		return nil
+	} else if err != nil {
+		logger.Error("failed-to-get-job", err)
+		return fmt.Errorf("failed to get job-process status: %s", err)
 	}
 
-	err = runcLifecycle.StopProcess(logger, bpmCfg, DefaultStopTimeout)
-	if err != nil {
+	if err := runcLifecycle.StopProcess(logger, bpmCfg, DefaultStopTimeout); err != nil {
 		logger.Error("failed-to-stop", err)
 	}
 

src/bpm/commands/trace.go

@@ -23,6 +23,9 @@ import (
 	"os/signal"
 
 	"github.com/spf13/cobra"
+
+	"bpm/models"
+	"bpm/runc/lifecycle"
 )
 
 var longText = `
@@ -57,20 +60,15 @@ func trace(cmd *cobra.Command, _ []string) error {
 	cmd.SilenceUsage = true
 
 	runcLifecycle := newRuncLifecycle()
-	job, err := runcLifecycle.GetProcess(bpmCfg)
-	if err != nil {
-		return fmt.Errorf("failed to get job: %s", err.Error())
-	}
-
-	if job == nil {
-		return errors.New("job is not running")
-	}
 
-	if job.Pid <= 0 {
-		return errors.New("no pid for job")
+	process, err := runcLifecycle.StatProcess(bpmCfg)
+	if lifecycle.IsNotExist(err) || process.Status == models.ProcessStateFailed {
+		return errors.New("process is not running or could not be found")
+	} else if err != nil {
+		return fmt.Errorf("failed to get process: %s", err)
 	}
 
-	straceCmd := exec.Command("strace", "-s", "100", "-f", "-y", "-yy", "-p", fmt.Sprintf("%d", job.Pid))
+	straceCmd := exec.Command("strace", "-s", "100", "-f", "-y", "-yy", "-p", fmt.Sprintf("%d", process.Pid))
 	straceCmd.Stdin = os.Stdin
 	straceCmd.Stdout = cmd.OutOrStdout()
 	straceCmd.Stderr = cmd.OutOrStderr()

src/bpm/integration/pid_test.go

@@ -94,7 +94,7 @@ var _ = Describe("pid", func() {
 			Expect(err).ShouldNot(HaveOccurred())
 
 			Eventually(session).Should(gexec.Exit(1))
-			Expect(session.Err).Should(gbytes.Say("Error: no pid for job"))
+			Expect(session.Err).Should(gbytes.Say("Error: process is not running or could not be found"))
 		})
 	})
 
@@ -104,7 +104,7 @@ var _ = Describe("pid", func() {
 			Expect(err).ShouldNot(HaveOccurred())
 
 			Eventually(session).Should(gexec.Exit(1))
-			Expect(session.Err).Should(gbytes.Say("Error: job is not running"))
+			Expect(session.Err).Should(gbytes.Say("Error: process is not running or could not be found"))
 		})
 	})
 

src/bpm/integration/shell_test.go

@@ -125,7 +125,7 @@ var _ = Describe("start", func() {
 			session, err := gexec.Start(command, GinkgoWriter, GinkgoWriter)
 			Expect(err).ShouldNot(HaveOccurred())
 			Eventually(session).Should(gexec.Exit(1))
-			Expect(session.Err).Should(gbytes.Say("does not exist"))
+			Expect(session.Err).Should(gbytes.Say("process is not running or could not be found"))
 		})
 	})
 

src/bpm/integration/trace_test.go

@@ -94,7 +94,7 @@ var _ = Describe("trace", func() {
 			session, err := gexec.Start(command, GinkgoWriter, GinkgoWriter)
 			Expect(err).ShouldNot(HaveOccurred())
 			Eventually(session).Should(gexec.Exit(1))
-			Expect(session.Err).Should(gbytes.Say("Error: no pid for job"))
+			Expect(session.Err).Should(gbytes.Say("Error: process is not running or could not be found"))
 		})
 	})
 
@@ -103,7 +103,7 @@ var _ = Describe("trace", func() {
 			session, err := gexec.Start(command, GinkgoWriter, GinkgoWriter)
 			Expect(err).ShouldNot(HaveOccurred())
 			Eventually(session).Should(gexec.Exit(1))
-			Expect(session.Err).Should(gbytes.Say("Error: job is not running"))
+			Expect(session.Err).Should(gbytes.Say("Error: process is not running or could not be found"))
 		})
 	})
 

src/bpm/runc/lifecycle/lifecycle.go

@@ -16,32 +16,41 @@
 package lifecycle
 
 import (
-	"bpm/config"
-	"bpm/models"
-	"bpm/runc/client"
-	"bpm/usertools"
 	"errors"
 	"fmt"
 	"io"
 	"os"
 	"os/exec"
 	"time"
 
-	"github.com/opencontainers/runtime-spec/specs-go"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 
 	"code.cloudfoundry.org/clock"
 	"code.cloudfoundry.org/lager"
+
+	"bpm/config"
+	"bpm/models"
+	"bpm/runc/client"
+	"bpm/usertools"
 )
 
 const (
 	ContainerSigQuitGracePeriod = 5 * time.Second
 	ContainerStatePollInterval  = 1 * time.Second
-	ContainerStateRunning       = "running"
-	ContainerStatePaused        = "paused"
-	ContainerStateStopped       = "stopped"
+
+	ContainerStateRunning = "running"
+	ContainerStatePaused  = "paused"
+	ContainerStateStopped = "stopped"
 )
 
-var TimeoutError = errors.New("failed to stop job within timeout")
+var (
+	timeoutError    = errors.New("failed to stop job within timeout")
+	isNotExistError = errors.New("process is not running or could not be found")
+)
+
+func IsNotExist(err error) bool {
+	return err == isNotExistError
+}
 
 //go:generate counterfeiter . UserFinder
 
@@ -151,18 +160,14 @@ func (j *RuncLifecycle) StartProcess(logger lager.Logger, bpmCfg *config.BPMConf
 	)
 }
 
-// GetProcess returns the following:
-// - process, nil if the process is running (and no errors were encountered)
-// - nil,nil if the process is not running and there is no other error
-// - nil,error if there is any other error getting the process beyond it not running
-func (j *RuncLifecycle) GetProcess(cfg *config.BPMConfig) (*models.Process, error) {
+func (j *RuncLifecycle) StatProcess(cfg *config.BPMConfig) (*models.Process, error) {
 	container, err := j.runcClient.ContainerState(cfg.ContainerID())
 	if err != nil {
 		return nil, err
 	}
 
 	if container == nil {
-		return nil, nil
+		return nil, isNotExistError
 	}
 
 	return newProcessFromContainerState(
@@ -231,7 +236,7 @@ func (j *RuncLifecycle) StopProcess(logger lager.Logger, cfg *config.BPMConfig,
 			}
 
 			j.clock.Sleep(ContainerSigQuitGracePeriod)
-			return TimeoutError
+			return timeoutError
 		}
 	}
 }