discoverspaces: Get subnets when provider doesn't support space discovery #7067

babbageclunk · 2017-03-03T03:09:06Z

Description of change

At the moment we only get subnets from the provider if it supports space discovery, but it would be useful to have them for providers that support networking (even if we don't have them tagged with a space). That will allow us to use them to set up ingress rules when a cross-model relation is created.

Change the discover spaces worker to collect subnets (with no associated spaces) if the provider doesn't support space discovery. Also change the AddSubnets API call to allow subnets without a space, and change the ListSubnets API method and command to handle empty space.

Rewrite the worker tests to not use JujuConnSuite.

QA steps

Bootstrap on AWS.
Run juju subnets to verify that subnets have been imported in by the discoverspaces worker - they'll come back with a blank space tag.

babbageclunk · 2017-03-03T03:36:09Z

!!build!!

wallyworld

Not 100% on the non-transactional nature of the discovery but it seems that's an existing issue

wallyworld · 2017-03-03T03:41:02Z

apiserver/common/networkingcommon/subnets.go

@@ -55,7 +55,7 @@ func NewAddSubnetsCache(api NetworkBacking) *addSubnetsCache {
 // up in the cache (or populates the cache if empty).
 func (cache *addSubnetsCache) validateSpace(spaceTag string) (*names.SpaceTag, error) {
 	if spaceTag == "" {
-		return nil, errors.Errorf("SpaceTag is required")
+		return nil, nil


Would be great if "" were a valid space name.
That would require a change to the names.v2 package though. But it would avoid all the special case code in this PR.

I'll put a TODO in for this. Maybe linked to a bug?

wallyworld · 2017-03-03T03:46:45Z

worker/discoverspaces/discoverspaces.go

 	if err != nil {
 		return errors.Trace(err)
 	}
-	stateSubnets, err := facade.ListSubnets(params.SubnetsFilters{})
+	var addSubnetsArgs params.AddSubnetsParams
+	collectMissingSubnets(&addSubnetsArgs, "", stateSubnetIds, subnets)


This logic really needs to be server side - it is not transactional as is.
But it seems the existing code suffers from the same problem.....

wallyworld · 2017-03-03T03:51:07Z

worker/discoverspaces/worker_test.go

 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
-	"gopkg.in/juju/names.v2"
+	//"gopkg.in/juju/names.v2"


wallyworld · 2017-03-03T03:51:22Z

worker/discoverspaces/worker_test.go

-		cf.addSubnetsCallback()
-	}
-	return cf.API.AddSubnets(args)
+	coretesting.BaseSuite


wallyworld · 2017-03-03T03:53:43Z

worker/discoverspaces/discoverspaces.go

@@ -297,3 +296,44 @@ func (dw *discoverspacesWorker) addSubnetsFromArgs(addSubnetsArgs params.AddSubn

 	return nil
 }
+
+func (dw *discoverspacesWorker) getStateSubnets() (set.Strings, error) {


can we call this getModelSubnets()
let's not leak the state.State abstraction

jameinel · 2017-03-03T04:19:13Z

The one bit of syntax that I think we want to consider is that "" should be considered a space. We'd like to try treating it as just a proper space that happens to not have a name. So it's more about "allow a space to not have a name" than "allow a subnet to not have a space". John =:->

…

On Mar 3, 2017 7:09 AM, "Christian Muirhead" ***@***.***> wrote: Description of change At the moment we only get subnets from the provider if it supports space discovery, but it would be useful to have them for providers that support networking (even if we don't have them tagged with a space). That will allow us to use them to set up ingress rules when a cross-model relation is created. Change the discover spaces worker to collect subnets (with no associated spaces) if the provider doesn't support space discovery. Also change the AddSubnets API call to allow subnets without a space, and change the ListSubnets API method and command to handle empty space. Rewrite the worker tests to not use JujuConnSuite. QA steps - Bootstrap on AWS. - Run juju subnets to verify that subnets have been imported in by the discoverspaces worker - they'll come back with a blank space tag. ------------------------------ You can view, comment on, or merge this pull request online at: #7067 Commit Summary - Refactor discoverspaces worker - Rewrite discoverspaces tests without JujuConnSuite - Get subnets when there's no space discovery available - Allow a blank space tag on subnets - Handle blank space tag in ListSubnets File Changes - *M* apiserver/common/networkingcommon/subnets.go <https://github.com/juju/juju/pull/7067/files#diff-0> (15) - *M* apiserver/common/networkingcommon/subnets_test.go <https://github.com/juju/juju/pull/7067/files#diff-1> (56) - *M* apiserver/subnets/subnets_test.go <https://github.com/juju/juju/pull/7067/files#diff-2> (2) - *M* apiserver/testing/stub_network.go <https://github.com/juju/juju/pull/7067/files#diff-3> (22) - *M* cmd/juju/subnet/list.go <https://github.com/juju/juju/pull/7067/files#diff-4> (12) - *M* cmd/juju/subnet/list_test.go <https://github.com/juju/juju/pull/7067/files#diff-5> (20) - *M* worker/discoverspaces/config_test.go <https://github.com/juju/juju/pull/7067/files#diff-6> (18) - *M* worker/discoverspaces/discoverspaces.go <https://github.com/juju/juju/pull/7067/files#diff-7> (106) - *M* worker/discoverspaces/fake_test.go <https://github.com/juju/juju/pull/7067/files#diff-8> (56) - *M* worker/discoverspaces/worker_test.go <https://github.com/juju/juju/pull/7067/files#diff-9> (421) Patch Links: - https://github.com/juju/juju/pull/7067.patch - https://github.com/juju/juju/pull/7067.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7067>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMYfajF3d7d6DqdxVIphJt-stnCu_nRks5rh4RTgaJpZM4MRz9w> .

jameinel · 2017-03-03T04:22:03Z

That also follows through with "What subnets could this endpoint be in". It's not all subnets it's the subnets in the space it was configured in. Just that the space name might be "". I think that can help shape some of the API so we're prepared for when we have concrete spaces everywhere. John =:->

…

On Mar 3, 2017 8:19 AM, "John Meinel" ***@***.***> wrote: The one bit of syntax that I think we want to consider is that "" should be considered a space. We'd like to try treating it as just a proper space that happens to not have a name. So it's more about "allow a space to not have a name" than "allow a subnet to not have a space". John =:-> On Mar 3, 2017 7:09 AM, "Christian Muirhead" ***@***.***> wrote: > Description of change > > At the moment we only get subnets from the provider if it supports space > discovery, but it would be useful to have them for providers that support > networking (even if we don't have them tagged with a space). That will > allow us to use them to set up ingress rules when a cross-model relation is > created. > > Change the discover spaces worker to collect subnets (with no associated > spaces) if the provider doesn't support space discovery. Also change the > AddSubnets API call to allow subnets without a space, and change the > ListSubnets API method and command to handle empty space. > > Rewrite the worker tests to not use JujuConnSuite. > QA steps > > - Bootstrap on AWS. > - Run juju subnets to verify that subnets have been imported in by > the discoverspaces worker - they'll come back with a blank space tag. > > ------------------------------ > You can view, comment on, or merge this pull request online at: > > #7067 > Commit Summary > > - Refactor discoverspaces worker > - Rewrite discoverspaces tests without JujuConnSuite > - Get subnets when there's no space discovery available > - Allow a blank space tag on subnets > - Handle blank space tag in ListSubnets > > File Changes > > - *M* apiserver/common/networkingcommon/subnets.go > <https://github.com/juju/juju/pull/7067/files#diff-0> (15) > - *M* apiserver/common/networkingcommon/subnets_test.go > <https://github.com/juju/juju/pull/7067/files#diff-1> (56) > - *M* apiserver/subnets/subnets_test.go > <https://github.com/juju/juju/pull/7067/files#diff-2> (2) > - *M* apiserver/testing/stub_network.go > <https://github.com/juju/juju/pull/7067/files#diff-3> (22) > - *M* cmd/juju/subnet/list.go > <https://github.com/juju/juju/pull/7067/files#diff-4> (12) > - *M* cmd/juju/subnet/list_test.go > <https://github.com/juju/juju/pull/7067/files#diff-5> (20) > - *M* worker/discoverspaces/config_test.go > <https://github.com/juju/juju/pull/7067/files#diff-6> (18) > - *M* worker/discoverspaces/discoverspaces.go > <https://github.com/juju/juju/pull/7067/files#diff-7> (106) > - *M* worker/discoverspaces/fake_test.go > <https://github.com/juju/juju/pull/7067/files#diff-8> (56) > - *M* worker/discoverspaces/worker_test.go > <https://github.com/juju/juju/pull/7067/files#diff-9> (421) > > Patch Links: > > - https://github.com/juju/juju/pull/7067.patch > - https://github.com/juju/juju/pull/7067.diff > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#7067>, or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAMYfajF3d7d6DqdxVIphJt-stnCu_nRks5rh4RTgaJpZM4MRz9w> > . >

Rearrange to add a path where we just get subnets when space discovery isn't supported by the provider. Extract code to get existing subnets and collect subnets that need to be added into functions that can be reused from the subnet discovery.

It was getting very tricky to add new tests for other cases using the full API and dummy provider. Rework the tests to use a fake API facade and fake provider that we can inject data and errors into more easily.

If the provider doesn't support space discovery, we still add subnets (with no space tag). This enables setting up ingress rules for cross-model relations.

babbageclunk · 2017-03-12T21:26:38Z

!!build!!

Subnets obtained from providers with no space discovery will have blank space tags - the API and client need to handle that.

This will enable simplifying the discover spaces API. At the moment the API requests these from the provider itself. It has a lot of extra complexity for cross-checking the sent subnet information with the details queried from the provider. This makes sense for the API underpinning the `juju add-subnet` command, where we want the user to provide either the provider id or the CIDR, and need to fill in the other fields. But it's very confusing to make the discover spaces worker go through the same path, when it has all the necessary info from the provider already.

Implemented the method standalone rather than using networkingcommon.AddSubnets. That has far more cleverness than is needed for this case and it overcomplicated how this worker behaves in tests.

babbageclunk · 2017-03-14T22:05:16Z

!!build!!

babbageclunk · 2017-03-14T22:30:50Z

$$merge$$

jujubot · 2017-03-14T22:32:14Z

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

wallyworld approved these changes Mar 3, 2017

View reviewed changes

babbageclunk added 3 commits March 12, 2017 16:19

Refactor discoverspaces worker

819757f

Rearrange to add a path where we just get subnets when space discovery isn't supported by the provider. Extract code to get existing subnets and collect subnets that need to be added into functions that can be reused from the subnet discovery.

Rewrite discoverspaces tests without JujuConnSuite

d613b14

It was getting very tricky to add new tests for other cases using the full API and dummy provider. Rework the tests to use a fake API facade and fake provider that we can inject data and errors into more easily.

Get subnets when there's no space discovery available

554d9ec

If the provider doesn't support space discovery, we still add subnets (with no space tag). This enables setting up ingress rules for cross-model relations.

babbageclunk force-pushed the discover-subnets-tests branch from b7344ac to 6682a5c Compare March 12, 2017 21:25

babbageclunk added 4 commits March 15, 2017 11:01

Handle blank space tag in ListSubnets

f823792

Subnets obtained from providers with no space discovery will have blank space tags - the API and client need to handle that.

Review changes

41a2843

Implement DiscoverSpaces.AddSubnets API method

8e87c5b

Implemented the method standalone rather than using networkingcommon.AddSubnets. That has far more cleverness than is needed for this case and it overcomplicated how this worker behaves in tests.

babbageclunk force-pushed the discover-subnets-tests branch from 6682a5c to 8e87c5b Compare March 14, 2017 22:04

Add TODO for allowing space with "" as a name

c276ea9

jujubot merged commit 7c2469e into juju:develop Mar 14, 2017

babbageclunk deleted the discover-subnets-tests branch March 20, 2017 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

discoverspaces: Get subnets when provider doesn't support space discovery #7067

discoverspaces: Get subnets when provider doesn't support space discovery #7067

babbageclunk commented Mar 3, 2017

babbageclunk commented Mar 3, 2017

wallyworld left a comment

wallyworld Mar 3, 2017

babbageclunk Mar 3, 2017

wallyworld Mar 3, 2017

wallyworld Mar 3, 2017

babbageclunk Mar 3, 2017

wallyworld Mar 3, 2017

wallyworld Mar 3, 2017

babbageclunk Mar 3, 2017

jameinel commented Mar 3, 2017 via email

jameinel commented Mar 3, 2017 via email

babbageclunk commented Mar 12, 2017

babbageclunk commented Mar 14, 2017

babbageclunk commented Mar 14, 2017

jujubot commented Mar 14, 2017

discoverspaces: Get subnets when provider doesn't support space discovery #7067

discoverspaces: Get subnets when provider doesn't support space discovery #7067

Conversation

babbageclunk commented Mar 3, 2017

Description of change

QA steps

babbageclunk commented Mar 3, 2017

wallyworld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameinel commented Mar 3, 2017 via email

jameinel commented Mar 3, 2017 via email

babbageclunk commented Mar 12, 2017

babbageclunk commented Mar 14, 2017

babbageclunk commented Mar 14, 2017

jujubot commented Mar 14, 2017