Merge pull request #1546 from rbradford/controller-error-handling

Tweak error handling in ciao-controller

ciao-controller/api.go

@@ -352,7 +352,10 @@ func serversAction(c *controller, w http.ResponseWriter, r *http.Request) (APIRe
 				return errorResponse(err), err
 			}
 
-			actionFunc(instanceID)
+			err = actionFunc(instanceID)
+			if err != nil {
+				return errorResponse(err), err
+			}
 		}
 	} else {
 		/* We want to act on all relevant instances */
@@ -367,7 +370,10 @@ func serversAction(c *controller, w http.ResponseWriter, r *http.Request) (APIRe
 				continue
 			}
 
-			actionFunc(instance.ID)
+			err = actionFunc(instance.ID)
+			if err != nil {
+				return errorResponse(err), err
+			}
 		}
 	}
 

ciao-controller/api/api.go

@@ -27,6 +27,7 @@ import (
 	"github.com/ciao-project/ciao/ciao-controller/types"
 	"github.com/ciao-project/ciao/service"
 	"github.com/ciao-project/ciao/uuid"
+	"github.com/golang/glog"
 	"github.com/gorilla/mux"
 )
 
@@ -277,6 +278,8 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			Error: data,
 		}
 
+		glog.Warningf("Returning error response to request: %s: %v", r.URL.String(), err)
+
 		b, err := json.Marshal(code)
 		if err != nil {
 			http.Error(w, http.StatusText(resp.status), resp.status)
@@ -296,7 +299,7 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	w.Header().Set("Content-Type", contentType)
 	w.WriteHeader(resp.status)
-	w.Write(b)
+	_, _ = w.Write(b)
 }
 
 func listResources(c *Context, w http.ResponseWriter, r *http.Request) (Response, error) {
@@ -919,8 +922,6 @@ func validPrivilege(visibility types.Visibility, privileged bool) bool {
 // createImage creates information about an image, but doesn't contain
 // any actual image.
 func createImage(context *Context, w http.ResponseWriter, r *http.Request) (Response, error) {
-	defer r.Body.Close()
-
 	vars := mux.Vars(r)
 	tenantID := vars["tenant"]
 
@@ -1025,8 +1026,6 @@ func createVolume(bc *Context, w http.ResponseWriter, r *http.Request) (Response
 	vars := mux.Vars(r)
 	tenant := vars["tenant"]
 
-	defer r.Body.Close()
-
 	body, err := ioutil.ReadAll(r.Body)
 	if err != nil {
 		return Response{http.StatusBadRequest, nil}, err
@@ -1139,8 +1138,6 @@ func volumeAction(bc *Context, w http.ResponseWriter, r *http.Request) (Response
 
 	var req interface{}
 
-	defer r.Body.Close()
-
 	body, err := ioutil.ReadAll(r.Body)
 	if err != nil {
 		return Response{http.StatusBadRequest, nil}, err
@@ -1170,8 +1167,6 @@ func createInstance(c *Context, w http.ResponseWriter, r *http.Request) (Respons
 	vars := mux.Vars(r)
 	tenant := vars["tenant"]
 
-	defer r.Body.Close()
-
 	body, err := ioutil.ReadAll(r.Body)
 	if err != nil {
 		return Response{http.StatusBadRequest, nil}, err
@@ -1262,8 +1257,6 @@ func instanceAction(c *Context, w http.ResponseWriter, r *http.Request) (Respons
 	tenant := vars["tenant"]
 	server := vars["instance_id"]
 
-	defer r.Body.Close()
-
 	body, err := ioutil.ReadAll(r.Body)
 	if err != nil {
 		return Response{http.StatusBadRequest, nil}, err

ciao-controller/client.go

@@ -147,7 +147,10 @@ func (client *ssntpClient) RemoveInstance(instanceID string) {
 	}
 
 	// notify anyone is listening for a state change
-	transitionInstanceState(i, payloads.Deleted)
+	err = transitionInstanceState(i, payloads.Deleted)
+	if err != nil {
+		glog.Warningf("Error transitioning CNCI to deleted: %v", err)
+	}
 }
 
 func (client *ssntpClient) instanceDeleted(payload []byte) {
@@ -222,7 +225,10 @@ func (client *ssntpClient) concentratorInstanceAdded(payload []byte) {
 		return
 	}
 
-	tenant.CNCIctrl.CNCIAdded(newCNCI.InstanceUUID)
+	err = tenant.CNCIctrl.CNCIAdded(newCNCI.InstanceUUID)
+	if err != nil {
+		glog.Warningf("Error adding CNCI: %v", err)
+	}
 }
 
 func (client *ssntpClient) traceReport(payload []byte) {
@@ -259,7 +265,10 @@ func (client *ssntpClient) nodeDisconnected(payload []byte) {
 	}
 
 	glog.Infof("Node %s disconnected", nodeDisconnected.Disconnected.NodeUUID)
-	client.ctl.ds.DeleteNode(nodeDisconnected.Disconnected.NodeUUID)
+	err = client.ctl.ds.DeleteNode(nodeDisconnected.Disconnected.NodeUUID)
+	if err != nil {
+		glog.Warningf("Error marking node as deleted in datastore: %v", err)
+	}
 }
 
 func (client *ssntpClient) unassignEvent(payload []byte) {
@@ -285,7 +294,10 @@ func (client *ssntpClient) unassignEvent(payload []byte) {
 	client.ctl.qs.Release(i.TenantID, payloads.RequestedResource{Type: payloads.ExternalIP, Value: 1})
 
 	msg := fmt.Sprintf("Unmapped %s from %s", event.UnassignedIP.PublicIP, event.UnassignedIP.PrivateIP)
-	client.ctl.ds.LogEvent(i.TenantID, msg)
+	err = client.ctl.ds.LogEvent(i.TenantID, msg)
+	if err != nil {
+		glog.Warningf("Error logging event: %v", err)
+	}
 }
 
 func (client *ssntpClient) assignEvent(payload []byte) {
@@ -303,7 +315,10 @@ func (client *ssntpClient) assignEvent(payload []byte) {
 	}
 
 	msg := fmt.Sprintf("Mapped %s to %s", event.AssignedIP.PublicIP, event.AssignedIP.PrivateIP)
-	client.ctl.ds.LogEvent(i.TenantID, msg)
+	err = client.ctl.ds.LogEvent(i.TenantID, msg)
+	if err != nil {
+		glog.Warningf("Error logging event: %v", err)
+	}
 }
 
 func (client *ssntpClient) EventNotify(event ssntp.Event, frame *ssntp.Frame) {
@@ -377,7 +392,10 @@ func (client *ssntpClient) startFailure(payload []byte) {
 			return
 		}
 
-		tenant.CNCIctrl.StartFailure(failure.InstanceUUID)
+		err = tenant.CNCIctrl.StartFailure(failure.InstanceUUID)
+		if err != nil {
+			glog.Warningf("Error adding StartFailure to datastore: %v", err)
+		}
 	}
 }
 
@@ -388,7 +406,7 @@ func (client *ssntpClient) stopFailure(payload []byte) {
 		glog.Warningf("Error unmarshalling StopFailure: %v", err)
 		return
 	}
-	client.ctl.ds.StopFailure(failure.InstanceUUID, failure.Reason)
+	err = client.ctl.ds.StopFailure(failure.InstanceUUID, failure.Reason)
 	if err != nil {
 		glog.Warningf("Error adding StopFailure to datastore: %v", err)
 	}
@@ -436,7 +454,10 @@ func (client *ssntpClient) assignError(payload []byte) {
 	client.ctl.qs.Release(failure.TenantUUID, payloads.RequestedResource{Type: payloads.ExternalIP, Value: 1})
 
 	msg := fmt.Sprintf("Failed to map %s to %s: %s", failure.PublicIP, failure.InstanceUUID, failure.Reason.String())
-	client.ctl.ds.LogError(failure.TenantUUID, msg)
+	err = client.ctl.ds.LogError(failure.TenantUUID, msg)
+	if err != nil {
+		glog.Warningf("Error logging error: %v", err)
+	}
 }
 
 func (client *ssntpClient) unassignError(payload []byte) {
@@ -449,7 +470,10 @@ func (client *ssntpClient) unassignError(payload []byte) {
 
 	// we can't unmap the IP - all we can do is log.
 	msg := fmt.Sprintf("Failed to unmap %s from %s: %s", failure.PublicIP, failure.InstanceUUID, failure.Reason.String())
-	client.ctl.ds.LogError(failure.TenantUUID, msg)
+	err = client.ctl.ds.LogError(failure.TenantUUID, msg)
+	if err != nil {
+		glog.Warningf("Error logging error: %v", err)
+	}
 }
 
 func (client *ssntpClient) ErrorNotify(err ssntp.Error, frame *ssntp.Frame) {

ciao-controller/cnci.go

@@ -102,7 +102,10 @@ func waitForEventTimeout(ch chan event, e event, timeout time.Duration) error {
 func (c *CNCI) transitionState(to CNCIState) {
 	glog.Infof("State transition to %s received for %s", to, c.instance.ID)
 
-	transitionInstanceState(c.instance, (string(to)))
+	err := transitionInstanceState(c.instance, (string(to)))
+	if err != nil {
+		glog.Warningf("Error transitioning instance %s to %s state", c.instance.ID, string(to))
+	}
 
 	// some state changes cause events
 	ch := c.eventCh
@@ -346,9 +349,9 @@ func (c *CNCIManager) CNCIStopped(id string) error {
 	}
 
 	cnci.transitionState(exited)
-	c.ctrl.restartInstance(cnci.instance.ID)
+	err := c.ctrl.restartInstance(cnci.instance.ID)
 
-	return nil
+	return errors.Wrap(err, "Error restarting instance")
 }
 
 // CNCIAdded will move the CNCI into the active state

ciao-controller/command.go

@@ -48,10 +48,18 @@ func (c *controller) restartInstance(instanceID string) error {
 	}
 
 	if !i.CNCI {
-		t.CNCIctrl.WaitForActiveSubnetString(i.Subnet)
+		err = t.CNCIctrl.WaitForActiveSubnetString(i.Subnet)
+		if err != nil {
+			return errors.Wrap(err, "Error waiting for active subnet")
+		}
 	}
 
-	go c.client.RestartInstance(i, &w, t)
+	go func() {
+		if err := c.client.RestartInstance(i, &w, t); err != nil {
+			glog.Warningf("Error restarting instance: %v", err)
+		}
+	}()
+
 	return nil
 }
 
@@ -70,7 +78,12 @@ func (c *controller) stopInstance(instanceID string) error {
 		return errors.New("You may not stop a pending instance")
 	}
 
-	go c.client.StopInstance(instanceID, i.NodeID)
+	go func() {
+		if err := c.client.StopInstance(instanceID, i.NodeID); err != nil {
+			glog.Warningf("Error stopping instance: %v", err)
+		}
+	}()
+
 	return nil
 }
 
@@ -111,7 +124,10 @@ func (c *controller) deleteInstanceSync(instanceID string) error {
 	case <-wait:
 		return nil
 	case <-time.After(2 * time.Minute):
-		transitionInstanceState(i, payloads.Hung)
+		err = transitionInstanceState(i, payloads.Hung)
+		if err != nil {
+			glog.Warningf("Error transitioning instance to hung state: %v", err)
+		}
 		return fmt.Errorf("timeout waiting for delete")
 	}
 }
@@ -136,7 +152,12 @@ func (c *controller) deleteInstance(instanceID string) error {
 		}
 	}
 
-	go c.client.DeleteInstance(instanceID, i.NodeID)
+	go func() {
+		if err := c.client.DeleteInstance(instanceID, i.NodeID); err != nil {
+			glog.Warningf("Error deleting instance: %v", err)
+		}
+	}()
+
 	return nil
 }
 
@@ -243,27 +264,34 @@ func (c *controller) startWorkload(w types.WorkloadRequest) ([]*types.Instance,
 
 		ok, err := instance.Allowed()
 		if err != nil {
-			instance.Clean()
+			_ = instance.Clean()
 			e = errors.Wrap(err, "Error checking if instance allowed")
 			continue
 		}
 
 		if ok {
 			err = instance.Add()
 			if err != nil {
-				instance.Clean()
+				_ = instance.Clean()
 				e = errors.Wrap(err, "Error adding instance")
 				continue
 			}
 
 			newInstances = append(newInstances, instance.Instance)
-			if w.TraceLabel == "" {
-				go c.client.StartWorkload(instance.newConfig.config)
-			} else {
-				go c.client.StartTracedWorkload(instance.newConfig.config, instance.startTime, w.TraceLabel)
-			}
+			go func(label string) {
+				var err error
+				if label == "" {
+					err = c.client.StartWorkload(instance.newConfig.config)
+				} else {
+					err = c.client.StartTracedWorkload(instance.newConfig.config, instance.startTime, w.TraceLabel)
+				}
+
+				if err != nil {
+					glog.Warningf("Error starting workload: %v", err)
+				}
+			}(w.TraceLabel)
 		} else {
-			instance.Clean()
+			_ = instance.Clean()
 			// stop if we are over limits
 			e = errors.New("Over quota")
 			continue

ciao-controller/compute.go

@@ -384,6 +384,10 @@ func (c *controller) CreateServer(tenant string, server api.CreateServerRequest)
 		servers.Servers = append(servers.Servers, server)
 	}
 
+	if e != nil {
+		_ = c.ds.LogError(tenant, fmt.Sprintf("Error launching instance(s): %v", e))
+	}
+
 	// If no instances launcher or if none converted bail early
 	if e != nil && len(servers.Servers) == 0 {
 		return server, e
@@ -409,9 +413,6 @@ func (c *controller) CreateServer(tenant string, server api.CreateServerRequest)
 		},
 	}
 
-	if e != nil {
-		c.ds.LogError(tenant, fmt.Sprintf("Error launching instance(s): %v", e))
-	}
 	return builtServers, nil
 }