Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
remote relations worker polls offer model #7522
Conversation
babbageclunk
approved these changes
Jun 21, 2017
A few minor questions and quibbles about naming, but looks good to me.
| + return errors.Trace(err) | ||
| + } | ||
| + // See if we need to remove the remote application proxy - we do this | ||
| + // on the offering side as there is 1:1 between proxy and consuming app. |
babbageclunk
Jun 21, 2017
Member
So in the offering side there's only one remote application that acts as the proxy for all of the consumers? I don't think I'd fully realised that before.
Does that mean we need to do refcounting on the offering side and remove that remote application when the refcount goes to 0 then? Or is that handled elsewhere?
wallyworld
Jun 21, 2017
Owner
There's one remote app proxy per consumer. These are given names of the form remote-. So when the relation goes away, the app proxy can too as each app proxy is tied to that one relation.
| + return nil, errors.NotFoundf("local application for %s", names.ReadableString(tag)) | ||
| +} | ||
| + | ||
| +// RelationUnitSettings returns the unt settings for the specified relation unit. |
| + } | ||
| + paramsSettings := make(params.Settings) | ||
| + for k, v := range settings { | ||
| + vString, ok := v.(string) |
babbageclunk
Jun 21, 2017
Member
Seems like we should tie down the type for unit.Settings() better. Or allow anything that is a Stringer?
wallyworld
Jun 21, 2017
Owner
Perhaps, but params.Settings has always been defined to be map[string]interface{}
I'm always wary of implicit typing via String() or whatever, especially for on the wire params. I think best to be explicit. We can revisit later, but it's a question for more than just cmr.
| @@ -490,7 +491,16 @@ func (conn *Conn) bindRequest(hdr *Header) (boundRequest, error) { | ||
| // runRequest runs the given request and sends the reply. | ||
| func (conn *Conn) runRequest(req boundRequest, arg reflect.Value, version int, observer Observer) { | ||
| + // If the request causes a panic, ensure we log that before closing the connection. | ||
| + defer func() { | ||
| + if panicResult := recover(); panicResult != nil { |
wallyworld
Jun 21, 2017
Owner
Yeah - the agent just restarted "for no reason" and debug-log exited. Really hard to track down the issue. When I added this code, it was oh so easy to see the bug.
| // RegisterRemoteRelation sets up the local model to participate | ||
| // in the specified relations. | ||
| RegisterRemoteRelations(relations ...params.RegisterRemoteRelation) ([]params.RemoteEntityIdResult, error) | ||
| // PublishLocalRelationChange publishes local relation changes to the | ||
| // model hosting the remote application involved in the relation. | ||
| PublishLocalRelationChange(params.RemoteRelationChangeEvent) error | ||
| + | ||
| + // WatchRemoteRelationUnits returns a watcher that notifies of localChanges to the |
babbageclunk
Jun 21, 2017
Member
I'm having trouble keeping the remote and local things straight here. Why is this WatchRemoteRelationUnits if it's watching local changes in local units? Just a comment error?
(Reminds me of The League of Gentlemen - "this is a local shop, for local people".)
wallyworld
Jun 21, 2017
Owner
I hear you. It is watching for changes in the local model for relations which happen to be remote because they span models.
| + } | ||
| + | ||
| + // Start a watcher to track changes to the units in the relation in the remote model. | ||
| + remoteRelationUnitsWatcher, err := w.remoteModelFacade.WatchRemoteRelationUnits(relationId) |
babbageclunk
Jun 21, 2017
Member
Ah, ok - so WatchRemoteRelationUnits is called on the remote facade, but it returns information about changes to local-for-it units. Maybe it should be called WatchRelationUnits instead?
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wallyworld commentedJun 20, 2017
Description of change
The remote relations worker now polls the offering model - communications is one way from consuming model -> offering model, addressing a routeability issue.
The line count is large, but there's a lot of paper shuffling done to factor out common code. Because of the change in semantics of the worker, both the crossmodelrelations and remoterelations facade now share functionality, eg receiving changes from the other side, watching for unit settings changes. This is now all contained in a apiserver/common/crossmodel package.
The remote relations worker gains a new sub worker to act on listener events for unit settings changes from the offering model.
Still todo - when a relation is removed from the offering side, this is not yet picked up in the listener on the consuming side. Also, the relation units watcher needs a variant that can take macaroons for auth because the consuming controller needs anonymous access to this watcher.
QA steps
run up a mediawiki->mysql cross model relations scenario