Add additional error handling to TaskWait operations

lib/apiservers/engine/backends/container.go

@@ -58,7 +58,6 @@ import (
 	"github.com/vmware/vic/lib/apiservers/engine/backends/portmap"
 	"github.com/vmware/vic/lib/apiservers/portlayer/client/containers"
 	"github.com/vmware/vic/lib/apiservers/portlayer/client/scopes"
-	"github.com/vmware/vic/lib/apiservers/portlayer/client/tasks"
 	"github.com/vmware/vic/lib/apiservers/portlayer/models"
 	"github.com/vmware/vic/lib/archive"
 	"github.com/vmware/vic/lib/config/executor"
@@ -189,52 +188,18 @@ const (
 	defaultEnvPath = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
 )
 
+// TODO: we should probably just make this purely a proxy thing. This is now just a passthrough
 func (c *Container) Handle(id, name string) (string, error) {
-	resp, err := c.containerProxy.Client().Containers.Get(containers.NewGetParamsWithContext(ctx).WithID(id))
-	if err != nil {
-		switch err := err.(type) {
-		case *containers.GetNotFound:
-			cache.ContainerCache().DeleteContainer(id)
-			return "", NotFoundError(name)
-		case *containers.GetDefault:
-			return "", InternalServerError(err.Payload.Message)
-		default:
-			return "", InternalServerError(err.Error())
-		}
-	}
-	return resp.Payload, nil
+	return c.containerProxy.Handle(id, name)
 }
 
 // docker's container.execBackend
 
 func (c *Container) TaskWaitToStart(cid, cname, eid string) error {
-	// obtain a portlayer client
-	client := c.containerProxy.Client()
-
-	handle, err := c.Handle(cid, cname)
-	if err != nil {
-		return err
-	}
-
-	// wait the Task to start
-	config := &models.TaskWaitConfig{
-		Handle: handle,
-		ID:     eid,
-	}
-
-	params := tasks.NewWaitParamsWithContext(ctx).WithConfig(config)
-	// FIXME: NEEDS CONTAINER PROXY
-	_, err = client.Tasks.Wait(params)
-	if err != nil {
-		switch err := err.(type) {
-		case *tasks.WaitInternalServerError:
-			return InternalServerError(err.Payload.Message)
-		default:
-			return InternalServerError(err.Error())
-		}
-	}
+	op := trace.NewOperation(context.TODO(), "")
+	defer trace.End(trace.Begin(fmt.Sprintf("%s: cname=(%s), cid=(%s), eid=(%s)", op, cname, cid, eid)))
 
-	return nil
+	return c.containerProxy.WaitTask(op, cid, cname, eid)
 }
 
 // ContainerExecCreate sets up an exec in a running container.
@@ -270,7 +235,7 @@ func (c *Container) ContainerExecCreate(name string, config *types.ExecConfig) (
 		// NOTE: we should investigate what we can add or manipulate in the handle in the portlayer to make this check even more granular. Maybe Add the actually State into the handle config or part of the container info could be "snapshotted"
 		// This check is now done off of the handle.
 		if state != "RUNNING" {
-			return ConflictError(fmt.Sprintf("Container %s is restarting, wait until the container is running", id))
+			return ConflictError(fmt.Sprintf("Container (%s) is not powered on, please start the container before attempting to an exec an operation", name))
 		}
 
 		op.Debugf("State checks succeeded for exec operation on container(%s)", id)

lib/apiservers/engine/backends/container_proxy.go

@@ -93,6 +93,7 @@ type VicContainerProxy interface {
 	// TODO: we should not be returning a swagger model here, however we do not have a solid architected return for this yet.
 	InspectTask(op trace.Operation, handle string, eid string, cid string) (*models.TaskInspectResponse, error)
 	BindTask(op trace.Operation, handle string, eid string) (string, error)
+	WaitTask(op trace.Operation, cid string, cname string, eid string) error
 
 	BindInteraction(handle string, name string, id string) (string, error)
 	UnbindInteraction(handle string, name string, id string) (string, error)
@@ -390,6 +391,37 @@ func (c *ContainerProxy) BindTask(op trace.Operation, handle string, eid string)
 	return respHandle, nil
 }
 
+func (c *ContainerProxy) WaitTask(op trace.Operation, cid string, cname string, eid string) error {
+	handle, err := c.Handle(cid, cname)
+	if err != nil {
+		return err
+	}
+
+	// wait the Task to start
+	config := &models.TaskWaitConfig{
+		Handle: handle,
+		ID:     eid,
+	}
+
+	params := tasks.NewWaitParamsWithContext(ctx).WithConfig(config)
+	_, err = c.client.Tasks.Wait(params)
+	if err != nil {
+		switch err := err.(type) {
+		case *tasks.WaitNotFound:
+			return InternalServerError("the Container(%s) has been shutdown during execution ofthe exec operation")
+		case *tasks.WaitPreconditionRequired:
+			return InternalServerError("container(%s) must be powered on in order perform the desired exec operation")
+		case *tasks.WaitInternalServerError:
+			return InternalServerError(err.Payload.Message)
+		default:
+			return InternalServerError(err.Error())
+		}
+	}
+
+	return nil
+
+}
+
 // AddContainerToScope adds a container, referenced by handle, to a scope.
 // If an error is return, the returned handle should not be used.
 //

lib/apiservers/portlayer/restapi/handlers/task_handlers.go

@@ -277,11 +277,20 @@ func (handler *TaskHandlersImpl) WaitHandler(params tasks.WaitParams) middleware
 	// wait task to set started field to something
 	err := task.Wait(&op, handle, params.Config.ID)
 	if err != nil {
-		op.Errorf("%s", err.Error())
-
-		return tasks.NewWaitInternalServerError().WithPayload(
-			&models.Error{Message: err.Error()},
-		)
+		switch err := err.(type) {
+		case *task.TaskPowerStateError:
+			op.Debugf("PowerStateError occured for task (%s) on handle (%s) while attempting to wait for the task to complete", params.Config.ID, handle)
+			return tasks.NewWaitPreconditionRequired().WithPayload(
+				&models.Error{Message: err.Error()})
+		case *task.TaskNotFoundError:
+			op.Debugf("TaskNotFoundError occured for task (%s) on handle (%s) while attempting to wait for the task to complete", params.Config.ID, handle)
+			return tasks.NewWaitNotFound().WithPayload(
+				&models.Error{Message: err.Error()})
+		default:
+			op.Errorf("%s", err.Error())
+			return tasks.NewWaitInternalServerError().WithPayload(
+				&models.Error{Message: err.Error()})
+		}
 	}
 
 	return tasks.NewWaitOK()

lib/apiservers/portlayer/swagger.json

@@ -1772,6 +1772,12 @@
 							"$ref": "#/definitions/Error"
 						}
 					},
+					"428": {
+						"description": "target resource is not powered on",
+						"schema": {
+							"$ref": "#/definitions/Error"
+						}
+					},
 					"500": {
 						"description": "Wait of task failed",
 						"schema": {

lib/portlayer/task/wait.go

@@ -35,7 +35,7 @@ func Wait(op *trace.Operation, h interface{}, id string) error {
 	if handle.Runtime != nil && handle.Runtime.PowerState != types.VirtualMachinePowerStatePoweredOn {
 		err := fmt.Errorf("Unable to wait for task when container %s is not running", id)
 		op.Errorf("%s", err)
-		return err
+		return TaskPowerStateError{Err: err}
 	}
 
 	_, okS := handle.ExecConfig.Sessions[id]
@@ -59,3 +59,11 @@ func Wait(op *trace.Operation, h interface{}, id string) error {
 	}
 	return c.WaitForExec(timeout, id)
 }
+
+type TaskPowerStateError struct {
+	Err error
+}
+
+func (t TaskPowerStateError) Error() string {
+	return t.Err.Error()
+}

tests/test-cases/Group1-Docker-Commands/1-38-Docker-Exec.robot

@@ -128,20 +128,21 @@ Exec During PowerOff
      ${rc}  ${output}=  Run And Return Rc And Output  docker %{VCH-PARAMS} pull ${busybox}
      Should Be Equal As Integers  ${rc}  0
      Should Not Contain  ${output}  Error
-     ${rc}  ${id}=  Run And Return Rc And Output  docker %{VCH-PARAMS} run -d ${busybox} /bin/top
+     ${suffix}=  Evaluate  '%{DRONE_BUILD_NUMBER}-' + str(random.randint(1000,9999))  modules=random
+     Set Test Variable  ${ExecPowerOffContainer}  I-Have-Two-Anonymous-Volumes-${suffix}
+     ${rc}  ${id}=  Run And Return Rc And Output  docker %{VCH-PARAMS} run -itd --name ${ExecPoweroffContainer} ${busybox} /bin/top
      Should Be Equal As Integers  ${rc}  0
+
      :FOR  ${idx}  IN RANGE  1  20
      \   Start Process  docker %{VCH-PARAMS} exec ${id} /bin/top  alias=exec-%{VCH-NAME}-${idx}  shell=true
-
-     Start Process  docker %{VCH-PARAMS} stop ${id}  alias=stop-%{VCH-NAME}-${id}  shell=true
+
+     ${rc}  ${id}=  Run And Return Rc And Output  docker %{VCH-PARAMS} stop ${id}
+     Should Be Equal As Integers  ${rc}  0
 
      ${combinedoutput}=  Set Variable
 
      :FOR  ${idx}  IN RANGE  1  20
      \   ${result}=  Wait For Process  exec-%{VCH-NAME}-${idx}  timeout=2 mins
      \   ${combinedOutput}=  Catenate  ${combinedOutput}  ${result.stderr}${\n}
 
-     ${stopResult}=  Wait For Process  stop-%{VCH-NAME}-${id}
-     Should Be Equal As Integers  ${stopResult.rc}  0
-
-     Should Contain  ${combinedOutput}  container (${id}) has been powered off
+     Should Contain  ${combinedOutput}  Conflict error from portlayer: Container (${ExecPoweroffContainer}) is not powered on, please start the container before attempting to an exec an operation