External controller updater worker #7924

axw · 2017-10-11T04:14:05Z

Description of change

We introduce a new worker that watches for
addition and removal of external controller
entries, and then watches for changes to
each one's API addresses. When the addresses
change, we update the external controller
details in the local controller's database.

To support this worker, two new facades are
introduced: ExternalControllerUpdater, and
CrossController. The former is an internal
controller facade, used by the worker to
watch, fetch and store the local knowledge
of external controllers. The latter is an
anonymous controller facade that is used by
other controllers to watch and fetch the
controller's API connection info.

There's a drive-by fix to stop importing
apiserver/authentication in workers. Workers
should never be using backend code directly,
so we move the AnonymousUsername constant
to the api package.

QA steps

juju bootstrap localhost l1 && juju deploy wordpress
juju bootstrap localhost l2 && juju deploy mysql && juju offer mysql:db
juju switch l1
juju consume l2:admin/default.mysql
juju relate wordpress mysql
juju enable-ha -c l2
watch l1's debug log for changes to external controller info

Documentation changes

None.

Bug reference

None.

wallyworld · 2017-10-11T17:42:36Z

api/crosscontroller/crosscontroller.go

+		return nil, errors.Trace(err)
+	}
+	info := results.Results[0].Info
+	return &crossmodel.ControllerInfo{


we now have a controller alias - let's populate that so we can use it when logging errors etc

wallyworld · 2017-10-11T19:58:28Z

state/watcher.go

+	w.watcher.WatchCollection(externalControllersC, in)
+	defer w.watcher.UnwatchCollection(externalControllersC, in)
+
+	// Get the initial documents in the collection.


this chuck to get the initial docs should probably be in an initial() func like is done for other watchers (for consistency)

wallyworld · 2017-10-11T20:05:53Z

api/crosscontroller/crosscontroller.go

+	}
+}
+
+func (c *Client) ControllerInfo(controllerUUID string) (*crossmodel.ControllerInfo, error) {


Given there's several of these now duplicated, is it time to introduce an api/common struct and embed in several api clients?

No, because

the one I added to apiserver/crossmodelrelations shouldn't be there, I added it and forgot to drop it
this one and the one in crosscontroller aren't duplicates, they have different signatures

wallyworld · 2017-10-12T15:10:14Z

cmd/jujud/agent/machine/manifolds.go

@@ -186,6 +188,16 @@ func Manifolds(config ManifoldsConfig) dependency.Manifolds {
 		externalUpdateProxyFunc = lxd.ConfigureLXDProxies
 	}

+	newExternalControllerWatcherClient := func(apiInfo *api.Info) (


It would be nice to be consistent with how the remoterelations worker is set up. That worker's manifold has a func to get the controller connection and then there's a shim.go in the worker package that constructs the client - so we pass in the connection rather than the facade client. I guess the use cases might be subtly different enough that maybe it doesn't make sense to consolidate on a single approach.

The idea was to avoid a dependency on the specific API facade, and have that passed in by the code configuring/constructing the manifold. My approach means that this package can just require an interface, and not be concerned at all with how it's fulfilled.

wallyworld · 2017-10-12T15:25:48Z

worker/externalcontrollerupdater/externalcontrollerupdater.go

+
+var logger = loggo.GetLogger("juju.worker.externalcontrollerupdater")
+
+type ExternalControllerUpdaterClient interface {


Would it be better not to group these 3 methods on a single interface? For a given external controller, we need to watch for changes and then ask for changes when the watcher fires. And then we need to take those changes and update the local controller. So Watch/GetInfo belong on one interface, and SetInfo() on another interface.

What does that buy? In practice they're all implemented by the same API, and conceptually they belong together as they're all dealing with the same source of information. i.e. we're watching, fetching and updating the local controller's knowledge of external controllers.

The same API but different targets. Just as Go uses small interfaces specific to a particular purpose, and then composes as needed into broader interfaces, I think we should do that here too (expect there's no need for the broader interface, just the individual ones). The facade connected to the local controller will just implement SetExternalControllerInfo(), and the one connected to the remote controller will implement the other two.

No, that's not how it works.

All of the methods in this interface are dealing with the local controller.

WatchExternalControllers watches for addition and removal of external controller records in the local controller

ExternalControllerInfo asks the local controller for its current information about an external controller (so we can make an API connection)

ExternalControllerInfo sets new external controller information in the local controller

The ExternalControllerWatcherClient interface a couple of types below is the one that communicates with the external controller. It has two methods:

WatchControllerInfo watches the external controller's API info for changes

ControllerInfo gets the external controller's current API info

wallyworld · 2017-10-12T22:06:50Z

worker/externalcontrollerupdater/externalcontrollerupdater.go

+		externalControllerInfo:             externalControllers.ExternalControllerInfo,
+		setExternalControllerInfo:          externalControllers.SetExternalControllerInfo,
+		newExternalControllerWatcherClient: newExternalControllerWatcherClient,
+		runner: worker.NewRunner(worker.RunnerParams{


I think we also need to add the runner to the catacomb of the main worker so it and all the subworkers are stopped when the main worker dies?

It is, see the Init in catacomb.Plan below.

Ah yes, so it is.

wallyworld

Thank you

wallyworld · 2017-10-18T00:23:13Z

worker/externalcontrollerupdater/externalcontrollerupdater.go

+				}
+				logger.Infof("starting watcher for external controller %q", tag.Id())
+				watchers.Add(tag)
+				w.runner.StartWorker(tag.Id(), func() (worker.Worker, error) {


The error from StartWorker should not be ignored.

wallyworld · 2017-10-18T00:24:08Z

worker/externalcontrollerupdater/externalcontrollerupdater_test.go

@@ -0,0 +1,209 @@
+// Copyright 2016 Canonical Ltd.


wallyworld · 2017-10-18T00:29:19Z

worker/externalcontrollerupdater/externalcontrollerupdater.go

+				return errors.Annotate(err, "caching external controller info")
+			}
+			logger.Infof("new controller info for controller %q: %v", w.tag.Id(), info)
+			if err := worker.Stop(nw); err != nil {


Should we also remove nw from the catacomb? Otherwise we are just adding reference to the catacomb each time we create the client again and get a new watcher?

the catacomb will remove the worker from its management when the worker stops

We introduce a new worker that watches for addition and removal of external controller entries, and then watches for changes to each one's API addresses. When the addresses change, we update the external controller details in the local controller's database. To support this worker, two new facades are introduced: ExternalControllerUpdater, and CrossController. The former is an internal controller facade, used by the worker to watch, fetch and store the local knowledge of external controllers. The latter is an anonymous controller facade that is used by other controllers to watch and fetch the controller's API connection info. There's a drive-by fix to stop importing apiserver/authentication in workers. Workers should never be using backend code directly, so we move the AnonymousUsername constant to the api package.

axw · 2017-10-18T02:02:06Z

$$merge$$

jujubot · 2017-10-18T02:04:06Z

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju

wallyworld reviewed Oct 11, 2017

View reviewed changes

axw force-pushed the cross-model-externalcontrollerupdater branch 2 times, most recently from 1b8678d to 4709bf1 Compare October 12, 2017 09:47

wallyworld reviewed Oct 12, 2017

View reviewed changes

axw force-pushed the cross-model-externalcontrollerupdater branch 7 times, most recently from 6ee9a6a to 25d3be8 Compare October 16, 2017 04:28

axw changed the title ~~WIP: external controller updater worker~~ External controller updater worker Oct 16, 2017

axw force-pushed the cross-model-externalcontrollerupdater branch from 25d3be8 to f98cb6b Compare October 16, 2017 06:28

wallyworld approved these changes Oct 18, 2017

View reviewed changes

axw force-pushed the cross-model-externalcontrollerupdater branch from f98cb6b to 5a26cd3 Compare October 18, 2017 02:01

jujubot merged commit bad2bef into juju:develop Oct 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External controller updater worker #7924

External controller updater worker #7924

axw commented Oct 11, 2017

wallyworld Oct 11, 2017

axw Oct 12, 2017

wallyworld Oct 11, 2017

axw Oct 12, 2017

wallyworld Oct 11, 2017

axw Oct 12, 2017

wallyworld Oct 12, 2017

axw Oct 13, 2017

wallyworld Oct 12, 2017

axw Oct 13, 2017

wallyworld Oct 13, 2017

axw Oct 13, 2017

wallyworld Oct 12, 2017

axw Oct 13, 2017

wallyworld Oct 13, 2017

wallyworld left a comment

wallyworld Oct 18, 2017

axw Oct 18, 2017

wallyworld Oct 18, 2017

axw Oct 18, 2017

wallyworld Oct 18, 2017

axw Oct 18, 2017

axw commented Oct 18, 2017

jujubot commented Oct 18, 2017


		var logger = loggo.GetLogger("juju.worker.externalcontrollerupdater")

		type ExternalControllerUpdaterClient interface {

External controller updater worker #7924

External controller updater worker #7924

Conversation

axw commented Oct 11, 2017

Description of change

QA steps

Documentation changes

Bug reference

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallyworld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axw commented Oct 18, 2017

jujubot commented Oct 18, 2017