Merge pull request #798 from jenting/bsc-1153928

Reboot can be triggered before skuba-update finish (bsc#1153928)
SUSE · Oct 28, 2019 · 8e5762a · 8e5762a
2 parents bc854b9 + 54bf82b
commit 8e5762a
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 39 deletions.
diff --git a/skuba-update/skuba_update/skuba_update.py b/skuba-update/skuba_update/skuba_update.py
@@ -76,9 +76,12 @@ def main():
 
     run_zypper_command(['zypper', 'ref', '-s'])
     if not args.annotate_only:
-        update()
+        code = update()
         restart_services()
-    annotate_updates_available()
+        annotate_updates_available()
+        reboot_sentinel_file(code)
+    else:
+        annotate_updates_available()
 
 
 def parse_args():
@@ -116,7 +119,8 @@ def update():
 
     code = run_zypper_patch()
     if is_restart_needed(code):
-        run_zypper_patch()
+        code = run_zypper_patch()
+    return code
 
 
 def annotate_updates_available():
@@ -249,6 +253,17 @@ def is_reboot_needed():
     ) == ZYPPER_EXIT_INF_REBOOT_NEEDED
 
 
+def reboot_sentinel_file(code):
+    # There are two instances in which we should create the
+    # REBOOT_REQUIRED_PATH file:
+    #
+    # 1. Zypper returned an exit code telling us to restart the system.
+    # 2. `zypper needs-rebooting` returns ZYPPER_EXIT_INF_REBOOT_NEEDED.
+    if code == ZYPPER_EXIT_INF_REBOOT_NEEDED or is_reboot_needed():
+        Path(REBOOT_REQUIRED_PATH).touch()
+    return code
+
+
 def is_not_false_str(string):
     """
     Returns true if the given string contains a non-falsey value.
@@ -284,20 +299,11 @@ def run_zypper_command(command, needsOutput=False):
 
 
 def run_zypper_patch():
-    code = run_zypper_command([
+    return run_zypper_command([
         'zypper', '--non-interactive',
         '--non-interactive-include-reboot-patches', 'patch'
     ])
 
-    # There are two instances in which we should create the
-    # REBOOT_REQUIRED_PATH file:
-    #
-    # 1. Zypper returned an exit code telling us to restart the system.
-    # 2. `zypper needs-rebooting` returns ZYPPER_EXIT_INF_REBOOT_NEEDED.
-    if code == ZYPPER_EXIT_INF_REBOOT_NEEDED or is_reboot_needed():
-        Path(REBOOT_REQUIRED_PATH).touch()
-    return code
-
 
 def run_command(command, needsOutput=True, added_env={}):
     """

diff --git a/skuba-update/test/unit/skuba_update_test.py b/skuba-update/test/unit/skuba_update_test.py
@@ -24,10 +24,10 @@
     update,
     run_command,
     run_zypper_command,
-    run_zypper_patch,
     node_name_from_machine_id,
     annotate,
     is_reboot_needed,
+    reboot_sentinel_file,
     annotate_updates_available,
     get_update_list,
     restart_services,
@@ -144,7 +144,6 @@ def mock_communicate():
             'zypper', '--non-interactive',
             '--non-interactive-include-reboot-patches', 'patch'
         ], stdout=None, stderr=None, env=ANY),
-        call(['zypper', 'needs-rebooting'], stdout=None, stderr=None, env=ANY),
         call(
             ['zypper', 'ps', '-sss'],
             stdout=-1, stderr=-1, env=ANY
@@ -157,6 +156,7 @@ def mock_communicate():
             ['systemctl', 'restart', 'some_service2'],
             stdout=None, stderr=None, env=ANY
         ),
+        call(['zypper', 'needs-rebooting'], stdout=None, stderr=None, env=ANY),
     ]
 
 
@@ -238,26 +238,23 @@ def mock_communicate():
             'zypper', '--non-interactive',
             '--non-interactive-include-reboot-patches', 'patch'
         ], stdout=None, stderr=None, env=ANY),
-        call([
-            'zypper', 'needs-rebooting'
-        ], stdout=None, stderr=None, env=ANY),
         call([
             'zypper', '--non-interactive',
             '--non-interactive-include-reboot-patches', 'patch'
         ], stdout=None, stderr=None, env=ANY),
-        call([
-            'zypper', 'needs-rebooting'
-        ], stdout=None, stderr=None, env=ANY),
         call(
             ['zypper', 'ps', '-sss'],
             stdout=-1, stderr=-1, env=ANY
         ),
+        call([
+            'zypper', 'needs-rebooting'
+        ], stdout=None, stderr=None, env=ANY),
     ]
 
 
 @patch('pathlib.Path.is_file')
 @patch('subprocess.Popen')
-def test_update_zypper_is_fine_but_created_needreboot(
+def test_update_zypper_is_fine_but_created_reboot_required(
         mock_subprocess, mock_is_file
 ):
     mock_process = Mock()
@@ -269,7 +266,7 @@ def test_update_zypper_is_fine_but_created_needreboot(
 
     exception = False
     try:
-        update()
+        reboot_sentinel_file(update())
     except PermissionError as e:
         exception = True
         msg = 'Permission denied: \'{0}\''.format(REBOOT_REQUIRED_PATH)
@@ -305,22 +302,6 @@ def test_run_zypper_command_failure(mock_subprocess):
     assert exception
 
 
-@patch('subprocess.Popen')
-def test_run_zypper_command_creates_file_on_102(mock_subprocess):
-    mock_process = Mock()
-    mock_process.communicate.return_value = (b'', b'')
-    mock_process.returncode = ZYPPER_EXIT_INF_REBOOT_NEEDED
-    mock_subprocess.return_value = mock_process
-    exception = False
-    try:
-        run_zypper_patch() == 'stdout'
-    except PermissionError as e:
-        exception = True
-        msg = 'Permission denied: \'{0}\''.format(REBOOT_REQUIRED_PATH)
-        assert msg in str(e)
-    assert exception
-
-
 @patch('builtins.open',
        mock_open(read_data='9ea12911449eb7b5f8f228294bf9209a'))
 @patch('subprocess.Popen')