From 843f2a87edfb0fb00594920f09437af6865f1d03 Mon Sep 17 00:00:00 2001 From: Pierre Equoy Date: Tue, 3 Dec 2024 10:46:14 +0100 Subject: [PATCH] Fail snap refresh/revert jobs if the `snap_update_test.py` script fails (bugfix) (#1616) * Fail snap refresh/revert jobs if the snap_update_test.py script fails Adding `set -eo pipefail` in the snapd refresh/revert jobs that initiate a reboot ensures that: - any failure in the `snap_update_test.py` is not masked by the pipe command (this is achieved by `set -o pipefail`) - any failure at any step of the `command:` field just fails the whole job (this is ensured by `set -e`). This is important to avoid the `reboot` command from being issued if something goes wrong in the script. Fix #1615 * Increase default timeout of the `snap_update_test.py` script Some devices in our test lab are really slow to download and apply snap updates. Increase timeout from 5 minutes to 10 minutes to hopefully help with this. * Revert the use of `set -e` * Indicate a failed job if an exception is caught when running snapd command When running a snapd revert or refresh command, a SnapdRequestError exception might be raised (for example when the refresh command failed to run within the given time). If this is the case, the script fails, and if it is executed as part of a Checkbox run, a `__result` file is generated and stored in the shared session directory to record the fact that this job failed. When the session is resumed, Checkbox will automatically resume the session and mark this test as failed. * Update unit tests for snap_update_test * Also fail job in case of an AsynException This exception can also be raised when calling snap commands using checkbox_support.snap_utils.snapd.Snapd. * Update providers/base/bin/snap_update_test.py Co-authored-by: Massimiliano * black is not happy * Fix flake8 --------- Co-authored-by: Massimiliano --- providers/base/bin/snap_update_test.py | 49 ++++++++++++++++--- providers/base/tests/test_snap_update_test.py | 26 ++++++++++ providers/base/units/snapd/jobs.pxu | 4 ++ 3 files changed, 72 insertions(+), 7 deletions(-) diff --git a/providers/base/bin/snap_update_test.py b/providers/base/bin/snap_update_test.py index 5585299dde..bd45599af1 100755 --- a/providers/base/bin/snap_update_test.py +++ b/providers/base/bin/snap_update_test.py @@ -21,10 +21,13 @@ import argparse from pathlib import Path import json +import os import sys import time from checkbox_support.snap_utils.snapd import Snapd +from checkbox_support.snap_utils.snapd import AsyncException +from checkbox_support.snap_utils.snapd import SnapdRequestError def guess_snaps() -> list: @@ -148,11 +151,29 @@ def snap_refresh(self): self.name, original_revision, self.revision ) ) - response = self.snapd.refresh( - self.name, - channel=self.snap_info.tracking_channel, - revision=self.revision, - ) + try: + response = self.snapd.refresh( + self.name, + channel=self.snap_info.tracking_channel, + revision=self.revision, + ) + except (SnapdRequestError, AsyncException) as exc: + checkbox_session_dir = os.getenv("PLAINBOX_SESSION_SHARE") + if checkbox_session_dir: + result = { + "outcome": "fail", + "comments": ( + "Marking the test as failed because it raised" + " the following:" + ) + + str(exc), + } + result_filename = os.path.join( + checkbox_session_dir, "__result" + ) + with open(result_filename, "wt") as result_f: + json.dump(result, result_f) + raise data["change_id"] = response["change"] print( "Snap operation finished. " @@ -170,7 +191,21 @@ def snap_revert(self): self.name, destination_rev, original_rev ) ) - response = self.snapd.revert(self.name) + try: + response = self.snapd.revert(self.name) + except (SnapdRequestError, AsyncException) as exc: + checkbox_session_dir = os.getenv("PLAINBOX_SESSION_SHARE") + if checkbox_session_dir: + result = { + "outcome": "fail", + "comments": exc.message, + } + result_filename = os.path.join( + checkbox_session_dir, "__result" + ) + with open(result_filename, "wt") as result_f: + json.dump(result, result_f) + raise data["change_id"] = response["change"] print( "Snap operation finished. " @@ -285,7 +320,7 @@ def main(args): ) parser.add_argument( "--timeout", - default=300, + default=600, help="Timeout for each task, in seconds (default: %(default)s))", ) diff --git a/providers/base/tests/test_snap_update_test.py b/providers/base/tests/test_snap_update_test.py index 1254271580..ee76d2cac9 100644 --- a/providers/base/tests/test_snap_update_test.py +++ b/providers/base/tests/test_snap_update_test.py @@ -1,5 +1,6 @@ import io import logging +import tempfile import unittest from unittest.mock import patch, mock_open, MagicMock @@ -14,6 +15,8 @@ snap_info_pi_kernel, ) +from checkbox_support.snap_utils.snapd import SnapdRequestError + class SnapUpdateTests(unittest.TestCase): @classmethod @@ -179,6 +182,17 @@ def test_snap_refresh_different_revision(self, mock_save_change_info): snap_update_test.SnapRefreshRevert.snap_refresh(mock_self) self.assertTrue(mock_self.snapd.refresh.called) + @patch("os.getenv") + def test_snap_refresh_exception(self, mock_getenv): + mock_self = MagicMock() + mock_self.snapd.refresh.side_effect = SnapdRequestError( + "test msg", "test" + ) + with self.assertRaises(SnapdRequestError): + with tempfile.TemporaryDirectory() as tmpdirname: + mock_getenv.return_value = tmpdirname + snap_update_test.SnapRefreshRevert.snap_refresh(mock_self) + @patch("snap_update_test.Snapd.revert") @patch("snap_update_test.load_change_info") @patch("snap_update_test.save_change_info") @@ -190,6 +204,18 @@ def test_snap_revert( self.assertTrue(snap_update_test.load_change_info.called) self.assertTrue(snap_update_test.save_change_info.called) + @patch("os.getenv") + @patch("snap_update_test.load_change_info") + def test_snap_revert_exception(self, mock_load_change_info, mock_getenv): + mock_self = MagicMock() + mock_self.snapd.revert.side_effect = SnapdRequestError( + "test msg", "test" + ) + with self.assertRaises(SnapdRequestError): + with tempfile.TemporaryDirectory() as tmpdirname: + mock_getenv.return_value = tmpdirname + snap_update_test.SnapRefreshRevert.snap_revert(mock_self) + def test_verify_invalid(self): mock_self = MagicMock() with self.assertRaises(SystemExit): diff --git a/providers/base/units/snapd/jobs.pxu b/providers/base/units/snapd/jobs.pxu index 2b7e99ffc4..d7fcf71d9d 100644 --- a/providers/base/units/snapd/jobs.pxu +++ b/providers/base/units/snapd/jobs.pxu @@ -32,6 +32,7 @@ requires: (snap_revision_info.name == "{name}") and snap_revision_info.stable_rev != snap_revision_info.original_installed_rev manifest.need_{type}_snap_update_test == "True" command: + set -o pipefail path="$PLAINBOX_SESSION_SHARE/{name}_snap_revision_info" logpath="$PLAINBOX_SESSION_SHARE/snap-refresh-{type}-{name}-to-stable-rev.log" snap_update_test.py --refresh --revision {stable_rev} --info-path "$path" {name} | tee "$logpath" @@ -81,6 +82,7 @@ category_id: snapd user: root depends: snapd/snap-verify-after-refresh-{type}-{name}-to-stable-rev command: + set -o pipefail path="$PLAINBOX_SESSION_SHARE/{name}_snap_revision_info" logpath="$PLAINBOX_SESSION_SHARE/snap-revert-{type}-{name}-from-stable-rev.log" snap_update_test.py --revert --info-path "$path" {name} | tee "$logpath" @@ -143,6 +145,7 @@ requires: (snap_revision_info.name == "{name}") and snap_revision_info.base_rev != snap_revision_info.original_installed_rev manifest.need_{type}_snap_update_test == "True" command: + set -o pipefail path="$PLAINBOX_SESSION_SHARE/{name}_snap_revision_info" logpath="$PLAINBOX_SESSION_SHARE/snap-refresh-{type}-{name}-to-base-rev.log" snap_update_test.py --refresh --revision {base_rev} --info-path "$path" {name} | tee "$logpath" @@ -192,6 +195,7 @@ category_id: snapd user: root depends: snapd/snap-verify-after-refresh-{type}-{name}-to-base-rev command: + set -o pipefail path="$PLAINBOX_SESSION_SHARE/{name}_snap_revision_info" logpath="$PLAINBOX_SESSION_SHARE/snap-revert-{type}-{name}-from-base-rev.log" snap_update_test.py --revert --info-path "$path" {name} | tee "$logpath"