all: don't log errors at warning level

- Don't log errors at warning level - Don't log ignored errors at warning level, make them debug as there is nothing the user is required to do. - Don't log errors and return them, just return the error - Don't log informational errors at warning level. - Don't log multiline messages.
bnhassin · May 16, 2016 · 02ba4af · 02ba4af
1 parent 68c9239
commit 02ba4af
Show file tree

Hide file tree

Showing 19 changed files with 41 additions and 52 deletions.
diff --git a/apiserver/addresser/addresser.go b/apiserver/addresser/addresser.go
@@ -109,7 +109,7 @@ func (api *AddresserAPI) CleanupIPAddresses() params.ErrorResult {
 		logger.Debugf("releasing dead IP address %q", ipAddressValue)
 		err := api.releaseIPAddress(netEnv, ipAddress)
 		if err != nil {
-			logger.Warningf("cannot release IP address %q: %v (will retry)", ipAddressValue, err)
+			logger.Debugf("cannot release IP address %q: %v (will retry)", ipAddressValue, err)
 			canRetry = true
 			continue
 		}
@@ -119,7 +119,7 @@ func (api *AddresserAPI) CleanupIPAddresses() params.ErrorResult {
 			continue
 		}
 		if err != nil {
-			logger.Warningf("failed to remove released IP address %q: %v (will retry)", ipAddressValue, err)
+			logger.Debugf("failed to remove released IP address %q: %v (will retry)", ipAddressValue, err)
 			canRetry = true
 			continue
 		}

diff --git a/container/lxd/initialisation.go b/container/lxd/initialisation.go
@@ -90,7 +90,7 @@ var configureZFS = func() {
 	).CombinedOutput()
 
 	if err != nil {
-		logger.Warningf("configuring zfs failed with %s: %s", err, string(output))
+		logger.Errorf("configuring zfs failed with %s: %s", err, string(output))
 	}
 }
 
@@ -101,7 +101,7 @@ var configureLXDBridge = func() error {
 		 * lxd-bridge; let's not fail here.
 		 */
 		if os.IsNotExist(err) {
-			logger.Warningf("couldn't find %s, not configuring it", lxdBridgeFile)
+			logger.Debugf("couldn't find %s, not configuring it", lxdBridgeFile)
 			return nil
 		}
 		return errors.Trace(err)
@@ -254,7 +254,7 @@ func findNextAvailableIPv4Subnet() (string, error) {
 	for _, address := range addrs {
 		addr, network, err := net.ParseCIDR(address.String())
 		if err != nil {
-			logger.Warningf("cannot parse address %q: %v (ignoring)", address.String(), err)
+			logger.Debugf("cannot parse address %q: %v (ignoring)", address.String(), err)
 			continue
 		}
 		if !ip10network.Contains(addr) {

diff --git a/downloader/download.go b/downloader/download.go
@@ -185,6 +185,6 @@ func cleanTempFile(f *os.File) {
 
 	f.Close()
 	if err := os.Remove(f.Name()); err != nil {
-		logger.Warningf("cannot remove temp file %q: %v", f.Name(), err)
+		logger.Errorf("cannot remove temp file %q: %v", f.Name(), err)
 	}
 }
diff --git a/environs/bootstrap/bootstrap.go b/environs/bootstrap/bootstrap.go
@@ -247,7 +247,7 @@ func Bootstrap(ctx environs.BootstrapContext, environ environs.Environ, args Boo
 		// even though the user didn't ask for it. We only do
 		// this when the image-stream is not "released" and
 		// the agent version hasn't been specified.
-		logger.Warningf("no prepackaged tools available")
+		logger.Infof("no prepackaged tools available")
 	}
 
 	ctx.Infof("Installing Juju agent on bootstrap instance")
@@ -420,7 +420,7 @@ func setBootstrapTools(environ environs.Environ, possibleTools coretools.List) (
 	if !isCompatibleVersion(newVersion, jujuversion.Current) {
 		compatibleVersion, compatibleTools := findCompatibleTools(possibleTools, jujuversion.Current)
 		if len(compatibleTools) == 0 {
-			logger.Warningf(
+			logger.Debugf(
 				"failed to find %s tools, will attempt to use %s",
 				jujuversion.Current, newVersion,
 			)
@@ -490,11 +490,11 @@ func setPrivateMetadataSources(env environs.Environ, metadataDir string) ([]*ima
 func validateConstraints(env environs.Environ, cons constraints.Value) error {
 	validator, err := env.ConstraintsValidator()
 	if err != nil {
-		return err
+		return errors.Trace(err)
 	}
 	unsupported, err := validator.Validate(cons)
 	if len(unsupported) > 0 {
-		logger.Warningf("unsupported constraints: %v", unsupported)
+		err = errors.Annotatef(err, "unsupported constraints: %v", unsupported)
 	}
 	return err
 }

diff --git a/juju/api.go b/juju/api.go
@@ -107,7 +107,7 @@ func newAPIFromStore(args NewAPIConnectionParams, apiOpen api.OpenFunc) (api.Con
 		if errorImportance(err0) < errorImportance(err1) {
 			err0, err1 = err1, err0
 		}
-		logger.Warningf("discarding API open error: %v", err1)
+		logger.Infof("discarding API open error: %v", err1)
 		return err0
 	}
 	try := parallel.NewTry(0, chooseError)
@@ -172,7 +172,7 @@ func newAPIFromStore(args NewAPIConnectionParams, apiOpen api.OpenFunc) (api.Con
 	if localerr := updateControllerAddresses(
 		args.Store, args.ControllerName, controllerDetails, hostPorts, addrConnectedTo,
 	); localerr != nil {
-		logger.Warningf("cannot cache API addresses: %v", localerr)
+		logger.Debugf("cannot cache API addresses: %v", localerr)
 	}
 	return st, nil
 }

diff --git a/jujuclient/file.go b/jujuclient/file.go
@@ -58,8 +58,7 @@ func (s *store) lock(operation string) (*fslock.Lock, error) {
 		return nil, errors.Trace(err)
 	}
 
-	logger.Warningf("breaking jujuclient lock : %s", lockName)
-	logger.Warningf("  lock holder message: %s", lock.Message())
+	logger.Infof("breaking jujuclient lock: %s, lock holder message: %s", lockName, lock.Message())
 
 	// If we are unable to acquire the lock within the lockTimeout,
 	// consider it broken for some reason, and break it.

diff --git a/network/address.go b/network/address.go
@@ -276,9 +276,9 @@ func SelectAddressBySpaces(addresses []Address, spaceNames ...SpaceName) (Addres
 	}
 
 	if len(spaceNames) == 0 {
-		logger.Warningf("no spaces to select addresses from")
+		logger.Errorf("no spaces to select addresses from")
 	} else {
-		logger.Warningf("no addresses found in spaces %s", spaceNames)
+		logger.Errorf("no addresses found in spaces %s", spaceNames)
 	}
 	return Address{}, false
 }
@@ -287,7 +287,7 @@ func SelectAddressBySpaces(addresses []Address, spaceNames ...SpaceName) (Addres
 // the given space name associated.
 func SelectHostsPortBySpaces(hps []HostPort, spaceNames ...SpaceName) ([]HostPort, bool) {
 	if len(spaceNames) == 0 {
-		logger.Warningf("host ports not filtered - no spaces given.")
+		logger.Errorf("host ports not filtered - no spaces given.")
 		return hps, false
 	}
 
@@ -303,7 +303,7 @@ func SelectHostsPortBySpaces(hps []HostPort, spaceNames ...SpaceName) ([]HostPor
 		return selectedHostPorts, true
 	}
 
-	logger.Warningf("no hostPorts found in spaces %s", spaceNames)
+	logger.Errorf("no hostPorts found in spaces %s", spaceNames)
 	return hps, false
 }
 

diff --git a/network/network.go b/network/network.go
@@ -354,7 +354,7 @@ func FilterLXCAddresses(addresses []Address) []Address {
 		return addresses
 	} else if err != nil {
 		// Just log it, as it's not fatal.
-		logger.Warningf("cannot open %q: %v", LXCNetDefaultConfig, err)
+		logger.Errorf("cannot open %q: %v", LXCNetDefaultConfig, err)
 		return addresses
 	}
 	defer file.Close()
@@ -410,15 +410,15 @@ func FilterLXCAddresses(addresses []Address) []Address {
 			// Discover all addresses of bridgeName interface.
 			addrs, err := InterfaceByNameAddrs(bridgeName)
 			if err != nil {
-				logger.Warningf("cannot get %q addresses: %v (ignoring)", bridgeName, err)
+				logger.Debugf("cannot get %q addresses: %v (ignoring)", bridgeName, err)
 				continue
 			}
 			logger.Debugf("%q has addresses %v", bridgeName, addrs)
 			return filterAddrs(bridgeName, addrs)
 		}
 	}
 	if err := scanner.Err(); err != nil {
-		logger.Warningf("failed to read %q: %v (ignoring)", LXCNetDefaultConfig, err)
+		logger.Debugf("failed to read %q: %v (ignoring)", LXCNetDefaultConfig, err)
 	}
 	return addresses
 }
diff --git a/provider/cloudsigma/environfirewall.go b/provider/cloudsigma/environfirewall.go
@@ -12,14 +12,14 @@ import "github.com/juju/juju/network"
 // OpenPorts opens the given ports for the whole environment.
 // Must only be used if the environment was setup with the FwGlobal firewall mode.
 func (env *environ) OpenPorts(ports []network.PortRange) error {
-	logger.Warningf("pretending to open ports %v for all instances", ports)
+	logger.Debugf("pretending to open ports %v for all instances", ports)
 	return nil
 }
 
 // ClosePorts closes the given ports for the whole environment.
 // Must only be used if the environment was setup with the FwGlobal firewall mode.
 func (env *environ) ClosePorts(ports []network.PortRange) error {
-	logger.Warningf("pretending to close ports %v for all instances", ports)
+	logger.Debugf("pretending to close ports %v for all instances", ports)
 	return nil
 }
 

diff --git a/provider/cloudsigma/environinstance.go b/provider/cloudsigma/environinstance.go
@@ -140,8 +140,7 @@ func (env *environ) Instances(ids []instance.Id) ([]instance.Instance, error) {
 
 	m, err := env.client.instanceMap()
 	if err != nil {
-		logger.Warningf("environ.Instances failed: %v", err)
-		return nil, err
+		return nil, errors.Annotate(err, "environ.Instances failed")
 	}
 
 	var found int
@@ -159,7 +158,7 @@ func (env *environ) Instances(ids []instance.Id) ([]instance.Instance, error) {
 		err = environs.ErrPartialInstances
 	}
 
-	return r, err
+	return r, errors.Trace(err)
 }
 
 // StopInstances shuts down the given instances.

diff --git a/provider/cloudsigma/environinstance_test.go b/provider/cloudsigma/environinstance_test.go
@@ -7,6 +7,7 @@ import (
 	"time"
 
 	"github.com/altoros/gosigma/mock"
+	"github.com/juju/errors"
 	"github.com/juju/loggo"
 	jc "github.com/juju/testing/checkers"
 	"github.com/juju/version"
@@ -108,7 +109,7 @@ func (s *environInstanceSuite) TestInstances(c *gc.C) {
 	ids = append(ids, instance.Id("fake-instance"))
 	instances, err = env.Instances(ids)
 	c.Assert(instances, gc.NotNil)
-	c.Assert(err, gc.Equals, environs.ErrPartialInstances)
+	c.Assert(errors.Cause(err), gc.Equals, environs.ErrPartialInstances)
 	c.Check(instances, gc.HasLen, 3)
 	c.Check(instances[0], gc.NotNil)
 	c.Check(instances[1], gc.NotNil)
@@ -119,7 +120,7 @@ func (s *environInstanceSuite) TestInstances(c *gc.C) {
 
 	instances, err = env.Instances(ids)
 	c.Assert(instances, gc.NotNil)
-	c.Assert(err, gc.Equals, environs.ErrNoInstances)
+	c.Assert(errors.Cause(err), gc.Equals, environs.ErrNoInstances)
 	c.Check(instances, gc.HasLen, 3)
 	c.Check(instances[0], gc.IsNil)
 	c.Check(instances[1], gc.IsNil)

diff --git a/state/allwatcher.go b/state/allwatcher.go
@@ -347,11 +347,11 @@ func getUnitAddresses(st *State, unitName string) (string, string, error) {
 	}
 	publicAddress, err := u.PublicAddress()
 	if err != nil {
-		logger.Warningf("getting a public address for unit %q failed: %q", u.Name(), err)
+		logger.Errorf("getting a public address for unit %q failed: %q", u.Name(), err)
 	}
 	privateAddress, err := u.PrivateAddress()
 	if err != nil {
-		logger.Warningf("getting a private address for unit %q failed: %q", u.Name(), err)
+		logger.Errorf("getting a private address for unit %q failed: %q", u.Name(), err)
 	}
 	return publicAddress.Value, privateAddress.Value, nil
 }
@@ -409,9 +409,6 @@ func (svc *backingService) updated(st *State, store *multiwatcherStore, id strin
 		}
 		serviceStatus, err := service.Status()
 		if err != nil {
-			logger.Warningf("reading service status for key %s: %v", key, err)
-		}
-		if err != nil && !errors.IsNotFound(err) {
 			return errors.Annotatef(err, "reading service status for key %s", key)
 		}
 		if err == nil {
@@ -875,13 +872,13 @@ func (p *backingOpenedPorts) removed(store *multiwatcherStore, modelUUID, id str
 			// always acting a little behind reality. It is reasonable
 			// that entities have been deleted from State but we're
 			// still seeing events related to them from the watcher.
-			logger.Warningf("cannot retrieve units for %q: %v", info.Id, err)
+			logger.Errorf("cannot retrieve units for %q: %v", info.Id, err)
 			return nil
 		}
 		// Update the ports on all units assigned to the machine.
 		for _, u := range units {
 			if err := updateUnitPorts(st, store, u); err != nil {
-				logger.Warningf("cannot update unit ports for %q: %v", u.Name(), err)
+				logger.Errorf("cannot update unit ports for %q: %v", u.Name(), err)
 			}
 		}
 	}

diff --git a/storage/constraints.go b/storage/constraints.go
@@ -62,7 +62,7 @@ func ParseConstraints(s string) (Constraints, error) {
 		}
 		if IsValidPoolName(field) {
 			if cons.Pool != "" {
-				logger.Warningf("pool name is already set to %q, ignoring %q", cons.Pool, field)
+				logger.Debugf("pool name is already set to %q, ignoring %q", cons.Pool, field)
 			} else {
 				cons.Pool = field
 			}
@@ -82,7 +82,7 @@ func ParseConstraints(s string) (Constraints, error) {
 			cons.Size = size
 			continue
 		}
-		logger.Warningf("ignoring unknown storage constraint %q", field)
+		logger.Debugf("ignoring unknown storage constraint %q", field)
 	}
 	if cons.Count == 0 && cons.Size == 0 && cons.Pool == "" {
 		return Constraints{}, errors.New("storage constraints require at least one field to be specified")

diff --git a/tools/lxdclient/client.go b/tools/lxdclient/client.go
@@ -279,7 +279,7 @@ func checkLXDBridgeConfiguration(conf string) error {
 		if strings.HasPrefix(line, "USE_LXD_BRIDGE=") {
 			b, err := strconv.ParseBool(strings.Trim(line[len("USE_LXD_BRIDGE="):], " \""))
 			if err != nil {
-				logger.Warningf("couldn't parse bool, skipping USE_LXD_BRIDGE check: %s", err)
+				logger.Debugf("couldn't parse bool, skipping USE_LXD_BRIDGE check: %s", err)
 				continue
 			}
 

diff --git a/tools/lxdclient/client_image.go b/tools/lxdclient/client_image.go
@@ -145,13 +145,7 @@ func (i *imageClient) EnsureImageExists(series string, sources []Remote, copyPro
 			forward: forwarder.Forward,
 		}
 		err = source.CopyImage(target, i.raw, []string{name}, adapter.copyProgress)
-		if err != nil {
-			// TODO(jam) Should this be fatal? Or just set lastErr
-			// and then continue on?
-			logger.Warningf("error copying image: %s", err)
-			return errors.Annotatef(err, "unable to get LXD image for %s", name)
-		}
-		return nil
+		return errors.Annotatef(err, "unable to get LXD image for %s", name)
 	}
 	return lastErr
 }

diff --git a/upgrades/preupgradesteps.go b/upgrades/preupgradesteps.go
@@ -18,16 +18,15 @@ import (
 // If any check fails, an error is returned which aborts the upgrade.
 func PreUpgradeSteps(st *state.State, agentConf agent.Config, isController, isMaster bool) error {
 	if err := checkDiskSpace(agentConf.DataDir()); err != nil {
-		return err
+		return errors.Trace(err)
 	}
 	if isController {
 		// Update distro info in case the new Juju controller version
 		// is aware of new supported series. We'll keep going if this
 		// fails, and the user can manually update it if they need to.
 		logger.Infof("updating distro-info")
-		if err := updateDistroInfo(); err != nil {
-			logger.Warningf("failed to update distro-info: %v", err)
-		}
+		err := updateDistroInfo()
+		return errors.Annotate(err, "failed to update distro-info")
 	}
 	return nil
 }

diff --git a/worker/deployer/simple.go b/worker/deployer/simple.go
@@ -286,7 +286,7 @@ func (ctx *SimpleContext) service(unitName string, renderer shell.Renderer) (dep
 func removeOnErr(err *error, path string) {
 	if *err != nil {
 		if err := os.RemoveAll(path); err != nil {
-			logger.Warningf("installer: cannot remove %q: %v", path, err)
+			logger.Errorf("installer: cannot remove %q: %v", path, err)
 		}
 	}
 }
diff --git a/wrench/wrench.go b/wrench/wrench.go
@@ -71,7 +71,7 @@ func IsActive(category, feature string) bool {
 	for lines.Scan() {
 		line := strings.TrimSpace(lines.Text())
 		if line == feature {
-			logger.Warningf("wrench for %s/%s is active", category, feature)
+			logger.Debugf("wrench for %s/%s is active", category, feature)
 			return true
 		}
 	}

diff --git a/wrench/wrench_test.go b/wrench/wrench_test.go
@@ -174,7 +174,7 @@ var notJujuUid = uint32(os.Getuid() + 1)
 
 func (s *wrenchSuite) AssertActivationLogged(c *gc.C) {
 	c.Assert(s.logWriter.Log(), jc.LogMatches, []jc.SimpleMessage{
-		{loggo.WARNING, `wrench for foo/bar is active`}})
+		{loggo.DEBUG, `wrench for foo/bar is active`}})
 }
 
 func (s *wrenchSuite) AssertNothingLogged(c *gc.C) {