Merge ~raharper/curtin:fix/tolerate-missing-probe-data-keys into curtin:master

[Ryan Harper]

block-discover: handle partial probe data

The probe_data provided to curtin may only include a subset of
the types of data that curtin knows how to parse. Currently if
one of the types is not present in the probe_data, curtin raises
Exceptions which prevent callers from getting any storage config
from curtin. This branch allows curtin to handle missing data
and will log the errors but not raise Exceptions, leaving the
caller to handle and decide what to do with the storage config
produced.

LP: #1852351

[Michael Hudson-Doyle (mwhudson)]

Thanks, just a couple of stray debugging prints but otherwise looks good to me.

review: Approve

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

PASSED: Continuous integration, rev:d92fa0cf24fbb756ef3fae45f5b90af7b0c1ab9e
https://jenkins.ubuntu.com/server/job/curtin-ci/3786/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/3786/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/3786/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/3786/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/3786/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/3786//rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Thanks for the review. Dropping the debug prints.

9a32920... by Ryan Harper on 2019-11-13

Drop debug prints

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

PASSED: Continuous integration, rev:9a329203cd4e00938744d4c64009803012cbc61e
https://jenkins.ubuntu.com/server/job/curtin-ci/3787/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/3787/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/3787/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/3787/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/3787/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/3787//rebuild

review: Approve (continuous-integration)

diff --git a/curtin/storage_config.py b/curtin/storage_config.py
index 03cfb4e..6177f48 100644
--- a/curtin/storage_config.py
+++ b/curtin/storage_config.py
@@ -389,15 +389,15 @@ class ProbertParser(object):
                     data = {}
                 self.class_data = data
             else:
-                raise ValueError('probe_data missing %s data' %
-                                 self.probe_data_key)
+                LOG.warning('probe_data missing %s data', self.probe_data_key)
+                self.class_data = {}
 
         # We keep a reference to the blockdev_data on the superclass
         # as each specific parser has common needs to reference
         # this data separate from the BlockdevParser class.
-        self.blockdev_data = self.probe_data.get('blockdev')
+        self.blockdev_data = self.probe_data.get('blockdev', {})
         if not self.blockdev_data:
-            raise ValueError('probe_data missing valid "blockdev" data')
+            LOG.warning('probe_data missing valid "blockdev" data')
 
     def parse(self):
         raise NotImplementedError()
@@ -575,7 +575,8 @@ class BcacheParser(ProbertParser):
             msg = ('Invalid "blockdev" value for cache device '
                    'uuid=%s' % cset_uuid)
             if not cset_uuid:
-                raise ValueError(msg)
+                LOG.warning(msg)
+                return None
 
             for devuuid, config in cache_data.items():
                 cache = _sb_get(config, 'cset.uuid')
@@ -588,6 +589,8 @@ class BcacheParser(ProbertParser):
             by_uuid = '/dev/bcache/by-uuid/' + uuid
             label = _sb_get(backing_data, 'dev.label')
             for devname, data in blockdev_data.items():
+                if not devname:
+                    continue
                 if devname.startswith('/dev/bcache'):
                     # DEVLINKS is a space separated list
                     devlinks = data.get('DEVLINKS', '').split()
@@ -595,7 +598,7 @@ class BcacheParser(ProbertParser):
                         return devname
             if label:
                 return label
-            raise ValueError('Failed to find bcache %s ' % (by_uuid))
+            LOG.warning('Failed to find bcache %s ' % (by_uuid))
 
         def _cache_mode(dev_data):
             # "1 [writeback]" -> "writeback"
@@ -605,12 +608,14 @@ class BcacheParser(ProbertParser):
 
             return None
 
+        if not self.blockdev_data:
+            return None
+
         backing_device = backing_data.get('blockdev')
         cache_device = _find_cache_device(backing_data, self.caching)
         cache_mode = _cache_mode(backing_data)
-        bcache_name = os.path.basename(
-            _find_bcache_devname(backing_uuid, backing_data,
-                                 self.blockdev_data))
+        bcache_name = os.path.basename(_find_bcache_devname(backing_uuid,
+                                       backing_data, self.blockdev_data))
         bcache_entry = {'type': 'bcache', 'id': 'disk-%s' % bcache_name,
                         'name': bcache_name}
 
@@ -1181,7 +1186,7 @@ class ZfsParser(ProbertParser):
         return (zpool_configs + zfs_configs, errors)
 
 
-def extract_storage_config(probe_data):
+def extract_storage_config(probe_data, strict=False):
     """ Examine a probert storage dictionary and extract a curtin
         storage configuration that would recreate all of the
         storage devices present in the provided data.
@@ -1233,7 +1238,10 @@ def extract_storage_config(probe_data):
     for e in errors:
         LOG.exception('Validation error: %s\n' % e)
     if len(errors) > 0:
-        raise RuntimeError("Extract storage config does not validate.")
+        errmsg = "Extract storage config does not validate."
+        LOG.warning(errmsg)
+        if strict:
+            raise RuntimeError(errmsg)
 
     # build and merge probed data into a valid storage config by
     # generating a config tree for each item in the probed data
diff --git a/tests/unittests/test_storage_config.py b/tests/unittests/test_storage_config.py
index d4113ea..f01f440 100644
--- a/tests/unittests/test_storage_config.py
+++ b/tests/unittests/test_storage_config.py
@@ -1,4 +1,5 @@
 # This file is part of curtin. See LICENSE file for copyright and license info.
+import copy
 import json
 from .helpers import CiTestCase, skipUnlessJsonSchema
 from curtin import storage_config
@@ -81,16 +82,15 @@ class TestProbertParser(CiTestCase):
         self.assertDictEqual(probe_data['blockdev'],
                              getattr(bdp, 'blockdev_data'))
 
-    def test_probert_parser_missing_required_probe_data_key_raises(self):
-        """ ProbertParser raises ValueError when probe_data_key_data gone. """
+    def test_probert_parser_handles_missing_required_probe_data_key(self):
+        """ ProbertParser handles missing probe_data_key_data. """
         key = self.random_string()
         probe_data = {'blockdev': {self.random_string(): self.random_string()}}
 
         class bdparser(baseparser):
             probe_data_key = key
 
-        with self.assertRaises(ValueError):
-            bdparser(probe_data)
+        self.assertIsNotNone(bdparser(probe_data))
 
 
 def _get_data(datafile):
@@ -132,11 +132,13 @@ class TestBcacheParser(CiTestCase):
         self.assertDictEqual(self.probe_data['bcache']['caching'],
                              bcachep.caching)
 
-    def test_bcache_parse_raise_err_no_blockdev_data(self):
-        """ BcacheParser raises ValueError on missing 'blockdev' dict."""
+    def test_bcache_parse_tolerates_missing_blockdev_data(self):
+        """ BcacheParser  ValueError on missing 'blockdev' dict."""
         del(self.probe_data['blockdev'])
-        with self.assertRaises(ValueError):
-            BcacheParser(self.probe_data)
+        b = BcacheParser(self.probe_data)
+        (configs, errors) = b.parse()
+        self.assertEqual([], configs)
+        self.assertEqual([], errors)
 
     @skipUnlessJsonSchema()
     def test_bcache_parse_extracts_bcache(self):
@@ -716,6 +718,25 @@ class TestExtractStorageConfig(CiTestCase):
                                      'type': 'disk'}]}}, extracted)
 
     @skipUnlessJsonSchema()
+    def test_probe_handles_missing_keys(self):
+        """ verify extract handles missing probe_data keys """
+        for missing_key in self.probe_data.keys():
+            probe_data = copy.deepcopy(self.probe_data)
+            del probe_data[missing_key]
+            extracted = storage_config.extract_storage_config(probe_data)
+            if missing_key != 'blockdev':
+                self.assertEqual(
+                    {'storage':
+                        {'version': 1,
+                         'config': [{'id': 'disk-sda', 'path': '/dev/sda',
+                                     'serial': 'QEMU_HARDDISK_QM00001',
+                                     'type': 'disk'}]}}, extracted)
+            else:
+                # empty config without blockdev data
+                self.assertEqual({'storage': {'config': [], 'version': 1}},
+                                 extracted)
+
+    @skipUnlessJsonSchema()
     def test_find_all_multipath(self):
         """ verify probed multipath paths are included in config. """
         self.probe_data = _get_data('probert_storage_multipath.json')