#1333 file locking for koji-gc
Merged 2 years ago by tkopecek. Opened 3 years ago by tkopecek.
tkopecek/koji issue1332  into  master

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:

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?

@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?

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

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

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.

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

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

2 years ago

removed --lock-file option

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

  • default value for --lock-file option
  • reorder imports
  • add file-locking to koji-gc
2 years ago

Ok, I've changed it, that option exists and default is /run/user/<uid>/koji-gc.lock.

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

rebased onto 757645564d6753bde768af2795249e9f2a3b259b

2 years ago

rebased onto 771fdd2

2 years ago

@jcupova removed testing-ready due to merge conflict.

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

2 years ago

Commit 76caa2e fixes this pull-request

Pull-Request has been merged by tkopecek

2 years ago