Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmr: Capture remote space info when creating a remote application #7340
Conversation
|
!!build!! |
| result.Endpoints = append(result.Endpoints, params.RemoteEndpoint{ | ||
| Name: ep.Name, | ||
| Interface: ep.Interface, | ||
| Role: ep.Role, | ||
| Scope: ep.Scope, | ||
| Limit: ep.Limit, | ||
| }) | ||
| + spaceNames.Add(appBindings[ep.Name]) |
wallyworld
May 15, 2017
Owner
Shouldn't we check that appBindings contains the ep.Name?
Or is that always guaranteed? Will appBindings always contain at the very list an entry for every ep.Name, where that entry may point to the default space? Maybe a comment to explain that? Though I think it would still be better to do a space, ok := appBindings[ep.Name] and error if ok=false just in case?
babbageclunk
May 15, 2017
•
Edited 1 time
-
babbageclunk
May 15, 2017
Member
-
May 15, 2017
My thinking was that if it didn't contain the endpoint then the result would be "" anyway, which is what we'd want here, because we want to get the provider info for the default space in that case. I think it warrants a comment, but I don't want to error if there's no binding. (I'll do a check to confirm that there isn't always an entry for an endpoint if it hasn't been explicitly bound.)
babbageclunk
May 15, 2017
Member
I'll change it to be more explicit, otherwise it's relying on DefaultSpaceName being "".
babbageclunk
May 15, 2017
•
Edited 1 time
-
babbageclunk
May 15, 2017
Member
-
May 15, 2017
From reading the code of Application.EndpointBindings, if there are no bindings it returns the default bindings for the charm - each endpoint will be mapped to "". It's not so clear reading the code for endpoint_bindings.go:mergeBindings (called from State.AddApplication), but I think the intent there is the same. So I think you're right, it should be an error if the binding isn't found. I'll do that.
| - releaser() | ||
| - } | ||
| - }() | ||
| +func (api *API) collectRemoteSpaces(st *state.State, spaceNames []string) (map[string]params.RemoteSpace, error) { |
| + results := make(map[string]params.RemoteSpace) | ||
| + for _, name := range spaceNames { | ||
| + space := environs.DefaultSpaceInfo | ||
| + if name != "" { |
babbageclunk
May 15, 2017
Member
I had a DefaultProviderSpaceId, but I replaced it with DefaultSpaceInfo when I changed the signature of Environ.ProviderSpaceInfo. I'll add another for DefaultSpaceName.
| + continue | ||
| + } | ||
| + remoteSpace := ParamsFromProviderSpaceInfo(providerSpace) | ||
| + // Use the name from state in case provider and state disagree. |
wallyworld
May 15, 2017
Owner
When would the disagreement happen?
If we are to use the state name, maybe pass it in as an arg to ParamsFromProviderSpaceInfo
But it does seem wrong to expect any difference?
babbageclunk
May 15, 2017
Member
I'm not sure - I think it could happen if the spaces had been discovered and then renamed in MAAS? Maybe that's not possible, but matching up the bindings wouldn't work unless the names match so it seems like reasonable defensive coding to enforce it.
| @@ -0,0 +1,85 @@ | ||
| +// Copyright 2015 Canonical Ltd. |
| + | ||
| +// ParamsFromProviderSpaceInfo converts a ProviderSpaceInfo into the | ||
| +// equivalent params.RemoteSpace. | ||
| +func ParamsFromProviderSpaceInfo(info *environs.ProviderSpaceInfo) params.RemoteSpace { |
babbageclunk
May 15, 2017
Member
No, they don't, good call - holdover from when they were in apiserver/common/crossmodelcommon.
| @@ -238,7 +238,7 @@ func (s *Suite) TestAdoptResources(c *gc.C) { | ||
| st := s.Factory.MakeModel(c, nil) | ||
| defer st.Close() | ||
| - env := mockEnviron{Stub: &testing.Stub{}} | ||
| + env := mockzEnviron{Stub: &testing.Stub{}} |
babbageclunk
May 15, 2017
Member
Doh, yup. Must have mashed the keyboard after running the tests but before committing. :( I should really rerun just before pushing to avoid that.
wallyworld
approved these changes
May 16, 2017
Can you also ensure dump-model handles the new remote application fields? And also migrate model?
Extra points for replacing "" with DefaultSpaceName in other places
| - releaser() | ||
| - } | ||
| - }() | ||
| +// collectRemoteSpaces gets provider information about all of the |
|
Machine got killed. :| !!build!! |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
Nil pointer error in agent tests. :( |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
babbageclunk commentedMay 15, 2017
Description of change
Change the Application facade to capture information (from the provider) about the spaces for a remote application when a remote relation is created or an offer is consumed. This will be used in other parts of the system to decide whether connections between units in each endpoint can use cloud-local addresses rather than going via the public internet (we're calling this short-circuiting).
This required rejigging the apiserver/application code to pass application offers around, since we now have spaces and bindings on them as well as the other information that was being passed around individually.
QA steps
Smoke test bootstrapping and making a remote relation.
No behaviour change yet - a future PR will add
ProviderSpaceInfosupport to a provider (probably ec2) and then adding a remote relation will capture space info for the remote application.