Implement subnetwork.get_or_create_default #67

chiniforooshan · 2017-09-01T19:31:05Z

coveralls · 2017-09-01T19:45:12Z

Coverage decreased (-0.1%) to 52.138% when pulling d05d4b8 on chiniforooshan:subnets into d3e9c56 on gvlproject:gce.

mbookman · 2017-09-01T21:21:26Z

cloudbridge/cloud/providers/gce/services.py

        if not name:
            name = 'subnet-{0}'.format(uuid.uuid4())
-        region = self.provider.region_name
+        if region is not None and zone is not None:


Does this require explicitly checking for "None" or can it be:

if region and zone:

N/A: this part is removed in the newer version #74.

mbookman · 2017-09-01T21:25:42Z

cloudbridge/cloud/providers/gce/services.py

@@ -774,10 +773,13 @@ def list(self, network=None, region=None, limit=None, marker=None):
                                     response.get('nextPageToken'),
                                     False, data=subnets)

-    def create(self, network, cidr_block, name=None, zone=None):
+    def create(self, network, cidr_block, name=None, zone=None, region=None):


Could you add a comment explaining how regions and zones are intended to work here.
I'm not sure if the implementation reflects what had been discussed.

I think the intended implementation is:

# GCE subnets are regional. The logic for determining the subnet region here is: # - If a region is provided, use it. # - If a zone is provided, use the region that it belongs to. # - If neither is provided, use the default region as set in the provider.

Also, region is not a method param in the interface (https://github.com/gvlproject/cloudbridge/blob/master/cloudbridge/cloud/interfaces/services.py#L728). Is it not possible to obtain the region from the zone object? and use the default region when the zone is not provided?

Done: fixed in #74.

mbookman · 2017-09-01T21:26:47Z

cloudbridge/cloud/providers/gce/services.py

        if not name:
            name = 'subnet-{0}'.format(uuid.uuid4())
-        region = self.provider.region_name
+        if region is not None and zone is not None:
+            cb.log.warning('Both parameters "zone" and "region" are provided')


Can the message also specify what the outcome is?
Something like:

Both parameters zone and region are provided; using region.

N/A: this part is removed in the newer version #74.

mbookman · 2017-09-01T21:27:32Z

cloudbridge/cloud/providers/gce/services.py

+        if region is not None and zone is not None:
+            cb.log.warning('Both parameters "zone" and "region" are provided')
+        if region is None:
+            region = self.provider.region_name
        if isinstance(zone, GCEPlacementZone):


As I read this, if a zone is specified it ends up being the thing used, even if a region was specified as well.
Is that what was intended?

N/A: this part is removed in the newer version #74.

mbookman · 2017-09-01T21:28:13Z

cloudbridge/cloud/providers/gce/services.py

                if subnet.id == response['targetId']:
                    return subnet
        except:
            return None

    def get_or_create_default(self, zone=None):
-        raise NotImplementedError('To be implemented')
+        """
+        Every GCP project comes with a default auto mode VPC network. An auto


I think this is true for every new GCP project, but is not true for old projects.

The same situation happens on AWS so if a provider-default does not exist, we create a CloudBridge-default: https://github.com/gvlproject/cloudbridge/blob/master/cloudbridge/cloud/providers/aws/services.py#L884

Done: fixed in #74.

nuwang

Thanks for these changes. A few additional comments below. We've also been tweaking things and the main change is that networks have been moved under networking for consistency. Now, networks, subnets, routers, and gateways are all categorized under networking.

nuwang · 2017-09-02T13:45:44Z

cloudbridge/cloud/providers/gce/resources.py

@@ -1384,7 +1384,7 @@ def region(self):

    @property
    def zone(self):
-        raise NotImplementedError('To be implemented')
+        raise NotImplementedError('GCP subnetworks are regional resources')


The zone is an optional parameter mainly for informational purposes so it's probably better to return None here.

Done: fixed in #74.

nuwang · 2017-09-02T13:50:50Z

cloudbridge/cloud/providers/gce/services.py

+        returns the subnetwork of the default network that spans the given
+        zone.
+        """
+        network = self.provider.network.get_by_name(


This should probably be replaced by self.provider.networking.networks.find(name=GCEFirewallsDelegate.DEFAULT_NETWORK)[0]

Also, in some recent refactoring we did, networks have been moved under networking for greater consistency, and the parameter order for network.create() has been changed.

Which also brings up the question, do we need the get_by_name function since the find function exists?

N/A: this part is removed in the newer version #74.

codecov-io · 2017-09-15T19:47:23Z

Codecov Report

Merging #67 into gce will decrease coverage by 0.09%.
The diff coverage is 12.5%.

@@            Coverage Diff            @@
##              gce      #67     +/-   ##
=========================================
- Coverage   51.06%   50.96%   -0.1%     
=========================================
  Files          21       21             
  Lines        4179     4187      +8     
  Branches      552      555      +3     
=========================================
  Hits         2134     2134             
- Misses       1996     2004      +8     
  Partials       49       49

Impacted Files	Coverage Δ
cloudbridge/cloud/providers/gce/resources.py	`27.44% <0%> (ø)`	⬆️
cloudbridge/cloud/providers/gce/services.py	`22.66% <13.33%> (-0.31%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3e9c56...3f2a648. Read the comment docs.

afgane · 2017-09-15T19:55:23Z

cloudbridge/cloud/providers/gce/services.py

@@ -626,28 +626,27 @@ def list(self, limit=None, marker=None, filter=None):
        except:
            return []

-    def create(self, name):
+    def _create(self, name, autoCreateSubnetworks):


autoCreateSubnetworks should be auto_create_subnetworks. Possibly even better to shorten it to just create_subnetworks.

Done: fixed in #74.

afgane · 2017-09-15T20:05:01Z

cloudbridge/cloud/providers/gce/services.py

+        network = self.provider.network.get_or_create_default()
+        subnets = self.list(network, zone)
+        if len(subnets) > 1:
+            cb.log.warning('The default network has more than one subnetwork '


This seems to go against the intent of this method. As a user, by calling get_or_create_default I'd expect to get something back. I guess it's not likely that this condition will be triggered as it's likely there will be only one subnet for the default network so it's probably ok but I wonder if perhaps we should instead just return a random subnet here as well?

Note that this line just logs a warning. Next lines will actually return something.

Ahh, yes, I see. I just took the next if as else if...

afgane · 2017-09-15T21:37:49Z

@chiniforooshan should we close this PR given #74? There's a couple more bits here but not sure if those are a result of the the git mixup...

Implemente subnetwork.get_or_create_default

d05d4b8

mbookman reviewed Sep 1, 2017

View reviewed changes

nuwang reviewed Sep 2, 2017

View reviewed changes

baizhang and others added 3 commits September 15, 2017 15:45

Add image_id and create_image methods to GCEInstance.

b61e3f1

Address review comment to use os.getenv() with default value.

3b2c1c4

Implemente subnetwork.get_or_create_default

3f2a648

afgane reviewed Sep 15, 2017

View reviewed changes

chiniforooshan mentioned this pull request Sep 15, 2017

Implement subnetwork.get_or_create_default #74

Merged

chiniforooshan closed this Sep 18, 2017

chiniforooshan deleted the subnets branch September 18, 2017 15:32

nuwang added this to the release 1.0.0 milestone Oct 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement subnetwork.get_or_create_default #67

Implement subnetwork.get_or_create_default #67

chiniforooshan commented Sep 1, 2017

coveralls commented Sep 1, 2017 •

edited

Loading

mbookman Sep 1, 2017

chiniforooshan Sep 15, 2017

mbookman Sep 1, 2017

afgane Sep 5, 2017

chiniforooshan Sep 15, 2017

mbookman Sep 1, 2017

chiniforooshan Sep 15, 2017

mbookman Sep 1, 2017

chiniforooshan Sep 15, 2017

mbookman Sep 1, 2017

afgane Sep 5, 2017

chiniforooshan Sep 15, 2017

nuwang left a comment

nuwang Sep 2, 2017

chiniforooshan Sep 15, 2017

nuwang Sep 2, 2017

chiniforooshan Sep 15, 2017

codecov-io commented Sep 15, 2017 •

edited

Loading

afgane Sep 15, 2017

chiniforooshan Sep 15, 2017

afgane Sep 15, 2017

chiniforooshan Sep 15, 2017

afgane Sep 15, 2017

afgane commented Sep 15, 2017

Implement subnetwork.get_or_create_default #67

Implement subnetwork.get_or_create_default #67

Conversation

chiniforooshan commented Sep 1, 2017

coveralls commented Sep 1, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuwang 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

codecov-io commented Sep 15, 2017 • edited Loading

Codecov Report

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

afgane commented Sep 15, 2017

coveralls commented Sep 1, 2017 •

edited

Loading

codecov-io commented Sep 15, 2017 •

edited

Loading