environs: Update the cross-model methods in NetworkingEnviron #7335

Merged
merged 1 commit into from May 12, 2017

Conversation

Projects
None yet
4 participants
Member

babbageclunk commented May 12, 2017

Description of change

After discussing the interface with jam it became clear that the
original definitions of the methods weren't right.

With the exception of MAAS which fully supports spaces, providers don't
have any way to determine which subnets should be in a space. State is
the authority in that case, so just passing ProviderId into
ProviderSpaceInfo doesn't work. (We could change the providers to tag
subnets with space information, but that's not straightforward - at the
provider level the subnets are shared between controllers, so multiple
controllers trying to manage the tags on one subnet could clobber each
other's changes unless care was taken.)

Change Networking.ProviderSpaceInfo to take a network.SpaceInfo
with state's view of the subnets instead - if the provider knows better it
can discard that, but otherwise the space info can be included in the result.

Also change IsSpaceRoutable to AreSpacesRoutable, which takes two
spaces. The arguments to this will be the ProviderSpaceInfos for the two
endpoints of the cross-model relation.

QA steps

No behaviour changes in this PR - it just tweaks interface methods with stub
implementations in the providers that support networking.

environs/networking.go
+ // space can be retrieved by passing DefaultSpace (which is nil).
+ //
+ // This method accepts a SpaceInfo with details of the space that
+ // we need provider details for - this is the Juju database's view
@wallyworld

wallyworld May 12, 2017

Owner

let's word it as Juju model

@babbageclunk

babbageclunk May 12, 2017

Member

Yup, good call.

environs/networking.go
+ // date).
+ //
+ // If the provider doesn't support space discovery then the Juju
+ // database's opinion of what subnets are in the space is
@wallyworld

wallyworld May 12, 2017

Owner

s/database/model

Update the cross-model methods in NetworkingEnviron
After discussing the interface with jam it became clear that the
original definitions of the methods weren't quite right.

With the exception of MAAS which fully supports spaces, providers don't
have any way to determine which subnets should be in a space. State is
the authority in that case, so just passing ProviderId into
ProviderSpaceInfo can't work. (We could change the providers to tag
subnets with space information, but that's not straightforward - at the
provider level the subnets are shared between controllers, so multiple
controllers trying to manage the tags on one subnet could clobber each
other's changes unless care was taken.)

Change ProviderSpaceInfo to take a network.SpaceInfo with state's view
of the subnets instead - if the provider knows better it can discard
that, but otherwise the space info can be included in the result.

Also rename IsSpaceRoutable to AreSpacesRoutable, which takes two
spaces. The arguments to this will be the ProviderSpaceInfos for the two
endpoints of the cross-model relation.
Member

babbageclunk commented May 12, 2017

$$merge$$

Contributor

jujubot commented May 12, 2017

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

Contributor

jujubot commented May 12, 2017

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

Owner

nskaggs commented May 12, 2017

$$merge$$

Contributor

jujubot commented May 12, 2017

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

Owner

nskaggs commented May 12, 2017

$$merge$$

@jujubot jujubot merged commit 3f25389 into juju:develop May 12, 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
Member

babbageclunk commented May 14, 2017

Hmm, grant check failed in different ways for the check and merge.

$$merge$$

Member

babbageclunk commented May 14, 2017

Gah, didn't refresh my page. Thanks @nskaggs!

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