Merge lp:~wesley-wiedenmeier/curtin/trusty-preserve into lp:~curtin-dev/curtin/trunk

[Wesley Wiedenmeier]

Fix LP:1524031

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

PASSED: Continuous integration, rev:347
https://server-team-jenkins.canonical.com/job/curtin-ci/238/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/235/console

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

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Hi Wesley,

Do you think we should use a lsb_release value to look up which tool to use to discover the partition instead of using the fallback?

If we're running an install (or preserve) on a Trusty system, won't blkid _always_ fail?
If so, then there's little point in exec'ing blkid when we know it will fail.

Does that make sense? Maybe look at the block/mkfs.py for one of those mappings for
extracting the right command for checking ptable state?

review: Needs Fixing

[Scott Moser (smoser)]

I think what you're fixing is the determination of the partition table on a block device.
I think it would be generally nicer then to do:

class pttypes
def get_partition_table_type(block_device):
    """Return the block partition table type for block_device
       Raise OSError on unexpected Error.
       return values of 'gpt', 'dos' or None"""

I'd put that method into curtin/block and use it from there, as it is seemingly a useful bit of function all by itself.
Then, some other thoughts:
a.) growpart in cloud-utils does this for all ubuntu releases as seen in 'get_table_format' in
http://bazaar.launchpad.net/~cloud-utils-dev/cloud-utils/trunk/view/head:/bin/growpart#L650

b.) it would be nice to have some less error prone way or like to identify valid values of partition types in the code, more than checking the string 'gpt' or 'mbr'.

You can't use Enum in python 2 without additional module, but you could do something like
  tests/vmtests/releases.py does with _Releases

class PartitionTableTypes(object):
   MBR = "MBR"
   GPT = "GPT"
   NONE = "Unpartitioned"

then always refer to types as
  from block import PartitionTableTypes
  return PartitionTableTypes.MBR

[Wesley Wiedenmeier (wesley-wiedenmeier)]

That makes sense.

It would definitely be nice to have all of the logic for getting information about the partition table somewhere in block because a lot of that si repeated throughout block_meta and I'm sure there are more bugs like this where I didn't accomodate for blkid failing on trusty in block meta.

I can move this logic into block this afternoon, probably in a new file (something like block.disk_info) because block.__init__ is getting pretty long.

It definitely would be nice to have constants defined as well for most of the storage config stuff because there are a lot of places in block meta where I used string comparison that could break. I'll get that added in as well

[Wesley Wiedenmeier (wesley-wiedenmeier)]

I moved the _nameset class from curtin.reporter.events into curtin.util.NameSet and created entries in that format for all of the differetn types of storage config commands, filesystem types, and partition table types and made the substitutions in block meta.

I think it would be nice to have a simple object model of the storage config that supports filtering storage config items and does preprocessing on the storage config to get the partition numbers sorted out, so I did all of this in a different branch that I should have ready to merge soon.

lp:~wesley-wiedenmeier/curtin/storage-config/

That will also let me rip out some of the ad-hoc storage config manipulation stuff I wrote last summer and replace it with code that is easy to test, that should get rid of some bugs.

I'll start on getting the disk information gathering stuff consolidated into a file in block and making sure that the functions all have fallbacks for trusty and switch to the fallback automatically so they don't exec anything they don't have to.

[lp:~wesley-wiedenmeier/curtin/trusty-preserve]

348. By Wesley Wiedenmeier on 2016-07-18

Remove \ and better format comments in disk handle

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

FAILED: Continuous integration, rev:348
https://server-team-jenkins.canonical.com/job/curtin-ci/315/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/325/console

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

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:348
https://server-team-jenkins.canonical.com/job/curtin-ci-ppc64/19/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/326/console

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

review: Approve (continuous-integration)

=== modified file 'curtin/commands/block_meta.py'
--- curtin/commands/block_meta.py	2016-06-08 21:58:29 +0000
+++ curtin/commands/block_meta.py	2016-07-18 18:59:02 +0000
@@ -452,23 +452,33 @@
             # Don't need to check state, return
             return
 
-        # Check state of current ptable
+        # Check state of current ptable, try to do this using blkid, but if
+        # blkid fails then try to fall back to using parted.
+        _possible_errors = (util.ProcessExecutionError, StopIteration,
+                            IndexError, AttributeError)
         try:
             (out, _err) = util.subp(["blkid", "-o", "export", disk],
                                     capture=True)
-        except util.ProcessExecutionError:
-            raise ValueError("disk '%s' has no readable partition table or \
-                cannot be accessed, but preserve is set to true, so cannot \
-                continue")
-        current_ptable = list(filter(lambda x: "PTTYPE" in x,
-                                     out.splitlines()))[0].split("=")[-1]
-        if current_ptable == "dos" and ptable != "msdos" or \
-                current_ptable == "gpt" and ptable != "gpt":
-            raise ValueError("disk '%s' does not have correct \
-                partition table, but preserve is set to true, so not \
-                creating table, so not creating table." % info.get('id'))
-        LOG.info("disk '%s' marked to be preserved, so keeping partition \
-                 table")
+            current_ptable = next(l.split('=')[1] for l in out.splitlines()
+                                  if 'TYPE' in l)
+        except _possible_errors:
+            try:
+                (out, _err) = util.subp(["parted", disk, "--script", "print"],
+                                        capture=True)
+                current_ptable = next(l.split()[-1] for l in out.splitlines()
+                                      if "Partition Table" in l)
+            except _possible_errors:
+                raise ValueError("disk '%s' has no readable partition table "
+                                 "or cannot be accessed, but preserve is set "
+                                 "to true, so cannot continue" % disk)
+        if not (current_ptable == ptable or
+                (current_ptable == "dos" and ptable == "msdos")):
+            raise ValueError("disk '%s' does not have correct "
+                             "partition table, but preserve is "
+                             "set to true, so not creating table."
+                             % info.get('id'))
+        LOG.info("disk '%s' marked to be preserved, so keeping partition "
+                 "table" % disk)
         return
 
     # Wipe the disk