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 <[email protected]>

* black is not happy

* Fix flake8

---------

Co-authored-by: Massimiliano <[email protected]>

Author: pieqq

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))",
     )
 

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):

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"