Initial implementation of Environs.Network Interface for OpenStack Pr… #7140
Conversation
|
!! testme !! |
provider/openstack/provider.go
Outdated
| @@ -648,6 +649,11 @@ func (e *Environ) Bootstrap(ctx environs.BootstrapContext, args environs.Bootstr | |||
| func (e *Environ) supportsNeutron() bool { | |||
| client := e.client() | |||
| endpointMap := client.EndpointsForRegion(e.cloud.Region) | |||
| if len(endpointMap) == 0 { | |||
| // In some corner cases the endpoingMap is empty. Default to | |||
There was a problem hiding this comment.
I think you'll only get an empty map back if the client has not authenticated? Probably need to change the order of some code, so that auth happens first.
There was a problem hiding this comment.
the 3 places calling supportsNeutron() try to get an authenticated client if the client isn't already.... looks like something else is going on. will investigate. all the goose output of endpoint data i saw did include "network".
There was a problem hiding this comment.
It turns out that even though the callers to supportsNeutron() were authenticating the client before calling. The client used by suportsNeutron() was not alway authenticated. There is a bigger question of why that should be looked at? I moved code around so that supportsNeutron() was checking for client authentication for now.
provider/openstack/provider.go
Outdated
| @@ -1003,6 +1009,7 @@ func (e *Environ) StartInstance(args environs.StartInstanceParams) (*environs.St | |||
| return nil, errors.Annotate(err, "getting initial networks") | |||
| } | |||
| usingNetwork := e.ecfg().network() | |||
| logger.Debugf("ecfg().network() returned (%s)", usingNetwork) | |||
There was a problem hiding this comment.
probably don't need this to be committed
|
!! testmeagain !! |
|
|
||
| // Subnets is part of the Networking interface. | ||
| func (n *LegacyNovaNetworking) Subnets(instId instance.Id, subnetIds []network.Id) ([]network.SubnetInfo, error) { | ||
| return nil, errors.NotSupportedf("nova subnet") |
There was a problem hiding this comment.
If we are going to do this here, then the discover spaces worker will need to be tweaked to handle this error and ignore it, since the worker thinks these methods should work. Add a todo also to the worker to revert the change when nova subnet support is added.
Should also add a test for this case.
provider/openstack/networking.go
Outdated
| // empty, in which case all known are returned. | ||
| func (n *NeutronNetworking) Subnets(instId instance.Id, subnetIds []network.Id) ([]network.SubnetInfo, error) { | ||
| var results []network.SubnetInfo | ||
| subIdSet := make(map[string]bool) |
There was a problem hiding this comment.
we have a juju/utils/set package which has a strings set implementation which will simplify the code below
There was a problem hiding this comment.
thank you - updated the function.
hmlanigan
force-pushed
the
networks-openstack
branch
from
02c497c
to
28b8a10
Compare
| if err != nil { | ||
| return nil, errors.Annotatef(err, "failed to retrieve subnets") | ||
| } | ||
| if len(subnetIds) == 0 { |
There was a problem hiding this comment.
This logic can be simplified using the functionality provided by Set. You just need to do a set difference between the set of passed in ids and those obtained by the list subnets call.
hmlanigan
force-pushed
the
networks-openstack
branch
from
28b8a10
to
ad713b8
Compare
|
!! runtest !! |
| modelSubnetIds, err := dw.getModelSubnets() | ||
| if err != nil { | ||
| subnets, err := environ.Subnets(instance.UnknownId, nil) | ||
| if err != nil && errors.IsNotSupported(err) { |
There was a problem hiding this comment.
you don't need the err != nil
…ovider. Updates to goose required to complete implementation. Update Discover Spaces worker for better messaging where Subnets not implemented for Nova. In the OpenStack Provider, modify Open()/SetConfig() so environ.clientUnlocked not overwritten by subsequent SetConfig() calls.
hmlanigan
force-pushed
the
networks-openstack
branch
from
ad713b8
to
964f0bc
Compare
|
|
|
I think you may need to have |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
…ovider.
Updates to goose required to complete.
Please provide the following details to expedite Pull Request review:
Description of change
This is part of the Cross Model Relations work and needed for juju Networking.
environs.Networking is implemented for the OpenStack Provider. Neutron networking will be
supported here, though not Nova networking. Updates will be made once more data is available from goose.
A small bug was fixed in supportsNeutron() as well to make Neutron networking the default if
not OpenStack Endpoints are found.
QA steps
juju deploy mysql
juju offer mysql:db hosted-db <-- copy url from results
juju show-endpoints
juju deploy ghost
juju consume
juju add-relation ghost:database hosted-db:db
Documentation changes
N/A
Bug reference
N/A