fix: add single retry of ssh-import-id on exit 1 (#6805)

Add a single retry on ssh-import-id to mitigate intermittent errors
from remote service avoiding boot failures.

Seeing 10-20% failure rate of exit 1's from ssh-import-id during
instance launches in github action runners.

Author: blackboxsw

cloudinit/config/cc_ssh_import_id.py

@@ -9,6 +9,8 @@
 
 import logging
 import pwd
+import time
+from contextlib import suppress
 
 from cloudinit import subp, util
 from cloudinit.cloud import Cloud
@@ -148,13 +150,16 @@ def import_ssh_ids(ids, user):
     else:
         LOG.error("Neither sudo nor doas available! Unable to import SSH ids.")
         return
-    LOG.debug("Importing SSH ids for user %s.", user)
+    retry_ssh_import(cmd, 0.5)
 
-    try:
+
+def retry_ssh_import(cmd: list, delay: float) -> None:
+    """Retry ssh-import-id once if it exits in error."""
+    with suppress(subp.ProcessExecutionError):
         subp.subp(cmd, capture=False)
-    except subp.ProcessExecutionError as exc:
-        util.logexc(LOG, "Failed to run command to import %s SSH ids", user)
-        raise exc
+        return
+    time.sleep(delay)
+    subp.subp(cmd, capture=False)
 
 
 def is_key_in_nested_dict(config: dict, search_key: str) -> bool:

tests/unittests/config/test_cc_ssh_import_id.py

@@ -6,11 +6,12 @@
 import pytest
 
 from cloudinit.config import cc_ssh_import_id
+from cloudinit.subp import ProcessExecutionError
 from tests.unittests.util import get_cloud
 
 LOG = logging.getLogger(__name__)
 
-MODPATH = "cloudinit.config.cc_ssh_import_ids."
+MODPATH = "cloudinit.config.cc_ssh_import_id."
 
 
 class TestIsKeyInNestedDict:
@@ -69,17 +70,17 @@ class TestHandleSshImportIDs:
             ({"ssh_import_id": ["bobkey"]}, "ssh-import-id is not installed"),
         ),
     )
-    @mock.patch("cloudinit.subp.which")
+    @mock.patch(MODPATH + "subp.which")
     def test_skip_inapplicable_configs(self, m_which, cfg, log, caplog):
         """Skip config without ssh_import_id"""
         m_which.return_value = None
         cloud = get_cloud("ubuntu")
         cc_ssh_import_id.handle("name", cfg, cloud, [])
         assert log in caplog.text
 
-    @mock.patch("cloudinit.ssh_util.pwd.getpwnam")
-    @mock.patch("cloudinit.config.cc_ssh_import_id.subp.subp")
-    @mock.patch("cloudinit.subp.which")
+    @mock.patch(MODPATH + "pwd.getpwnam")
+    @mock.patch(MODPATH + "subp.subp")
+    @mock.patch(MODPATH + "subp.which")
     def test_use_sudo(self, m_which, m_subp, m_getpwnam):
         """Check that sudo is available and use that"""
         m_which.return_value = "/usr/bin/ssh-import-id"
@@ -98,9 +99,9 @@ def test_use_sudo(self, m_which, m_subp, m_getpwnam):
             capture=False,
         )
 
-    @mock.patch("cloudinit.ssh_util.pwd.getpwnam")
-    @mock.patch("cloudinit.config.cc_ssh_import_id.subp.subp")
-    @mock.patch("cloudinit.subp.which")
+    @mock.patch(MODPATH + "pwd.getpwnam")
+    @mock.patch(MODPATH + "subp.subp")
+    @mock.patch(MODPATH + "subp.which")
     def test_use_doas(self, m_which, m_subp, m_getpwnam):
         """Check that doas is available and use that"""
         m_which.side_effect = [None, "/usr/bin/doas"]
@@ -111,9 +112,9 @@ def test_use_doas(self, m_which, m_subp, m_getpwnam):
             ["doas", "-u", user, "ssh-import-id"] + ids, capture=False
         )
 
-    @mock.patch("cloudinit.ssh_util.pwd.getpwnam")
-    @mock.patch("cloudinit.config.cc_ssh_import_id.subp.subp")
-    @mock.patch("cloudinit.subp.which")
+    @mock.patch(MODPATH + "pwd.getpwnam")
+    @mock.patch(MODPATH + "subp.subp")
+    @mock.patch(MODPATH + "subp.which")
     def test_use_neither_sudo_nor_doas(
         self, m_which, m_subp, m_getpwnam, caplog
     ):
@@ -125,3 +126,45 @@ def test_use_neither_sudo_nor_doas(
         assert (
             "Neither sudo nor doas available! Unable to import SSH ids"
         ) in caplog.text
+
+    @mock.patch(MODPATH + "time.sleep")
+    @mock.patch(MODPATH + "pwd.getpwnam")
+    @mock.patch(MODPATH + "subp.which")
+    def test_retry_once_on_exit_code_1(
+        self, m_which, m_getpwnam, m_sleep, mocker
+    ):
+        """Only attempt one retry when ssh-import-id exits with code 1."""
+        m_subp = mocker.patch(
+            "cloudinit.config.cc_ssh_import_id.subp.subp",
+            side_effect=[
+                ProcessExecutionError(exit_code=1, stderr="try1"),
+                ProcessExecutionError(exit_code=1, stderr="try2"),
+            ],
+        )
+        m_which.return_value = "/usr/bin/ssh-import-id"
+        with pytest.raises(
+            ProcessExecutionError,
+            match=r"(?s)Unexpected error while running command.*try2",
+        ):
+            cc_ssh_import_id.import_ssh_ids(["waffle"], "bob")
+
+        assert m_subp.call_count == 2
+        assert m_sleep.call_count == 1
+
+    @mock.patch(MODPATH + "time.sleep")
+    @mock.patch(MODPATH + "pwd.getpwnam")
+    @mock.patch(MODPATH + "subp.which")
+    def test_retry_with_success(self, m_which, m_getpwnam, m_sleep, mocker):
+        """Retry succeeds on ssh-import-id with a retry."""
+        m_subp = mocker.patch(
+            "cloudinit.config.cc_ssh_import_id.subp.subp",
+            side_effect=[
+                ProcessExecutionError(exit_code=1, stderr="try1"),
+                None,
+            ],
+        )
+        m_which.return_value = "/usr/bin/ssh-import-id"
+        cc_ssh_import_id.import_ssh_ids(["waffle"], "bob")
+
+        assert m_subp.call_count == 2
+        assert m_sleep.call_count == 1