tmt icon indicating copy to clipboard operation
tmt copied to clipboard

Only first user can run provision -h local. Other lack lock file permissions.

Open lukaszachy opened this issue 2 years ago • 8 comments

As a root create two users and execute tmt run for each of them in sequence:

# useradd A
# su -l A
$ tmt init -t mini
$ tmt run -a provision -h local
....
total: 1 test passed

$ logout

# useradd B
# su -l B
$ tmt init -t mini
$ tmt run -a provision -h local
...
warn: Test failed to manage its pidfile.
...
total: 1 error

Debug output shows

            errr /script-00 (pidfile locking)
                output.txt: /var/tmp/tmt/run-007/plans/example/execute/data/guest/default-0/script-00-1/output.txt
        Workdir '/var/tmp/tmt/run-007/plans/example/report/default-0' created.
        Read file '/var/tmp/tmt/run-007/plans/example/execute/data/guest/default-0/script-00-1/output.txt'.
                content: flock: cannot open lock file /var/tmp/tmt-test.pid.lock: Permission denied

lukaszachy avatar Apr 19 '24 13:04 lukaszachy

Summary from the discussion: We could make the pidfile writeable by anyone. Looks like a first good issue. Let's see!

psss avatar Jun 25 '24 09:06 psss

Summary from the discussion: We could make the pidfile writeable by anyone. Looks like a first good issue. Let's see!

@psss I've been thinking about two possible solutions

  1. Make the pidfile creation dependent on the current user, this way permission errors are directly avoided, i.e. creating the file at some place under the user's home directory
  2. Changing the owner of the pidfile (777 permissions don't work :confused:) to the current one?, but this would need root permissions so it could be a problem as well

mcasquer avatar Sep 24 '24 09:09 mcasquer

Make the pidfile creation dependent on the current user, this way permission errors are directly avoided, i.e. creating the file at some place under the user's home directory

I'm afraid that would break tmt-reboot when called from outside of the test session as we need to have a standard location where to look for the lock:

https://github.com/teemtee/tmt/blob/6d08a7bcdf83cc5a3dedd978115109b4bfbb3314/tmt/steps/execute/scripts/tmt-reboot#L3-L9

Changing the owner of the pidfile (777 permissions don't work 😕) to the current one?, but this would need root permissions so it could be a problem as well

Hm, could it be caused by the sticky bit of /var/tmp? So that only owner of the file can get the lock? I quickly tried with a world-writable directory:

mkdir /var/tmp/pid
chmod ugo+rwx /var/tmp/pid
TMT_TEST_PIDFILE_ROOT=/var/tmp/pid tmt run -av provision -h local

This seems to be working. Not sure how safe this approach would be though. @happz, any thoughts on this?

psss avatar Sep 24 '24 16:09 psss

The only hard rule is probably the location, pid file must be possible to find from multiple tmt invocations so one can reboot the machine while the other is still running. So any directory on which they can agree should be fine. This most likely rules out $HOME, one instance might be running under root, other under test with sudo, so home directory will be different and therefore permissions will kill it anyway. /var/tmp/pid should work, but I think we had a problem when the pid file was located under /var/tmp/tmt, let me try find it in history. Might be relevant for /var/tmp/pid.

happz avatar Sep 25 '24 13:09 happz

"Change the default test pidfile directory to /var/tmp": https://github.com/teemtee/tmt/commit/7552550ff21ee3864814159be587e4604c125137

happz avatar Sep 25 '24 13:09 happz

@psss @happz I'll try to be more specific, likely I am missing something obvious

Including the following code at internal.py creates once the desired path and gives it full permissions:

pid_directory = '/var/tmp/pid'
Path(pid_directory).mkdir(parents=True, exist_ok=True)
os.chmod(pid_directory, 0o777)
TEST_PIDFILE_ROOT = Path(pid_directory)  # noqa: S108 insecure usage of temporary dir

After tmt run ... the pidfile is successfully created

total: 1 test passed
$ ls -lrsta /var/tmp/pid
total 4
4 drwxrwxrwt. 12 root root 4096 Sep 26 04:43 ..
0 -rw-r--r--   1 root root    0 Sep 26 04:43 tmt-test.pid.lock
0 drwxrwxrwx   2 root root   31 Sep 26 04:43 .

But if It try with a different user

$ tmt run -a provision -h local
Error: failed while importing tmt package: [Errno 1] Operation not permitted: '/var/tmp/pid'

For sure, I can access the directory, so that's why I thought about the owner issue

mcasquer avatar Sep 26 '24 08:09 mcasquer

@psss @happz I'll try to be more specific, likely I am missing something obvious

Including the following code at internal.py creates once the desired path and gives it full permissions:

pid_directory = '/var/tmp/pid'
Path(pid_directory).mkdir(parents=True, exist_ok=True)
os.chmod(pid_directory, 0o777)
TEST_PIDFILE_ROOT = Path(pid_directory)  # noqa: S108 insecure usage of temporary dir

After tmt run ... the pidfile is successfully created

total: 1 test passed
$ ls -lrsta /var/tmp/pid
total 4
4 drwxrwxrwt. 12 root root 4096 Sep 26 04:43 ..
0 -rw-r--r--   1 root root    0 Sep 26 04:43 tmt-test.pid.lock
0 drwxrwxrwx   2 root root   31 Sep 26 04:43 .

But if It try with a different user

$ tmt run -a provision -h local
Error: failed while importing tmt package: [Errno 1] Operation not permitted: '/var/tmp/pid'

For sure, I can access the directory, so that's why I thought about the owner issue

Can you run it with TMT_SHOW_TRACEBACK=1 envvar set? My guess: directory exists, but it's owned by root, your user is not root and tries to chmod of a directory owned by root. OS will not let that happen.

happz avatar Sep 26 '24 08:09 happz

Can you run it with TMT_SHOW_TRACEBACK=1 envvar set? My guess: directory exists, but it's owned by root, your user is not root and tries to chmod of a directory owned by root. OS will not let that happen.

@happz thanks ! Indeed, if only the chmod is done when the user matches to the file owner the issue is solved, I will send a PR for this issue :)

mcasquer avatar Sep 26 '24 11:09 mcasquer

Moved to 1.43 as the underlying PRs still need more work, and they were already moved.

thrix avatar Jan 28 '25 14:01 thrix

The pidfile fix breaks virtual provision, this will need more time, moving to backlog.

psss avatar Mar 26 '25 07:03 psss