Merge lp:~gmb/maas/fix-my-stupidity into lp:maas

Proposed by Graham Binns
Status: Merged
Approved by: Graham Binns
Approved revision: 2509
Merged at revision: 2508
Proposed branch: lp:~gmb/maas/fix-my-stupidity
Merge into: lp:maas
Diff against target: 175 lines (+77/-65)
2 files modified
src/maasserver/models/nodegroupinterface.py (+65/-59)
src/maasserver/models/tests/test_nodegroupinterface.py (+12/-6)
To merge this branch: bzr merge lp:~gmb/maas/fix-my-stupidity
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+225516@code.launchpad.net

Commit message

Fix TestNodeGroupInterface.test_clean_ip_ranges_works_with_ipv6_ranges()
so that it no longer uses factory.make_ip_range().

Description of the change

Fix TestNodeGroupInterface.test_clean_ip_ranges_works_with_ipv6_ranges()
so that it no longer uses factory.make_ip_range().

make_ip_range() makes no guarantees about the size of the range, other
than that the lower bound will be lower than the upper. So it was
possible for the returned IP range to span only two addresses, which
meant that when test_clean_ip_ranges_works_with_ipv6_ranges added 1 to
the lowest address in the range and subtracted 1 from the highest - in
order to definte a range within a range - it would end up just reversing
the bounds.

To fix this, I've made test_clean_ip_ranges_works_with_ipv6_ranges()
just use get_random_ipv6_network(), which it was already doing, and
which returns a network with a minimum size of 8.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Yup, that should do it. Maybe make_ip_range should take a "minimum size" argument, too, but that would be harder to implement I suppose.

By the way, clean_ip_ranges looks as if it could be a lot prettier if it just started with "if I have nothing to do here, return right away."

review: Approve
Revision history for this message
Graham Binns (gmb) wrote :

> Yup, that should do it. Maybe make_ip_range should take a "minimum size"
> argument, too, but that would be harder to implement I suppose.

Yeah.

> By the way, clean_ip_ranges looks as if it could be a lot prettier if it just
> started with "if I have nothing to do here, return right away."

Agreed. I'll change that as a drive-by.

lp:~gmb/maas/fix-my-stupidity updated
2509. By Graham Binns

Quick exit-early change for clean_ip_range(s|bounds).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/maasserver/models/nodegroupinterface.py'
--- src/maasserver/models/nodegroupinterface.py 2014-07-03 09:42:15 +0000
+++ src/maasserver/models/nodegroupinterface.py 2014-07-03 17:00:27 +0000
@@ -225,70 +225,76 @@
225225
226 def clean_ip_range_bounds(self):226 def clean_ip_range_bounds(self):
227 """Ensure that the static and dynamic ranges have sane bounds."""227 """Ensure that the static and dynamic ranges have sane bounds."""
228 if (self.is_managed and (self.static_ip_range_low and228 if not (self.is_managed and (self.static_ip_range_low and
229 self.static_ip_range_high)):229 self.static_ip_range_high)):
230 errors = {}230 # Exit early with nothing to do.
231 ip_range_low = IPAddress(self.ip_range_low)231 return
232 ip_range_high = IPAddress(self.ip_range_high)232
233 static_ip_range_low = IPAddress(self.static_ip_range_low)233 errors = {}
234 static_ip_range_high = IPAddress(self.static_ip_range_high)234 ip_range_low = IPAddress(self.ip_range_low)
235235 ip_range_high = IPAddress(self.ip_range_high)
236 message_base = (236 static_ip_range_low = IPAddress(self.static_ip_range_low)
237 "Lower bound %s is higher than upper bound %s")237 static_ip_range_high = IPAddress(self.static_ip_range_high)
238 try:238
239 IPRange(static_ip_range_low, static_ip_range_high)239 message_base = (
240 except AddrFormatError:240 "Lower bound %s is higher than upper bound %s")
241 message = (241 try:
242 message_base % (242 IPRange(static_ip_range_low, static_ip_range_high)
243 self.static_ip_range_low, self.static_ip_range_high))243 except AddrFormatError:
244 errors['static_ip_range_low'] = [message]244 message = (
245 errors['static_ip_range_high'] = [message]245 message_base % (
246 try:246 self.static_ip_range_low, self.static_ip_range_high))
247 IPRange(ip_range_low, ip_range_high)247 errors['static_ip_range_low'] = [message]
248 except AddrFormatError:248 errors['static_ip_range_high'] = [message]
249 message = (249 try:
250 message_base % (self.ip_range_low, self.ip_range_high))250 IPRange(ip_range_low, ip_range_high)
251 errors['ip_range_low'] = [message]251 except AddrFormatError:
252 errors['ip_range_high'] = [message]252 message = (
253253 message_base % (self.ip_range_low, self.ip_range_high))
254 if errors:254 errors['ip_range_low'] = [message]
255 raise ValidationError(errors)255 errors['ip_range_high'] = [message]
256
257 if errors:
258 raise ValidationError(errors)
256259
257 def clean_ip_ranges(self):260 def clean_ip_ranges(self):
258 """Ensure that the static and dynamic ranges don't overlap."""261 """Ensure that the static and dynamic ranges don't overlap."""
259 if (self.is_managed and (self.static_ip_range_low and262 if not (self.is_managed and (self.static_ip_range_low and
260 self.static_ip_range_high)):263 self.static_ip_range_high)):
261 errors = {}264 # Nothing to do; bail out.
262 ip_range_low = IPAddress(self.ip_range_low)265 return
263 ip_range_high = IPAddress(self.ip_range_high)266
264 static_ip_range_low = IPAddress(self.static_ip_range_low)267 errors = {}
265 static_ip_range_high = IPAddress(self.static_ip_range_high)268 ip_range_low = IPAddress(self.ip_range_low)
266269 ip_range_high = IPAddress(self.ip_range_high)
267 static_range = IPRange(270 static_ip_range_low = IPAddress(self.static_ip_range_low)
268 static_ip_range_low, static_ip_range_high)271 static_ip_range_high = IPAddress(self.static_ip_range_high)
269 dynamic_range = IPRange(272
270 ip_range_low, ip_range_high)273 static_range = IPRange(
271274 static_ip_range_low, static_ip_range_high)
272 # This is a bit unattractive, but we can't use IPSet for275 dynamic_range = IPRange(
273 # large networks - it's far too slow. What we actually care276 ip_range_low, ip_range_high)
274 # about is whether the lows and highs of the static range277
275 # fall within the dynamic range and vice-versa, which278 # This is a bit unattractive, but we can't use IPSet for
276 # IPRange gives us.279 # large networks - it's far too slow. What we actually care
277 networks_overlap = (280 # about is whether the lows and highs of the static range
278 static_ip_range_low in dynamic_range or281 # fall within the dynamic range and vice-versa, which
279 static_ip_range_high in dynamic_range or282 # IPRange gives us.
280 ip_range_low in static_range or283 networks_overlap = (
281 ip_range_high in static_range284 static_ip_range_low in dynamic_range or
282 )285 static_ip_range_high in dynamic_range or
283 if networks_overlap:286 ip_range_low in static_range or
284 message = "Static and dynamic IP ranges may not overlap."287 ip_range_high in static_range
285 errors = {288 )
286 'ip_range_low': [message],289 if networks_overlap:
287 'ip_range_high': [message],290 message = "Static and dynamic IP ranges may not overlap."
288 'static_ip_range_low': [message],291 errors = {
289 'static_ip_range_high': [message],292 'ip_range_low': [message],
290 }293 'ip_range_high': [message],
291 raise ValidationError(errors)294 'static_ip_range_low': [message],
295 'static_ip_range_high': [message],
296 }
297 raise ValidationError(errors)
292298
293 def clean_fields(self, *args, **kwargs):299 def clean_fields(self, *args, **kwargs):
294 super(NodeGroupInterface, self).clean_fields(*args, **kwargs)300 super(NodeGroupInterface, self).clean_fields(*args, **kwargs)
295301
=== modified file 'src/maasserver/models/tests/test_nodegroupinterface.py'
--- src/maasserver/models/tests/test_nodegroupinterface.py 2014-07-03 09:56:26 +0000
+++ src/maasserver/models/tests/test_nodegroupinterface.py 2014-07-03 17:00:27 +0000
@@ -28,7 +28,10 @@
28from maasserver.testing import reload_object28from maasserver.testing import reload_object
29from maasserver.testing.factory import factory29from maasserver.testing.factory import factory
30from maasserver.testing.testcase import MAASServerTestCase30from maasserver.testing.testcase import MAASServerTestCase
31from netaddr import IPNetwork31from netaddr import (
32 IPAddress,
33 IPNetwork,
34 )
32from provisioningserver.utils.network import make_network35from provisioningserver.utils.network import make_network
3336
3437
@@ -237,11 +240,14 @@
237 def test_clean_ip_ranges_works_with_ipv6_ranges(self):240 def test_clean_ip_ranges_works_with_ipv6_ranges(self):
238 network = factory.get_random_ipv6_network()241 network = factory.get_random_ipv6_network()
239 interface = make_interface(network)242 interface = make_interface(network)
240 dynamic_low, dynamic_high = factory.make_ipv6_range(network)243 interface.ip_range_low = unicode(
241 interface.ip_range_low = unicode(dynamic_low)244 IPAddress(network.first))
242 interface.ip_range_high = unicode(dynamic_high)245 interface.ip_range_high = unicode(
243 interface.static_ip_range_low = unicode(dynamic_low + 1)246 IPAddress(network.last))
244 interface.static_ip_range_high = unicode(dynamic_high - 1)247 interface.static_ip_range_low = unicode(
248 IPAddress(network.first + 1))
249 interface.static_ip_range_high = unicode(
250 IPAddress(network.last - 1))
245 exception = self.assertRaises(251 exception = self.assertRaises(
246 ValidationError, interface.full_clean)252 ValidationError, interface.full_clean)
247 message = "Static and dynamic IP ranges may not overlap."253 message = "Static and dynamic IP ranges may not overlap."