sts-sponsors team mailing list archive

[Igor Brovtsin <mp+440659@xxxxxxxxxxxxxxxxxx>]

Igor Brovtsin has proposed merging ~igor-brovtsin/maas:dgx-kernel-string-parser into maas:master.

Commit message:
Improved kernel weight calculation in `get_release_version_from_string`
with more flexible subarch string parser.

The new parser does not use fixed indices, supports all the formats
we currently have (including the proper usage of "subarch" string) and
allows us to be flexible with subarch strings.

The updated weight calculation has the following rules:

- HWE-edge kernels weigh more than HWE kernels
- HWE kernels weigh more than GA kernels
  (regardless of HWE status)
- Platform-optimised kernels weigh more than generic ones
- Kernels with non-generic flavours weigh more than generic ones

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~igor-brovtsin/maas/+git/maas/+merge/440659

This is the first (and the simplest) part of DGX-related changes. Subsequent changes are dependent on this one though.
-- 
Your team MAAS Maintainers is requested to review the proposed merge of ~igor-brovtsin/maas:dgx-kernel-string-parser into maas:master.

diff --git a/src/maasserver/utils/osystems.py b/src/maasserver/utils/osystems.py
index 9b09303..3782bcb 100644
--- a/src/maasserver/utils/osystems.py
+++ b/src/maasserver/utils/osystems.py
@@ -16,6 +16,7 @@ __all__ = [
     "list_osystem_choices",
     "list_release_choices",
     "make_hwe_kernel_ui_text",
+    "parse_subarch_kernel_string",
     "release_a_newer_than_b",
     "validate_hwe_kernel",
 ]
@@ -151,12 +152,8 @@ def make_hwe_kernel_ui_text(hwe_kernel):
     if not hwe_kernel:
         return hwe_kernel
     # Fall back on getting it from DistroInfo.
-    kernel_list = hwe_kernel.split("-")
-    if len(kernel_list) >= 2:
-        kernel = kernel_list[1]
-    else:
-        kernel = hwe_kernel
-    ubuntu_release = get_release(kernel)
+    _, release, _, _ = parse_subarch_kernel_string(hwe_kernel)
+    ubuntu_release = get_release(release)
     if ubuntu_release is None:
         return hwe_kernel
     else:
@@ -403,6 +400,94 @@ def get_release(string):
     return release
 
 
+class InvalidSubarchKernelStringError(Exception):
+    """TODO"""
+
+
+def parse_subarch_kernel_string(string):
+    """Extracts meaningful info from the 3.4-ish subarch kernel string
+    with the following format:
+    ```
+        [platform-](ga|hwe[-edge])-(release)-[flavour-][edge]
+    ```
+
+    This format matches all the older kernel subarches, but also
+    allows having the platform specified. Examples of supported strings:
+      - `hwe-x`
+      - `ga-22.04`
+      - `hwe-22.04-lowlatency`
+      - `nvidia-ga-22.04`
+      - `raspi-hwe-edge-22.04-lowlatency`
+
+    :param string: kernel subarch string
+
+    FIXME: This is a workaround for 3.4, it is not intended to become
+      a long-term solution (unless we fail spectacularly with improved
+      kernel model)
+    """
+    # Kernel string is dash-separated in this format
+    parts = string.split("-")
+
+    if len(parts) == 1:
+        # Legacy format (v1 probably?): subarch is an actual subarch!
+        # (now called platform because of "subarch" being abused)
+        return "", "", string, ""
+
+    # Figure out the kernel channel
+    channel_index = None
+    channels_found = 0
+    for possible_channel in ("hwe", "ga"):
+        try:
+            channel_index = parts.index(possible_channel)
+            channels_found += 1
+        except ValueError:
+            # We expect ValueError
+            pass
+        else:
+            channel = possible_channel
+    if channels_found > 1:
+        raise InvalidSubarchKernelStringError(
+            f"Kernel {string} has multiple channels specified!"
+        )
+    if channel_index is None:
+        # Once again, subarch is an actual subarch! We don't do that
+        # anymore though.
+        return "", "", string, ""
+
+    # Everything before channel is considered as platform
+    platform = ""
+    if channel_index > 0:
+        platform = "-".join(parts[:channel_index])
+        parts = parts[channel_index:]
+
+    # HWE channel could be also "hwe-edge", let's check if that is the case
+    if channel == "hwe":
+        if len(parts) > 1 and parts[1] == "edge":
+            channel = "hwe-edge"
+            # Get rid of that extra element so that release will be
+            # at index 1
+            parts = parts[1:]
+        elif parts[-1] == "edge":
+            channel = "hwe-edge"
+            parts = parts[:-1]
+
+    # By this moment we should have release in parts[1] and flavour
+    # as the rest of the parts.
+    if len(parts) < 2:
+        raise ValueError(f"Kernel {string} has no release specified")
+    release = parts[1]
+    kflavor = "-".join(parts[2:])
+    return channel, release, platform, kflavor
+
+
+# Coefficients for release sorting:
+FLAVOURED_WEIGHT = 10
+NOT_OLD_HWE_WEIGHT = 100  # New format kernels should weigh more
+PLATFORM_WEIGHT = 1000
+HWE_CHANNEL_WEIGHT = 10000
+HWE_EDGE_CHANNEL_WEIGHT = 100000
+
+
 def get_release_version_from_string(string):
     """Convert an Ubuntu release, version, or kernel into a version tuple.
 
@@ -430,43 +515,44 @@ def get_release_version_from_string(string):
     """
     parts = string.split("-")
     parts_len = len(parts)
+    ubuntu_release = None
+    platform = ""
+    release = ""
+    kflavor = ""
     if parts_len == 1:
-        # Just the release name, e.g xenial or 16.04
-        release = string
-        weight = 0
-    elif parts_len == 2:
-        # hwe kernel, e.g hwe-x or hwe-16.04
-        release = parts[1]
+        # Just the release name, e.g xenial or 16.04 (always GA)
+        if string != "rolling":
+            ubuntu_release = get_release(string)
+            if ubuntu_release is None:
+                platform = string
+            else:
+                release = string
+        channel = "ga"
         weight = 0
-    elif parts_len == 3:
-        # hwe edge or lowlatency kernel,
-        # e.g hwe-16.04-edge or hwe-16.04-lowlatency
-        release = parts[1]
-        if parts[2] == "edge":
-            weight = 1
-        else:
-            weight = 0
-    elif parts_len == 4:
-        # hwe edge lowlatency kernel, e.g hwe-16.04-lowlatency-edge
-        release = parts[1]
-        if parts[3] == "edge":
-            weight = 1
-        else:
-            weight = 0
     else:
-        raise ValueError("Unknown release or kernel %s!" % string)
+        channel, release, platform, kflavor = parse_subarch_kernel_string(
+            string
+        )
+        weight = 0
+
+        # hwe kernels should only have a higher weight when using hwe-<version>
+        # format. This ensures the old (hwe-{letter}) format maps to the ga kernel.
+        if channel == "hwe":
+            # `hwe-{letter}` should not get HWE weight bump
+            weight = HWE_CHANNEL_WEIGHT if len(release) > 1 else 0
+        elif channel == "hwe-edge":
+            weight = HWE_EDGE_CHANNEL_WEIGHT
 
-    # hwe kernels should only have a higher weight when using the new format
-    # which is hwe-<version>. This ensures the old format maps to the ga
-    # kernel.
-    if parts[0] == "hwe" and len(parts[1]) > 1:
-        weight += 1
+    # Newer format kernel strings should weigh more
+    # First condition is to bump platform-optimised kernels too
+    if (parts_len > 1 or platform) and (channel != "hwe" or len(release) > 1):
+        weight += NOT_OLD_HWE_WEIGHT
 
     if release == "rolling":
         # Rolling kernels are always the latest
         version = [999, 999]
     else:
-        ubuntu_release = get_release(release)
+        ubuntu_release = ubuntu_release or get_release(release)
         if ubuntu_release is None:
             raise ValueError(
                 "%s not found amongst the known Ubuntu releases!" % string
@@ -476,6 +562,14 @@ def get_release_version_from_string(string):
         # Convert the version into a list of ints
         version = [int(seg) for seg in version.split(".")]
 
+    if kflavor and kflavor != "generic":
+        weight += FLAVOURED_WEIGHT
+
+    if platform and platform != "generic":
+        # Platform-specific kernels should have higher weight over
+        # generic ones.
+        weight += PLATFORM_WEIGHT
+
     return tuple(version + [weight])
 
 
diff --git a/src/maasserver/utils/tests/test_osystems.py b/src/maasserver/utils/tests/test_osystems.py
index 67f950f..3b094bb 100644
--- a/src/maasserver/utils/tests/test_osystems.py
+++ b/src/maasserver/utils/tests/test_osystems.py
@@ -21,11 +21,14 @@ from maasserver.testing.factory import factory
 from maasserver.testing.osystems import make_usable_osystem
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils.osystems import (
+    FLAVOURED_WEIGHT,
     get_distro_series_initial,
     get_release_from_db,
     get_release_from_distro_info,
     get_release_requires_key,
     get_release_version_from_string,
+    HWE_CHANNEL_WEIGHT,
+    HWE_EDGE_CHANNEL_WEIGHT,
     list_all_releases_requiring_keys,
     list_all_usable_osystems,
     list_all_usable_releases,
@@ -33,6 +36,7 @@ from maasserver.utils.osystems import (
     list_osystem_choices,
     list_release_choices,
     make_hwe_kernel_ui_text,
+    NOT_OLD_HWE_WEIGHT,
     release_a_newer_than_b,
     validate_hwe_kernel,
     validate_min_hwe_kernel,
@@ -524,67 +528,111 @@ class TestGetReleaseVersionFromString(MAASServerTestCase):
                 "GA kernel",
                 {
                     "string": "ga-%s" % version_str,
-                    "expected": version_tuple + tuple([0]),
+                    "expected": version_tuple + tuple([NOT_OLD_HWE_WEIGHT]),
                 },
             ),
             (
                 "GA low latency kernel",
                 {
                     "string": "ga-%s-lowlatency" % version_str,
-                    "expected": version_tuple + tuple([0]),
+                    "expected": version_tuple
+                    + tuple([NOT_OLD_HWE_WEIGHT + FLAVOURED_WEIGHT]),
                 },
             ),
             (
                 "New style kernel",
                 {
                     "string": "hwe-%s" % version_str,
-                    "expected": version_tuple + tuple([1]),
+                    "expected": version_tuple
+                    + tuple([HWE_CHANNEL_WEIGHT + NOT_OLD_HWE_WEIGHT]),
                 },
             ),
             (
                 "New style edge kernel",
                 {
                     "string": "hwe-%s-edge" % version_str,
-                    "expected": version_tuple + tuple([2]),
+                    "expected": version_tuple
+                    + tuple([HWE_EDGE_CHANNEL_WEIGHT + NOT_OLD_HWE_WEIGHT]),
                 },
             ),
             (
                 "New style low latency kernel",
                 {
                     "string": "hwe-%s-lowlatency" % version_str,
-                    "expected": version_tuple + tuple([1]),
+                    "expected": version_tuple
+                    + tuple(
+                        [
+                            HWE_CHANNEL_WEIGHT
+                            + NOT_OLD_HWE_WEIGHT
+                            + FLAVOURED_WEIGHT
+                        ]
+                    ),
                 },
             ),
             (
                 "New style edge low latency kernel",
                 {
                     "string": "hwe-%s-lowlatency-edge" % version_str,
-                    "expected": version_tuple + tuple([2]),
+                    "expected": version_tuple
+                    + tuple(
+                        [
+                            HWE_EDGE_CHANNEL_WEIGHT
+                            + NOT_OLD_HWE_WEIGHT
+                            + FLAVOURED_WEIGHT
+                        ]
+                    ),
                 },
             ),
             (
                 "Rolling kernel",
-                {"string": "hwe-rolling", "expected": tuple([999, 999, 1])},
+                {
+                    "string": "hwe-rolling",
+                    "expected": tuple(
+                        [999, 999, HWE_CHANNEL_WEIGHT + NOT_OLD_HWE_WEIGHT]
+                    ),
+                },
             ),
             (
                 "Rolling edge kernel",
                 {
                     "string": "hwe-rolling-edge",
-                    "expected": tuple([999, 999, 2]),
+                    "expected": tuple(
+                        [
+                            999,
+                            999,
+                            HWE_EDGE_CHANNEL_WEIGHT + NOT_OLD_HWE_WEIGHT,
+                        ]
+                    ),
                 },
             ),
             (
                 "Rolling lowlatency kernel",
                 {
                     "string": "hwe-rolling-lowlatency",
-                    "expected": tuple([999, 999, 1]),
+                    "expected": tuple(
+                        [
+                            999,
+                            999,
+                            HWE_CHANNEL_WEIGHT
+                            + NOT_OLD_HWE_WEIGHT
+                            + FLAVOURED_WEIGHT,
+                        ]
+                    ),
                 },
             ),
             (
                 "Rolling lowlatency edge kernel",
                 {
                     "string": "hwe-rolling-lowlatency-edge",
-                    "expected": tuple([999, 999, 2]),
+                    "expected": tuple(
+                        [
+                            999,
+                            999,
+                            HWE_EDGE_CHANNEL_WEIGHT
+                            + NOT_OLD_HWE_WEIGHT
+                            + FLAVOURED_WEIGHT,
+                        ]
+                    ),
                 },
             ),
         )