Merge "Reduce output from validation action" into main

charms/tempest-k8s/charmcraft.yaml

@@ -106,6 +106,14 @@ actions:
       - found in test list "list1"
       - AND match regex "one" or "two"
       - AND don't match regex "three"
+
+      A summary of the results will be printed.
+      The complete results may be large,
+      and will be written to /var/lib/tempest/workspace/tempest-validation.log
+      in the workload container.
+      This can be copied to your local machine using juju,
+      for example:
+        juju scp --container tempest tempest/0:/var/lib/tempest/workspace/tempest-validation.log tempest-validation.log
     additionalProperties: false
     params:
       regex:

charms/tempest-k8s/src/charm.py

@@ -52,16 +52,19 @@ from ops_sunbeam.config_contexts import (
 )
 from utils.constants import (
     CONTAINER,
+    TEMPEST_ADHOC_OUTPUT,
     TEMPEST_CONCURRENCY,
     TEMPEST_CONF,
     TEMPEST_HOME,
     TEMPEST_LIST_DIR,
-    TEMPEST_OUTPUT,
     TEMPEST_READY_KEY,
     TEMPEST_TEST_ACCOUNTS,
     TEMPEST_WORKSPACE,
     TEMPEST_WORKSPACE_PATH,
 )
+from utils.types import (
+    TempestEnvVariant,
+)
 from utils.validators import (
     validated_schedule,
 )
@@ -187,7 +190,9 @@ class TempestOperatorCharm(sunbeam_charm.OSBaseOperatorCharmK8S):
         handlers.append(self.grafana)
         return handlers
 
-    def _get_environment_for_tempest(self) -> Dict[str, str]:
+    def _get_environment_for_tempest(
+        self, variant: TempestEnvVariant
+    ) -> Dict[str, str]:
         """Return a dictionary of environment variables.
 
         To be used with pebble commands that run tempest discover, etc.
@@ -209,10 +214,10 @@ class TempestOperatorCharm(sunbeam_charm.OSBaseOperatorCharmK8S):
             "TEMPEST_CONF": TEMPEST_CONF,
             "TEMPEST_HOME": TEMPEST_HOME,
             "TEMPEST_LIST_DIR": TEMPEST_LIST_DIR,
-            "TEMPEST_OUTPUT": TEMPEST_OUTPUT,
             "TEMPEST_TEST_ACCOUNTS": TEMPEST_TEST_ACCOUNTS,
             "TEMPEST_WORKSPACE": TEMPEST_WORKSPACE,
             "TEMPEST_WORKSPACE_PATH": TEMPEST_WORKSPACE_PATH,
+            "TEMPEST_OUTPUT": variant.output_path(),
         }
 
     def get_unit_data(self, key: str) -> Optional[str]:
@@ -239,7 +244,9 @@ class TempestOperatorCharm(sunbeam_charm.OSBaseOperatorCharmK8S):
             )
             return
 
-        env = self._get_environment_for_tempest()
+        # This is environment sent to the scheduler service,
+        # for periodic checks.
+        env = self._get_environment_for_tempest(TempestEnvVariant.PERIODIC)
         pebble = self.pebble_handler()
         try:
             pebble.init_tempest(env)
@@ -288,19 +295,31 @@ class TempestOperatorCharm(sunbeam_charm.OSBaseOperatorCharmK8S):
         exclude_regex: str = event.params["exclude-regex"].strip()
         test_list: str = event.params["test-list"].strip()
 
-        env = self._get_environment_for_tempest()
+        env = self._get_environment_for_tempest(TempestEnvVariant.ADHOC)
         try:
-            output = self.pebble_handler().run_tempest_tests(
+            summary = self.pebble_handler().run_tempest_tests(
                 regexes, exclude_regex, test_list, serial, env
             )
         except RuntimeError as e:
-            event.fail(str(e))
+            # put the message in set_results instead of event.fail,
-            # still print the message,
+            # because event.fail message is not always displayed to the user:
-            # because it could be a lot of output from tempest,
+            # https://bugs.launchpad.net/juju/+bug/2052765
-            # and we want it neatly formatted
+            event.set_results({"error": str(e)})
-            print(e)
+            event.fail()
             return
-        print(output)
+        event.set_results(
+            {
+                "summary": summary,
+                "info": (
+                    "For detailed results, copy the log file from the container by running:\n"
+                    + self.get_copy_log_cmd()
+                ),
+            }
+        )
+
+    def get_copy_log_cmd(self) -> str:
+        """Get the juju command to copy the ad-hoc tempest log locally."""
+        return f"$ juju scp -m {self.model.name} --container {CONTAINER} {self.unit.name}:{TEMPEST_ADHOC_OUTPUT} validation.log"
 
     def _on_get_lists_action(self, event: ops.charm.ActionEvent) -> None:
         """List tempest test lists action."""

charms/tempest-k8s/src/handlers.py

@@ -41,9 +41,10 @@ from utils.constants import (
     OPENSTACK_PROJECT,
     OPENSTACK_ROLE,
     OPENSTACK_USER,
+    TEMPEST_ADHOC_OUTPUT,
     TEMPEST_HOME,
     TEMPEST_LIST_DIR,
-    TEMPEST_OUTPUT,
+    TEMPEST_PERIODIC_OUTPUT,
 )
 
 logger = logging.getLogger(__name__)
@@ -241,7 +242,7 @@ class TempestPebbleHandler(sunbeam_chandlers.ServicePebbleHandler):
         ]
 
         try:
-            output = self.execute(
+            summary = self.execute(
                 args,
                 user="tempest",
                 group="tempest",
@@ -249,14 +250,14 @@ class TempestPebbleHandler(sunbeam_chandlers.ServicePebbleHandler):
                 exception_on_error=True,
                 environment=env,
             )
-        except ops.pebble.ExecError as e:
+        except ops.pebble.ExecError:
-            if e.stdout:
+            raise RuntimeError(
-                output = f"{e.stdout}\n\n{e.stderr}"
+                "Error during test execution.\n"
-            else:
+                "For more information, copy log file from container by running:\n"
-                output = e.stderr
+                + self.charm.get_copy_log_cmd()
-            raise RuntimeError(output)
+            )
 
-        return output
+        return summary
 
 
 class TempestUserIdentityRelationHandler(sunbeam_rhandlers.RelationHandler):
@@ -576,7 +577,14 @@ class LoggingRelationHandler(sunbeam_rhandlers.RelationHandler):
             recursive=True,
             relation_name=self.relation_name,
             alert_rules_path="src/loki_alert_rules",
-            logs_scheme={"tempest": {"log-files": [TEMPEST_OUTPUT]}},
+            logs_scheme={
+                "tempest": {
+                    "log-files": [
+                        TEMPEST_PERIODIC_OUTPUT,
+                        TEMPEST_ADHOC_OUTPUT,
+                    ]
+                }
+            },
         )
         return interface
 

charms/tempest-k8s/src/templates/tempest-run-wrapper.j2

@@ -1,17 +1,39 @@
 #!/bin/bash
 # Do not change this file, this file is managed by juju.
 
-# set -e is important, to ensure the script bails out
+(flock -n 9 || {
-# if there are issues, such as lock not acquired,
+    # curly braces are important here, as they retain the same execution environment.
-# or failure in one of the tempest steps.
+    # we don't want to start a new subshell
-set -ex
+    echo "Lock could not be acquired, another test run is in progress."
-
+    exit 1
-(flock -n 9 || (echo "lock could not be acquired"; exit 1)
+}
-
-discover-tempest-config --test-accounts "$TEMPEST_TEST_ACCOUNTS" --out "$TEMPEST_CONF"
 
+# log everything to a tmpfile
 TMP_FILE="$(mktemp)"
-tempest run --workspace "$TEMPEST_WORKSPACE" "$@" 2>&1 | tee "$TMP_FILE"
+
+echo ":: discover-tempest-config" >> "$TMP_FILE"
+if discover-tempest-config --test-accounts "$TEMPEST_TEST_ACCOUNTS" --out "$TEMPEST_CONF" >> "$TMP_FILE" 2>&1; then
+    echo ":: tempest run" >> "$TMP_FILE"
+    tempest run --workspace "$TEMPEST_WORKSPACE" "$@" >> "$TMP_FILE" 2>&1
+else
+    echo ":: skipping tempest run because discover-tempest-config had errors" >> "$TMP_FILE"
+fi
+
+# tempest and discover-tempest-config can output escape sequences,
+# so remove them to neaten the output.
+sed $'s/\033\[[0-9;]*m//g' -i "$TMP_FILE"
+
+# After everything, move it to the actual output.
+# This ensures we don't have issues with logging libs pushing partial files,
+# if we were to stream to the final output.
 mv "$TMP_FILE" "$TEMPEST_OUTPUT"
 
+SUMMARY="$(awk '/^Totals$/,/Sum of execute/ { print }' < "$TEMPEST_OUTPUT")"
+if [[ -n "$SUMMARY" ]]; then
+    echo "$SUMMARY"
+else
+    echo "Error running the tests, please view the log file"
+    exit 1
+fi
+
 ) 9>/var/lock/tempest

charms/tempest-k8s/src/utils/constants.py

@@ -21,7 +21,8 @@ TEMPEST_CONF = f"{TEMPEST_WORKSPACE_PATH}/etc/tempest.conf"
 TEMPEST_TEST_ACCOUNTS = f"{TEMPEST_WORKSPACE_PATH}/test_accounts.yaml"
 TEMPEST_LIST_DIR = "/tempest_test_lists"
 # this file will contain the output from tempest's latest test run
-TEMPEST_OUTPUT = f"{TEMPEST_WORKSPACE_PATH}/tempest-output.log"
+TEMPEST_PERIODIC_OUTPUT = f"{TEMPEST_WORKSPACE_PATH}/tempest-periodic.log"
+TEMPEST_ADHOC_OUTPUT = f"{TEMPEST_WORKSPACE_PATH}/tempest-validation.log"
 # This is the workspace name registered with tempest.
 # It will be saved in a file in $HOME/.tempest/
 TEMPEST_WORKSPACE = "tempest"

charms/tempest-k8s/src/utils/types.py

@@ -0,0 +1,37 @@
+# Copyright 2024 Canonical Ltd.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Types for the tempest charm."""
+from enum import (
+    Enum,
+)
+
+from .constants import (
+    TEMPEST_ADHOC_OUTPUT,
+    TEMPEST_PERIODIC_OUTPUT,
+)
+
+
+class TempestEnvVariant(Enum):
+    """Represent a variant of the standard tempest environment."""
+
+    PERIODIC = 1
+    ADHOC = 2
+
+    def output_path(self) -> str:
+        """Return the correct tempest output path."""
+        return (
+            TEMPEST_PERIODIC_OUTPUT
+            if self.value == self.PERIODIC.value
+            else TEMPEST_ADHOC_OUTPUT
+        )

charms/tempest-k8s/tests/unit/test_tempest_charm.py

@@ -25,9 +25,14 @@ import ops_sunbeam.test_utils as test_utils
 import yaml
 from utils.constants import (
     CONTAINER,
+    TEMPEST_ADHOC_OUTPUT,
     TEMPEST_HOME,
+    TEMPEST_PERIODIC_OUTPUT,
     TEMPEST_READY_KEY,
 )
+from utils.types import (
+    TempestEnvVariant,
+)
 
 TEST_TEMPEST_ENV = {
     "OS_REGION_NAME": "RegionOne",
@@ -44,7 +49,7 @@ TEST_TEMPEST_ENV = {
     "TEMPEST_CONF": "/var/lib/tempest/workspace/etc/tempest.conf",
     "TEMPEST_HOME": "/var/lib/tempest",
     "TEMPEST_LIST_DIR": "/tempest_test_lists",
-    "TEMPEST_OUTPUT": "/var/lib/tempest/workspace/tempest-output.log",
+    "TEMPEST_OUTPUT": "/var/lib/tempest/workspace/tempest-validation.log",
     "TEMPEST_TEST_ACCOUNTS": "/var/lib/tempest/workspace/test_accounts.yaml",
     "TEMPEST_WORKSPACE": "tempest",
     "TEMPEST_WORKSPACE_PATH": "/var/lib/tempest/workspace",
@@ -248,8 +253,10 @@ class TestTempestOperatorCharm(test_utils.CharmTestCase):
             "test-list": "",
         }
         self.harness.charm._on_validate_action(action_event)
-        action_event.fail.assert_called_with(
+        action_event.fail.assert_called_once()
-            "'test(' is an invalid regex: missing ), unterminated subpattern at position 4"
+        self.assertEqual(
+            "'test(' is an invalid regex: missing ), unterminated subpattern at position 4",
+            action_event.set_results.call_args.args[0]["error"],
         )
 
         self.harness.remove_relation(logging_rel_id)
@@ -281,8 +288,10 @@ class TestTempestOperatorCharm(test_utils.CharmTestCase):
             "test-list": "nonexistent",
         }
         self.harness.charm._on_validate_action(action_event)
-        action_event.fail.assert_called_with(
+        action_event.fail.assert_called_once()
-            "'nonexistent' is not a known test list. Please run list-tests action to view available lists."
+        self.assertEqual(
+            "'nonexistent' is not a known test list. Please run list-tests action to view available lists.",
+            action_event.set_results.call_args.args[0]["error"],
         )
 
         self.harness.remove_relation(logging_rel_id)
@@ -401,7 +410,7 @@ class TestTempestOperatorCharm(test_utils.CharmTestCase):
         action_event.fail.assert_called_once()
         self.assertIn(
             "No filter parameters provided",
-            action_event.fail.call_args.args[0],
+            action_event.set_results.call_args.args[0]["error"],
         )
         exec_mock.assert_not_called()
 
@@ -610,3 +619,12 @@ class TestTempestOperatorCharm(test_utils.CharmTestCase):
         self.harness.charm.set_tempest_ready = mock.Mock()
         self.harness.charm._on_upgrade_charm(mock.Mock())
         self.harness.charm.set_tempest_ready.assert_called_once_with(False)
+
+    def test_tempest_env_variant(self):
+        """Test env variant for tempest returns correct path."""
+        self.assertEqual(
+            TempestEnvVariant.PERIODIC.output_path(), TEMPEST_PERIODIC_OUTPUT
+        )
+        self.assertEqual(
+            TempestEnvVariant.ADHOC.output_path(), TEMPEST_ADHOC_OUTPUT
+        )