Fixes #22273: Fix migration failure when a service has thousands of ports defined

netbox/ipam/graphql/types.py

@@ -244,7 +244,7 @@ class RouteTargetType(PrimaryObjectType):
 
 @strawberry_django.type(
     models.Service,
-    exclude=('parent_object_type', 'parent_object_id'),
+    exclude=('_ports_lowest', 'parent_object_type', 'parent_object_id'),
     filters=ServiceFilter,
     pagination=True
 )
@@ -264,7 +264,7 @@ class ServiceType(ContactsMixin, PrimaryObjectType):
 
 @strawberry_django.type(
     models.ServiceTemplate,
-    fields='__all__',
+    exclude=('_ports_lowest',),
     filters=ServiceTemplateFilter,
     pagination=True
 )

netbox/ipam/migrations/0089_default_ordering_indexes.py

@@ -32,9 +32,12 @@ class Migration(migrations.Migration):
             model_name='role',
             index=models.Index(fields=['weight', 'name'], name='ipam_role_weight_01396b_idx'),
         ),
+        # Adding a dummy index, to allow a safe migration in case updating users already have services
+        # with a large number of ports configured (see issue #22273)
+        # Will get removed in 0091_alter_service_index_and_ordering
         migrations.AddIndex(
             model_name='service',
-            index=models.Index(fields=['protocol', 'ports', 'id'], name='ipam_servic_protoco_687d13_idx'),
+            index=models.Index(fields=['id'], name='ipam_servic_protoco_687d13_idx'),
         ),
         migrations.AddIndex(
             model_name='vlangroup',

netbox/ipam/migrations/0091_alter_service_index_and_ordering.py

@@ -0,0 +1,58 @@
+from django.db import migrations, models
+
+
+def populate__ports_lowest(apps, schema_editor):
+    Service = apps.get_model('ipam', 'Service')
+    ServiceTemplate = apps.get_model('ipam', 'ServiceTemplate')
+    CHUNK_SIZE = 500
+
+    for model in (Service, ServiceTemplate):
+        chunk = []
+        qs = model.objects.filter(_ports_lowest__isnull=True).only('id', 'ports', '_ports_lowest')
+        for obj in qs.iterator(chunk_size=CHUNK_SIZE):
+            if obj.ports:
+                obj._ports_lowest = min(obj.ports)
+                chunk.append(obj)
+            if len(chunk) >= CHUNK_SIZE:
+                model.objects.bulk_update(chunk, ['_ports_lowest'])
+                chunk = []
+        if chunk:
+            model.objects.bulk_update(chunk, ['_ports_lowest'])
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('ipam', '0090_vlangroup_recompute_total_vlan_ids'),
+    ]
+
+    operations = [
+        migrations.RemoveIndex(
+            model_name='service',
+            name='ipam_servic_protoco_687d13_idx',
+        ),
+        migrations.AddField(
+            model_name='service',
+            name='_ports_lowest',
+            field=models.PositiveIntegerField(blank=True, null=True),
+        ),
+        migrations.AddField(
+            model_name='servicetemplate',
+            name='_ports_lowest',
+            field=models.PositiveIntegerField(blank=True, null=True),
+        ),
+        migrations.RunPython(populate__ports_lowest, migrations.RunPython.noop),
+        migrations.AddIndex(
+            model_name='service',
+            index=models.Index(
+                fields=['protocol', '_ports_lowest', 'id'],
+                name='ipam_servic_protoco_e2901d_idx'
+            ),
+        ),
+        migrations.AlterModelOptions(
+            name='service',
+            options={
+                'ordering': ('protocol', '_ports_lowest', 'id')
+            },
+        ),
+    ]

netbox/ipam/models/services.py

@@ -31,10 +31,22 @@ class ServiceBase(models.Model):
         ),
         verbose_name=_('port numbers')
     )
+    _ports_lowest = models.PositiveIntegerField(
+        null=True,
+        blank=True,
+    )
 
     class Meta:
         abstract = True
 
+    def save(self, *args, **kwargs):
+        # On saving find the smallest port and save for default ordering
+        self._ports_lowest = min(self.ports) if self.ports else None
+        update_fields = kwargs.get('update_fields')
+        if update_fields is not None and '_ports_lowest' not in update_fields:
+            kwargs['update_fields'] = list(update_fields) + ['_ports_lowest']
+        super().save(*args, **kwargs)
+
     def __str__(self):
         return f'{self.name} ({self.get_protocol_display()}/{self.port_list})'
 
@@ -74,7 +86,6 @@ class Service(ContactsMixin, ServiceBase, PrimaryModel):
         ct_field='parent_object_type',
         fk_field='parent_object_id'
     )
-
     name = models.CharField(
         max_length=100,
         verbose_name=_('name')
@@ -93,9 +104,9 @@ class Service(ContactsMixin, ServiceBase, PrimaryModel):
 
     class Meta:
         indexes = (
-            models.Index(fields=('protocol', 'ports', 'id')),  # Default ordering
+            models.Index(fields=('protocol', '_ports_lowest', 'id')),  # Default ordering
             models.Index(fields=('parent_object_type', 'parent_object_id')),
         )
-        ordering = ('protocol', 'ports', 'pk')  # (protocol, port) may be non-unique
+        ordering = ('protocol', '_ports_lowest', 'id')
         verbose_name = _('application service')
         verbose_name_plural = _('application services')

netbox/ipam/tests/test_models.py

@@ -6,8 +6,10 @@ from netaddr import IPNetwork, IPSet
 
 from dcim.models import Site, SiteGroup
 from ipam.choices import *
+from ipam.constants import SERVICE_PORT_MAX, SERVICE_PORT_MIN
 from ipam.models import *
 from utilities.data import string_to_ranges
+from virtualization.models import VirtualMachine
 
 
 class AggregateTestCase(TestCase):
@@ -926,3 +928,73 @@ class VLANTestCase(TestCase):
         vlan.group = vlangroups[2]
         with self.assertRaises(ValidationError):
             vlan.full_clean()
+
+
+class ServiceTemplateTestCase(TestCase):
+
+    def test_servicetemplate_lowest_port(self):
+        """
+        Test lowest port setting for servicetemplate
+        """
+        template = ServiceTemplate(
+            name='Template 1',
+            protocol=ServiceProtocolChoices.PROTOCOL_TCP,
+            ports=[80, 443, 22, 8080],  # small test list
+        )
+        template.full_clean()
+        template.save()
+        self.assertEqual(template._ports_lowest, 22)
+
+    def test_servicetemplate_single_port(self):
+        """
+        Test with a single port
+        """
+        template = ServiceTemplate(
+            name='Template 2',
+            protocol=ServiceProtocolChoices.PROTOCOL_UDP,
+            ports=[53],
+        )
+        template.full_clean()
+        template.save()
+        self.assertEqual(template._ports_lowest, 53)
+
+    def test_servicetemplate_empty_ports(self):
+        """
+        Test with empty ports list
+        """
+        template = ServiceTemplate(
+            name='Template 3',
+            protocol=ServiceProtocolChoices.PROTOCOL_TCP,
+            ports=[],
+        )
+        self.assertRaises(ValidationError, template.full_clean)
+
+
+class ServiceTestCase(TestCase):
+
+    @classmethod
+    def setUpTestData(cls):
+        site = Site.objects.create(
+            name='Site 1',
+            slug='site-1',
+        )
+        VirtualMachine.objects.create(
+            name='virtual machine 1',
+            site=site,
+        )
+
+    def test_large_service(self):
+        """
+        Test creation of service with large number of ports.
+        Related to issue #22273
+        """
+        service = Service(
+            name='Service 1',
+            protocol=ServiceProtocolChoices.PROTOCOL_TCP,
+            ports=list(range(SERVICE_PORT_MIN, SERVICE_PORT_MAX)),
+            parent=VirtualMachine.objects.first(),
+        )
+        service.full_clean()
+        # Testing .save() is the important part, to check for database problems
+        service.save()
+        self.assertEqual(service._ports_lowest, SERVICE_PORT_MIN)