[RHELC-1124] Refactor applock to have a more robust API. #979

jochapma · 2023-11-02T15:09:23Z

[RHELC-1124] Refactor ApplicationLock to have a more consistent API

Jira Issues: RHELC-1124

Checklist

PR has been tested manually in a VM (either author or reviewer)
Jira issue has been made public if possible
[RHELC-] is part of the PR title
GitHub label has been added to help with Release notes
PR title explains the change from the user's point of view
Code and tests are documented properly
The commits are squashed to as few commits as possible (without losing data)
When merged: Jira issue has been updated to Release Pending if relevant

codecov · 2023-11-02T15:11:12Z

Codecov Report

Attention: Patch coverage is 83.78378% with 6 lines in your changes missing coverage. Please review.

Project coverage is 95.54%. Comparing base (05e0d66) to head (577ec86).
Report is 150 commits behind head on main.

Files with missing lines	Patch %	Lines
convert2rhel/applock.py	81.25%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #979   +/-   ##
=======================================
  Coverage   95.54%   95.54%           
=======================================
  Files          53       53           
  Lines        4642     4648    +6     
  Branches      815      816    +1     
=======================================
+ Hits         4435     4441    +6     
- Misses        129      132    +3     
+ Partials       78       75    -3

Flag	Coverage Δ
centos-linux-7	`90.71% <83.78%> (+0.01%)`	⬆️
centos-linux-8	`91.64% <83.78%> (+0.01%)`	⬆️
centos-linux-9	`91.68% <83.78%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

has-bot · 2023-11-02T15:43:41Z

/packit test --labels tier0

@oamg/conversions-qe please review results and provide ack.

Log | Bot Usage

abadger · 2023-11-06T09:26:22Z

convert2rhel/applock.py

+        :returns: the file contents as an integer, or None if it doesn't exist
+        :raises: ApplicationLockedError if the contents are corrupt
+        """
+        if os.path.exists(self._pidfile):


Note: To avoid races, it's better to do a try: except: around the open() rather than check for file existence first.

abadger · 2023-11-06T09:28:41Z

convert2rhel/applock.py

        # Our process ID
        self._pid = os.getpid()


Suggested change

# Our process ID

self._pid = os.getpid()

# Our process ID. We save this when the lock is created so it will be consistent even if we check from inside a fork.

self._pid = os.getpid()

convert2rhel/applock.py

abadger · 2023-11-06T10:03:52Z

convert2rhel/applock.py

-
+        pid = self._read_pidfile()
+        if pid == self._pid:
+            return
        if self._pid_exists(pid):
            raise ApplicationLockedError("%s locked by process %d" % (self._pidfile, pid))
        # The lock file was created by a process that has exited;
        # remove it and try again.
        loggerinst.info("Cleaning up lock held by exited process %d." % pid)
        os.unlink(self._pidfile)


There is a race condition here too, where something else may have already unlinked the file so we need to catch the exception.

Also.... I think the race could also mean that self._pidfile might not contain the same pid as we checked with _pid_exists (meaning between us reading the pid and unlinking the file, another process has unlinked the original and created their new one in its place.) Is there some way to fix that with some combination of counting links to the lockfile, adding one new link to the file, and checking whether the count of links matches how many we think it should at each step of the way? (And if it doesn't match, having some sort of time.sleep(random) before retrying?) (I'm not sure if there is a way or not. I think there might be but a quick internet search didn't turn up a recipe so I might be overly optimisitc. If not, it's probably better to err on the side of telling the user to check whether the lock is stale rather than overwriting the lock).

Venefilyn · 2024-03-06T10:55:20Z

@jochapma there's 3 merge commits in the PR. If you can, drop them and rebase with main instead

convert2rhel/applock.py

Venefilyn

Overall seems fine, some exceptions being re-raised but not with the same exception. There is a file open that is never closed, which could be an issue

convert2rhel/applock.py

Venefilyn · 2024-05-15T10:52:23Z

convert2rhel/applock.py

+            # In Python 3 this could be changed to FileNotFoundError.
+            if exc.errno == errno.ENOENT:
+                return None
+            raise


Same here, we should be raising exception again

We have to leave the file open and close it later explicitly because the file lock is tied to the file descriptor. The file lock is required to plug a race condition in which the pid file is overwritten by another process between the point where we read its contents and unlink it.

The bare raise statement reraises the current exception; @abadger requested that I use this idiom.

Venefilyn · 2024-05-15T10:53:05Z

convert2rhel/applock.py

+        except OSError as exc:
+            # In Python 3 this could be changed to FileNotFoundError.
+            if exc.errno == errno.ENOENT:
+                return
+            raise


This isn't covered by tests

Venefilyn · 2024-05-15T10:55:06Z

convert2rhel/applock.py

+        """Release the advisory file lock we hold on the PID file
+        and close the open file descriptor."""
+        if self._pidfile_fp:
+            fcntl.flock(self._pidfile_fp, fcntl.LOCK_UN)


What does this do? Comments would be nice

Venefilyn

Smaller comments, will look at tests next. The flags didn't seen intuitive or something we see every day so having comments of what the flags does would be nice

convert2rhel/applock.py

Venefilyn · 2024-06-13T12:07:52Z

convert2rhel/applock.py

+        with open(self._pidfile, "r") as filep:
+            fcntl.flock(filep, fcntl.LOCK_EX)
+            try:
+                file_contents = filep.read()
+                pid = int(file_contents.rstrip())
+                if pid == self._pid:
+                    return
+                if self._pid_exists(pid):
+                    raise ApplicationLockedError("%s locked by process %d" % (self._pidfile, pid))
+                # The lock file was created by a process that has exited;
+                # remove it and try again.
+                loggerinst.info("Cleaning up lock held by exited process %d." % pid)
+                self._safe_unlink()


Could we not reuse the logic of is_locked()? Otherwise we might change it in the future on one part and miss the other etc.

I was unable to find a way to avoid a race condition in unlink without keeping a file pointer open across multiple methods, which all my reviewers didn't like. So I removed the automatic cleanup; this also has the benefit of simplifying everything considerably, a good idea since this is my last week.

What this does is add another "can't happen" case (if the lock file somehow doesn't get cleaned up), but that can be easily remedied manually.

In the course of doing that, I addressed the is_locked() reuse.

Co-authored-by: Freya Gustavsson <freya@venefilyn.se>

jochapma · 2024-08-28T18:42:46Z

/packit build

jochapma changed the title ~~Refactored applock to have a more robust API.~~ [RHELC-1124] Refactor applock to have a more robust API. Nov 2, 2023

jochapma added the tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. label Nov 2, 2023

abadger reviewed Nov 6, 2023

View reviewed changes

convert2rhel/applock.py Show resolved Hide resolved

abadger reviewed Nov 6, 2023

View reviewed changes

jochapma force-pushed the bugs/rhelc-1124 branch from f6cd858 to bbbf692 Compare March 6, 2024 18:33

jochapma added 4 commits March 6, 2024 13:34

Refactored applock to have a more robust API.

5da05db

Test that locking is idempotent.

8a1fbc7

Address potential race conditions.

d01a15b

Address Toshio's commments.

431844e

jochapma force-pushed the bugs/rhelc-1124 branch from bbbf692 to 431844e Compare March 6, 2024 18:36

Make PID file close explicit (needed for file locking).

04c7e3e

github-advanced-security bot found potential problems Mar 7, 2024

View reviewed changes

convert2rhel/applock.py Fixed Show fixed Hide fixed

Add flock() calls.

5f3aa7e

github-advanced-security bot found potential problems Mar 11, 2024

View reviewed changes

convert2rhel/applock.py Fixed Show resolved Hide resolved

Merge branch 'oamg:main' into bugs/rhelc-1124

8b93251

r0x0d reviewed May 2, 2024

View reviewed changes

convert2rhel/applock.py Outdated Show resolved Hide resolved

r0x0d reviewed May 2, 2024

View reviewed changes

convert2rhel/applock.py Outdated Show resolved Hide resolved

jochapma and others added 2 commits May 7, 2024 09:32

Merge branch 'oamg:main' into bugs/rhelc-1124

df8b5a0

Don't use 'is not None' idiom.

c154b1b

jochapma added the kind/bug-fix A bug has been fixed label May 7, 2024

jochapma marked this pull request as ready for review May 7, 2024 14:02

Venefilyn requested changes May 15, 2024

View reviewed changes

Rework control flow to be able to use context manager with lock file.

383e33c

Venefilyn reviewed Jun 13, 2024

View reviewed changes

Improve commenting.

43431db

Co-authored-by: Freya Gustavsson <freya@venefilyn.se>

jochapma and others added 5 commits August 28, 2024 09:34

Document fcntl flags.

3abb270

Co-authored-by: Freya Gustavsson <freya@venefilyn.se>

Document fcntl flags.

e45531d

Co-authored-by: Freya Gustavsson <freya@venefilyn.se>

Document fcntl flags.

2012108

Co-authored-by: Freya Gustavsson <freya@venefilyn.se>

Document fcntl flags.

86347be

Co-authored-by: Freya Gustavsson <freya@venefilyn.se>

Removed auto-cleanup code.

311888b

jochapma requested a review from a team as a code owner August 28, 2024 17:34

jochapma added 2 commits August 29, 2024 08:43

Improve error message when application is locked.

577ec86

Prettified a warning message.

cbedf55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHELC-1124] Refactor applock to have a more robust API. #979

[RHELC-1124] Refactor applock to have a more robust API. #979

jochapma commented Nov 2, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading

has-bot commented Nov 2, 2023

abadger Nov 6, 2023

abadger Nov 6, 2023

abadger Nov 6, 2023

Venefilyn commented Mar 6, 2024

Venefilyn left a comment

Venefilyn May 15, 2024

jochapma May 15, 2024

jochapma May 15, 2024

Venefilyn May 15, 2024

Venefilyn May 15, 2024

Venefilyn left a comment

Venefilyn Jun 13, 2024

jochapma Aug 28, 2024

jochapma commented Aug 28, 2024

[RHELC-1124] Refactor applock to have a more robust API. #979

Are you sure you want to change the base?

[RHELC-1124] Refactor applock to have a more robust API. #979

Conversation

jochapma commented Nov 2, 2023 • edited Loading

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

has-bot commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Venefilyn commented Mar 6, 2024

Venefilyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Venefilyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochapma commented Aug 28, 2024

jochapma commented Nov 2, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading