2093291: Make locking more reliable

[jirihnidek]

• Card ID: ENT-5077
• BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2093291
• When the /run is mounted read-only, then try to create lock file in /run/users, which should never be mounted as read-only. The path to user's runtime directory should be defined in $XDG_RUNTIME_DIR
• Added some doc strings and type hints for LockFile class and added more debug prints
• Added some unit tests

[jirihnidek]

The changes in ActionLock works, however I think they are a bit more complicated than actually needed. I think what is needed is something simpler, i.e.:

* if `XDG_RUNTIME_DIR` is set, assume it is usable: according to the XDG basedir spec [1] it has various constraints already (ownership, permissions, etc), which are usually satisfied by applications that set it in modern systems (e.g. systemd/pam, podman, etc); in case it is manually set by the user, the user must take care of creating it properly already; because of this, applications generally use `XDG_RUNTIME_DIR` directly, and misconfigurations are quite problematic already

* fallback to `/run` as done so far

Once one of the two is picked, use it directly; Lock.__init__() takes care already to create e.g. missing subdirectories in it. The scenario I don't think it is worth considering is "read-only /usr and no XDG_RUNTIME_DIR set": I checked various setups, and I did not find any situation in which this happens:

* as user on a normal Red Hat system, `XDG_RUNTIME_DIR` is set and usable (subscription-manager does not run as root, so I'm simply mentioning here the situation)

* as root on a normal Red Hat system, when logging in from an user using `su -`: `XDG_RUNTIME_DIR` is not set -> `/run` is usable, use it

* as root on a normal Red Hat system, when logging in from ssh: `XDG_RUNTIME_DIR` is set to `/run/user/0` -> usable, use it

* testing the instructions with buildah of rhbz#2093291: `XDG_RUNTIME_DIR` is set to `/run/user/$id_of_user_staring_buildah` -> usable, use it

Hmm, I wonder whether running subscription-manager from ssh will not use the same lock as used by subscription-manager as run in other contexts, e.g. when logging in locally...

[1] https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

sudo -i

[ptoscano]

sudo -i

Hmm I tried this on a RHEL 9 VM, and after sudo -i there are no XDG_* environment variables set (XDG_RUNTIME_DIR included). What do you get in this situation?

[jirihnidek]

sudo -i

Hmm I tried this on a RHEL 9 VM, and after sudo -i there are no XDG_* environment variables set (XDG_RUNTIME_DIR included). What do you get in this situation?

Yeah, no XDG_* environment variable. Fedora has XDG_DATA_DIRS, but doesn't help in this case. I haven't test any service, but I would not be surprised if there are not any XDG_* environment variables too.

BTW: acquire() method of Lock class is blocking in all cases despite blocking = False, because we call fcntl.flock() only with fcntl.LOCK_EX and we don't call it with fnctl.LOCK_NB, when blocking is False. I hope that I will not find any other skeleton in this code. :-/

[ptoscano]

Coming back to this:

• some of my old notes are still not addressed/answered
• wrt my previous review (https://github.com/candlepin/subscription-manager/pull/3134#pullrequestreview-1135944124), it isn't clear to me what was the followup on it; in particular, I think the additions to ActionLock are a bit inconsistent, as there are 3 different ways the 3 possible directories are used/tried

[jirihnidek]

Coming back to this:

* some of my old notes are still not addressed/answered

* wrt my previous review ([2093291: Make locking more reliable #3134 (review)](https://github.com/candlepin/subscription-manager/pull/3134#pullrequestreview-1135944124)), it isn't clear to me what was the followup on it; in particular, I think the additions to `ActionLock` are a bit inconsistent, as there are 3 different ways the 3 possible directories are used/tried

What specific notes were not addressed/answered?

There are really three different ways and three possible directories and all of them are used in three different situations and all of them are correct.

[ptoscano]

What specific notes were not addressed/answered?

There are still some inline notes open, and what I wrote about them is still valid in the current revision.

There are really three different ways and three possible directories and all of them are used in three different situations and all of them are correct.

As I wrote earlier, I cannot reproduce the situation that makes one of them needed -- I still think using two, either $XDG_RUNTIME_DIR if set or /run as fallback should work just fine. Can you please help me try to reproduce the other situation?

[github-actions[bot]]

Coverage (computed on Fedora latest) •

File	Stmts	Miss	Cover	Missing
subscription_manager
lock.py	180	35	80%	50, 111–112, 115–116, 143, 152–159, 180, 209–210, 217, 221, 226, 238, 273–274, 289, 292–294, 296, 302–304, 306–309
TOTAL	18108	4426	75%

Tests	Skipped	Failures	Errors	Time
2625	7 :zzz:	0 :x:	0 :fire:	55.475s :stopwatch:

[jirihnidek]

There are three possible scenarios:

1. Classical locking, when /run/rhsm/cert.pid is readable/writable
2. Environment variable $XDG_RUNTIME_DIR exists and it is possible to use: "${XDG_RUNTIME_DIR}/rhsm/cert.pid"
3. Environment variable does not exists $XDG_RUNTIME_DIR (e.g. sudo is used, or the env. var. is not set, or this env. var. was unset, etc.) Thus we try to create/read/write lock file in this directory: "/run/user/${UID}"

[ptoscano]

There are three possible scenarios:

1. Classical locking, when `/run/rhsm/cert.pid` is readable/writable

2. Environment variable `$XDG_RUNTIME_DIR` exists and it is possible to use: `"${XDG_RUNTIME_DIR}/rhsm/cert.pid"`

3. Environment variable does not exists `$XDG_RUNTIME_DIR` (e.g. `sudo` is used, or the env. var. is not set, or this env. var. was unset, etc.) Thus we try to create/read/write lock file in this directory: `"/run/user/${UID}"`

I don't see any difference between (1) and (3), and both seem to me the same case of "$XDG_RUNTIME_DIR is not set". I still think we should not over-complicate this, and rather do the following:

• $XDG_RUNTIME_DIR is set and can be used -> use it
• $XDG_RUNTIME_DIR is not set or it is not usable -> use /run as done so far

which is what I suggested already 4 months ago... https://github.com/candlepin/subscription-manager/pull/3134#pullrequestreview-1135944124

[jirihnidek]

There are three possible scenarios:

1. Classical locking, when `/run/rhsm/cert.pid` is readable/writable

2. Environment variable `$XDG_RUNTIME_DIR` exists and it is possible to use: `"${XDG_RUNTIME_DIR}/rhsm/cert.pid"`

3. Environment variable does not exists `$XDG_RUNTIME_DIR` (e.g. `sudo` is used, or the env. var. is not set, or this env. var. was unset, etc.) Thus we try to create/read/write lock file in this directory: `"/run/user/${UID}"`

I don't see any difference between (1) and (3), and both seem to me the same case of "$XDG_RUNTIME_DIR is not set". I still think we should not over-complicate this, and rather do the following:

* `$XDG_RUNTIME_DIR` is set and can be used -> use it

* `$XDG_RUNTIME_DIR` is not set or it is not usable -> use `/run` as done so far

which is what I suggested already 4 months ago... #3134 (review)

There is difference between (1) and (3)... Variant (1) uses /run/rhsm/, which is read-only in buildah environment. Variant (3) uses directory that is writable in this environment and it especially useful in the case, when somebody tries to use sudo in buildah environment (e.g. copy-pasted code containing sudo at the beginning in the command). I would not implement it, if I didn't have good reason for that.

[ptoscano]

There are three possible scenarios:

1. Classical locking, when `/run/rhsm/cert.pid` is readable/writable

2. Environment variable `$XDG_RUNTIME_DIR` exists and it is possible to use: `"${XDG_RUNTIME_DIR}/rhsm/cert.pid"`

3. Environment variable does not exists `$XDG_RUNTIME_DIR` (e.g. `sudo` is used, or the env. var. is not set, or this env. var. was unset, etc.) Thus we try to create/read/write lock file in this directory: `"/run/user/${UID}"`

I don't see any difference between (1) and (3), and both seem to me the same case of "$XDG_RUNTIME_DIR is not set". I still think we should not over-complicate this, and rather do the following:

* `$XDG_RUNTIME_DIR` is set and can be used -> use it

* `$XDG_RUNTIME_DIR` is not set or it is not usable -> use `/run` as done so far

which is what I suggested already 4 months ago... #3134 (review)

There is difference between (1) and (3)... Variant (1) uses /run/rhsm/, which is read-only in buildah environment. Variant (3) uses directory that is writable in this environment and it especially useful in the case, when somebody tries to use sudo in buildah environment (e.g. copy-pasted code containing sudo at the beginning in the command). I would not implement it, if I didn't have good reason for that.

However in buildah $XDG_RUNTIME_DIR is set (variant (2)), which is what I suggest to try to use as first step.

[jirihnidek]

There are three possible scenarios:

1. Classical locking, when `/run/rhsm/cert.pid` is readable/writable

2. Environment variable `$XDG_RUNTIME_DIR` exists and it is possible to use: `"${XDG_RUNTIME_DIR}/rhsm/cert.pid"`

3. Environment variable does not exists `$XDG_RUNTIME_DIR` (e.g. `sudo` is used, or the env. var. is not set, or this env. var. was unset, etc.) Thus we try to create/read/write lock file in this directory: `"/run/user/${UID}"`

I don't see any difference between (1) and (3), and both seem to me the same case of "$XDG_RUNTIME_DIR is not set". I still think we should not over-complicate this, and rather do the following:

* `$XDG_RUNTIME_DIR` is set and can be used -> use it

* `$XDG_RUNTIME_DIR` is not set or it is not usable -> use `/run` as done so far

which is what I suggested already 4 months ago... #3134 (review)

There is difference between (1) and (3)... Variant (1) uses /run/rhsm/, which is read-only in buildah environment. Variant (3) uses directory that is writable in this environment and it especially useful in the case, when somebody tries to use sudo in buildah environment (e.g. copy-pasted code containing sudo at the beginning in the command). I would not implement it, if I didn't have good reason for that.

However in buildah $XDG_RUNTIME_DIR is set (variant (2)), which is what I suggest to try to use as first step.

I would not change first step to not change current behavior, when /run/rhsm is readable/writable.

[ptoscano]

I would not change first step to not change current behavior, when /run/rhsm is readable/writable.

OK, makes sense; let's keep only step (1) and (2) for now, please. (1) fails so far only in buildah, and (2) covers it using $XDG_RUNTIME_DIR. In case it fails somehow, I'd like to see how that scenario looks like exactly, and it can be worked around by setting $XDG_RUNTIME_DIR anyway.