Improve change logs aka history (#294)

Improve data model of change logs for objects.

Prior to this commit, we had 3 tables, one for objects created, 
one for objects deleted and another one for changes to objects.

This caused complexity inside the ORM layer and inefficient queries at times.

The idea of this change is to include all kind of changes in just a single 
table to reduce said complexity and perform optimizations across all 3 levels.

.github/workflows/tests.yml

@@ -87,7 +87,8 @@ jobs:
               },
           }
           SECRET_KEY = 'TEST'
-          LOGGING = {}" > serveradmin/local_settings.py
+          TIME_ZONE = 'Europe/Berlin'
+          LOGGING = {}" > serveradmin/local_settings.py          
           cat serveradmin/local_settings.py
           pipenv run python -m serveradmin migrate
       - name: Tests

Pipfile

@@ -25,6 +25,7 @@ django_compressor = "<3.0.0"
 # Serveradmin extras:
 paramiko = "~=2.10"
 pexpect = "<5.0.0"
+rich = "*"
 
 [dev-packages]
 # Used to build the sphinx docs

serveradmin/resources/views.py

@@ -122,14 +122,14 @@ def index(request):
         if page > hosts_pager.num_pages:
             page = 1
 
-        hosts = hosts_pager.page(page)
+        hosts_page = hosts_pager.page(page)
     except (PageNotAnInteger, EmptyPage):
         raise SuspiciousOperation('{} is not a valid!'.format(page))
 
     sprite_url = settings.MEDIA_URL + 'graph_sprite/' + collection.name
     template_info.update({
         'columns': columns,
-        'hosts': hosts,
+        'hosts': hosts_page,
         'page': page,
         'per_page': per_page,
         'matched_hostnames': matched_hostnames,

serveradmin/serverdb/admin.py

@@ -17,7 +17,6 @@
     Server,
     ServerRelationAttribute,
     ServerStringAttribute,
-    ChangeDelete,
 )
 
 
@@ -107,4 +106,3 @@ def get_hovertext(self, obj):
 admin.site.register(Servertype, ServertypeAdmin)
 admin.site.register(Attribute, AttributeAdmin)
 admin.site.register(Server, ServerAdmin)
-admin.site.register(ChangeDelete)

serveradmin/serverdb/migrations/0011_create_change_table.py

@@ -0,0 +1,26 @@
+# Generated by Django 3.2.16 on 2022-10-24 13:37
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+import serveradmin
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('serverdb', '0010_delete_change'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='Change',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('object_id', models.IntegerField(db_index=True)),
+                ('change_type', models.CharField(choices=[('create', 'create'), ('change', 'change'), ('delete', 'delete')], max_length=6)),
+                ('change_json', models.JSONField(encoder=serveradmin.serverdb.models.Change.ChangeJSONEncoder)),
+                ('commit', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='serverdb.changecommit')),
+            ],
+        ),
+    ]

serveradmin/serverdb/migrations/0012_migrate_change_tables.py

@@ -0,0 +1,112 @@
+# Generated by Django 3.2.16 on 2022-10-24 13:39
+
+import math
+
+from django.db import migrations, transaction, connection
+from rich.progress import Progress
+
+BATCH_SIZE = 100000
+
+
+def migrate_change_add(apps, schema_editor):
+    change_add = apps.get_model('serverdb', 'ChangeAdd')
+    total = change_add.objects.count()
+    batches = math.ceil(total / BATCH_SIZE)
+
+    with Progress() as progress:
+        migration = progress.add_task("\t[green]Migrating ChangeAdd data...", total=batches)
+
+        with connection.cursor() as cursor:
+            while not progress.finished:
+                with transaction.atomic():
+                    cursor.execute("""
+                        WITH moved AS (
+                            DELETE FROM serverdb_changeadd
+                            WHERE id IN (SELECT id FROM serverdb_changeadd ORDER BY id DESC LIMIT %s FOR UPDATE)
+                            RETURNING 
+                                server_id as object_id,
+                                'create' as change_type,
+                                attributes_json::jsonb as change_json,
+                                commit_id
+                        )
+                        INSERT INTO serverdb_change (object_id, change_type, change_json, commit_id) 
+                        SELECT * FROM moved;
+                    """, [BATCH_SIZE])
+
+                progress.update(migration, advance=1)
+
+
+def migrate_change_delete(apps, schema_editor):
+    change_delete = apps.get_model('serverdb', 'ChangeDelete')
+    total = change_delete.objects.count()
+    batches = math.ceil(total / BATCH_SIZE)
+
+    with Progress() as progress:
+        migration = progress.add_task("\t[green]Migrating ChangeDelete data...", total=batches)
+
+        with connection.cursor() as cursor:
+            while not progress.finished:
+                with transaction.atomic():
+                    cursor.execute("""
+                        WITH moved AS (
+                            DELETE FROM serverdb_changedelete
+                            WHERE id IN (SELECT id FROM serverdb_changedelete ORDER BY id DESC LIMIT %s FOR UPDATE)
+                            RETURNING 
+                                server_id as object_id,
+                                'delete' as change_type,
+                                attributes_json::jsonb as change_json,
+                                commit_id
+                        )
+                        INSERT INTO serverdb_change (object_id, change_type, change_json, commit_id)
+                        SELECT * FROM moved;
+                    """, [BATCH_SIZE])
+
+                progress.update(migration, advance=1)
+
+
+def migrate_change_update(apps, schema_editor):
+    change_update = apps.get_model('serverdb', 'ChangeUpdate')
+    total = change_update.objects.count()
+    batches = math.ceil(total / BATCH_SIZE)
+
+    with Progress() as progress:
+        migration = progress.add_task("\t[green]Migrating ChangeUpdate data...", total=batches)
+
+        with connection.cursor() as cursor:
+            while not progress.finished:
+                with transaction.atomic():
+                    cursor.execute("""
+                        WITH moved AS (
+                            DELETE FROM serverdb_changeupdate
+                            WHERE id IN (SELECT id FROM serverdb_changeupdate ORDER BY id DESC LIMIT %s FOR UPDATE)
+                            RETURNING 
+                                server_id as object_id,
+                                'change' as change_type,
+                                updates_json::jsonb as change_json,
+                                commit_id
+                        )
+                        INSERT INTO serverdb_change (object_id, change_type, change_json, commit_id)
+                        SELECT * FROM moved;
+                    """, [BATCH_SIZE])
+
+                progress.update(migration, advance=1)
+
+
+class Migration(migrations.Migration):
+    atomic = False
+
+    dependencies = [
+        ('serverdb', '0011_create_change_table'),
+    ]
+
+    # The migration of the old table is in a dedicated migration to avoid
+    # running them together with the DDL statements which cause a full table
+    # lock.
+    #
+    # This migration can safely be aborted by pressing <CTRL>-<C> at any time.
+    # It can be continued any time by starting it again.
+    operations = [
+        migrations.RunPython(migrate_change_add),
+        migrations.RunPython(migrate_change_delete),
+        migrations.RunPython(migrate_change_update),
+    ]

serveradmin/serverdb/migrations/0013_change_index.py

@@ -0,0 +1,24 @@
+# Generated by Django 3.2.16 on 2022-11-03 16:12
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('serverdb', '0012_migrate_change_tables'),
+    ]
+
+    operations = [
+        # Apparently Django doesn't support creating indexes on jsonb keys yet
+        migrations.RunSQL(
+            """
+            CREATE INDEX IF NOT EXISTS 
+                serverdb_change_change_json_hostname
+            ON
+                serverdb_change ((change_json->'hostname'))
+            WHERE
+                change_type in ('create', 'delete');
+            """
+        ),
+    ]

serveradmin/serverdb/migrations/0014_delete_deprecated_change_models.py

@@ -0,0 +1,38 @@
+# Generated by Django 3.2.18 on 2023-03-15 13:11
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('serverdb', '0013_change_index'),
+    ]
+
+    operations = [
+        migrations.AlterUniqueTogether(
+            name='changedelete',
+            unique_together=None,
+        ),
+        migrations.RemoveField(
+            model_name='changedelete',
+            name='commit',
+        ),
+        migrations.AlterUniqueTogether(
+            name='changeupdate',
+            unique_together=None,
+        ),
+        migrations.RemoveField(
+            model_name='changeupdate',
+            name='commit',
+        ),
+        migrations.DeleteModel(
+            name='ChangeAdd',
+        ),
+        migrations.DeleteModel(
+            name='ChangeDelete',
+        ),
+        migrations.DeleteModel(
+            name='ChangeUpdate',
+        ),
+    ]

serveradmin/serverdb/models.py

@@ -9,10 +9,20 @@
 
 from typing import Union
 
+from django.core.serializers.json import DjangoJSONEncoder
 from django.db.models import Q
 from netaddr import EUI
 from distutils.util import strtobool
-from ipaddress import ip_network, IPv4Interface, IPv6Interface, ip_interface
+from ipaddress import (
+    IPv4Address,
+    IPv6Address,
+    ip_interface,
+    IPv4Interface,
+    IPv6Interface,
+    ip_network,
+    IPv4Network,
+    IPv6Network,
+)
 
 from django.contrib.auth.models import User
 from django.core.exceptions import ValidationError
@@ -737,9 +747,9 @@ class Meta:
 
 
 class ChangeCommit(models.Model):
-    change_on = models.DateTimeField(default=now, db_index=True)
     user = models.ForeignKey(User, null=True, on_delete=models.PROTECT)
     app = models.ForeignKey(Application, null=True, on_delete=models.PROTECT)
+    change_on = models.DateTimeField(default=now, db_index=True)
 
     class Meta:
         app_label = 'serverdb'
@@ -748,52 +758,65 @@ def __str__(self):
         return str(self.change_on)
 
 
-class ChangeDelete(models.Model):
-    commit = models.ForeignKey(ChangeCommit, on_delete=models.CASCADE)
-    server_id = models.IntegerField(db_index=True)
-    attributes_json = models.TextField()
-
-    class Meta:
-        app_label = 'serverdb'
-        unique_together = [['commit', 'server_id']]
-
-    @property
-    def attributes(self):
-        return json.loads(self.attributes_json)
-
-    def __str__(self):
-        return '{0}: {1}'.format(str(self.commit), self.server_id)
+class Change(models.Model):
+    class Type(models.TextChoices):
+        CREATE = 'create', _('create')
+        CHANGE = 'change', _('change')
+        DELETE = 'delete', _('delete')
 
+    class ChangeJSONEncoder(DjangoJSONEncoder):
+        _NETWORK_TYPES = (
+            IPv4Address,
+            IPv6Address,
+            IPv4Network,
+            IPv6Network,
+            EUI,
+        )
 
-class ChangeUpdate(models.Model):
+        # This is close to json_encode_extra used in adminapi.request but with
+        # two differences. First we don't need to take care about BaseFilter
+        # objects as they are already "resolved". Second DjangoJSONEncoder
+        # handles datetime on its own. Third we don't blindly cast everything
+        # else to string but explicitly check for the types we now and fail
+        # for the others.
+        def default(self, obj):
+            # Handles ServerInetAttribute and ServerMACAddressAttribute values
+            if isinstance(obj, self._NETWORK_TYPES):
+                return str(obj)
+            # Handles MultiAttr values
+            elif isinstance(obj, set):
+                return list(obj)
+
+            return super().default(obj)
+
+    object_id = models.IntegerField(db_index=True)
+    # XXX: Add migration with PostgreSQL native enum
+    change_type = models.CharField(choices=Type.choices, max_length=6)
+    change_json = models.JSONField(encoder=ChangeJSONEncoder)
     commit = models.ForeignKey(ChangeCommit, on_delete=models.CASCADE)
-    server_id = models.IntegerField(db_index=True)
-    updates_json = models.TextField()
 
     class Meta:
         app_label = 'serverdb'
-        unique_together = [['commit', 'server_id']]
 
-    @property
-    def updates(self):
-        return json.loads(self.updates_json)
-
-    def __str__(self):
-        return '{0}: {1}'.format(str(self.commit), self.server_id)
+    def hostname(self):
+        """Get last hostname for object of Change
 
+        Get the current hostname if the object still exists or the last known
+        one when deleted.
 
-class ChangeAdd(models.Model):
-    commit = models.ForeignKey(ChangeCommit, on_delete=models.CASCADE)
-    server_id = models.IntegerField(db_index=True)
-    attributes_json = models.TextField()
+        :return:
+        """
 
-    class Meta:
-        app_label = 'serverdb'
-        unique_together = [['commit', 'server_id']]
-
-    @property
-    def attributes(self):
-        return json.loads(self.attributes_json)
-
-    def __str__(self):
-        return '{0}: {1}'.format(str(self.commit), self.server_id)
+        if self.change_type == Change.Type.DELETE:
+            return self.change_json['hostname']
+        else:
+            try:
+                return Server.objects.filter(
+                    pk=self.object_id).only('hostname').get().hostname
+            except Server.DoesNotExist:
+                # We seem to have re-used the same object_id in the past in
+                # some exceptions - in such as case just pick the latest.
+                return Change.objects.filter(
+                    object_id=self.object_id,
+                    change_type=Change.Type.DELETE
+                ).order_by('-id').first().change_json['hostname']