Fix #223: Enhance Network "get_next" methods to optionally allocate/reserve at the same time by rmhasan · Pull Request #266 · dropbox/nsot

rmhasan · 2017-02-06T05:09:58Z

Users can send a post request to next_network and next_address actions. If it is a post request then the action will get the networks or addresses and then save them as network objects in the database as allocated. If the user sends the reserve flag and sets it as true, then it will be saved in the reserved state.

nickpegg

Sorry about taking so long to review this! Looks pretty good besides a few comments I had.

nickpegg · 2017-05-11T19:53:20Z

nsot/api/views.py

+                                 site=models.Site.objects.get(pk=site_pk),
+                                 parent=parent,
+                                 state=state,
+                                 is_ip=is_ip)


You don't have to worry about setting is_ip since it's auto-generated for you here:

nsot/nsot/models.py

Lines 1124 to 1125 in 4282f7c

if network.network_address == network.broadcast_address:

self.is_ip = True

Sorry about any confusion! That field should have had editable = False set to signal that.

Okay I'll set editable for is_ip to false.

nickpegg · 2017-05-11T19:57:08Z

nsot/api/views.py

-        return self.success(networks)
+        if request.method == 'POST':
+            state = 'reserved' if qpbool(params.get('reserve', False)) else 'allocated'
+            is_ip = True if prefix_length == 32 or prefix_length == 128 else False


This comment is moot if you no longer set is_ip, but there should be an IP version check here since it's valid to have a v6 prefix length of /32.

nickpegg · 2017-05-11T20:04:53Z

nsot/api/views.py

+            )
+            self.allocate_networks(networks, prefix_length, site_pk, network, state=state, is_ip=is_ip)
+        else:
+            networks = network.get_next_network(


Suggestion: Since you're returning the unicode representation of the Networks in all cases, you could simplify this code by getting rid of this else and then set networks with as_objects=True right before the if statement.

nickpegg · 2017-05-11T20:12:07Z

nsot/api/views.py

+                    prefix_length = 128
+                self.allocate_networks(addresses, prefix_length, site_pk, network, state=state, is_ip=True)
+        else:
+            addresses = network.get_next_address(num, strict, as_objects=False)


Same thing as with next_network, it looks like this else can be removed if you move the setting of addresses to before the if with as_objects=True.

nickpegg · 2017-05-11T20:16:04Z

nsot/api/views.py

-
-        return self.success(networks)
+        if request.method == 'POST':
+            state = 'reserved' if qpbool(params.get('reserve', False)) else 'allocated'


These state strings are set as constants on the Network model, which you should use just in case we ever change the string in the future:

nsot/nsot/models.py

Lines 690 to 693 in 067699c

ALLOCATED = 'allocated'

ASSIGNED = 'assigned'

ORPHANED = 'orphaned'

RESERVED = 'reserved'

nickpegg · 2017-05-11T20:16:59Z

nsot/api/views.py

-        return self.success(addresses)
+        if request.method == 'POST':
+            addresses = network.get_next_address(num, strict, as_objects=True)
+            state = 'reserved' if qpbool(params.get('reserve', False)) else 'allocated'


Same thing as with next_network, you should use the Network state constants here

nickpegg · 2017-05-11T20:35:07Z

tests/model_tests/test_networks.py

    child = models.Network.objects.create(site = site, cidr = u'2001:db8:abcd:0012::0/97')
    expected = [ipaddress.ip_network(u'2001:db8:abcd:12::8000:0/128')]
-    assert parent.get_next_network(128, strict = True) == expected 
+    assert parent.get_next_network(128, strict = True) == expected


Was this whitespace addition unintentional?

Yeah it was added unintentionally.

rmhasan · 2017-05-31T06:21:39Z

Hi @nickpegg
I added the following changes

I removed setting is_ip from the next_network and next_address views.
In model.Network, for the is_ip field I set editable to False.
In the next_network and next_address views, I use the RESERVED and ALLOCATED constants from
the model.Network class
In the next_network and next_address views, I moved the network.get_next_network function
call out of the if clause, and set the as_objects paramter to True. Also I removed the else clause
Also take a look at Allows users to send post requests to next_network and next_address pynsot#132. It allows users to send post requests to next_network and
next_address

EDIT:

Also in the Interface model functions get_ancestors and get_descendants, I am removing
a separate list for id's. This change makes the functions more space efficient.

jathanism

Sorry about the delay in responding! We got busy again!

So this all looks good, exept for my comments on .allocat_networks() which is just an attempt to simplify the API to avoid some redundancy!

Thank you so much for your contributions!

jathanism · 2017-06-27T16:18:55Z

nsot/api/views.py

+    def allocate_networks(self, networks, prefix_length, site_pk, parent, state='allocated'):
+        site = models.Site.objects.get(pk=site_pk)
+        for n in networks:
+            obj = models.Network(


I think it would be cleaner to remove the prefix_length argument from this method and just allow it to take a list of CIDRs.

Because broadcast_address, ip_version, parent are all automatically populated and overridden internally so providing them up front is redundant.

Then this could just be:

for n in networks: obj = models.Network.create(cidr=n, site=site, state=state)

jathanism · 2017-06-27T16:22:00Z

nsot/api/views.py

-        addresses = network.get_next_address(num, strict, as_objects=False)
-
-        return self.success(addresses)
+        addresses = network.get_next_address(num, strict, as_objects=True)


If we ditch prefix_length as an argument to .allocate_networks() we can probably go back to returning a list of unicode strings (as_objects=False) and then the version/prefix_length calculation can also go away!

jathanism · 2017-06-27T16:25:08Z

nsot/api/views.py

+            else:
+                state = models.Network.ALLOCATED
+            self.allocate_networks(networks, prefix_length, site_pk, network, state=state)
+        return self.success([unicode(x) for x in networks])


Same here, too... We could get revert to as_objects=False and return self.success(networks) here as well as not passing prefix_length to self.allocate_networks(...). :)

jathanism · 2017-07-13T18:33:18Z

@rmhasan We miss you! <3

User can send a post request to next_network and next_address actions. If it is a post request, then the action will get the networks or addresses and then save them in as network objects in the db

If we requests that networks be allocated into the reserved states through the next_network and next_address actions, then they should be part of the results when the command nsot networks list -c 10.2.1.0/24 reserved

- removed code to set is_ip and also set editable to False for is_ip in Network model - using RESERVED and ALLOCATED constants from model.Network class as state strings instead of the actual strings, 'reserved' and 'allocated' - in next_network and next_address views, moved network.get_next_network function call out of the if clause. Also set as_objects paramter for network.get_next_network to True

get_descendants, I am removing the separate id lists. There is no need to make a list for the Interface id's.

- will pass as_objects=False to models.Network.get_next_network to get CIDR's - removed prefix_length & parent argument to NetworkViewSet.allocate_networks - broadcast_address, ip_version, parent are all automatically populated and overridden internally - returns return value from model.Network.get_next_network directly

rmhasan · 2017-09-14T06:37:21Z

Hi @jathanism, sorry I've been gone for so long. I implemented the changes you asked for, please take a look when you get a chance.

jathanism · 2017-09-14T18:51:04Z

@rmhasan Awesome! Welcome back. This is great! We'll go ahead and get this released soon. :)

rmhasan changed the base branch from master to develop February 6, 2017 05:10

rmhasan mentioned this pull request Feb 6, 2017

Allows users to send post requests to next_network and next_address dropbox/pynsot#132

Open

rmhasan mentioned this pull request Feb 19, 2017

Enhance Network "get_next" methods to optionally allocate/reserve at the same time #223

Open

nickpegg requested changes May 11, 2017

View reviewed changes

rmhasan mentioned this pull request May 16, 2017

Interface Tree traversal #271

Merged

jathanism changed the title ~~Fix for #223~~ Fix #223: Enhance Network "get_next" methods to optionally allocate/reserve at the same time May 24, 2017

nickpegg approved these changes Jun 27, 2017

View reviewed changes

jathanism suggested changes Jun 27, 2017

View reviewed changes

jathanism reviewed Jun 27, 2017

View reviewed changes

rmhasan and others added 5 commits September 14, 2017 00:20

Fix for #223

683ba11

User can send a post request to next_network and next_address actions. If it is a post request, then the action will get the networks or addresses and then save them in as network objects in the db

Added more tests

6df9e56

If we requests that networks be allocated into the reserved states through the next_network and next_address actions, then they should be part of the results when the command nsot networks list -c 10.2.1.0/24 reserved

For Interface model functions get_ancestors and

8f8c824

get_descendants, I am removing the separate id lists. There is no need to make a list for the Interface id's.

jathanism approved these changes Sep 14, 2017

View reviewed changes

jathanism added this to the v1.2.3 milestone Sep 20, 2017

nickpegg merged commit 16783bd into dropbox:develop Sep 20, 2017

	if network.network_address == network.broadcast_address:
	self.is_ip = True

	ALLOCATED = 'allocated'
	ASSIGNED = 'assigned'
	ORPHANED = 'orphaned'
	RESERVED = 'reserved'

Conversation

rmhasan commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickpegg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmhasan commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jathanism left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jathanism commented Jul 13, 2017

Uh oh!

rmhasan commented Sep 14, 2017

Uh oh!

jathanism commented Sep 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rmhasan commented Feb 6, 2017 •

edited

Loading

rmhasan commented May 31, 2017 •

edited

Loading