subnets: store ProviderNetworkId in state and display it #7152

Merged
merged 8 commits into from Mar 26, 2017

Conversation

Projects
None yet
3 participants
Member

babbageclunk commented Mar 24, 2017

Description of change

The GCE provider reports the id of the network for subnets that it finds. This information is potentially useful (for example to distinguish between subnets with overlapping ranges that are actually on different networks), so we should keep track of it in the database and display it.

Includes a driveby fix to remove the embedded interfaces from the shims in apiserver/networkingcommon - they just masked possible errors with unimplemented interface methods.

QA steps

Bootstrap a controller on GCE against a project with subnets (it needs to have more than just a legacy network). Run juju subnets - the name of the network should be reported (as provider-network-id) against each subnet.

babbageclunk added some commits Mar 23, 2017

Driveby fix for shims
Stop the space- and subnetShims from embedding the interface they were
supposed to be implementing. They were never constructed with an
instance of the interface, so the effect of the embedding was to
suppress compile errors that the interface wasn't fully implemented. The
code using them happens to work because it never calls those methods,
but if it did it would compile fine and then panic at runtime. Instead
remove the unused methods (and the embedding).
Member

babbageclunk commented Mar 24, 2017

!!build!!

A quibble about the naming consistency

- VLANTag int `json:"vlan-tag,omitempty"`
- Zones []string `json:"zones,omitempty"`
+ SubnetTag string `json:"subnet-tag,omitempty"`
+ SubnetProviderId string `json:"subnet-provider-id,omitempty"`
@wallyworld

wallyworld Mar 24, 2017

Owner

SubnetProviderId vs ProviderNetworkId

Ideally these would be consistent.
ie
SubnetProviderId
NetworkProviderId

I prefer the other way around but what's done is done

@babbageclunk

babbageclunk Mar 24, 2017

Member

(as discussed on IRC)
Unfortunately these names are already inconsistent in the codebase. I picked this version based on the existing ones in github.com/juju/juju/network/network.go:
On InterfaceInfo

  • ProviderSubnetId
  • ProviderSpaceId
  • ProviderVLANId
  • ProviderAddressId

On SubnetInfo:

  • SpaceProviderId

I could name it NetworkProviderId on network.SubnetInfo and params.AddSubnetArgs (and ProviderNetworkId elsewhere) so it's consistent within a type, but that seems more confusing (and means more tech debt work if/when we decide to make them all consistent).

Member

babbageclunk commented Mar 24, 2017

$$merge$$

Contributor

jujubot commented Mar 24, 2017

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

Member

babbageclunk commented Mar 24, 2017

!!chittychitty!!

Contributor

jujubot commented Mar 24, 2017

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

Member

babbageclunk commented Mar 24, 2017

Need to update the dependency on description once juju/description#7 is merged.

Update description dependency
Adds Subnet.ProviderNetworkId in the model.
Member

babbageclunk commented Mar 26, 2017

Added the needed migration bits.

$$merge$$

Contributor

jujubot commented Mar 26, 2017

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

@jujubot jujubot merged commit 2af3dd2 into juju:develop Mar 26, 2017

@babbageclunk babbageclunk deleted the babbageclunk:store-network branch Mar 26, 2017

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