cmr: Add remote spaces and bindings to remote applications #7319

Merged
merged 5 commits into from May 9, 2017

Conversation

Projects
None yet
3 participants
Member

babbageclunk commented May 9, 2017

Description of change

We need to capture provider-specific information about the spaces that remote application endpoints are bound to, so that we can use that information to decide whether we can use cloud-local addresses for units in cross-model relations.

Add state.RemoteApplication.Spaces and .Bindings - these will be stored when a remote application is created. This information can then be used to decide what address should be used.

Since these details won't change and don't need to be queried separately from the remote application they're stored in the remote application document rather than in a separate collection.

QA Steps

None at the moment - the space information won't be populated until a follow-up PR.

babbageclunk added some commits May 4, 2017

Add spaces and bindings to state.RemoteApplication
These are stored as attributes on the remote application document,
rather than in their own collection - they won't change and don't need
to be queried independently.
Update juju/description dependency
This adds remote spaces and bindings to remote applications.
Include remote application status in export
Allows the remote application test to be unskipped - it was failing due
to the unexported data sanity check.

Some initial concerns about layering to be fixed.

state/migration_export_test.go
@@ -1146,9 +1147,7 @@ func (s *MigrationExportSuite) newResource(c *gc.C, appName, name string, revisi
func (s *MigrationExportSuite) TestRemoteApplications(c *gc.C) {
// NOTE: the key 'c#<name>' isn't overly useful for someone looking
@wallyworld

wallyworld May 9, 2017

Owner

Is this comment still relevant? Seems like a cut and paste typo?

@babbageclunk

babbageclunk May 9, 2017

Member

It was a comment that Tim added about the global key for remote applications - I guess he found leftover rows in the status collection and was trying to work out what collection they belonged to. Should I change the global key prefix for remote applications to something a bit easier to search for?

@wallyworld

wallyworld May 9, 2017

Owner

Ah sorry, my mistake. We use c# since metawatcher requires a single char global key and 'a' and 'r' are taken. We should look into that.

state/migration_export_test.go
- c.Skip("the remote application needs to export the assocated status " +
- "document for the remote application 'c#grave-rainbow'.")
- _, err := s.State.AddRemoteApplication(state.AddRemoteApplicationParams{
+ sApp, err := s.State.AddRemoteApplication(state.AddRemoteApplicationParams{
@wallyworld

wallyworld May 9, 2017

Owner

Convention is typically to call this dbApp if we need to disambiguate

state/remoteapplication.go
+
+// RemoteSpace represents a space in another model that endpoints are
+// bound to.
+type RemoteSpace struct {
@wallyworld

wallyworld May 9, 2017

Owner

Is there a reason why this isn't remoteSpaceDoc?
And remoteSubnetDoc below?
To match what's done for remoteEndpointDoc.
And there'd be a separate struct used for public use outside of state package. We want to layer mongo/state data structs separately from structs used outside of state. That's either done via embedding the fooDoc in a Foo struct, or using a help func to map between the unexported state struct doc and the exported public struct.

@babbageclunk

babbageclunk May 9, 2017

Member

Ok - so we'd keep RemoteSpace and RemoteSubnet, but they'd just be what gets returned from RemoteApplication.Spaces(), and remoteSpaceDoc and remoteSubnetDoc (with the same attributes, respectively) would be what's stored in the remoteApplicationDoc?

state/remoteapplication.go
+}
+
+// RemoteSubnet represents a subnet in another model. Unfortunately we
+// can't reuse state.Subnet - it's tied to the subnets collection.
@wallyworld

wallyworld May 9, 2017

Owner

We don't need this last sentence. We would not want to re-use state.Subnet regardless as that would be a layer violation.

Member

babbageclunk commented May 9, 2017

$$merge$$

Contributor

jujubot commented May 9, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented May 9, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10840

Member

babbageclunk commented May 9, 2017

Looks like a spurious failure in APIContextSuite.TestDomainCookie on Windows.

$$merge$$

Contributor

jujubot commented May 9, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented May 9, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10843

Member

babbageclunk commented May 9, 2017

Another one: DebugHooksServerSuite.TestRunHook :(

$$merge$$

Contributor

jujubot commented May 9, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 7c18315 into juju:develop May 9, 2017

1 check failed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

@babbageclunk babbageclunk deleted the babbageclunk:remote-spaces branch May 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment