Merge pull request juju#11491 from howbazaar/unit-agent-worker-loggers

juju#11491 The purpose of this work is to pass loggers into the worker manifolds and remove the package level loggers. This work is particularly targeting the workers for the uniter. This work will mean when we merge the unit and machine agents the logs from the uniter workers will go to the right place. There is a drive-by fix for the intermittent test failure in the featuretests package for the metrics watcher. The old attempt at fixing the failure with a WaitForModelWatchersIdle was not correct. The new fix has been tested under stress and stress-race loops locally. Some worker tests were reworked a little so we didn't repeatedly have to add the loggers to many places, and instead makes use of suite methods. ## QA steps Bootstrap and deploy and agent. If the engine manifolds all start up properly, then everything is wired up. Need to confirm with k8s and lxd to ensure we are covering all aspects of the manifolds. ## Documentation changes Everything is internal, nothing to document.
orbital-transfer-survey · Apr 24, 2020 · 44b37b0 · 44b37b0
2 parents c135b2d + 40fb251
commit 44b37b0
Show file tree

Hide file tree

Showing 50 changed files with 528 additions and 319 deletions.
diff --git a/cmd/jujud/agent/caasoperator/manifolds.go b/cmd/jujud/agent/caasoperator/manifolds.go
@@ -209,6 +209,7 @@ func Manifolds(config ManifoldsConfig) dependency.Manifolds {
 			ValidateMigration: config.ValidateMigration,
 			NewFacade:         migrationminion.NewFacade,
 			NewWorker:         migrationminion.NewWorker,
+			Logger:            loggo.GetLogger("juju.worker.migrationminion"),
 		}),
 
 		// The proxy config updater is a leaf worker that sets http/https/apt/etc
@@ -240,6 +241,7 @@ func Manifolds(config ManifoldsConfig) dependency.Manifolds {
 		apiAddressUpdaterName: ifNotMigrating(apiaddressupdater.Manifold(apiaddressupdater.ManifoldConfig{
 			AgentName:     agentName,
 			APICallerName: apiCallerName,
+			Logger:        loggo.GetLogger("juju.worker.apiaddressupdater"),
 		})),
 
 		// The charmdir resource coordinates whether the charm directory is

diff --git a/cmd/jujud/agent/machine/manifolds.go b/cmd/jujud/agent/machine/manifolds.go
@@ -553,6 +553,7 @@ func commonManifolds(config ManifoldsConfig) dependency.Manifolds {
 			ValidateMigration: config.ValidateMigration,
 			NewFacade:         migrationminion.NewFacade,
 			NewWorker:         migrationminion.NewWorker,
+			Logger:            loggo.GetLogger("juju.worker.migrationminion"),
 		}),
 
 		// We also run another clock updater to feed time updates into
@@ -909,6 +910,7 @@ func IAASManifolds(config ManifoldsConfig) dependency.Manifolds {
 		apiAddressUpdaterName: ifNotMigrating(apiaddressupdater.Manifold(apiaddressupdater.ManifoldConfig{
 			AgentName:     agentName,
 			APICallerName: apiCallerName,
+			Logger:        loggo.GetLogger("juju.worker.apiaddressupdater"),
 		})),
 
 		machineActionName: ifNotMigrating(machineactions.Manifold(machineactions.ManifoldConfig{

diff --git a/cmd/jujud/agent/unit/manifolds.go b/cmd/jujud/agent/unit/manifolds.go
@@ -234,6 +234,7 @@ func Manifolds(config ManifoldsConfig) dependency.Manifolds {
 			Check:         migrationflag.IsTerminal,
 			NewFacade:     migrationflag.NewFacade,
 			NewWorker:     migrationflag.NewWorker,
+			// No Logger defined in migrationflag package.
 		}),
 		migrationMinionName: migrationminion.Manifold(migrationminion.ManifoldConfig{
 			AgentName:         agentName,
@@ -244,6 +245,7 @@ func Manifolds(config ManifoldsConfig) dependency.Manifolds {
 			ValidateMigration: config.ValidateMigration,
 			NewFacade:         migrationminion.NewFacade,
 			NewWorker:         migrationminion.NewWorker,
+			Logger:            loggo.GetLogger("juju.worker.migrationminion"),
 		}),
 
 		// The logging config updater is a leaf worker that indirectly
@@ -264,6 +266,7 @@ func Manifolds(config ManifoldsConfig) dependency.Manifolds {
 		apiAddressUpdaterName: ifNotMigrating(apiaddressupdater.Manifold(apiaddressupdater.ManifoldConfig{
 			AgentName:     agentName,
 			APICallerName: apiCallerName,
+			Logger:        loggo.GetLogger("juju.worker.apiaddressupdater"),
 		})),
 
 		// The proxy config updater is a leaf worker that sets http/https/apt/etc
@@ -320,6 +323,7 @@ func Manifolds(config ManifoldsConfig) dependency.Manifolds {
 			CharmDirName:          charmDirName,
 			HookRetryStrategyName: hookRetryStrategyName,
 			TranslateResolverErr:  uniter.TranslateFortressErrors,
+			Logger:                loggo.GetLogger("juju.worker.uniter"),
 		})),
 
 		// TODO (mattyw) should be added to machine agent.

diff --git a/cmd/jujud/run_test.go b/cmd/jujud/run_test.go
@@ -349,7 +349,7 @@ func (s *RunTestSuite) runListenerForAgent(c *gc.C, agent string) {
 		socket.Network = "unix"
 		socket.Address = fmt.Sprintf("%s/run.socket", agentDir)
 	}
-	listener, err := uniter.NewRunListener(socket)
+	listener, err := uniter.NewRunListener(socket, loggo.GetLogger("test"))
 	c.Assert(err, jc.ErrorIsNil)
 	listener.RegisterRunner("foo/1", &mockRunner{c})
 	s.AddCleanup(func(*gc.C) {

diff --git a/featuretests/api_meterstatus_test.go b/featuretests/api_meterstatus_test.go
@@ -34,6 +34,12 @@ func (s *meterStatusIntegrationSuite) SetUpTest(c *gc.C) {
 	state := s.OpenAPIAs(c, s.unit.UnitTag(), password)
 	s.status = meterstatus.NewClient(state, s.unit.UnitTag())
 	c.Assert(s.status, gc.NotNil)
+
+	// Ask for the MetricsManager as part of setup, so the metrics
+	// document is created before any of the tests care.
+	_, err = s.State.MetricsManager()
+	c.Assert(err, jc.ErrorIsNil)
+
 	// Ensure that all the creation events have flowed through the system.
 	s.WaitForModelWatchersIdle(c, s.Model.UUID())
 }
@@ -75,12 +81,6 @@ func (s *meterStatusIntegrationSuite) TestWatchMeterStatus(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 	err = mm.SetLastSuccessfulSend(time.Now())
 	c.Assert(err, jc.ErrorIsNil)
-	// While in theory it is only one event, when the metrics manager is first
-	// asked for, if it doesn't exist it adds a document. Then the set last
-	// successful send changes that document, so there are actually two changes
-	// from the database perspective. Here we wait for the model to be idle
-	// before checking for one change.
-	s.WaitForModelWatchersIdle(c, s.State.ModelUUID())
 	wc.AssertOneChange()
 
 	// meter status does not change on every failed

diff --git a/worker/apiaddressupdater/apiaddressupdater.go b/worker/apiaddressupdater/apiaddressupdater.go
@@ -8,28 +8,13 @@ import (
 	"sync"
 
 	"github.com/juju/errors"
-	"github.com/juju/loggo"
 	"gopkg.in/juju/worker.v1"
 
 	corenetwork "github.com/juju/juju/core/network"
 	"github.com/juju/juju/core/watcher"
 	"github.com/juju/juju/network"
 )
 
-var logger = loggo.GetLogger("juju.worker.apiaddressupdater")
-
-// APIAddressUpdater is responsible for propagating API addresses.
-//
-// In practice, APIAddressUpdater is used by a machine agent to watch
-// API addresses in state and write the changes to the agent's config file.
-type APIAddressUpdater struct {
-	addresser APIAddresser
-	setter    APIAddressSetter
-
-	mu      sync.Mutex
-	current []corenetwork.ProviderHostPorts
-}
-
 // APIAddresser is an interface that is provided to NewAPIAddressUpdater
 // which can be used to watch for API address changes.
 type APIAddresser interface {
@@ -43,13 +28,46 @@ type APIAddressSetter interface {
 	SetAPIHostPorts(servers []corenetwork.HostPorts) error
 }
 
+// Config defines the operation of a Worker.
+type Config struct {
+	Addresser APIAddresser
+	Setter    APIAddressSetter
+	Logger    Logger
+}
+
+// Validate returns an error if config cannot drive a Worker.
+func (config Config) Validate() error {
+	if config.Addresser == nil {
+		return errors.NotValidf("nil Addresser")
+	}
+	if config.Setter == nil {
+		return errors.NotValidf("nil Setter")
+	}
+	if config.Logger == nil {
+		return errors.NotValidf("nil Logger")
+	}
+	return nil
+}
+
+// APIAddressUpdater is responsible for propagating API addresses.
+//
+// In practice, APIAddressUpdater is used by a machine agent to watch
+// API addresses in state and write the changes to the agent's config file.
+type APIAddressUpdater struct {
+	config Config
+
+	mu      sync.Mutex
+	current []corenetwork.ProviderHostPorts
+}
+
 // NewAPIAddressUpdater returns a worker.Worker that watches for changes to
 // API addresses and then sets them on the APIAddressSetter.
-// TODO(fwereade): this should have a config struct, and some validation.
-func NewAPIAddressUpdater(addresser APIAddresser, setter APIAddressSetter) (worker.Worker, error) {
+func NewAPIAddressUpdater(config Config) (worker.Worker, error) {
+	if err := config.Validate(); err != nil {
+		return nil, err
+	}
 	handler := &APIAddressUpdater{
-		addresser: addresser,
-		setter:    setter,
+		config: config,
 	}
 	w, err := watcher.NewNotifyWorker(watcher.NotifyConfig{
 		Handler: handler,
@@ -62,7 +80,7 @@ func NewAPIAddressUpdater(addresser APIAddresser, setter APIAddressSetter) (work
 
 // SetUp is part of the watcher.NotifyHandler interface.
 func (c *APIAddressUpdater) SetUp() (watcher.NotifyWatcher, error) {
-	return c.addresser.WatchAPIHostPorts()
+	return c.config.Addresser.WatchAPIHostPorts()
 }
 
 // Handle is part of the watcher.NotifyHandler interface.
@@ -72,7 +90,7 @@ func (c *APIAddressUpdater) Handle(_ <-chan struct{}) error {
 		return err
 	}
 
-	logger.Debugf("updating API hostPorts to %+v", hps)
+	c.config.Logger.Debugf("updating API hostPorts to %+v", hps)
 	c.mu.Lock()
 	c.current = hps
 	c.mu.Unlock()
@@ -88,14 +106,14 @@ func (c *APIAddressUpdater) Handle(_ <-chan struct{}) error {
 		hpsToSet[i] = hps.HostPorts()
 	}
 
-	if err := c.setter.SetAPIHostPorts(hpsToSet); err != nil {
+	if err := c.config.Setter.SetAPIHostPorts(hpsToSet); err != nil {
 		return fmt.Errorf("error setting addresses: %v", err)
 	}
 	return nil
 }
 
 func (c *APIAddressUpdater) getAddresses() ([]corenetwork.ProviderHostPorts, error) {
-	addresses, err := c.addresser.APIHostPorts()
+	addresses, err := c.config.Addresser.APIHostPorts()
 	if err != nil {
 		return nil, fmt.Errorf("error getting addresses: %v", err)
 	}

diff --git a/worker/apiaddressupdater/apiaddressupdater_test.go b/worker/apiaddressupdater/apiaddressupdater_test.go
@@ -9,6 +9,9 @@ import (
 	"path/filepath"
 	"time"
 
+	"github.com/juju/errors"
+	"github.com/juju/loggo"
+	"github.com/juju/testing"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 	"gopkg.in/juju/worker.v1"
@@ -52,7 +55,12 @@ func (s *apiAddressSetter) SetAPIHostPorts(servers []corenetwork.HostPorts) erro
 
 func (s *APIAddressUpdaterSuite) TestStartStop(c *gc.C) {
 	st, _ := s.OpenAPIAsNewMachine(c, state.JobHostUnits)
-	worker, err := apiaddressupdater.NewAPIAddressUpdater(apimachiner.NewState(st), &apiAddressSetter{})
+	worker, err := apiaddressupdater.NewAPIAddressUpdater(
+		apiaddressupdater.Config{
+			Addresser: apimachiner.NewState(st),
+			Setter:    &apiAddressSetter{},
+			Logger:    loggo.GetLogger("test"),
+		})
 	c.Assert(err, jc.ErrorIsNil)
 	worker.Kill()
 	c.Assert(worker.Wait(), gc.IsNil)
@@ -65,7 +73,12 @@ func (s *APIAddressUpdaterSuite) TestAddressInitialUpdate(c *gc.C) {
 
 	setter := &apiAddressSetter{servers: make(chan []corenetwork.HostPorts, 1)}
 	st, _ := s.OpenAPIAsNewMachine(c, state.JobHostUnits)
-	updater, err := apiaddressupdater.NewAPIAddressUpdater(apimachiner.NewState(st), setter)
+	updater, err := apiaddressupdater.NewAPIAddressUpdater(
+		apiaddressupdater.Config{
+			Addresser: apimachiner.NewState(st),
+			Setter:    setter,
+			Logger:    loggo.GetLogger("test"),
+		})
 	c.Assert(err, jc.ErrorIsNil)
 	defer workertest.CleanKill(c, updater)
 
@@ -94,7 +107,12 @@ func (s *APIAddressUpdaterSuite) TestAddressInitialUpdate(c *gc.C) {
 func (s *APIAddressUpdaterSuite) TestAddressChange(c *gc.C) {
 	setter := &apiAddressSetter{servers: make(chan []corenetwork.HostPorts, 1)}
 	st, _ := s.OpenAPIAsNewMachine(c, state.JobHostUnits)
-	worker, err := apiaddressupdater.NewAPIAddressUpdater(apimachiner.NewState(st), setter)
+	worker, err := apiaddressupdater.NewAPIAddressUpdater(
+		apiaddressupdater.Config{
+			Addresser: apimachiner.NewState(st),
+			Setter:    setter,
+			Logger:    loggo.GetLogger("test"),
+		})
 	c.Assert(err, jc.ErrorIsNil)
 	defer func() { c.Assert(worker.Wait(), gc.IsNil) }()
 	defer worker.Kill()
@@ -176,7 +194,12 @@ LXC_BRIDGE="ignored"`[1:])
 
 	setter := &apiAddressSetter{servers: make(chan []corenetwork.HostPorts, 1)}
 	st, _ := s.OpenAPIAsNewMachine(c, state.JobHostUnits)
-	w, err := apiaddressupdater.NewAPIAddressUpdater(apimachiner.NewState(st), setter)
+	w, err := apiaddressupdater.NewAPIAddressUpdater(
+		apiaddressupdater.Config{
+			Addresser: apimachiner.NewState(st),
+			Setter:    setter,
+			Logger:    loggo.GetLogger("test"),
+		})
 	c.Assert(err, jc.ErrorIsNil)
 	defer func() { c.Assert(w.Wait(), gc.IsNil) }()
 	defer w.Kill()
@@ -228,3 +251,56 @@ LXC_BRIDGE="ignored"`[1:])
 		c.Assert(servers, jc.DeepEquals, []corenetwork.HostPorts{expServer1, expServerUpd})
 	}
 }
+
+type ValidateSuite struct {
+	testing.IsolationSuite
+}
+
+var _ = gc.Suite(&ValidateSuite{})
+
+func (*ValidateSuite) TestValid(c *gc.C) {
+	err := validConfig().Validate()
+	c.Check(err, jc.ErrorIsNil)
+}
+
+func (*ValidateSuite) TestMissingAddresser(c *gc.C) {
+	config := validConfig()
+	config.Addresser = nil
+	checkNotValid(c, config, "nil Addresser not valid")
+}
+
+func (*ValidateSuite) TestMissingSetter(c *gc.C) {
+	config := validConfig()
+	config.Setter = nil
+	checkNotValid(c, config, "nil Setter not valid")
+}
+
+func (*ValidateSuite) TestMissingLogger(c *gc.C) {
+	config := validConfig()
+	config.Logger = nil
+	checkNotValid(c, config, "nil Logger not valid")
+}
+
+func validConfig() apiaddressupdater.Config {
+	return apiaddressupdater.Config{
+		Addresser: struct{ apiaddressupdater.APIAddresser }{},
+		Setter: struct {
+			apiaddressupdater.APIAddressSetter
+		}{},
+		Logger: loggo.GetLogger("test"),
+	}
+}
+
+func checkNotValid(c *gc.C, config apiaddressupdater.Config, expect string) {
+	check := func(err error) {
+		c.Check(err, gc.ErrorMatches, expect)
+		c.Check(err, jc.Satisfies, errors.IsNotValid)
+	}
+
+	err := config.Validate()
+	check(err)
+
+	worker, err := apiaddressupdater.NewAPIAddressUpdater(config)
+	c.Check(worker, gc.IsNil)
+	check(err)
+}
diff --git a/worker/apiaddressupdater/manifold.go b/worker/apiaddressupdater/manifold.go
@@ -17,20 +17,34 @@ import (
 	"github.com/juju/juju/cmd/jujud/agent/engine"
 )
 
+// Logger represents the methods used for logging messages.
+type Logger interface {
+	Errorf(string, ...interface{})
+	Infof(string, ...interface{})
+	Debugf(string, ...interface{})
+}
+
 // ManifoldConfig defines the names of the manifolds on which a Manifold will depend.
-type ManifoldConfig engine.AgentAPIManifoldConfig
+type ManifoldConfig struct {
+	AgentName     string
+	APICallerName string
+	Logger        Logger
+}
 
 // Manifold returns a dependency manifold that runs an API address updater worker,
 // using the resource names defined in the supplied config.
 func Manifold(config ManifoldConfig) dependency.Manifold {
-	typedConfig := engine.AgentAPIManifoldConfig(config)
-	return engine.AgentAPIManifold(typedConfig, newWorker)
+	typedConfig := engine.AgentAPIManifoldConfig{
+		AgentName:     config.AgentName,
+		APICallerName: config.APICallerName,
+	}
+	return engine.AgentAPIManifold(typedConfig, config.newWorker)
 }
 
 // newWorker trivially wraps NewAPIAddressUpdater for use in a engine.AgentAPIManifold.
 // It's not tested at the moment, because the scaffolding necessary is too
 // unwieldy/distracting to introduce at this point.
-var newWorker = func(a agent.Agent, apiCaller base.APICaller) (worker.Worker, error) {
+func (config ManifoldConfig) newWorker(a agent.Agent, apiCaller base.APICaller) (worker.Worker, error) {
 	tag := a.CurrentConfig().Tag()
 
 	// TODO(fwereade): use appropriate facade!
@@ -47,7 +61,11 @@ var newWorker = func(a agent.Agent, apiCaller base.APICaller) (worker.Worker, er
 	}
 
 	setter := agent.APIHostPortsSetter{Agent: a}
-	w, err := NewAPIAddressUpdater(facade, setter)
+	w, err := NewAPIAddressUpdater(Config{
+		Addresser: facade,
+		Setter:    setter,
+		Logger:    config.Logger,
+	})
 	if err != nil {
 		return nil, errors.Trace(err)
 	}