Merge ~ogayot/curtin:flag-logical+swap into curtin:master

[Olivier Gayot]

block_meta_v2: don't solely expect flag='logical' for logical partitions

Most of the time, a logical partition has its flag set to 'local'.
However, it some cases, it can also have flag set to 'swap', 'boot' and
possibly other values.

To determine if a partition is actually logical on a DOS partition
table, we need to check the partition number.

In block_meta_v2, we expect logical partitions to have their flag set to 'logical'. However, there are valid scenarios where this is a wrong assumption.

One use-case is the creation of a logical swap partition. This partition would end up with flag='swap' and not flag='logical' ; despite being a logical partition.

Despite being an unsupported use-case in Ubuntu, sometimes a bootable ESP partition is a logical partition.
In curtin's storage_config code, we have the following statement:

 # if the boot flag is set, use this as the flag, logical
 # flag is not required as we can determine logical via
 # partition number

https://git.launchpad.net/curtin/tree/curtin/storage_config.py?id=33411a726532d24e982e29c62f48abd82e2bc4fe#n809

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:80f3d638a3d11a53400d8caa382fe75cc501de33
https://jenkins.canonical.com/server-team/job/curtin-ci/96/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/96/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/96/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/96/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/96/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/96//rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:677ad31f9c2c8adce07451fb35371c8498a065f7
https://jenkins.canonical.com/server-team/job/curtin-ci/97/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/97/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/97/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/97/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/97/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/97//rebuild

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

Unit test please. I think it's fine to extract this to a utility function to make this easier.

[Olivier Gayot (ogayot)]

Thanks for the suggestion. Updated!

I've made is_logical(action) a static method so it can be tested with more ease.

[Olivier Gayot (ogayot)]

NOTE: I've only fixed the code for storage config version 2. Maybe we need to update storage version 1 too.

[Dan Bungert (dbungert)]

Thanks!

review: Approve

diff --git a/curtin/commands/block_meta_v2.py b/curtin/commands/block_meta_v2.py
index 79dfd39..ae0b058 100644
--- a/curtin/commands/block_meta_v2.py
+++ b/curtin/commands/block_meta_v2.py
@@ -248,12 +248,24 @@ class DOSPartTable(SFDiskPartTable):
     label = 'dos'
     _extended = None
 
+    @staticmethod
+    def is_logical(action) -> bool:
+        flag = action.get('flag', None)
+        if flag == 'logical':
+            return True
+        # In some scenarios, a swap partition can be in the extended
+        # partition. When it does, the flag is set to 'swap'.
+        # In some other scenarios, a bootable partition can also be in the
+        # extended partition. This is not a supported use-case but is
+        # yet another scenario where flag is not set to 'logical'.
+        return action.get('number', 0) > 4
+
     def add(self, action):
         flag = action.get('flag', None)
         start = action.get('offset', None)
         if start is not None:
             start = self.bytes2sectors(start)
-        if flag == 'logical':
+        if self.is_logical(action):
             if self._extended is None:
                 raise Exception("logical partition without extended partition")
             prev = None
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 82530ff..dc03359 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -3259,6 +3259,20 @@ label: dos
             f'name="{name}" attrs="{attrs}"'
         self.assertEqual(expected, pte.render())
 
+    def test_v2_dos_is_logical(self):
+        action = {"flag": "logical"}
+        self.assertTrue(block_meta_v2.DOSPartTable.is_logical(action))
+        action = {"flag": "logical", "number": 5}
+        self.assertTrue(block_meta_v2.DOSPartTable.is_logical(action))
+        action = {"flag": "swap", "number": 2}
+        self.assertFalse(block_meta_v2.DOSPartTable.is_logical(action))
+        action = {"flag": "swap", "number": 5}
+        self.assertTrue(block_meta_v2.DOSPartTable.is_logical(action))
+        action = {"flag": "boot", "number": 5}
+        self.assertTrue(block_meta_v2.DOSPartTable.is_logical(action))
+        action = {"flag": "swap"}
+        self.assertFalse(block_meta_v2.DOSPartTable.is_logical(action))
+
 
 class TestPartitionNeedsResize(CiTestCase):