Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add code to remoterelations worker to publish local changes to remoe model #6637
Conversation
|
!!build!! |
| + Name string `json:"name"` | ||
| + Life Life `json:"life"` | ||
| + Status string `json:"status"` | ||
| + ModelUUID string `json:"model-uuid"` |
reedobrien
Dec 1, 2016
Contributor
Other structs here have comments on the members. These seem self-evident (well they all do really), so maybe add for consistency. Or is there a rule of thumb I am missing.
wallyworld
Dec 1, 2016
Owner
Yeah, in this file the structs tended to have attribute comments, which makes it a PITA to add new structs. Elsewhere we don't bother. I'll add comments for consistency.
| @@ -340,7 +341,17 @@ type RemoteRelationsChange struct { | ||
| // RemoteRelationsChanges holds a set of RemoteRelationsChange structures. | ||
| type RemoteRelationsChanges struct { | ||
| - Changes []RemoteRelationsChange `json:"changes,omitempty"` | ||
| + Changes []RemoteModelRelationsChange `json:"changes,omitempty"` |
| + Changes []RemoteModelRelationsChange `json:"changes,omitempty"` | ||
| +} | ||
| + | ||
| +// RemoteModelRelationsChange holds the model uuid and changes to |
reedobrien
Dec 1, 2016
Contributor
holds the remote model UUID? Or maybe more generic since the member variable is described.
| @@ -44,7 +46,12 @@ func (st *mockState) ExportLocalEntity(entity names.Tag) (string, error) { | ||
| if err := st.NextErr(); err != nil { | ||
| return "", err | ||
| } | ||
| - return "token-" + entity.Id(), nil | ||
| + if token, ok := st.exportedEntities[entity.Id()]; ok { | ||
| + return token, errors.AlreadyExistsf(entity.Id()) |
reedobrien
Dec 1, 2016
Contributor
format string? Or NewAlreadyExists? Mock code so doesn't matter. Curious about choice here though.
wallyworld
Dec 1, 2016
Owner
NewAlreadyExists wraps an underlying error. We don't need that here. We just want an error satisfying IsAlreadyExists
| @@ -24,7 +26,7 @@ type RemoteRelationsFacade interface { | ||
| @@ -170,7 +177,9 @@ func (w *Worker) handleApplicationChanges(applicationIds []string) error { | ||
| logger.Debugf("started watcher for remote application %q", name) | ||
| appWorker, err := newRemoteApplicationWorker( | ||
| relationsWatcher, | ||
| + result.Result.ModelUUID, |
wallyworld
Dec 1, 2016
Owner
Nothing explicit. I find keyed fields useful mainly for when there are nil defaults. Otherwise it's just unnecessary boilerplate.
| + | ||
| + remoteRelationId params.RemoteEntityId | ||
| + remoteUnitIds map[string]params.RemoteEntityId | ||
| + |
| + relationTag: relationTag, | ||
| + ruw: ruw, | ||
| + changes: changes, | ||
| + remoteUnitIds: make(map[string]params.RemoteEntityId), |
reedobrien
Dec 1, 2016
Contributor
s/Id/ID ? There's a bunch of these, so maybe it's just inertia/legacy.
|
QA checks out. |
| Status string `json:"status"` | ||
| + | ||
| + // ModelUUID is the UUId of the model hosting the application. |
axw
Dec 1, 2016
Member
UUID. This shouldn't be needed though? We spoke about having the worker always talk to the controller, and the controller does the cross-model comms. The controller knows which model the remote application belongs to already.
wallyworld
Dec 1, 2016
Owner
I looked at how the JAAS code works and you make an API connection to the model facade on which you want to operate - the api code redirects as needed to the right controller. You still need the model uuid to do that. You get an the api connection info, set the model uuid, and open the api connection - the login process will redirect if the target controller is not the one hosting the model. From there, the worker just makes an api call to the facade which under the covers is connected to the correct target model.
| @@ -93,7 +94,10 @@ func (r *RemoteEntities) ExportLocalEntity(entity names.Tag) (string, error) { | ||
| return aa, nil | ||
| } | ||
| if err := r.st.run(ops); err != nil { | ||
| - return "", errors.Trace(err) | ||
| + // Where error is AlreadyExists, we still return the |
| + // and relation unit watcher can occur at any time. | ||
| + // This prevents the possibility of the same unit being exported | ||
| + // simultaneously. | ||
| + exportMutex sync.Mutex |
| + if err != nil { | ||
| + return errors.Annotatef(err, "exporting relation %v", w.relationTag) | ||
| + } | ||
| + if results[0].Error != nil && results[0].Error.Code != params.CodeAlreadyExists { |
| + changedUnitNames = append(changedUnitNames, name) | ||
| + } | ||
| + unitNamesToExport := append(changedUnitNames, change.Departed...) | ||
| + logger.Debugf("exporting units: %v", unitNamesToExport) |
axw
Dec 2, 2016
Member
I suggest moving this to where we actually call the export API, otherwise this could get quite noisy
| + event := ¶ms.RemoteRelationChangeEvent{ | ||
| + RelationId: w.remoteRelationId, | ||
| + } | ||
| + for i := range change.Departed { |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wallyworld commentedNov 30, 2016
The remote relations worker gather the changes in response to a relation units watcher event, and calls the publish api on the remote relations facade to get said changes sent to the remote model.
To make things efficient, the ExportLocalEntity behaviour has been tweaked so that if an entity is exported twice, the existing token is returned with an AlreadyExists error to save a second api call.
QA: bootstrap lxd to ensure no breakages