state: Second approach to improve the error handling.

state/state.go

 // The state package enables reading, observing, and changing
 // the state stored in ZooKeeper of a whole environment
 // managed by juju.
 package state
 
 import (
         "fmt"
         "launchpad.net/goyaml"
         "launchpad.net/gozk/zookeeper"
         "launchpad.net/juju-core/juju/charm"
         "net/url"
         "strings"
 )
 
 const (
         zkEnvironmentPath = "/environment"
         zkMachinesPath    = "/machines"
         zkTopologyPath    = "/topology"
 )
 
 // State represents the state of an environment
 // managed by juju.
 type State struct {
         zk  *zookeeper.Conn
         fwd *sshForwarder
 }
 
 // AddMachine creates a new machine state.
-func (s *State) AddMachine() (*Machine, error) {
-        path, err := s.zk.Create("/machines/machine-", "", zookeeper.SEQUENCE, zkPermAll)
-        if err != nil {
-                return nil, err
+func (s *State) AddMachine() (m *Machine, err error) {
+        defer errorContext(&err, "can't add a new machine: %v")
+        var path string
+        if path, err = s.zk.Create("/machines/machine-", "", zookeeper.SEQUENCE, zkPermAll); err != nil {
+                return
         }
         key := strings.Split(path, "/")[2]
         addMachine := func(t *topology) error {
                 return t.AddMachine(key)
         }
         if err = retryTopologyChange(s.zk, addMachine); err != nil {
-                return nil, err
+                return
         }
         return &Machine{s, key}, nil
 }
 
 // RemoveMachine removes the machine with the given id.
-func (s *State) RemoveMachine(id int) error {
+func (s *State) RemoveMachine(id int) (err error) {
+        defer errorContext(&err, "can't remove machine %d: %v", id)
         key := machineKey(id)
         removeMachine := func(t *topology) error {
                 if !t.HasMachine(key) {
                         return fmt.Errorf("machine not found")
                 }
                 hasUnits, err := t.MachineHasUnits(key)
                 if err != nil {
                         return err
                 }
                 if hasUnits {
                         return fmt.Errorf("machine has units")
                 }
                 return t.RemoveMachine(key)
         }
-        if err := retryTopologyChange(s.zk, removeMachine); err != nil {
-                return fmt.Errorf("can't remove machine %d: %v", id, err)
+        if err = retryTopologyChange(s.zk, removeMachine); err != nil {
+                return
         }
         return zkRemoveTree(s.zk, fmt.Sprintf("/machines/%s", key))
 }
 
 // WatchMachines watches for new Machines added or removed.
 func (s *State) WatchMachines() *MachinesWatcher {
         return newMachinesWatcher(s)
 }
 
 // WatchEnvironConfig returns a watcher for observing
 // changes to the environment configuration.
 func (s *State) WatchEnvironConfig() *ConfigWatcher {
         return newConfigWatcher(s, zkEnvironmentPath)
 }
 
 // EnvironConfig returns the current configuration of the environment.
 func (s *State) EnvironConfig() (*ConfigNode, error) {
         return readConfigNode(s.zk, zkEnvironmentPath)
 }
 
 // Machine returns the machine with the given id.
 func (s *State) Machine(id int) (*Machine, error) {
         key := machineKey(id)
         topology, err := readTopology(s.zk)
         if err != nil {
-                return nil, err
+                return nil, fmt.Errorf("can't get machine %d: %v", id, err)
         }
         if !topology.HasMachine(key) {
                 return nil, fmt.Errorf("machine %d not found", id)
         }
         return &Machine{s, key}, nil
 }
 
 // AllMachines returns all machines in the environment.
 func (s *State) AllMachines() ([]*Machine, error) {
         topology, err := readTopology(s.zk)
         if err != nil {
-                return nil, err
+                return nil, fmt.Errorf("can't get all machines: %v", err)
         }
         machines := []*Machine{}
         for _, key := range topology.MachineKeys() {
                 machines = append(machines, &Machine{s, key})
         }
         return machines, nil
 }
 
 // AddCharm adds the ch charm with curl to the state.
 // bundleUrl must be set to a URL where the bundle for ch
 // may be downloaded from.
 // On success the newly added charm state is returned.
-func (s *State) AddCharm(ch charm.Charm, curl *charm.URL, bundleURL *url.URL, bundleSha256 string) (*Charm, error) {
+func (s *State) AddCharm(ch charm.Charm, curl *charm.URL, bundleURL *url.URL, bundleSha256 string) (stch *Charm, err error) {
+        defer errorContext(&err, "can't add charm %q: %v", curl)
         data := &charmData{
                 Meta:         ch.Meta(),
                 Config:       ch.Config(),
                 BundleURL:    bundleURL.String(),
                 BundleSha256: bundleSha256,
         }
         yaml, err := goyaml.Marshal(data)
         if err != nil {
-                return nil, err
+                return
         }
         path, err := charmPath(curl)
         if err != nil {
-                return nil, err
+                return
         }
         _, err = s.zk.Create(path, string(yaml), 0, zkPermAll)
         if err != nil {
-                return nil, err
+                return
         }
-        return newCharm(s, curl, data)
+        stch, err = newCharm(s, curl, data)

[niemeyer, 2012/06/13 12:32:38]

Please revert. The original version is fine.

[TheMue, 2012/06/13 12:54:34]

Done.

+        if err != nil {
+                return
+        }
+        return stch, nil
 }
 
 // Charm returns a charm by the given id.
-func (s *State) Charm(curl *charm.URL) (*Charm, error) {
+func (s *State) Charm(curl *charm.URL) (stch *Charm, err error) {
+        defer errorContext(&err, "can't get charm %q: %v", curl)
         path, err := charmPath(curl)
         if err != nil {
-                return nil, err
+                return
         }
         yaml, _, err := s.zk.Get(path)
         if zookeeper.IsError(err, zookeeper.ZNONODE) {
-                return nil, fmt.Errorf("charm not found: %q", curl)
+                return
         }
         if err != nil {
-                return nil, err
+                return
         }
         data := &charmData{}
-        if err := goyaml.Unmarshal([]byte(yaml), data); err != nil {
-                return nil, err
+        if err = goyaml.Unmarshal([]byte(yaml), data); err != nil {
+                return
         }
-        return newCharm(s, curl, data)
+        stch, err = newCharm(s, curl, data)

[niemeyer, 2012/06/13 12:32:38]

Ditto.

[TheMue, 2012/06/13 12:54:34]

Done.

+        if err != nil {
+                return
+        }
+        return stch, nil
 }
 
 // AddService creates a new service state with the given unique name
 // and the charm state.
-func (s *State) AddService(name string, ch *Charm) (*Service, error) {
+func (s *State) AddService(name string, ch *Charm) (service *Service, err error) {
+        defer errorContext(&err, "can't add service %q: %v", name)
         details := map[string]interface{}{"charm": ch.URL().String()}
         yaml, err := goyaml.Marshal(details)
         if err != nil {
-                return nil, err
+                return
         }
         path, err := s.zk.Create("/services/service-", string(yaml), zookeeper.SEQUENCE, zkPermAll)
         if err != nil {
-                return nil, err
+                return
         }
         key := strings.Split(path, "/")[2]
-        service := &Service{s, key, name}
+        service = &Service{s, key, name}
         // Create an empty configuration node.
         _, err = createConfigNode(s.zk, service.zkConfigPath(), map[string]interface{}{})
         if err != nil {
-                return nil, err

[niemeyer, 2012/06/13 12:32:38]

Please revert all of these changes. The original "return nil, err" was correct
across the board, and will work fine with errorContext. With this change,
AddService is now returning a service even when there are errors, which we don't
want.

[TheMue, 2012/06/13 12:54:34]

Done.

+                return
         }
         // Create a parent node for the service units
         _, err = s.zk.Create(service.zkPath()+"/units", "", 0, zkPermAll)
         if err != nil {
-                return nil, err
+                return
         }
         addService := func(t *topology) error {
                 if _, err := t.ServiceKey(name); err == nil {
                         // No error, so service name already in use.
-                        return fmt.Errorf("service name %q is already in use", name)
+                        return fmt.Errorf("service name is already in use")
                 }
                 return t.AddService(key, name)
         }
         if err = retryTopologyChange(s.zk, addService); err != nil {
-                return nil, err
+                return
         }
         return service, nil
 }
 
 // RemoveService removes a service from the state. It will
 // also remove all its units and break any of its existing
 // relations.
-func (s *State) RemoveService(svc *Service) error {
+func (s *State) RemoveService(svc *Service) (err error) {
+        defer errorContext(&err, "can't remove service %q: %v", svc.Name())
         // TODO Remove relations first, to prevent spurious hook execution.
 
         // Remove the units.
         units, err := svc.AllUnits()
         if err != nil {
-                return err
+                return
         }
         for _, unit := range units {
                 if err = svc.RemoveUnit(unit); err != nil {
-                        return err
+                        return fmt.Errorf("can't remove unit %q: %v", unit.Name(), err)

[niemeyer, 2012/06/13 12:32:38]

No need to adorn it further. RemoveUnit itself will be taking care of that.

[TheMue, 2012/06/13 12:54:34]

Done.

                 }
         }
         // Remove the service from the topology.
         removeService := func(t *topology) error {
                 if !t.HasService(svc.key) {
                         return stateChanged
                 }
                 t.RemoveService(svc.key)
                 return nil
         }
         if err = retryTopologyChange(s.zk, removeService); err != nil {
-                return err
+                return
         }
-        return zkRemoveTree(s.zk, svc.zkPath())
+        if err = zkRemoveTree(s.zk, svc.zkPath()); err != nil {

[niemeyer, 2012/06/13 12:32:38]

Please revert.

[TheMue, 2012/06/13 12:54:34]

Done.

+                return
+        }
+        return nil
 }
 
 // Service returns a service state by name.
 func (s *State) Service(name string) (*Service, error) {
         topology, err := readTopology(s.zk)
         if err != nil {
-                return nil, err
+                return nil, fmt.Errorf("can't get service %q: %v", name, err)
         }
         key, err := topology.ServiceKey(name)
         if err != nil {
                 return nil, err

[niemeyer, 2012/06/13 12:32:38]

This error may come out as "can't find service key service-00000042".
errorContext should be used here too.

[TheMue, 2012/06/13 12:54:34]

Done.

         }
         return &Service{s, key, name}, nil
 }
 
 // AllServices returns all deployed services in the environment.
-func (s *State) AllServices() ([]*Service, error) {
+func (s *State) AllServices() (services []*Service, err error) {
+        defer errorContext(&err, "can't get all services: %v")
         topology, err := readTopology(s.zk)
         if err != nil {
-                return nil, err
+                return
         }
-        services := []*Service{}
+        services = []*Service{}
         for _, key := range topology.ServiceKeys() {
-                name, err := topology.ServiceName(key)
+                var name string

[niemeyer, 2012/06/13 12:32:38]

Please revert and have "return nil, err" below.

[TheMue, 2012/06/13 12:54:34]

Done.

+                name, err = topology.ServiceName(key)
                 if err != nil {
-                        return nil, err
+                        return
                 }
                 services = append(services, &Service{s, key, name})
         }
         return services, nil
 }
 
 // Unit returns a unit by name.
-func (s *State) Unit(name string) (*Unit, error) {
+func (s *State) Unit(name string) (unit *Unit, err error) {
+        defer errorContext(&err, "can't get unit %q: %v", name)
         serviceName, _, err := parseUnitName(name)
         if err != nil {
-                return nil, err
+                return
         }
         service, err := s.Service(serviceName)
         if err != nil {
-                return nil, err
+                return
         }
         return service.Unit(name)
 }
 
 // addRelationNode creates the relation node.
 func (s *State) addRelationNode(scope RelationScope) (string, error) {
         path, err := s.zk.Create("/relations/relation-", "", zookeeper.SEQUENCE, zkPermAll)
         if err != nil {
                 return "", err
         }
         relationKey := strings.Split(path, "/")[2]
         // Create the settings node only if the scope is global.
         // In case of container scoped relations the creation per
         // container occurs in ServiceRelation.AddUnit.
         if scope == ScopeGlobal {
                 _, err = s.zk.Create(path+"/settings", "", 0, zkPermAll)
                 if err != nil {
                         return "", err
                 }
         }
         return relationKey, nil
 }
 
 // addRelationEndpointNode creates the endpoint role node below its relation node·
 // for the given relation endpoint.
 func (s *State) addRelationEndpointNode(relationKey string, endpoint RelationEndpoint) error {
         path := fmt.Sprintf("/relations/%s/%s", relationKey, string(endpoint.RelationRole))
-        _, err := s.zk.Create(path, "", 0, zkPermAll)
-        return err
+        if _, err := s.zk.Create(path, "", 0, zkPermAll); err != nil {

[niemeyer, 2012/06/13 12:32:38]

Please revert.

[TheMue, 2012/06/13 12:54:34]

Done.

+                return err
+        }
+        return nil
 }
 
 // AddRelation creates a new relation with the given endpoints.··
-func (s *State) AddRelation(endpoints ...RelationEndpoint) (*Relation, []*ServiceRelation, error) {
+func (s *State) AddRelation(endpoints ...RelationEndpoint) (rel *Relation, svcrels []*ServiceRelation, err error) {
+        defer errorContext(&err, "can't add relation: %v")
         switch len(endpoints) {
         case 1:
                 if endpoints[0].RelationRole != RolePeer {
-                        return nil, nil, fmt.Errorf("can't add non-peer relation with a single service")
+                        return nil, nil, fmt.Errorf("non-peer relation with a single service")
                 }
         case 2:
                 if !endpoints[0].CanRelateTo(&endpoints[1]) {
-                        return nil, nil, fmt.Errorf("can't add relation between %s and %s", endpoints[0], endpoints[1])
+                        return nil, nil, fmt.Errorf("relation between %s and %s is invalid", endpoints[0], endpoints[1])
                 }
         default:
-                return nil, nil, fmt.Errorf("can't add relations between %d services", len(endpoints))
+                return nil, nil, fmt.Errorf("relations between %d services are invalid", len(endpoints))
         }
         t, err := readTopology(s.zk)
         if err != nil {
-                return nil, nil, err
+                return
         }
         // Check if the relation already exists.
         relationKey, err := t.RelationKey(endpoints...)
         if err != nil {
                 if _, ok := err.(*NoRelationError); !ok {
-                        return nil, nil, err
+                        return
                 }
         }
         if relationKey != "" {
                 return nil, nil, fmt.Errorf("relation already exists")
         }
         scope := ScopeGlobal
         for _, endpoint := range endpoints {
                 if endpoint.RelationScope == ScopeContainer {
                         scope = ScopeContainer
                         break
@@ skipping 42 matching lines @@
                         service := &topoRelationService{
                                 RelationRole: serviceRelation.RelationRole(),
                                 RelationName: serviceRelation.RelationName(),
                         }
                         relation.Services[serviceRelation.serviceKey] = service
                 }
                 return t.AddRelation(relationKey, relation)
         }
         err = retryTopologyChange(s.zk, addRelation)
         if err != nil {
-                return nil, nil, err
+                return
         }
         return &Relation{s, relationKey}, serviceRelations, nil
 }
 
 // RemoveRelation removes the relation.
 func (s *State) RemoveRelation(relation *Relation) error {
         removeRelation := func(t *topology) error {
-                _, err := t.Relation(relation.key)
-                if err != nil {
-                        return fmt.Errorf("can't remove relation: %v", err)
+                if _, err := t.Relation(relation.key); err != nil {
+                        return err
                 }
                 return t.RemoveRelation(relation.key)
         }
-        // TODO: Improve high-level errors, no passing of low-level·
-        // errors directly to the caller.
-        return retryTopologyChange(s.zk, removeRelation)
+        if err := retryTopologyChange(s.zk, removeRelation); err != nil {
+                return fmt.Errorf("can't remove relation: %v", err)
+        }
+        return nil
 }