PR#1333: file locking for koji-gc - koji

koji

#1333 file locking for koji-gc

Merged 4 years ago by tkopecek. Opened 5 years ago by tkopecek.

tkopecek/koji issue1332 into master

more detailed help message

Tomas Kopecek • 4 years ago

e9ed8bd

default value for --lock-file option

Tomas Kopecek • 4 years ago

3367c6e

add file-locking to koji-gc

Tomas Kopecek • 4 years ago

771fdd2

util/koji-gc

file modified

+38 -1

		`@@ -7,7 +7,7 @@`
		`# Mike McLean <mikem@redhat.com>`

		`from __future__ import absolute_import`
		`-`
		`+ import fcntl`
		`import datetime`
		`import fnmatch`
		`import optparse`
		`@@ -111,6 +111,12 @@`
		`parser.add_option("--weburl", default="http://localhost/koji", metavar="URL",`
		`help=_("url of koji web server (for use in notifications)"))`
		`parser.add_option("-s", "--server", help=_("url of koji XMLRPC server"))`
		`+ parser.add_option("--lock-file", help=_("koji-gc will wait while specified file exists. "`
		`+ "Default path is /run/user/<uid>/koji-gc.lock. "`
		`+ "For service usage /var/lock/koji-gc.lock is "`
		`+ "recommended."))`
		`+ parser.add_option("--exit-on-lock", action="store_true",`
		`+ help=_("quit if --lock-file exists, don't wait"))`
		`#parse once to get the config file`
		`(options, args) = parser.parse_args()`

		`@@ -148,6 +154,8 @@`
		`['trashcan_tag', None, 'string'],`
		`['no_ssl_verify', None, 'boolean'],`
		`['timeout', None, 'integer'],`
		`+ ['lock_file', None, 'string'],`
		`+ ['exit_on_lock', None, 'boolean'],`
		`]`
		`for name, alias, type in cfgmap:`
		`if alias is None:`
		`@@ -954,12 +962,41 @@`

		`session_opts = koji.grab_session_options(options)`
		`session = koji.ClientSession(options.server, session_opts)`
		`+`
		`rv = 0`
		`try:`
		`+ lock_fd = None`
		`+ if not options.lock_file:`
		`+ options.lock_file = '/run/user/%d/koji-gc.lock' % os.getuid()`
		`+ # acquire lock file`
		`+ while not lock_fd:`
		`+ # fail, if it is completely inaccessible`
		`+ lock_fd = os.open(options.lock_file, os.O_CREAT \| os.O_RDWR)`
		`+ try:`
		`+ fcntl.flock(lock_fd, fcntl.LOCK_EX \| fcntl.LOCK_NB)`
		`+ break`
		`+ except (IOError, OSError):`
		`+ if options.exit_on_lock:`
		`+ try:`
		`+ session.logout()`
		`+ except:`
		`+ pass`
		`+ sys.exit(1)`
		`+ os.close(lock_fd)`
		`+ lock_fd = None`
		`+ if options.debug:`
		`+ print("Waiting on lock: %s" % options.lock_file)`
		`+ time.sleep(10)`
		`+`
		`if not options.skip_main:`
		`rv = main(args)`
		`if not rv:`
		`rv = 0`
		`+`
		`+ if lock_fd:`
		`+ # release lock file`
		`+ fcntl.flock(lock_fd, fcntl.LOCK_UN)`
		`+ os.close(lock_fd)`
		`except KeyboardInterrupt:`
		`pass`
		`except SystemExit:`

tkopecek commented 5 years ago

Fixes: https://pagure.io/koji/issue/1332

ngompa commented 5 years ago

:thumbsup:

ktdreyer commented 5 years ago

This adds some more complexity for users who want to enable garbage collection in their Koji instances.

What value do you recommend for the --lock-file option out of the box? I'm thinking like /var/run/user/<uid>/koji-gc.lock

To make this easier for users, can we document the recommended location? Or make it a default?

Edited 5 years ago by ktdreyer

tkopecek commented 5 years ago

@ktdreyer I intended to not change original behaviour at all, so --lock-file is only optional and needn't to be used at all. So, that's the reason, why it has no default (so it is disabled) - maybe just creating some documentation (or document all util/*) at all would be more helpful?

ktdreyer commented 5 years ago

Would you please say more about where you intend to store your lock file when you deploy this change?

tkopecek commented 5 years ago

As gc needs to be run under the root, /var/lock/koji-gc.lock should work.

mikem commented 5 years ago

As gc needs to be run under the root

koji-gc is just a client tool for Koji. It does not need to be run as root, or on any sort of privileged host (it does not access /mnt/koji directly). It can even be run in test mode without admin permissions or authentication.

tkopecek commented 5 years ago

Ah, thought, that it does some real deletion by itself. So, in such case it is more problematic. I expect people running it via cron/root, where I would use that systemwide location (or /var/lock/koji-gc.lock) or manually. In manual case I wouldn't use lock-file at all (as it can be handled by user) or use /run/user/<uid>/koji-gc.lock

Edited 5 years ago by tkopecek

ktdreyer commented 4 years ago

It sounds like we could reduce the complexity of the implementation by always writing to /run/user/<uid>/koji-gc.lock (don't make it optional)

rebased onto caada44c18cb32746085def6aa29fadb6836535b

4 years ago

tkopecek commented 4 years ago

removed --lock-file option

alexi commented 4 years ago

It sounds like we could reduce the complexity of the implementation by always writing to /run/user/<uid>/koji-gc.lock (don't make it optional)

I might be too late to this party, but I think the possibility of specifying the location of the lockfile is useful. It allows us to put the file on shared storage and make sure only one machine in the cluster is running koji-gc at any one time.

3 new commits added