Merge lp:~jtv/maas/zero-vlan-tag-is-not-unique into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Zero VLAN tag should not be unique.

We have a "unique" constraint on Network.vlan_tag, but this should not apply to the zero tag! Django does not support conditional unique constraints (except in model code, which can't detect paradoxes that may occur at lower database isolation levels) so there was only one thing for it. Allow the field to be null, and normalise zero to None. I thought I was able to avoid this, but forgot about the database constraint. :(

Jeroen

[Julian Edwards (julian-edwards)]

Looks great, I especially like the thought put into the random tag in the factory func.

Can I persuade you to go and edit the old migration though please, having multiple migrations for the same new model is ugly, embarrassing and inefficient. It's not been released to trusty yet so it's fine.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks. I've updated the migration. I was more cautious about this after the recent incident where a release unbeknownst to us hit juuuust the wrong migration number.

=== modified file 'src/maasserver/migrations/0066_rename_vlan_add_link_node_network.py'
--- src/maasserver/migrations/0066_rename_vlan_add_link_node_network.py	2014-02-04 09:08:13 +0000
+++ src/maasserver/migrations/0066_rename_vlan_add_link_node_network.py	2014-02-06 04:08:29 +0000
@@ -1,9 +1,8 @@
 # -*- coding: utf-8 -*-
-import datetime
-
-from django.db import models
+from south.utils import datetime_utils as datetime
 from south.db import db
 from south.v2 import SchemaMigration
+from django.db import models
 
 
 class Migration(SchemaMigration):
@@ -18,7 +17,7 @@
             ('name', self.gf('django.db.models.fields.CharField')(unique=True, max_length=255)),
             ('ip', self.gf('django.db.models.fields.GenericIPAddressField')(unique=True, max_length=39)),
             ('netmask', self.gf('django.db.models.fields.GenericIPAddressField')(max_length=39)),
-            ('vlan_tag', self.gf('django.db.models.fields.PositiveSmallIntegerField')(unique=True)),
+            ('vlan_tag', self.gf('django.db.models.fields.PositiveSmallIntegerField')(unique=True, null=True, blank=True)),
             ('description', self.gf('django.db.models.fields.CharField')(default=u'', max_length=255, blank=True)),
         ))
         db.send_create_signal(u'maasserver', ['Network'])
@@ -67,7 +66,7 @@
             'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
             'email': ('django.db.models.fields.EmailField', [], {'unique': 'True', 'max_length': '75', 'blank': 'True'}),
             'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
-            'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': u"orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}),
+            'groups': ('django.db.models.fields.related.ManyToManyField', [], {'symmetrical': 'False', 'related_name': "u'user_set'", 'blank': 'True', 'to': u"orm['auth.Group']"}),
             u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
             'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
             'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
@@ -75,7 +74,7 @@
             'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
             'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
             'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}),
-            'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': u"orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}),
+            'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'symmetrical': 'False', 'related_name': "u'user_set'", 'blank': 'True', 'to': u"orm['auth.Permission']"}),
             'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'})
         },
         u'contenttypes.contenttype': {
@@ -131,7 +130,7 @@
             'content': ('metadataserver.fields.BinaryField', [], {'blank': 'True'}),
             'filename': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
             u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
-            'key': ('django.db.models.fields.CharField', [], {'default': "u'c9791474-8d7b-11e3-8453-9c4e363b1c94'", 'unique': 'True', 'max_length': '36'}),
+            'key': ('django.db.models.fields.CharField', [], {'default': "u'1cef8bac-8ee3-11e3-8252-94de80b61466'", 'unique': 'True', 'max_length': '36'}),
             'owner': ('django.db.models.fields.related.ForeignKey', [], {'default': 'None', 'to': u"orm['auth.User']", 'null': 'True', 'blank': 'True'})
         },
         u'maasserver.macaddress': {
@@ -149,7 +148,7 @@
             'ip': ('django.db.models.fields.GenericIPAddressField', [], {'unique': 'True', 'max_length': '39'}),
             'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '255'}),
             'netmask': ('django.db.models.fields.GenericIPAddressField', [], {'max_length': '39'}),
-            'vlan_tag': ('django.db.models.fields.PositiveSmallIntegerField', [], {'unique': 'True'})
+            'vlan_tag': ('django.db.models.fields.PositiveSmallIntegerField', [], {'unique': 'True', 'null': 'True', 'blank': 'True'})
         },
         u'maasserver.node': {
             'Meta': {'object_name': 'Node'},
@@ -172,7 +171,7 @@
             'routers': ('djorm_pgarray.fields.ArrayField', [], {'default': 'None', 'dbtype': "u'macaddr'", 'null': 'True', 'blank': 'True'}),
             'status': ('django.db.models.fields.IntegerField', [], {'default': '0', 'max_length': '10'}),
             'storage': ('django.db.models.fields.IntegerField', [], {'default': '0'}),
-            'system_id': ('django.db.models.fields.CharField', [], {'default': "u'node-c97b5392-8d7b-11e3-8453-9c4e363b1c94'", 'unique': 'True', 'max_length': '41'}),
+            'system_id': ('django.db.models.fields.CharField', [], {'default': "u'node-1cf030fc-8ee3-11e3-8252-94de80b61466'", 'unique': 'True', 'max_length': '41'}),
             'tags': ('django.db.models.fields.related.ManyToManyField', [], {'to': u"orm['maasserver.Tag']", 'symmetrical': 'False'}),
             'token': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['piston.Token']", 'null': 'True'}),
             'updated': ('django.db.models.fields.DateTimeField', [], {}),
@@ -258,7 +257,7 @@
             'is_approved': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
             'key': ('django.db.models.fields.CharField', [], {'max_length': '18'}),
             'secret': ('django.db.models.fields.CharField', [], {'max_length': '32'}),
-            'timestamp': ('django.db.models.fields.IntegerField', [], {'default': '1391504854L'}),
+            'timestamp': ('django.db.models.fields.IntegerField', [], {'default': '1391659184L'}),
             'token_type': ('django.db.models.fields.IntegerField', [], {}),
             'user': ('django.db.models.fields.related.ForeignKey', [], {'blank': 'True', 'related_name': "'tokens'", 'null': 'True', 'to': u"orm['auth.User']"}),
             'verifier': ('django.db.models.fields.CharField', [], {'max_length': '10'})
 
=== modified file 'src/maasserver/models/network.py'
--- src/maasserver/models/network.py	2014-02-05 14:12:38 +0000
+++ src/maasserver/models/network.py	2014-02-06 04:08:29 +0000
@@ -54,7 +54,7 @@
         help_text="Network mask (e.g. 255.255.255.0).")
 
     vlan_tag = PositiveSmallIntegerField(
-        editable=True, blank=False, unique=True,
+        editable=True, null=True, blank=True, unique=True,
         help_text="A 12-bit field specifying the VLAN to which the frame "
                   "belongs. The hexadecimal values of 0x000 and 0xFFF "
                   "are reserved. All other values may be used as VLAN "
@@ -85,13 +85,18 @@
 
     def __unicode__(self):
         net = unicode(self.get_network().cidr)
-        if self.vlan_tag == 0:
+        # A vlan_tag of zero normalises to None.  But __unicode__ may be
+        # called while we're not in a clean state, so handle zero as well.
+        if self.vlan_tag is None or self.vlan_tag == 0:
             return net
         else:
             return "%s(tag:%x)" % (net, self.vlan_tag)
 
     def clean_vlan_tag(self):
         """Validator for `vlan_tag`."""
+        if self.vlan_tag is None:
+            # Always OK.
+            return
         if self.vlan_tag == 0xFFF:
             raise ValidationError(
                 {'tag': ["Cannot use reserved value 0xFFF."]})
@@ -133,6 +138,9 @@
             raise ValidationError("Invalid network address: %s" % e)
         # Normalise self.ip.  This strips off any host bits from the address.
         self.ip = unicode(net.cidr.ip)
+        # Normalise self.vlan_tag.  A zero value ("not a VLAN") becomes None.
+        if self.vlan_tag == 0:
+            self.vlan_tag = None
 
     def validate_unique(self, exclude=None):
         super(Network, self).validate_unique(exclude=exclude)
 
=== modified file 'src/maasserver/models/tests/test_network.py'
--- src/maasserver/models/tests/test_network.py	2014-02-05 10:46:49 +0000
+++ src/maasserver/models/tests/test_network.py	2014-02-06 04:08:29 +0000
@@ -14,9 +14,11 @@
 __metaclass__ = type
 __all__ = []
 
+from operator import attrgetter
 from random import randint
 
 from django.core.exceptions import ValidationError
+from maasserver.models import Network
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from netaddr import IPNetwork
@@ -52,11 +54,10 @@
         self.assertEqual('10.9.0.0', network.ip)
 
     def test_vlan_tag_can_be_zero_through_hex_ffe(self):
-        min_tag = 0
+        self.assertIsNone(factory.make_network(vlan_tag=0).vlan_tag)
+        self.assertEqual(1, factory.make_network(vlan_tag=1).vlan_tag)
         max_tag = 0xfff - 1
         self.assertEqual(
-            min_tag, factory.make_network(vlan_tag=min_tag).vlan_tag)
-        self.assertEqual(
             max_tag, factory.make_network(vlan_tag=max_tag).vlan_tag)
 
     def test_reserved_vlan_tag_does_not_validate(self):
@@ -69,6 +70,20 @@
         self.assertRaises(
             ValidationError, factory.make_network, vlan_tag=-1)
 
+    def test_vlan_tag_normalises_zero_to_None(self):
+        self.assertIsNone(factory.make_network(vlan_tag=0).vlan_tag)
+
+    def test_nonzero_vlan_tag_is_unique(self):
+        tag = randint(1, 0xffe)
+        factory.make_network(vlan_tag=tag)
+        self.assertRaises(ValidationError, factory.make_network, vlan_tag=tag)
+
+    def test_zero_vlan_tag_is_not_unique(self):
+        networks = [factory.make_network(vlan_tag=0) for _ in range(3)]
+        self.assertEqual(
+            sorted(networks, key=attrgetter('id')),
+            list(Network.objects.filter(vlan_tag=None).order_by('id')))
+
     def test_get_network_returns_network(self):
         net = factory.getRandomNetwork()
         self.assertEqual(net, factory.make_network(network=net).get_network())
@@ -112,11 +127,20 @@
         network = factory.make_network(network=IPNetwork(cidr), vlan_tag=0)
         self.assertEqual(cidr, unicode(network))
 
-    def test_unicode_includes_tag_if_nonzero(self):
+    def test_unicode_includes_tag_if_set(self):
         cidr = '10.9.0.0/16'
         network = factory.make_network(network=IPNetwork(cidr), vlan_tag=0xabc)
         self.assertEqual("%s(tag:abc)" % cidr, unicode(network))
 
+    def test_unicode_treats_unclean_zero_tag_as_unset(self):
+        net = IPNetwork('10.1.1.0/24')
+        network = factory.make_network(network=net)
+        network.vlan_tag = None
+        proper_unicode = unicode(network)
+        network.vlan_tag = 0
+        unclean_unicode = unicode(network)
+        self.assertEqual(proper_unicode, unclean_unicode)
+
     def test_disallows_identical_networks_with_same_netmask(self):
         existing_network = factory.make_network()
         self.assertRaises(
 
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2014-02-04 17:28:30 +0000
+++ src/maasserver/testing/factory.py	2014-02-06 04:08:29 +0000
@@ -543,6 +543,9 @@
 
         :param network: An `IPNetwork`.  If given, the `ip` and `netmask`
             fields will be taken from this.
+        :param vlan_tag: A number between 1 and 0xffe inclusive to create a
+            VLAN, or 0 to create a non-VLAN network, or None to make a random
+            choice.
         """
         if name is None:
             name = factory.make_name()
@@ -553,7 +556,13 @@
         if description is None:
             description = self.getRandomString()
         if vlan_tag is None:
-            vlan_tag = random.randint(0, 0xFFE)
+            # Caller wants it random.  To avoid hiding once-in-4094 bugs when
+            # it comes out zero, skew the odds in favour of zero: fifty-fifty
+            # choice between a zero and a nonzero tag.
+            if random.randint(0, 1) == 1:
+                vlan_tag = random.randint(1, 0xFFE)
+            else:
+                vlan_tag = 0
         network = Network(
             name=name, ip=ip, netmask=netmask, vlan_tag=vlan_tag,
             description=description)