From e9d26325f714d32a26cff21477b2fd4c6bfa102c Mon Sep 17 00:00:00 2001 From: Samuel Allan Date: Thu, 1 Feb 2024 12:54:43 +1030 Subject: [PATCH] Reduce output from validation action The results from tempest can be very large, so reduce the output from the validation action (only print the summary), and provide an efficient method for obtaining the complete log file. Change-Id: Ie8366ce6744146b63b751e3df528b9405bfa21a7 --- charms/tempest-k8s/charmcraft.yaml | 8 ++++ charms/tempest-k8s/src/charm.py | 43 +++++++++++++------ charms/tempest-k8s/src/handlers.py | 28 +++++++----- .../src/templates/tempest-run-wrapper.j2 | 40 +++++++++++++---- charms/tempest-k8s/src/utils/constants.py | 3 +- charms/tempest-k8s/src/utils/types.py | 37 ++++++++++++++++ .../tests/unit/test_tempest_charm.py | 30 ++++++++++--- 7 files changed, 151 insertions(+), 38 deletions(-) create mode 100644 charms/tempest-k8s/src/utils/types.py diff --git a/charms/tempest-k8s/charmcraft.yaml b/charms/tempest-k8s/charmcraft.yaml index 5b1ead80..40b06c17 100644 --- a/charms/tempest-k8s/charmcraft.yaml +++ b/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: diff --git a/charms/tempest-k8s/src/charm.py b/charms/tempest-k8s/src/charm.py index 02bd5458..49920e23 100755 --- a/charms/tempest-k8s/src/charm.py +++ b/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)) - # still print the message, - # because it could be a lot of output from tempest, - # and we want it neatly formatted - print(e) + # put the message in set_results instead of event.fail, + # because event.fail message is not always displayed to the user: + # https://bugs.launchpad.net/juju/+bug/2052765 + event.set_results({"error": str(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.""" diff --git a/charms/tempest-k8s/src/handlers.py b/charms/tempest-k8s/src/handlers.py index 51974161..ae1b35f3 100644 --- a/charms/tempest-k8s/src/handlers.py +++ b/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: - if e.stdout: - output = f"{e.stdout}\n\n{e.stderr}" - else: - output = e.stderr - raise RuntimeError(output) + except ops.pebble.ExecError: + raise RuntimeError( + "Error during test execution.\n" + "For more information, copy log file from container by running:\n" + + self.charm.get_copy_log_cmd() + ) - 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 diff --git a/charms/tempest-k8s/src/templates/tempest-run-wrapper.j2 b/charms/tempest-k8s/src/templates/tempest-run-wrapper.j2 index 01033fc0..50c7d8d7 100644 --- a/charms/tempest-k8s/src/templates/tempest-run-wrapper.j2 +++ b/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 -# if there are issues, such as lock not acquired, -# or failure in one of the tempest steps. -set -ex - -(flock -n 9 || (echo "lock could not be acquired"; exit 1) - -discover-tempest-config --test-accounts "$TEMPEST_TEST_ACCOUNTS" --out "$TEMPEST_CONF" +(flock -n 9 || { + # curly braces are important here, as they retain the same execution environment. + # we don't want to start a new subshell + echo "Lock could not be acquired, another test run is in progress." + exit 1 +} +# 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 diff --git a/charms/tempest-k8s/src/utils/constants.py b/charms/tempest-k8s/src/utils/constants.py index 1c40d56e..c794502e 100644 --- a/charms/tempest-k8s/src/utils/constants.py +++ b/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" diff --git a/charms/tempest-k8s/src/utils/types.py b/charms/tempest-k8s/src/utils/types.py new file mode 100644 index 00000000..db9d726f --- /dev/null +++ b/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 + ) diff --git a/charms/tempest-k8s/tests/unit/test_tempest_charm.py b/charms/tempest-k8s/tests/unit/test_tempest_charm.py index e9a10906..2c36a0a2 100644 --- a/charms/tempest-k8s/tests/unit/test_tempest_charm.py +++ b/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( - "'test(' is an invalid regex: missing ), unterminated subpattern at position 4" + action_event.fail.assert_called_once() + 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( - "'nonexistent' is not a known test list. Please run list-tests action to view available lists." + action_event.fail.assert_called_once() + 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 + )