Skip to content
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

nixos/test-runner: Fix execute() flakiness #142747

Merged
merged 2 commits into from
Nov 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions nixos/doc/manual/development/writing-nixos-tests.section.md
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,11 @@ The following methods are available on machine objects:
`execute`

: Execute a shell command, returning a list `(status, stdout)`.
Takes an optional parameter `check_return` that defaults to `True`.
Setting this parameter to `False` will not check for the return code
and return -1 instead. This can be used for commands that shut down
the VM and would therefore break the pipe that would be used for
retrieving the return code.

`succeed`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,13 @@ start_all()
<listitem>
<para>
Execute a shell command, returning a list
<literal>(status, stdout)</literal>.
<literal>(status, stdout)</literal>. Takes an optional
parameter <literal>check_return</literal> that defaults to
<literal>True</literal>. Setting this parameter to
<literal>False</literal> will not check for the return code
and return -1 instead. This can be used for commands that shut
down the VM and would therefore break the pipe that would be
used for retrieving the return code.
</para>
</listitem>
</varlistentry>
Expand Down
40 changes: 28 additions & 12 deletions nixos/lib/test-driver/test-driver.py
Original file line number Diff line number Diff line change
Expand Up @@ -581,24 +581,40 @@ def require_unit_state(self, unit: str, require_state: str = "active") -> None:
+ "'{}' but it is in state ‘{}’".format(require_state, state)
)

def execute(self, command: str) -> Tuple[int, str]:
def _next_newline_closed_block_from_shell(self) -> str:
assert self.shell
output_buffer = []
while True:
# This receives up to 4096 bytes from the socket
chunk = self.shell.recv(4096)
if not chunk:
# Probably a broken pipe, return the output we have
break

decoded = chunk.decode()
output_buffer += [decoded]
if decoded[-1] == "\n":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this guaranteed to be the last character? What happens if we receive two lines in one recv call?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this breaks. But I don't see how we would receive two lines when we base64-encode the output and tell base64 to not add \n after 76 characters

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the exit code is won't be printed before this method returns, so lgtm.

break
return "".join(output_buffer)

def execute(self, command: str, check_return: bool = True) -> Tuple[int, str]:
self.connect()

out_command = "( set -euo pipefail; {} ); echo '|!=EOF' $?\n".format(command)
out_command = f"( set -euo pipefail; {command} ) | (base64 --wrap 0; echo)\n"
assert self.shell
self.shell.send(out_command.encode())

output = ""
status_code_pattern = re.compile(r"(.*)\|\!=EOF\s+(\d+)")
# Get the output
output = base64.b64decode(self._next_newline_closed_block_from_shell())

while True:
chunk = self.shell.recv(4096).decode(errors="ignore")
match = status_code_pattern.match(chunk)
if match:
output += match[1]
status_code = int(match[2])
return (status_code, output)
output += chunk
if not check_return:
return (-1, output.decode())

# Get the return code
self.shell.send("echo ${PIPESTATUS[0]}\n".encode())
rc = int(self._next_newline_closed_block_from_shell().strip())

return (rc, output.decode())

def shell_interact(self) -> None:
"""Allows you to interact with the guest shell
Expand Down
4 changes: 2 additions & 2 deletions nixos/tests/hibernate.nix
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ in makeTest {
"mkswap /dev/vda1 -L swap",
# Install onto /mnt
"nix-store --load-db < ${pkgs.closureInfo {rootPaths = [installedSystem];}}/registration",
"nixos-install --root /mnt --system ${installedSystem} --no-root-passwd",
"nixos-install --root /mnt --system ${installedSystem} --no-root-passwd --no-channel-copy >&2",
)
machine.shutdown()
Expand All @@ -110,7 +110,7 @@ in makeTest {
)
# Hibernate machine
hibernate.succeed("systemctl hibernate &")
hibernate.execute("systemctl hibernate &", check_return=False)
hibernate.wait_for_shutdown()
# Restore machine from hibernation, validate our ramfs file is there.
Expand Down
2 changes: 1 addition & 1 deletion nixos/tests/kexec.nix
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import ./make-test-python.nix ({ pkgs, lib, ...} : {
testScript =
''
machine.wait_for_unit("multi-user.target")
machine.execute("systemctl kexec &")
machine.execute("systemctl kexec &", check_return=False)
machine.connected = False
machine.wait_for_unit("multi-user.target")
'';
Expand Down
3 changes: 2 additions & 1 deletion nixos/tests/switch-test.nix
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,8 @@ import ./make-test-python.nix ({ pkgs, ...} : {
machine.succeed("touch /testpath")
machine.wait_until_succeeds("test -f /testpath-modified")
machine.succeed("rm /testpath /testpath-modified")
machine.succeed("rm /testpath")
machine.succeed("rm /testpath-modified")
switch_to_specialisation("with-path-modified")
machine.succeed("touch /testpath")
Expand Down