Automatically remove stale locks by enkore · Pull Request #1674 · borgbackup/borg

enkore · 2016-10-02T10:04:41Z

Supersedes #1246

Fixes #562

codecov-io · 2016-10-02T10:23:14Z

Current coverage is 84.37% (diff: 76.00%)

Merging #1674 into master will decrease coverage by 0.08%

@@             master      #1674   diff @@
==========================================
  Files            20         20          
  Lines          6609       6664    +55   
  Methods           0          0          
  Messages          0          0          
  Branches       1125       1136    +11   
==========================================
+ Hits           5582       5623    +41   
- Misses          754        767    +13   
- Partials        273        274     +1

Powered by Codecov. Last update b1ac207...6b88da2

ThomasWaldmann · 2016-10-09T23:47:39Z

src/borg/remote.py

                raise ValueError('log level missing, fix this code')
+        env_vars = []
+        if bool(os.environ.get('BORG_UNIQUE_HOSTNAME')):
+            env_vars.append('BORG_UNIQUE_HOSTNAME=yes')


a while ago I changed semantics of env vars. in early borg and attic, non-present env vars meant False and any present ones meant True (even if e.g. FOO=False or FOO=0 was used - they would be evaluated as truish).

after my change, env vars are handled like yes() input. So, it is FOO=0 and FOO=1 or FOO=yes and FOO=no now.

So, I think it should be:

unique_hostname = os.environ.get('BORG_UNIQUE_HOSTNAME') if unique_hostname is not None: env_vars.append('BORG_UNIQUE_HOSTNAME=%s' % unique_hostname)

Ok, there are some other places that do bool(os.environ.get('B_U_H')), I'll change those as well for consistent behaviour.

Done. I added prompt=True|False to yes() to facilitate this. Output now looks like this:

Remote: Enabled removal of stale repository locks Remote: Killed stale lock tiger.3300-0. Remote: Removed stale exclusive roster lock for pid 3300. Remote: Removed stale exclusive roster lock for pid 3300. Enabled removal of stale cache locks Killed stale lock tiger.3292-0. Removed stale exclusive roster lock for pid 3292. Removed stale exclusive roster lock for pid 3292.

The doubled roster locks are because this is done on loading, so if it's not immediately saved that gets doubled. I think we could just move those down to INFO log level, or remove them.

ThomasWaldmann · 2016-10-09T23:49:29Z

src/borg/remote.py

            remote_path = args.remote_path or os.environ.get('BORG_REMOTE_PATH', 'borg')
            remote_path = replace_placeholders(remote_path)
-            return [remote_path, 'serve'] + opts
+            return env_vars + [remote_path, 'serve'] + opts


as this is no-cover: did you test this manually, using a ssh: repo location?

Yeah it works.

ThomasWaldmann · 2016-10-10T00:01:44Z

src/borg/testsuite/locking.py

+        dead_lock = ExclusiveLock(lockpath, id=dead_id).acquire()
+        with ExclusiveLock(lockpath, id=our_id, kill_stale_locks=True):
+            with pytest.raises(NotMyLock):
+                dead_lock.release()


69 can only acquire the lock, if there is no other lock for lockpath.
but there is dead_lock. as it is dead, and kill_stale_locks, it gets killed.
but why does 71 raise NotMyLock then and not NotLocked?
and why is it allowed to enter the with-block if the lock is still considered present?

NotMyLock because there is an active lock and it belongs to 69. NotLocked would be the result if we de-indentend 70/71 one level, in which case 69 would be unlocked, and since 68 is dead, there is no lock = NotLocked.

enkore · 2016-11-03T16:48:37Z

src/borg/platform/posix.pyx

+# the lock made by the parent, so it needs to use the same PID for that.
+_pid = os.getpid()
+# XXX this sometimes requires live internet access for issuing a DNS query in the background.
+_hostname = socket.gethostname()


I don't think there's a way around that API-wise, if the machine is configured that way.

enkore · 2016-11-03T16:49:44Z

Feature-wise this is done. Output could use some polishing perhaps:

Done. I added prompt=True|False to yes() to facilitate this. Output now looks like this:
Remote: Enabled removal of stale repository locks
Remote: Killed stale lock tiger.3300-0.
Remote: Removed stale exclusive roster lock for pid 3300.
Remote: Removed stale exclusive roster lock for pid 3300.
Enabled removal of stale cache locks
Killed stale lock tiger.3292-0.
Removed stale exclusive roster lock for pid 3292.
Removed stale exclusive roster lock for pid 3292.
The doubled roster locks are because this is done on loading, so if it's not immediately saved that gets doubled. I think we could just move those down to INFO log level, or remove them.

#1674 (comment)

ThomasWaldmann

some small nitpicks

ThomasWaldmann · 2016-11-05T01:25:50Z

src/borg/cache.py

        self.manifest = manifest
        self.path = path or os.path.join(get_cache_dir(), repository.id_str)
+        self.unique_hostname = yes(true_msg='Enabled removal of stale cache locks',
+                                   env_var_override='BORG_UNIQUE_HOSTNAME', prompt=False, env_msg=None)


BORG_UNIQUE_HOSTNAME is a bit ambiguous / unintuitive maybe.
It is meant as boolean, but from the name, it could also be meant as a var holding a hostname.

BORG_HOSTNAME_IS_UNIQUE maybe?

this also affects the python variable names.

ThomasWaldmann · 2016-11-05T02:08:00Z

src/borg/platform/posix.pyx

+
+    return local_pid_alive(pid)
+
+def local_pid_alive(pid):


pep8: add one more blank line above this one.

ThomasWaldmann · 2016-11-05T02:09:00Z

src/borg/platform/posix.pyx

+    try:
+        # This doesn't work on Windows.
+        # This does not kill anything, 0 means "see if we can send a signal to this process or not".
+        # Possible errors: No such process (== stale lock) or permission denied (not a stale lock)


add a dot at end of sentence.

ThomasWaldmann · 2016-11-05T02:10:03Z

src/borg/platform/posix.pyx

+        if err.errno == errno.ESRCH:
+            # ESRCH = no such process
+            return False
+        # Any other error (eg. permissions) mean that the process ID refers to a live process


... means ... (and add a dot at the end).

ThomasWaldmann · 2016-11-05T19:06:45Z

Note:

I reviewed the log levels and they seem ok as they are. Users should get notified if there are stale locks because this points to a problem in their setup.

Not sure though if we always want to read the "Enabled removal ..." msg from yes() all the time.

enkore · 2016-11-05T19:12:56Z

They're quick and distinct feedback that it worked / is set up correctly (on both ends). I guess we could move it to INFO level, moving the emission of the message from yes() to the call site.

I'll apply that and the other suggestions you made.

enkore · 2016-11-07T19:13:52Z

squash, merge?

ThomasWaldmann

yes, squash some, merge.

If BORG_UNIQUE_HOSTNAME shell variable is set, stale locks in both cache and repository are deleted. Stale lock is defined as a lock that's originating from the same hostname as us, and correspond to a pid that no longer exists. This fixes borgbackup#562

(testability)

…ling

enkore · 2016-11-07T20:58:45Z

@verygreen :)

enkore mentioned this pull request Oct 2, 2016

WIP: Deleting stale locks #1246

Closed

enkore force-pushed the f/stale-lock-murderer branch from 9ea8a64 to 487b5d1 Compare October 5, 2016 22:48

enkore changed the title ~~(wip) Automatically remove stale locks~~ Automatically remove stale locks Oct 7, 2016

ThomasWaldmann reviewed Oct 9, 2016

View reviewed changes

ThomasWaldmann reviewed Oct 10, 2016

View reviewed changes

enkore self-assigned this Oct 10, 2016

enkore force-pushed the f/stale-lock-murderer branch from a0345af to 6b88da2 Compare October 29, 2016 10:00

enkore removed their assignment Nov 3, 2016

enkore commented Nov 3, 2016

View reviewed changes

ThomasWaldmann reviewed Nov 5, 2016

View reviewed changes

ThomasWaldmann approved these changes Nov 7, 2016

View reviewed changes

verygreen and others added 7 commits November 7, 2016 21:54

Move platform-dependent code to platform package

c562f77

Use logging instead of prints

8e1df7a

import platform module instead of functions

7930d05

(testability)

platform: bump API version (and check consistency)

2bd8ac7

Add tests for stale lock killing and platform.process_alive

cc14975

Parse & pass BORG_HOSTNAME_IS_UNIQUE env var to enable stale lock kil…

676e69c

…ling

enkore force-pushed the f/stale-lock-murderer branch from 10a6771 to 676e69c Compare November 7, 2016 20:57

enkore merged commit cf44954 into borgbackup:master Nov 7, 2016

enkore deleted the f/stale-lock-murderer branch November 7, 2016 20:58

Uh oh!

Conversation

enkore commented Oct 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 84.37% (diff: 76.00%)

Uh oh!

ThomasWaldmann Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enkore commented Nov 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThomasWaldmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomasWaldmann commented Nov 5, 2016

Uh oh!

enkore commented Nov 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enkore commented Nov 7, 2016

Uh oh!

ThomasWaldmann left a comment

Choose a reason for hiding this comment

Uh oh!

enkore commented Nov 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

enkore commented Oct 2, 2016 •

edited

Loading

codecov-io commented Oct 2, 2016 •

edited

Loading

ThomasWaldmann Oct 9, 2016 •

edited

Loading

enkore commented Nov 3, 2016 •

edited

Loading

enkore commented Nov 5, 2016 •

edited

Loading