/juju

Rewritten spaces discovery code #7383

Merged
merged 5 commits into from Jun 1, 2017

Conversation

Reviewers

@hmlanigan hmlanigan

@howbazaar howbazaar

@jameinel jameinel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@wupeka @jujubot @jameinel @howbazaar @hmlanigan
@wupeka
Member

wupeka commented May 24, 2017 •
Edited 1 time

Description of change

  1. Add a method of manually refreshing spaces from substrate (other than restart of jujud) - juju reload-spaces
  2. Make sure that at when add-model returns spaces are loaded

QA steps

  1. Add space in MAAS, verify that after juju reload-spaces the space is visible in juju
  2. Add subnet in GCE (substrate not supporting spaces), verify that after juju reload-spaces the subnet is visible in juju
  3. Make sure that juju add-model foobar; juju -m foobar list-spaces shows the proper spaces (currently it doesn't as there's a timing issue and add-model returns before spaces are loaded)
  4. Make sure that after bootstrap both controller and default model have spaces loaded

Documentation changes

  1. New CLI command juju reload-spaces
  2. When juju add-model returns spaces are loaded

Bug reference

https://bugs.launchpad.net/juju/+bug/1662264
https://bugs.launchpad.net/juju/+bug/1623324
@wupeka
Member

wupeka commented May 24, 2017

!!test!!
@wupeka
Member

wupeka commented May 24, 2017

!!spuriousfailure!!
@wupeka
Member

wupeka commented May 24, 2017

!!onceagain!!

@wupeka wupeka changed the title from [WiP] Rewritten spaces discovery code to Rewritten spaces discovery code May 24, 2017

version/version.go
@@ -19,7 +19,7 @@ import (
// The presence and format of this constant is very important.
// The debian/rules build recipe uses this value for the version
// number of the release package.
-const version = "2.2-rc1"
+const version = "2.2-rc4"
@hmlanigan

hmlanigan May 24, 2017

Member

Out of curiosity, why is the version changing in this PR?

@wupeka

wupeka May 24, 2017

Member

A mistake corrected by later commit

@wupeka
- Add reload-spaces CLI command and ReloadSpaces API call 
- Add single space reloads procedures in state
- Reload spaces synchronously when creating model and bootstraping
- Remove discoverspaces API and discoverspaces worker
264944d
@howbazaar

howbazaar requested changes May 25, 2017
View changes

Newer controllers may have older models in them. Are we certain that the Facade will never be called by an agent or client that isn't running in the apiserver?

api/facadeversions.go
@@ -30,7 +30,6 @@ var facadeVersions = map[string]int{
"Cloud": 1,
"Controller": 3,
"Deployer": 1,
- "DiscoverSpaces": 2,
@howbazaar

howbazaar May 25, 2017

Owner

We can't remove the apiserver component.

@wupeka

wupeka May 25, 2017

Member

DiscoverSpaces is only called from a worker within controller.

@wupeka
Member

wupeka commented May 25, 2017

!!test!!
@wupeka
Member

wupeka commented May 25, 2017

!!retest!!
@jameinel

jameinel reviewed May 25, 2017
View changes

a couple of tweaks.

I agree with your assessment that its safe to delete the discover spaces worker.

But I think it is the most hygenic to bump the Spaces API version if we're adding a new function. (unless we've already bumped the version since 2.1 since we haven't had another stable release of the facade.)

api/spaces/spaces.go
@@ -72,3 +72,12 @@ func (api *API) ListSpaces() ([]params.Space, error) {
}
return response.Results, err
}
+
+// ReloadSpaces reloads spaces from substrate
+func (api *API) ReloadSpaces() error {
@jameinel

jameinel May 25, 2017

Owner

Adding ReloadSpaces to the API should be a version bump of the Spaces API.

@wupeka

wupeka May 29, 2017

Member

Done.

apiserver/discoverspaces/discoverspaces.go
-// NewAPIWithBacking creates an API instance from the given network
-// backing (primarily useful from tests).
-func NewAPIWithBacking(st networkingcommon.NetworkBacking, resources facade.Resources, authorizer facade.Authorizer) (*API, error) {
- if !authorizer.AuthController() {
@jameinel

jameinel May 25, 2017

Owner

So this seems to be that no agents can ever call this, which should mean it is ok to remove. (we don't ever support running heterogenous controllers even if we do support agents not being in lock step)

@wupeka

wupeka May 29, 2017

Member

Ack.

apiserver/modelmanager/modelmanager.go
@@ -326,6 +326,11 @@ func (m *ModelManagerAPI) CreateModel(args params.ModelCreateArgs) (params.Model
}
defer st.Close()
+ err = st.ReloadSpaces(env)
+ if err != nil {
@jameinel

jameinel May 25, 2017

Owner

I often like to do these as:
if err := st.ReloadSpaces(env); err != nil {
}

it means the scope of the 'err' variable is only for the if/else blocks.

Not something that needs to be changed, as we're certainly not consistent everywhere. Just something I often do.

@wupeka

wupeka May 29, 2017

Member

Ack

apiserver/spaces/spaces.go
+
+// RefreshSpaces refreshes spaces from substrate
+func (api *spacesAPI) ReloadSpaces() error {
+ canRead, err := api.authorizer.HasPermission(permission.ReadAccess, api.backing.ModelTag())
@jameinel

jameinel May 25, 2017

Owner

I don't think Read permission is enough to invoke ReloadSpaces. I would expect Write level to mutate database objects.

We also need the APIServer test that tries with different permissions and ensures that we have the right perm set. (We should try as a readonly user/no perms and show that we get denied, but it succeeds with Write or Admin).

@wupeka

wupeka May 29, 2017

Member

Done

apiserver/spaces/spaces.go
+ if !canRead {
+ return common.ServerError(common.ErrPerm)
+ }
+ env, err := environs.GetEnviron(api.backing, environs.New)
@jameinel

jameinel May 25, 2017

Owner

this looks a little odd to use api.backing to get the environ, in order to pass the environ into api.backing.ReloadSpaces.
Is there a reason api.backing.ReloadSpaces doesn't grab its own environ?

This may be better for layering/testing, it just looks a little odd.

@wupeka

wupeka May 29, 2017

Member

We're using ReloadSpaces in 3 places and that's the only one where we have fo fetch the environment - in others we already have it so we don't have to fetch it.

cmd/juju/space/reload.go
+
+// listCommand displays a list of all spaces known to Juju.
+type ReloadCommand struct {
+ SpaceCommandBase
@jameinel

jameinel May 25, 2017

Owner

I'm curious what we get from SpaceCommandBase vs just a ModelCommandBase

@wupeka

wupeka May 29, 2017

Member

SpaceAPI with ReloadSpaces()

cmd/juju/space/reload.go
+ err := api.ReloadSpaces()
+ if err != nil {
+ if errors.IsNotSupported(err) {
+ ctx.Infof("cannot reload spaces: %v", err)
@jameinel

jameinel May 25, 2017

Owner

I would rather see a message about "Controller does not support reloading spaces", etc.
Though I'd rather see the check in general use the api.Version and have us bump the API version in order to know whether it is available or not.
This does work, but we could tell without an extra round trip once we know the Facade versions supported by the server.

@wupeka

wupeka May 29, 2017

Member

Done in api/spaces/spaces.go

cmd/juju/space/space.go
@@ -35,7 +35,7 @@ type SpaceAPI interface {
// TODO(dimitern): All of the following api methods should take
// names.SpaceTag instead of name, the only exceptions are
// AddSpace, and RenameSpace as the named space doesn't exist
- // yet.
+ // yet.a
@jameinel

jameinel May 25, 2017

Owner

typo

@wupeka

wupeka May 29, 2017

Member

Ack.

cmd/juju/space/space.go
@@ -137,6 +140,10 @@ func (m *mvpAPIShim) ListSpaces() ([]params.Space, error) {
return m.facade.ListSpaces()
}
+func (m *mvpAPIShim) ReloadSpaces() error {
+ return m.facade.ReloadSpaces()
@jameinel

jameinel May 25, 2017

Owner

seems strange to need an "mvpAPIShim". I wonder if there is something we should flag here to come back and get rid of.
'mvp' is short for "minimum viable product" which is usually "quickest and dirtiest thing to get something out the door".

@wupeka

wupeka May 29, 2017

Member

Noted, although I don't think it's the right PR to do this refactoring.

@jameinel

jameinel May 31, 2017

Owner

agreed

state/spacesdiscovery.go
+)
+
+func (st *State) getModelSubnets() (set.Strings, error) {
+ subnets, err := st.AllSubnets()
@jameinel

jameinel May 25, 2017

Owner

I didn't go through any of this code thoroughly as I understand it is just ported from the old worker, right?
I believe we talked about a couple things that weren't great with the old code, so if you want me to look it over, I can, but please ask.

@wupeka

wupeka May 29, 2017

Member

The logic is ported from the old code but there are some differences (e.g. with failure handling) so please do look at it.

state/spacesdiscovery.go
+func (st *State) ReloadSpaces(environ environs.Environ) error {
+ netEnviron, ok := environs.SupportsNetworking(environ)
+ if !ok {
+ logger.Debugf("not a networking environ")
@jameinel

jameinel May 25, 2017

Owner

So we call ReloadSpaces in a number of ways (always at every add space), so I don't know that this should be a higher warning level. But it would sort of be nice if the calls to Reload that were triggered by a user would indicate why we aren't updating any records.

Maybe we need to return an error, and have the built-in calls recognize it and discard it?
And then user-initiated calls can get a "space discovery not supported on this provider" ?

@wupeka

wupeka May 29, 2017

Member

Fixed - I'm returning NotSupported error (is this the right one for the circumstances?)

state/spacesdiscovery_test.go
+ "github.com/juju/juju/state"
+)
+
+type networkLessEnviron struct {
@jameinel

jameinel May 25, 2017

Owner

Again, I'm assuming test coverage was ported over from the old worker.

@wupeka

wupeka May 29, 2017

Member

These tests were written from scratch.

@jameinel

jameinel approved these changes May 31, 2017
View changes

a couple of comments about names and such but mostly good.
I don't know if I saw the test for a space with 2 subnets becoming a space with 4 subnets. but I think there was one with 1 becoming 2? similarly for having 1 space and then having 2 spaces.
I'm guessing I just missed that one.

api/spaces/spaces.go
@@ -75,6 +75,9 @@ func (api *API) ListSpaces() ([]params.Space, error) {
// ReloadSpaces reloads spaces from substrate
func (api *API) ReloadSpaces() error {
+ if api.facade.BestAPIVersion() < 3 {
+ return errors.NewNotSupported(nil, "Controller does not support reloading spaces")
@jameinel

jameinel May 31, 2017

Owner

Should this be left alone and done higher up in the cli to give a better message there?

apiserver/modelmanager/modelmanager.go
- return result, errors.Annotate(err, "Failed to perform spaces discovery")
+ if err = st.ReloadSpaces(env); err != nil {
+ if errors.IsNotSupported(err) {
+ logger.Debugf("Not performing spaces load on a non-networking environment")
@jameinel

jameinel May 31, 2017

Owner

Not reloading spaces for a non-networking environment.

This does look like a case where we would want 2 different error messages. Hm...
Maybe it doesn't matter. All it changes is what error message would be logged. In both cases we would actually just continue on our way.

cmd/juju/space/space.go
@@ -137,6 +140,10 @@ func (m *mvpAPIShim) ListSpaces() ([]params.Space, error) {
return m.facade.ListSpaces()
}
+func (m *mvpAPIShim) ReloadSpaces() error {
+ return m.facade.ReloadSpaces()
@jameinel

jameinel May 25, 2017

Owner

seems strange to need an "mvpAPIShim". I wonder if there is something we should flag here to come back and get rid of.
'mvp' is short for "minimum viable product" which is usually "quickest and dirtiest thing to get something out the door".

@wupeka

wupeka May 29, 2017

Member

Noted, although I don't think it's the right PR to do this refactoring.

@jameinel

jameinel May 31, 2017

Owner

agreed

state/spacesdiscovery.go
+ "github.com/juju/juju/network"
+)
+
+func (st *State) getModelSubnets() (set.Strings, error) {
@jameinel

jameinel May 31, 2017

Owner

I think this is actually get getSubnetProviderIds or something that is clearer we aren't talking about internal juju subnets, but just a set of ProviderIDs.

That said, maybe not worth renaming because really it just needs to go. Because these checks are the ones that are preventing us from determining renames etc.
So really this function at the least needs to be a map rather than a set.

@wupeka

wupeka Jun 1, 2017

Member

I'll leave it as it is for now, the 2nd part of this work (post-2.2) will be blocking add-{subnet,space}, support renaming, etc.

state/spacesdiscovery.go
+ }
+ return errors.Trace(st.LoadSpacesFromProvider(spaces))
+ } else {
+ logger.Debugf("environ does not support space discovery")
@jameinel

jameinel May 31, 2017

Owner

probably 'environ does not support space discovery, falling back to just subnet discovery' since we aren't just exiting here.

@wupeka

wupeka Jun 1, 2017

Member

ack

state/spacesdiscovery.go
+
+// LoadSubnetsFromProvider loads subnets into state.
+// Currently it does not delete removed subnets.
+func (st *State) LoadSubnetsFromProvider(subnets []network.SubnetInfo) error {
@jameinel

jameinel May 31, 2017

Owner

I'm not sure if this is "Load" so much as "Record" because you are passing in the subnets for it to track, right?

@wupeka

wupeka Jun 1, 2017

Member

Ack

state/spacesdiscovery.go
+ ProviderNetworkId: subnet.ProviderNetworkId,
+ CIDR: subnet.CIDR,
+ VLANTag: subnet.VLANTag,
+ AvailabilityZone: firstZone,
@jameinel

jameinel May 31, 2017

Owner

This definitely feels like a bad representation.
Its going to cause us to provision machines in a fixed zone once people start asking for a machine in a given space (mapping to a given subnet).
This will be especially bad on GCE/Openstack where subnets can actually span all zones.
I would tend to do "if len(...AZ) == 1" and otherwise not set it until we can actually fix the implementation to properly take a list.
Thoughts?

@wupeka

wupeka Jun 1, 2017

Member

This is current behaviour that I don't want to break.

state/spacesdiscovery.go
+
+// LoadSpacesFromProvider loads providerSpaces into state.
+// Currently it does not delete removed spaces.
+func (st *State) LoadSpacesFromProvider(providerSpaces []network.SpaceInfo) error {
@jameinel

jameinel May 31, 2017

Owner

"Load" to me would be to contact the provider to get the data. Since we're passing it in "Record" or "Save" seem appropriate.

@wupeka

wupeka Jun 1, 2017

Member

Ack

state/spacesdiscovery_test.go
+}
+
+var twoSpaces = []network.SpaceInfo{
+ {
@jameinel

jameinel May 31, 2017

Owner

is there a reason this isn't just {spaceOne, spaceTwo}

@wupeka

wupeka Jun 1, 2017

Member

space{One,Two}[0] actually, and there was no reason.

@jameinel

jameinel approved these changes May 31, 2017
View changes

a couple of comments about names and such but mostly good.
I don't know if I saw the test for a space with 2 subnets becoming a space with 4 subnets. but I think there was one with 1 becoming 2? similarly for having 1 space and then having 2 spaces.
I'm guessing I just missed that one.

@wupeka
Member

wupeka commented Jun 1, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Jun 1, 2017

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

jujubot commented Jun 1, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/11055
@wupeka
Member

wupeka commented Jun 1, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Jun 1, 2017

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

@jujubot jujubot merged commit 18c923d into juju:develop Jun 1, 2017
1 check failed

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment