Merge ~dbungert/curtin:lp-1987236-chzdev into curtin:master

[Dan Bungert]

curthooks: make chzdev_export non-fatal on error

LP: #1987236

[Dan Bungert (dbungert)]

still pending test

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

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

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

review: Approve (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

Looks OKish to me. Are there other reasons why chzdev_export might fail? Swallowing failure like this is always a bit dodgy.

[Dan Bungert (dbungert)]

> Looks OKish to me. Are there other reasons why chzdev_export might fail?
> Swallowing failure like this is always a bit dodgy.

We could make this more specific and only swallow exit code 8:
       8 Empty selection
https://github.com/hreinecke/s390-tools-1/blob/39382ec5fdaa78c8b9e3534ff9537af7ce130607/zdev/man/chzdev.8#L634

360d590... by Dan Bungert on 2022-08-30

curthooks: narrow the chzdev_export error skip

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

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

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

review: Approve (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

I think you can make this simpler by using the rcs= argument to subp? util.subp(cmd, capture=True, rcs=[0, 8])

If this doesn't work for some reason, feel free to ignore me :)

[Dan Bungert (dbungert)]

Ah, a good reminder about the rcs arg. In this case though I don't think it's what we want. In the rc == 8 case, we do not want to continue on to the chzdev_prepare_for_import() step. We could add 8 to the rcs chzdev_export, but then we would need to convince ourselves that the later steps handle empty input or find some other way to not do the chzdev_prepare_for_import(). I propose that we stick to my current plan.

[Michael Hudson-Doyle (mwhudson)]

Oh right, I see that subp doesn't tell you which of the acceptable rc values actually happened. Oh well. Have it your way :)

review: Approve

diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
index 141eb76..4001166 100644
--- a/curtin/commands/curthooks.py
+++ b/curtin/commands/curthooks.py
@@ -218,7 +218,11 @@ def chzdev_persist_active_online(cfg, target):
 
     LOG.info('Persisting zdevice configuration in target')
     target_etc = paths.target_path(target, 'etc')
-    (chzdev_conf, _) = chzdev_export(active=True, online=True)
+    ERR_EMPTY_SELECTION = 8  # No settings found to export
+    (chzdev_conf, _, ec) = chzdev_export(active=True, online=True)
+    if ec == ERR_EMPTY_SELECTION:
+        LOG.info('z specific devices not found')
+        return
     chzdev_persistent = chzdev_prepare_for_import(chzdev_conf)
     chzdev_import(data=chzdev_persistent,
                   persistent=True, noroot=True, base={'/etc': target_etc})
@@ -240,7 +244,11 @@ def chzdev_export(active=True, online=True, persistent=False,
         cmd.extend(['--persistent'])
     cmd.extend(['--export', export_file])
 
-    return util.subp(cmd, capture=True)
+    try:
+        out, err = util.subp(cmd, capture=True)
+        return (out, err, 0)
+    except util.ProcessExecutionError as proc_ex_err:
+        return (None, None, proc_ex_err.exit_code)
 
 
 def chzdev_import(data=None, persistent=True, noroot=True, base=None,
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index f87c17a..5ca0b8c 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -1792,7 +1792,7 @@ class TestCurthooksChzdev(CiTestCase):
         """chzdev_persist uses export, sends to prepare & import consumes."""
         export_value = self.random_string()
         import_value = self.random_string().encode()
-        m_chz_export.return_value = (export_value, '')
+        m_chz_export.return_value = (export_value, '', 0)
         m_chz_prepare.return_value = import_value
         curthooks.chzdev_persist_active_online({}, self.target)
         self.assertEqual(1, m_chz_export.call_count)
@@ -1803,6 +1803,18 @@ class TestCurthooksChzdev(CiTestCase):
                                         persistent=True, noroot=True,
                                         base={'/etc': self.target + '/etc'})
 
+    @patch('curtin.commands.curthooks.chzdev_export')
+    @patch('curtin.commands.curthooks.chzdev_prepare_for_import')
+    @patch('curtin.commands.curthooks.chzdev_import')
+    def test_chzdev_skip_empty_selection(self, m_chz_import,
+                                         m_chz_prepare, m_chz_export):
+        """when chzdev_export returns an empty selection error, bail"""
+        m_chz_export.return_value = (None, None, 8)
+        curthooks.chzdev_persist_active_online({}, self.target)
+        self.assertEqual(1, m_chz_export.call_count)
+        self.assertEqual(0, m_chz_prepare.call_count)
+        self.assertEqual(0, m_chz_import.call_count)
+
     def test_export_defaults_to_stdout(self):
         """chzdev_export returns (stdout, stderr) from subp."""
         self.m_subp.return_value = (self.chzdev_export, '')