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.

Sign up for GitHub

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

Jump to bottom

Make the pidfile accessible by everyone #3252

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Make the pidfile accessible by everyone #3252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4786897
Makes the pidfile accessible by everyone
mcasquer Oct 1, 2024
2dd6f74
Adds a check to only create the pid directory
mcasquer Nov 4, 2024
d2ee700
Fixes the tmt reboot test, updating the pidfile location in the scripts.
mcasquer Nov 6, 2024
74ec495
Updates the environment variables in out-of-session test.
mcasquer Nov 6, 2024
f9c23a0
Removes the sudo(s) from tmt commands and instead apply it directly
mcasquer Nov 6, 2024
026d3ed
Removes the self.become this way the condition is applied
mcasquer Nov 7, 2024
0f1a806
Changes bfu_* options to use unprivileged Fedora container image
mcasquer Nov 12, 2024
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
8 changes: 7 additions & 1 deletion tests/execute/reboot/out-of-session.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@
rlJournalStart
rlPhaseStartSetup
rlRun "PROVISION_HOW=${PROVISION_HOW:-container}"

# To allow running the test outside of tmt.
rlRun "TMT_TEST_PIDFILE_ROOT=/var/tmp/tmt/pid"
rlRun "TMT_TEST_PIDFILE=$TMT_TEST_PIDFILE_ROOT/tmt-test.pid"
rlRun "TMT_TEST_PIDFILE_LOCK=$TMT_TEST_PIDFILE.lock"

rlRun "run=\$(mktemp -d -p /var/tmp)" 0 "Create run directory"
rlRun "set -o pipefail"
rlRun "pushd out-of-session"
Expand Down Expand Up @@ -39,7 +45,7 @@ rlJournalStart
# and use it to issue a `tmt-reboot` from outside the direct process tree of the test.
set -x

tmt_reboot_command="export TMT_TEST_PIDFILE=/var/tmp/tmt-test.pid; export TMT_TEST_PIDFILE_LOCK=/var/tmp/tmt-test.pid.lock; export TMT_DEBUG=1; tmt-reboot"
tmt_reboot_command="export TMT_TEST_PIDFILE=$TMT_TEST_PIDFILE; export TMT_TEST_PIDFILE_LOCK=$TMT_TEST_PIDFILE_LOCK; export TMT_DEBUG=1; tmt-reboot"

if [ "$PROVISION_HOW" = "container" ]; then
podman_exec="$(sed -nr 's/\s*Run command: (podman exec .*) \/bin\/bash.*cd.*/\1/p' tmt.output | head -1)"
Expand Down
6 changes: 4 additions & 2 deletions tests/provision/facts/test-guest.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
# vim: dict+=/usr/share/beakerlib/dictionary.vim cpt=.,w,b,u,t,i,k
. /usr/share/beakerlib/beakerlib.sh || exit 1

TEST_IMAGE_PREFIX="localhost/tmt/tests/container"

rlJournalStart
rlPhaseStartSetup
rlRun "PROVISION_HOW=${PROVISION_HOW:-local}"
Expand Down Expand Up @@ -40,8 +42,8 @@ rlJournalStart
fi

elif [ "$PROVISION_HOW" = "container" ]; then
provision_options="--image fedora:39"
bfu_provision_options="$provision_options --user=nobody"
provision_options="--image $TEST_IMAGE_PREFIX/fedora/39:latest"
bfu_provision_options="--image $TEST_IMAGE_PREFIX/fedora/39/unprivileged:latest --user=fedora"

arch="$(arch)"
distro="Fedora Linux 39 (Container Image)"
Expand Down
6 changes: 5 additions & 1 deletion tmt/steps/execute/internal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@
format_timestamp,
)

# Note: if modified, pidfile root, filename, and lock filename must be
# also changed in `tmt-reboot` and `tmt-reboot-core` scripts.
TEST_PIDFILE_FILENAME = 'tmt-test.pid'
TEST_PIDFILE_LOCK_FILENAME = f'{TEST_PIDFILE_FILENAME}.lock'

#: The default directory for storing test pid file.
TEST_PIDFILE_ROOT = Path('/var/tmp') # noqa: S108 insecure usage of temporary dir
TEST_PIDFILE_ROOT = Path('/var/tmp/tmt/pid') # noqa: S108 insecure usage of temporary dir


def effective_pidfile_root() -> Path:
Expand Down Expand Up @@ -258,6 +260,8 @@ def _test_environment(
assert isinstance(self.parent, tmt.steps.execute.Execute)
assert self.parent.plan.my_run is not None

environment['TMT_TEST_PIDFILE_ROOT'] = EnvVarValue(
effective_pidfile_root())
environment['TMT_TEST_PIDFILE'] = EnvVarValue(
effective_pidfile_root() / TEST_PIDFILE_FILENAME)
environment['TMT_TEST_PIDFILE_LOCK'] = EnvVarValue(
Expand Down
4 changes: 3 additions & 1 deletion tmt/steps/execute/scripts/tmt-reboot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@

[ -n "$TMT_DEBUG" ] && set -x

TMT_TEST_PIDFILE_LOCK="${TMT_TEST_PIDFILE_LOCK:-/var/tmp/tmt-test.pid}"
TMT_TEST_PIDFILE_ROOT="${TMT_TEST_PIDFILE_ROOT:-/var/tmp/tmt/pid}"
TMT_TEST_PIDFILE="${TMT_TEST_PIDFILE:-$TMT_TEST_PIDFILE_ROOT/tmt-test.pid}"
TMT_TEST_PIDFILE_LOCK="${TMT_TEST_PIDFILE_LOCK:-$TMT_TEST_PIDFILE.lock}"

PATH=/sbin:/usr/sbin:$PATH

Expand Down
4 changes: 3 additions & 1 deletion tmt/steps/execute/scripts/tmt-reboot-core
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

[ -n "$TMT_DEBUG" ] && set -x

TMT_TEST_PIDFILE="${TMT_TEST_PIDFILE:-/var/tmp/tmt-test.pid}"
TMT_TEST_PIDFILE_ROOT="${TMT_TEST_PIDFILE_ROOT:-/var/tmp/tmt/pid}"
TMT_TEST_PIDFILE="${TMT_TEST_PIDFILE:-$TMT_TEST_PIDFILE_ROOT/tmt-test.pid}"
TMT_TEST_PIDFILE_LOCK="${TMT_TEST_PIDFILE_LOCK:-$TMT_TEST_PIDFILE.lock}"

if [ ! -e "$TMT_TEST_PIDFILE" ]; then
echo "There is no running test to signal the reboot to!"
Expand Down
12 changes: 12 additions & 0 deletions tmt/steps/provision/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -923,6 +923,17 @@ def setup(self) -> None:

Setup the guest after it has been started. It is called after :py:meth:`Guest.start`.
"""
from tmt.steps.execute.internal import effective_pidfile_root
Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing I noticed: there's Guest.setup() and GuestSsh.setup(). The former is pretty empty, and you are adding first code into it; the latter already contains some code.

Byw GuestSsh.setup() does not call its base class implementation, it lacks super().setup() call. Looks like an omission which was fine, Guest.setup() did nothing so it didn't hurt. Now it does, and we need to call it. Please, add the super() call to GuestSsh so we have the chain complete.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So with this you mean it'd better if I move the code from Guest.setup() to the GuestSsh one and add the super().setup() ? or only do the last part in the Guest.setup() function?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nono, I think Guest is the right place. We just need one extra call at the beginning of GuestSsh.setup, and that would be calling its base class implementation of the method it overrides.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, code updated, thanks!

sudo = 'sudo' if not self.facts.is_superuser else ''
pid_directory = effective_pidfile_root()
self.execute(ShellScript(
f"""
if [ ! -d {pid_directory} ]; then \
martinhoyer marked this conversation as resolved.
Show resolved Hide resolved
{sudo} mkdir -p {pid_directory} \
&& {sudo} chmod ugo+rwx {pid_directory}; \
fi
"""
))

# A couple of requiremens for this field:
#
Expand Down Expand Up @@ -1709,6 +1720,7 @@ def is_ready(self) -> bool:
return self.primary_address is not None

def setup(self) -> None:
super().setup()
if self.is_dry_run:
return
if not self.facts.is_superuser and self.become:
Expand Down
Loading