Merge pull request juju#10694 from howbazaar/worker-cleanup

juju#10694 Pass a logger into the apicaller manifold config, and use that logger internally.
hopkings2008 · Oct 8, 2019 · af0c715 · af0c715
2 parents 9cd2aed + f9b1e61
commit af0c715
Show file tree

Hide file tree

Showing 11 changed files with 41 additions and 20 deletions.
diff --git a/cmd/jujud/agent/caasoperator/manifolds.go b/cmd/jujud/agent/caasoperator/manifolds.go
@@ -130,6 +130,7 @@ func Manifolds(config ManifoldsConfig) dependency.Manifolds {
 			APIOpen:              api.Open,
 			APIConfigWatcherName: apiConfigWatcherName,
 			NewConnection:        apicaller.OnlyConnect,
+			Logger:               loggo.GetLogger("juju.worker.apicaller"),
 		}),
 
 		clockName: clockManifold(config.Clock),

diff --git a/cmd/jujud/agent/checkconnection.go b/cmd/jujud/agent/checkconnection.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/juju/cmd"
 	"github.com/juju/errors"
+	"github.com/juju/loggo"
 	"gopkg.in/juju/names.v3"
 
 	"github.com/juju/juju/agent"
@@ -23,7 +24,7 @@ type ConnectFunc func(agent.Agent) (io.Closer, error)
 // ConnectAsAgent really connects to the API specified in the agent
 // config. It's extracted so tests can pass something else in.
 func ConnectAsAgent(a agent.Agent) (io.Closer, error) {
-	return apicaller.ScaryConnect(a, api.Open)
+	return apicaller.ScaryConnect(a, api.Open, loggo.GetLogger("juju.agent"))
 }
 
 type checkConnectionCommand struct {

diff --git a/cmd/jujud/agent/machine/manifolds.go b/cmd/jujud/agent/machine/manifolds.go
@@ -438,6 +438,7 @@ func commonManifolds(config ManifoldsConfig) dependency.Manifolds {
 			APIOpen:              api.Open,
 			NewConnection:        apicaller.ScaryConnect,
 			Filter:               connectFilter,
+			Logger:               loggo.GetLogger("juju.worker.apicaller"),
 		}),
 
 		// The upgrade steps gate is used to coordinate workers which

diff --git a/cmd/jujud/agent/model/manifolds.go b/cmd/jujud/agent/model/manifolds.go
@@ -145,6 +145,7 @@ func commonManifolds(config ManifoldsConfig) dependency.Manifolds {
 			APIOpen:       api.Open,
 			NewConnection: apicaller.OnlyConnect,
 			Filter:        apiConnectFilter,
+			Logger:        config.LoggingContext.GetLogger("juju.worker.apicaller"),
 		}),
 
 		// The logging config updater listens for logging config updates

diff --git a/cmd/jujud/agent/unit/manifolds.go b/cmd/jujud/agent/unit/manifolds.go
@@ -152,6 +152,7 @@ func Manifolds(config ManifoldsConfig) dependency.Manifolds {
 			APIOpen:              api.Open,
 			NewConnection:        apicaller.ScaryConnect,
 			Filter:               connectFilter,
+			Logger:               loggo.GetLogger("juju.worker.apicaller"),
 		}),
 
 		// The log sender is a leaf worker that sends log messages to some

diff --git a/worker/apicaller/connect.go b/worker/apicaller/connect.go
@@ -46,13 +46,13 @@ var (
 )
 
 // OnlyConnect logs into the API using the supplied agent's credentials.
-func OnlyConnect(a agent.Agent, apiOpen api.OpenFunc) (api.Connection, error) {
+func OnlyConnect(a agent.Agent, apiOpen api.OpenFunc, logger Logger) (api.Connection, error) {
 	agentConfig := a.CurrentConfig()
 	info, ok := agentConfig.APIInfo()
 	if !ok {
 		return nil, errors.New("API info not available")
 	}
-	conn, _, err := connectFallback(apiOpen, info, agentConfig.OldPassword())
+	conn, _, err := connectFallback(apiOpen, info, agentConfig.OldPassword(), logger)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
@@ -79,7 +79,7 @@ func OnlyConnect(a agent.Agent, apiOpen api.OpenFunc) (api.Connection, error) {
 // until it's managed to log in, and any suicide-cutoff point we pick here
 // will be objectively bad in some circumstances.)
 func connectFallback(
-	apiOpen api.OpenFunc, info *api.Info, fallbackPassword string,
+	apiOpen api.OpenFunc, info *api.Info, fallbackPassword string, logger Logger,
 ) (
 	conn api.Connection, didFallback bool, err error,
 ) {
@@ -183,7 +183,7 @@ func shortModelUUID(model names.ModelTag) string {
 // This is clearly a mess but at least now it's a documented and localized
 // mess; it should be used only when making the primary API connection for
 // a machine or unit agent running in its own process.
-func ScaryConnect(a agent.Agent, apiOpen api.OpenFunc) (_ api.Connection, err error) {
+func ScaryConnect(a agent.Agent, apiOpen api.OpenFunc, logger Logger) (_ api.Connection, err error) {
 	agentConfig := a.CurrentConfig()
 	info, ok := agentConfig.APIInfo()
 	if !ok {
@@ -206,7 +206,7 @@ func ScaryConnect(a agent.Agent, apiOpen api.OpenFunc) (_ api.Connection, err er
 	}()
 
 	// Start connection...
-	conn, usedOldPassword, err := connectFallback(apiOpen, info, oldPassword)
+	conn, usedOldPassword, err := connectFallback(apiOpen, info, oldPassword, logger)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}

diff --git a/worker/apicaller/connect_test.go b/worker/apicaller/connect_test.go
@@ -6,6 +6,7 @@ package apicaller_test
 import (
 	"errors"
 
+	"github.com/juju/loggo"
 	"github.com/juju/testing"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
@@ -55,7 +56,7 @@ func testEntityFine(c *gc.C, life apiagent.Life) {
 			stub:   stub,
 			model:  coretesting.ModelTag,
 			entity: entity,
-		}, apiOpen)
+		}, apiOpen, loggo.GetLogger("test"))
 	}
 
 	conn, err := lifeTest(c, stub, apiagent.Alive, connect)
@@ -84,7 +85,7 @@ func (*ScaryConnectSuite) TestEntityDead(c *gc.C) {
 			stub:   stub,
 			model:  coretesting.ModelTag,
 			entity: entity,
-		}, apiOpen)
+		}, apiOpen, loggo.GetLogger("test"))
 	}
 
 	conn, err := lifeTest(c, stub, apiagent.Dead, connect)
@@ -113,7 +114,7 @@ func (*ScaryConnectSuite) TestEntityDenied(c *gc.C) {
 			stub:   stub,
 			model:  coretesting.ModelTag,
 			entity: entity,
-		}, apiOpen)
+		}, apiOpen, loggo.GetLogger("test"))
 	}
 
 	conn, err := lifeTest(c, stub, apiagent.Dead, connect)
@@ -141,7 +142,7 @@ func (*ScaryConnectSuite) TestEntityUnknownLife(c *gc.C) {
 			stub:   stub,
 			model:  coretesting.ModelTag,
 			entity: entity,
-		}, apiOpen)
+		}, apiOpen, loggo.GetLogger("test"))
 	}
 
 	conn, err := lifeTest(c, stub, apiagent.Life("zombie"), connect)
@@ -255,7 +256,7 @@ func checkChangePassword(c *gc.C, stub *testing.Stub) error {
 			stub:   stub,
 			model:  coretesting.ModelTag,
 			entity: entity,
-		}, apiOpen)
+		}, apiOpen, loggo.GetLogger("test"))
 	}
 
 	conn, err := lifeTest(c, stub, apiagent.Alive, connect)

diff --git a/worker/apicaller/manifold.go b/worker/apicaller/manifold.go
@@ -13,9 +13,16 @@ import (
 	"github.com/juju/juju/api/base"
 )
 
+// Logger represents the methods used by the worker to log details.
+type Logger interface {
+	Debugf(string, ...interface{})
+	Infof(string, ...interface{})
+	Errorf(string, ...interface{})
+}
+
 // ConnectFunc is responsible for making and validating an API connection
 // on behalf of an agent.
-type ConnectFunc func(agent.Agent, api.OpenFunc) (api.Connection, error)
+type ConnectFunc func(agent.Agent, api.OpenFunc, Logger) (api.Connection, error)
 
 // ManifoldConfig defines a Manifold's dependencies.
 type ManifoldConfig struct {
@@ -50,6 +57,9 @@ type ManifoldConfig struct {
 	// Filter is used to specialize responses to connection errors
 	// made on behalf of different kinds of agent.
 	Filter dependency.FilterFunc
+
+	// Logger is used to write logging statements for the worker.
+	Logger Logger
 }
 
 // Manifold returns a manifold whose worker wraps an API connection
@@ -79,7 +89,7 @@ func (config ManifoldConfig) startFunc() dependency.StartFunc {
 			return nil, err
 		}
 
-		conn, err := config.NewConnection(agent, config.APIOpen)
+		conn, err := config.NewConnection(agent, config.APIOpen, config.Logger)
 		if errors.Cause(err) == ErrChangedPassword {
 			return nil, dependency.ErrBounce
 		} else if err != nil {

diff --git a/worker/apicaller/manifold_test.go b/worker/apicaller/manifold_test.go
@@ -5,6 +5,7 @@ package apicaller_test
 
 import (
 	"github.com/juju/errors"
+	"github.com/juju/loggo"
 	"github.com/juju/testing"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
@@ -42,8 +43,9 @@ func (s *ManifoldSuite) SetUpTest(c *gc.C) {
 		APIOpen: func(*api.Info, api.DialOpts) (api.Connection, error) {
 			panic("just a fake")
 		},
-		NewConnection: func(a agent.Agent, apiOpen api.OpenFunc) (api.Connection, error) {
+		NewConnection: func(a agent.Agent, apiOpen api.OpenFunc, logger apicaller.Logger) (api.Connection, error) {
 			c.Check(apiOpen, gc.NotNil) // uncomparable
+			c.Check(logger, gc.NotNil)  // uncomparable
 			s.AddCall("NewConnection", a)
 			if err := s.NextErr(); err != nil {
 				return nil, err
@@ -53,6 +55,7 @@ func (s *ManifoldSuite) SetUpTest(c *gc.C) {
 		Filter: func(err error) error {
 			panic(err)
 		},
+		Logger: loggo.GetLogger("test"),
 	}
 	s.manifold = apicaller.Manifold(s.manifoldConfig)
 	checkFilter := func() {

diff --git a/worker/apicaller/retry_test.go b/worker/apicaller/retry_test.go
@@ -5,6 +5,7 @@ package apicaller_test
 
 import (
 	"github.com/juju/errors"
+	"github.com/juju/loggo"
 	"github.com/juju/testing"
 	jc "github.com/juju/testing/checkers"
 	"github.com/juju/utils"
@@ -45,7 +46,7 @@ func (s *RetryStrategySuite) TestOnlyConnectSuccess(c *gc.C) {
 	// TODO(katco): 2016-08-09: lp:1611427
 	strategy := utils.AttemptStrategy{Min: 3}
 	conn, err := strategyTest(stub, strategy, func(apiOpen api.OpenFunc) (api.Connection, error) {
-		return apicaller.OnlyConnect(&mockAgent{stub: stub, entity: testEntity}, apiOpen)
+		return apicaller.OnlyConnect(&mockAgent{stub: stub, entity: testEntity}, apiOpen, loggo.GetLogger("test"))
 	})
 	checkOpenCalls(c, stub, "new", "new", "new")
 	c.Check(conn, gc.NotNil)
@@ -63,7 +64,7 @@ func (s *RetryStrategySuite) TestOnlyConnectOldPasswordSuccess(c *gc.C) {
 	// TODO(katco): 2016-08-09: lp:1611427
 	strategy := utils.AttemptStrategy{Min: 3}
 	conn, err := strategyTest(stub, strategy, func(apiOpen api.OpenFunc) (api.Connection, error) {
-		return apicaller.OnlyConnect(&mockAgent{stub: stub, entity: testEntity}, apiOpen)
+		return apicaller.OnlyConnect(&mockAgent{stub: stub, entity: testEntity}, apiOpen, loggo.GetLogger("test"))
 	})
 	checkOpenCalls(c, stub, "new", "old", "old", "old")
 	c.Check(err, jc.ErrorIsNil)
@@ -93,7 +94,7 @@ func checkWaitProvisionedError(c *gc.C, connect apicaller.ConnectFunc) (api.Conn
 	// TODO(katco): 2016-08-09: lp:1611427
 	strategy := utils.AttemptStrategy{Min: 3}
 	conn, err := strategyTest(stub, strategy, func(apiOpen api.OpenFunc) (api.Connection, error) {
-		return connect(&mockAgent{stub: stub, entity: testEntity}, apiOpen)
+		return connect(&mockAgent{stub: stub, entity: testEntity}, apiOpen, loggo.GetLogger("test"))
 	})
 	checkOpenCalls(c, stub, "new", "new", "new", "new")
 	return conn, err
@@ -122,7 +123,7 @@ func checkWaitNeverProvisioned(c *gc.C, connect apicaller.ConnectFunc) (api.Conn
 	// TODO(katco): 2016-08-09: lp:1611427
 	strategy := utils.AttemptStrategy{Min: 3}
 	conn, err := strategyTest(stub, strategy, func(apiOpen api.OpenFunc) (api.Connection, error) {
-		return connect(&mockAgent{stub: stub, entity: testEntity}, apiOpen)
+		return connect(&mockAgent{stub: stub, entity: testEntity}, apiOpen, loggo.GetLogger("test"))
 	})
 	checkOpenCalls(c, stub, "new", "new", "new", "new")
 	return conn, err

diff --git a/worker/apicaller/worker.go b/worker/apicaller/worker.go
@@ -5,14 +5,15 @@ package apicaller
 
 import (
 	"github.com/juju/errors"
-	"github.com/juju/loggo"
 	"gopkg.in/juju/worker.v1"
 	"gopkg.in/tomb.v2"
 
 	"github.com/juju/juju/api"
 )
 
-var logger = loggo.GetLogger("juju.worker.apicaller")
+// logger is here to stop the desire of creating a package level logger.
+// Don't do this, instead use the one passed as manifold config.
+var logger interface{}
 
 // newAPIConnWorker returns a worker that exists for as long as the associated
 // connection, and provides access to a base.APICaller via its manifold's Output