-
Notifications
You must be signed in to change notification settings - Fork 371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support to execute tests on multiple distros #2725
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2725 +/- ##
===========================================
+ Coverage 72.03% 72.04% +0.01%
===========================================
Files 104 104
Lines 15832 15832
Branches 2265 2265
===========================================
+ Hits 11404 11406 +2
- Misses 3909 3912 +3
+ Partials 519 514 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -40,121 +42,147 @@ | |||
from tests_e2e.scenarios.lib.logging import log | |||
|
|||
|
|||
class AgentLisaTestContext(AgentTestContext): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this class local to AgentTestSuite
|
||
if self.context.working_directory.exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleanup of the working directory was getting in the way of debugging, since sometimes one needs to check its contents. Instead of cleaning up, now I use a unique working directory per runbook execution.
The cleanup was mainly targeted to the developer scenario, so with these changes development machines may start accumulating many test files. That can be handled by our helper scripts.
def _clean_up(self) -> None: | ||
""" | ||
Cleans up any leftovers from the test suite run. Currently just an empty placeholder for future use. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleanup used to remove the working directory. Since this is part of the protocol enforced by self.execute(), I left it here as an empty method in case we need it in the future.
|
||
# Fail the entire test suite if any test failed | ||
assert_that(failed).described_as("One or more tests failed").is_length(0) | ||
if len(failed) > 0: | ||
fail(f"{[self.context.suite_name]} One or more tests failed: {failed}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using fail() instead of assert)that() improves the formatting of the error message a little bit.
if result.stdout != "": | ||
separator = "\n" if "\n" in result.stdout else " " | ||
log.info("stdout:%s%s", separator, result.stdout) | ||
if result.stderr != "": | ||
separator = "\n" if "\n" in result.stderr else " " | ||
log.error("stderr:%s%s", separator, result.stderr) | ||
|
||
if result.exit_code != 0: | ||
raise Exception(f"[{command_line}] failed. Exit code: {result.exit_code}") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to raise the exception after logging the command's output
logs_file_name="$HOME/logs.tgz" | ||
# Note that we do "set -euxo pipefail" only after executing "tar". That command exits with code 1 on warnings | ||
# and we do not want to consider those as failures. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what was preventing log collection on warnings
@@ -51,9 +51,22 @@ if [ "$#" -ne 0 ] || [ -z ${package+x} ] || [ -z ${version+x} ]; then | |||
fi | |||
|
|||
# | |||
# The service name is walinuxagent in Ubuntu and waagent elsewhere | |||
# Find the command to manage services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ubuntu has a 'service' wrapper on top of 'systemctl' but some of the distros I added don't, so our script needs to handle that difference.
Note that other distros that we will add do not use systemd, and services are managed with the 'service' command.
@@ -1,29 +1,23 @@ | |||
name: azure | |||
name: Daily |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using the daily.yml both for creating new VMs or use existing ones. With the addition of the combinator to execute multiple distros that is no longer possible.
I split the existing VM setup into a new runbook in the samples directory.
|
||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ssh proxy settings will be shared by multiple runbooks, so I refactored it into its own file
@@ -23,14 +23,20 @@ RUN \ | |||
# \ | |||
# Install basic dependencies \ | |||
# \ | |||
apt-get install -y git python3.10 python3.10-dev curl && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curl was previously use to download Poetry in order to setup LISA, but their new installation is not based on Poetry so curl is no longer needed
ln /usr/bin/python3.10 /usr/bin/python3 && \ | ||
\ | ||
# \ | ||
# Install LISA dependencies \ | ||
# \ | ||
apt-get install -y git gcc libgirepository1.0-dev libcairo2-dev qemu-utils libvirt-dev \ | ||
python3-pip python3-venv && \ | ||
\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest Ubuntu image on Dockerhub doesn't have zip, so installing explicitly
exit $exit_code | ||
fi | ||
|
||
chmod +r "$logs_file_name" | ||
chmod a+r "$logs_file_name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"a" (i.e. "all") needs to be given explicitly, since Mariner's UMASK turns the read permission off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The PR includes support to test execution on multiple distros. The working directory needs to account for multiple test suites executed concurrently, the log needs to add a field identifying the test suite and distro, and there are a few distro-compatibility fixes in the remote scripts.
The PR also includes a fix for an issue that was preventing log collection in some cases.
The list of distros in this PR matches the DCR v2 prototype, but additional changes will be needed later.