Merge ~r00ta/maas:lp-2058377-fix-ha-architectures into maas:master

[Jacopo Rota]

fix: lp-2058377. Fix get_hwe_kernels, get_latest_complete_set queries.

Currently, the query

resource_sets = self.sets.order_by("-id").annotate(
   files_count=Count("files__id", distinct=True),
   files_size=Sum("files__size"),
   sync_size=Sum("files__bootresourcefilesync__size"),
   )

is calculating the wrong sum for the files size because the `Sum("files__bootresourcefilesync__size")` is performing a FULL OUTER JOIN on the bootresourcefilesync set, ending up with duplicated rows and duplicating the files_size by consequence.

The original query in 3.4 was

>>> print(b.sets.order_by("-id").annotate(
... files_count=Count("files__id"),
... files_size=Sum("files__largefile__size"),
... files_total_size=Sum("files__largefile__total_size"),
... ).query)

SELECT
  "maasserver_bootresourceset"."id",
  "maasserver_bootresourceset"."created",
  "maasserver_bootresourceset"."updated",
  "maasserver_bootresourceset"."resource_id",
  "maasserver_bootresourceset"."version",
  "maasserver_bootresourceset"."label",
  COUNT(
    "maasserver_bootresourcefile"."id"
  ) AS "files_count",
  SUM("maasserver_largefile"."size") AS "files_size",
  SUM(
    "maasserver_largefile"."total_size"
  ) AS "files_total_size"
FROM
  "maasserver_bootresourceset"
  LEFT OUTER JOIN "maasserver_bootresourcefile" ON (
    "maasserver_bootresourceset"."id" = "maasserver_bootresourcefile"."resource_set_id"
  )
  LEFT OUTER JOIN "maasserver_largefile" ON (
    "maasserver_bootresourcefile"."largefile_id" = "maasserver_largefile"."id"
  )
WHERE
  "maasserver_bootresourceset"."resource_id" = 9
GROUP BY
  "maasserver_bootresourceset"."id"
ORDER BY
  "maasserver_bootresourceset"."id" DESC

and in the case of multiple resource sets it would have produced (on a 3.4 env)

 id | created | updated | resource_id | version | label | files_count | files_size | files_total_size
----+-------------------------------+-------------------------------+-------------+----------+-----------+-------------+------------+------------------
 21 | 2024-03-20 09:22:53.089051+00 | 2024-03-20 09:22:53.109408+00 | 9 | 20240319 | candidate | 3 | 11563368 | 592121637
 15 | 2024-03-20 06:43:25.761506+00 | 2024-03-20 08:58:58.392482+00 | 9 | 20240318 | stable | 3 | 592011128 | 592011128

The proposed query leverages on a subquery to sum the files sizes in the resource set using a subquery. The resulting query is

 SELECT
  "maasserver_bootresourceset"."id",
  "maasserver_bootresourceset"."created",
  "maasserver_bootresourceset"."updated",
  "maasserver_bootresourceset"."resource_id",
  "maasserver_bootresourceset"."version",
  "maasserver_bootresourceset"."label",
  COUNT(
    DISTINCT "maasserver_bootresourcefile"."id"
  ) AS "files_count",
  (
    SELECT
      SUM(U0."size") AS "group_size"
    FROM
      "maasserver_bootresourcefile" U0
    WHERE
      U0."resource_set_id" = "maasserver_bootresourceset"."id"
    GROUP BY
      U0."resource_set_id"
    LIMIT
      1
  ) AS "files_size",
  SUM(
    "maasserver_bootresourcefilesync"."size"
  ) AS "sync_size"
FROM
  "maasserver_bootresourceset"
  LEFT OUTER JOIN "maasserver_bootresourcefile" ON (
    "maasserver_bootresourceset"."id" = "maasserver_bootresourcefile"."resource_set_id"
  )
  LEFT OUTER JOIN "maasserver_bootresourcefilesync" ON (
    "maasserver_bootresourcefile"."id" = "maasserver_bootresourcefilesync"."file_id"
  )
WHERE
  "maasserver_bootresourceset"."resource_id" = 9
GROUP BY
  "maasserver_bootresourceset"."id"
ORDER BY
  "maasserver_bootresourceset"."id" DESC

and it would produce (on a 3.5 env)

 id | created | updated | resource_id | version | label | files_count | files_size | sync_size
----+-------------------------------+-------------------------------+-------------+----------+-----------+-------------+------------+------------
 20 | 2024-03-20 09:27:41.751309+00 | 2024-03-20 09:27:41.770736+00 | 8 | 20240318 | stable | 3 | 592105406 | 11528040
 14 | 2024-03-20 08:04:51.163702+00 | 2024-03-20 09:27:01.581783+00 | 8 | 20240319 | candidate | 3 | 592215851 | 1184431702
(2 rows)

[Alexsander de Souza (alexsander-souza)]

+1

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp-2058377-fix-ha-architectures lp:~r00ta/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: fab53ebca4a870a481dc772288823122ddf38a61

review: Approve

diff --git a/src/maasserver/models/bootresource.py b/src/maasserver/models/bootresource.py
index 7d46aae..a5a94f6 100644
--- a/src/maasserver/models/bootresource.py
+++ b/src/maasserver/models/bootresource.py
@@ -14,7 +14,9 @@ from django.db.models import (
     IntegerField,
     JSONField,
     Manager,
+    OuterRef,
     Prefetch,
+    Subquery,
     Sum,
 )
 
@@ -200,6 +202,7 @@ class BootResourceManager(Manager):
         UPD: because of 3.4 transition from "subarches" to "platforms",
         expect some confusion ahead.
         """
+        from maasserver.models.bootresourcefile import BootResourceFile
         from maasserver.utils.osystems import (
             get_release_version_from_string,
             parse_subarch_kernel_string,
@@ -210,9 +213,17 @@ class BootResourceManager(Manager):
         if not architecture:
             architecture = ""
 
+        # In order to calculate the sum of the bootresourcefilesync we have to FULL OUTER JOIN the tables and the rows are
+        # duplicated: this is why we need a subquery to calculate the total files size.
+        subquery = (
+            BootResourceFile.objects.filter(resource_set_id=OuterRef("pk"))
+            .values("resource_set_id")
+            .annotate(group_size=Sum("size"))
+            .values("group_size")[:1]
+        )
         sets_prefetch = BootResourceSet.objects.annotate(
             files_count=Count("files__id", distinct=True),
-            files_size=Sum("files__size"),
+            files_size=Subquery(subquery),
             sync_size=Sum("files__bootresourcefilesync__size"),
         )
         sets_prefetch = sets_prefetch.prefetch_related("files")
@@ -571,15 +582,24 @@ class BootResource(CleanSave, TimestampedModel):
     def get_latest_complete_set(self):
         """Return latest `BootResourceSet` where all `BootResouceFile`'s
         are complete."""
+        from maasserver.models.bootresourcefile import BootResourceFile
         from maasserver.models.node import RegionController
 
         if (
             not hasattr(self, "_prefetched_objects_cache")
             or "sets" not in self._prefetched_objects_cache
         ):
+            # In order to calculate the sum of the bootresourcefilesync we have to FULL OUTER JOIN the tables and the rows are
+            # duplicated: this is why we need a subquery to calculate the total files size.
+            subquery = (
+                BootResourceFile.objects.filter(resource_set_id=OuterRef("pk"))
+                .values("resource_set_id")
+                .annotate(group_size=Sum("size"))
+                .values("group_size")[:1]
+            )
             resource_sets = self.sets.order_by("-id").annotate(
                 files_count=Count("files__id", distinct=True),
-                files_size=Sum("files__size"),
+                files_size=Subquery(subquery),
                 sync_size=Sum("files__bootresourcefilesync__size"),
             )
         else:
diff --git a/src/maasserver/testing/factory.py b/src/maasserver/testing/factory.py
index 8ddeb37..71d7ced 100644
--- a/src/maasserver/testing/factory.py
+++ b/src/maasserver/testing/factory.py
@@ -2490,10 +2490,10 @@ class Factory(maastesting.factory.Factory):
         )
 
         if synced:
-            for st in synced:
-                sync_size = rfile.size if st[1] < 0 else st[1]
+            for region_controller, size in synced:
+                sync_size = rfile.size if size < 0 else size
                 rfile.bootresourcefilesync_set.create(
-                    region=st[0], size=sync_size
+                    region=region_controller, size=sync_size
                 )
 
         return rfile
diff --git a/src/maasserver/tests/test_bootresources.py b/src/maasserver/tests/test_bootresources.py
index 8a2ae38..b9e5902 100644
--- a/src/maasserver/tests/test_bootresources.py
+++ b/src/maasserver/tests/test_bootresources.py
@@ -715,6 +715,91 @@ class TestBootResourceTransactional(MAASTransactionServerTestCase):
             logger.output,
         )
 
+    def test_get_latest_complete_set_not_in_sync_between_regions(self):
+        self.patch(bootresources.Event.objects, "create_region_event")
+        name, architecture, product = make_product()
+        second_region = factory.make_RegionController()
+        resource = factory.make_BootResource(
+            rtype=BOOT_RESOURCE_TYPE.SYNCED,
+            name=name,
+            architecture=architecture,
+            kflavor="generic",
+        )
+        resource_set = factory.make_BootResourceSet(
+            resource,
+            version=product["version_name"],
+        )
+        factory.make_boot_resource_file_with_content(
+            resource_set,
+            filename=product["ftype"],
+            filetype=product["ftype"],
+            synced=[
+                (self.region, -1),
+                (second_region, 0),
+            ],  # Not in sync between regions
+        )
+        self.assertIsNone(resource.get_latest_complete_set())
+
+    def test_get_latest_complete_set_has_completed(self):
+        self.patch(bootresources.Event.objects, "create_region_event")
+        name, architecture, product = make_product()
+        second_region = factory.make_RegionController()
+        resource = factory.make_BootResource(
+            rtype=BOOT_RESOURCE_TYPE.SYNCED,
+            name=name,
+            architecture=architecture,
+            kflavor="generic",
+        )
+        resource_set = factory.make_BootResourceSet(
+            resource,
+            version=product["version_name"],
+        )
+        factory.make_boot_resource_file_with_content(
+            resource_set,
+            filename=product["ftype"],
+            filetype=product["ftype"],
+            synced=[
+                (self.region, -1),
+                (second_region, -1),
+            ],  # In Sync between regions
+        )
+        self.assertEqual(resource_set, resource.get_latest_complete_set())
+
+    def test_get_latest_complete_multiple_sets_incomplete(self):
+        self.patch(bootresources.Event.objects, "create_region_event")
+        name, architecture, product = make_product()
+        second_region = factory.make_RegionController()
+        resource = factory.make_BootResource(
+            rtype=BOOT_RESOURCE_TYPE.SYNCED,
+            name=name,
+            architecture=architecture,
+            kflavor="generic",
+        )
+        resource_set = factory.make_BootResourceSet(
+            resource,
+            version=product["version_name"],
+        )
+        incomplete_resource_set = factory.make_BootResourceSet(
+            resource,
+            version="alpha",
+        )
+        factory.make_boot_resource_file_with_content(
+            resource_set,
+            filename=product["ftype"],
+            filetype=product["ftype"],
+            synced=[
+                (self.region, -1),
+                (second_region, -1),
+            ],  # This is the latest completed
+        )
+        factory.make_boot_resource_file_with_content(
+            incomplete_resource_set,
+            filename=product["ftype"],
+            filetype=product["ftype"],
+            synced=[(self.region, 0)],  # Not in sync yet
+        )
+        self.assertEqual(resource_set, resource.get_latest_complete_set())
+
     def test_insert_doesnt_print_error_when_first_import(self):
         name, architecture, product = make_product()
         with transaction.atomic():