Merge lp:~smoser/curtin/trunk.lp1523779 into lp:~curtin-dev/curtin/trunk

[Scott Moser]

fix bug in install_grub to partition when storage_config provided.

when installing with a storage config and grub is to be installed
on a partition (as it is in powerpc) we were not able to look
up the device that was to be installed on.

This puts a temporary bit of code in place that finds that device
via imports from commands.block_meta. The result is that
storage_config now works for lvm on power8 systems.

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

FAILED: Continuous integration, rev:322
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~smoser/curtin/trunk.lp1523779/+merge/281000/+edit-commit-message

https://server-team-jenkins.canonical.com/job/curtin-ci/70/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/67/console

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

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

The change as it is right now is to add a method 'extract_storage_ordered_dict' to get an ordered dictionary, where that code was being done inside 'meta_custom' directly.

The reason for doing that is that we need to use it from curthooks.
Previously, if custom storage was used and 'grub_device' existed on a partition it woudl fail.
That was because we could only look up by serial or path.

Now, we use extract_storage_ordered_dict method to get an ordered dictionary and then get a path to that volume (or partition) via get_path_to_storage_volume.

I dont like the inline import : i've done that here only because I dont like importing from block_meta at all. I'd rather move all this to curtin.block but I dont think its ready to go there yet.

I could add another method in block_meta that
  get_path_to_storage_voulme_with_config_id() or seomthing like that and then only use one method from curthooks.

[Ryan Harper (raharper)]

That makes sense. I don't think it'd be too much trouble to put the wrapper in... but which ever method reminds use we need to migrate that into block is fine with me.

[Ryan Harper (raharper)]

Via IRC, missing grub_device in storage config will fallback to grub_install on mounted devices. That's sufficient for now.

review: Approve

[lp:~smoser/curtin/trunk.lp1523779]

324. By Scott Moser on 2015-12-18

explain the no-grub-dev found path a bit

325. By Scott Moser on 2015-12-18

some cleanup and doc and FIXME

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

FAILED: Continuous integration, rev:325
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~smoser/curtin/trunk.lp1523779/+merge/281000/+edit-commit-message

https://server-team-jenkins.canonical.com/job/curtin-ci/72/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/69/console

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

review: Needs Fixing (continuous-integration)

[lp:~smoser/curtin/trunk.lp1523779]

326. By Scott Moser on 2015-12-18

fix keyerror if no install_devices

327. By Scott Moser on 2015-12-18

handle lvm devices

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

FAILED: Continuous integration, rev:327
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~smoser/curtin/trunk.lp1523779/+merge/281000/+edit-commit-message

https://server-team-jenkins.canonical.com/job/curtin-ci/75/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/72/console

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Approve (continuous-integration)

=== modified file 'curtin/commands/block_meta.py'
--- curtin/commands/block_meta.py	2015-12-08 01:23:09 +0000
+++ curtin/commands/block_meta.py	2015-12-18 22:14:07 +0000
@@ -1070,6 +1070,21 @@
                          not supported")
 
 
+def extract_storage_ordered_dict(config):
+    storage_config = config.get('storage', {})
+    if not storage_config:
+        raise ValueError("no 'storage' entry in config")
+    scfg = storage_config.get('config')
+    if not scfg:
+        raise ValueError("invalid storage config data")
+
+    # Since storage config will often have to be searched for a value by its
+    # id, and this can become very inefficient as storage_config grows, a dict
+    # will be generated with the id of each component of the storage_config as
+    # its index and the component of storage_config as its value
+    return OrderedDict((d["id"], d) for (i, d) in enumerate(scfg))
+
+
 def meta_custom(args):
     """Does custom partitioning based on the layout provided in the config
     file. Section with the name storage contains information on which
@@ -1092,23 +1107,9 @@
     state = util.load_command_environment()
     cfg = config.load_command_config(args, state)
 
-    storage_config = cfg.get('storage', {})
-    if not storage_config:
-        raise Exception("storage configuration is required by mode '%s' "
-                        "but not provided in the config file" % CUSTOM)
-    storage_config_data = storage_config.get('config')
-
-    if not storage_config_data:
-        raise ValueError("invalid storage config data")
-
-    # Since storage config will often have to be searched for a value by its
-    # id, and this can become very inefficient as storage_config grows, a dict
-    # will be generated with the id of each component of the storage_config as
-    # its index and the component of storage_config as its value
-    storage_config_dict = OrderedDict((d["id"], d) for (i, d) in
-                                      enumerate(storage_config_data))
-
-    for command in storage_config_data:
+    storage_config_dict = extract_storage_ordered_dict(cfg)
+
+    for item_id, command in storage_config_dict.items():
         handler = command_handlers.get(command['type'])
         if not handler:
             raise ValueError("unknown command type '%s'" % command['type'])
@@ -1116,7 +1117,7 @@
             handler(command, storage_config_dict)
         except Exception as error:
             LOG.error("An error occured handling '%s': %s - %s" %
-                      (command.get('id'), type(error).__name__, error))
+                      (item_id, type(error).__name__, error))
             raise
 
     return 0
 
=== modified file 'curtin/commands/curthooks.py'
--- curtin/commands/curthooks.py	2015-12-17 18:25:51 +0000
+++ curtin/commands/curthooks.py	2015-12-18 22:14:07 +0000
@@ -282,27 +282,37 @@
 
 def setup_grub(cfg, target):
     # target is the path to the mounted filesystem
+
+    # FIXME: these methods need moving to curtin.block
+    # and using them from there rather than commands.block_meta
+    from curtin.commands.block_meta import (extract_storage_ordered_dict,
+                                            get_path_to_storage_volume)
+
     grubcfg = cfg.get('grub', {})
 
     # copy legacy top level name
     if 'grub_install_devices' in cfg and 'install_devices' not in grubcfg:
         grubcfg['install_devices'] = cfg['grub_install_devices']
 
-    if 'storage' in cfg:
+    # if there is storage config, look for devices tagged with 'grub_device'
+    storage_cfg_odict = None
+    try:
+        storage_cfg_odict = extract_storage_ordered_dict(cfg)
+    except ValueError as e:
+        pass
+
+    if storage_cfg_odict:
         storage_grub_devices = []
-        for item in cfg.get('storage').get('config'):
-            if item.get('grub_device'):
-                if item.get('serial'):
-                    storage_grub_devices.append(
-                        block.lookup_disk(item.get('serial')))
-                elif item.get('path'):
-                    storage_grub_devices.append(item.get('path'))
-                else:
-                    raise ValueError("setup_grub cannot find path to disk \
-                        '%s'" % item.get('id'))
+        for item_id, item in storage_cfg_odict.items():
+            if not item.get('grub_device'):
+                continue
+            LOG.debug("checking: %s", item)
+            storage_grub_devices.append(
+                get_path_to_storage_volume(item_id, storage_cfg_odict))
         if len(storage_grub_devices) > 0:
             grubcfg['install_devices'] = storage_grub_devices
 
+    LOG.debug("install_devices: %s", grubcfg.get('install_devices'))
     if 'install_devices' in grubcfg:
         instdevs = grubcfg.get('install_devices')
         if isinstance(instdevs, str):
@@ -310,11 +320,20 @@
         if instdevs is None:
             LOG.debug("grub installation disabled by config")
     else:
+        # If there were no install_devices found then we try to do the right
+        # thing.  That right thing is basically installing on all block
+        # devices that are mounted.  On powerpc, though it means finding PrEP
+        # partitions.
         devs = block.get_devices_for_mp(target)
         blockdevs = set()
         for maybepart in devs:
-            (blockdev, part) = block.get_blockdev_for_partition(maybepart)
-            blockdevs.add(blockdev)
+            try:
+                (blockdev, part) = block.get_blockdev_for_partition(maybepart)
+                blockdevs.add(blockdev)
+            except ValueError as e:
+                # if there is no syspath for this device such as a lvm
+                # or raid device, then a ValueError is raised here.
+                LOG.debug("failed to find block device for %s", maybepart)
 
         if platform.machine().startswith("ppc64"):
             # assume we want partitions that are 4100 (PReP). The snippet here