Uploaded image for project: 'RHEL Conversions'
  1. RHEL Conversions
  2. RHELC-1124

Clean up ApplicationLock's API to have a consistent model

XMLWordPrintable

    • False
    • False
    • Hide

      None

      Show
      None
    • 2
    • 2023-R4, 2024-R2
    • Testable

      The convert2rhel lockfile that we implemented for 1.4 has API that have slightly different conceptual ideas of what a lock is.

      *ApplicationLock.try_to_lock() consults the lockfile on disk and if it exists it fails. This treats the lock as global to the system but held by one specific application.

      • ApplicationLock.is_locked asks whether this instance of ApplicationLock took out the lock or not via the instance attribute _locked. Viewing this is viewing whether the lock is held by this specific lock.
      • A third concept that Joe and I discussed was having a lock whose view is whether this application has locked the resource.

      Joe and I discussed using _locked as a refcount to push locking into the third camp but decided that refcounting would be harder to implement and the complexity was unnecessary for our use so we didn't want to go down that path. We also decided that we could remove _locked as that was only used by the unittests. We could change is_locked to check the lockfile's existence exactly like try_to_create. This would leave us with the lock representing the first concept which seems suitable.

      However, I think the third concept is the one that most closely aligns with what we mean. This lock exists to say that this invocation of /usr/bin/convert2rhel currently "owns the system" and no other invocations of /usr/bin/convert2rhel should be able to start running. I think we could achieve that by making try_to_lock() succeed if this convert2rhel process holds the lock. I think we could do this by having convert2rhel check whether the pid in the lock file was our own pid if we discover the lockfile exists.

      If that sounds good, here are the things that we need to do:

      Acceptance Criteria

      • Remove the _locked attribute.
      • Adjust is_locked to use the lockfile to determine if the lock is held.
      • Update unittests to check for the lockfile to determine locked and unlocked state where needed.
      • Add another check in try_to_lock() which checks whether we (the running convert2rhel process) is who owns the lockfile (by checking that the pid stored in the file matches with our pid). If so, succeed.
      • Question: Might the difference between whether the lock is held or not and whether we hold the lock or not matter for someone's use cases? If so we should think about the naming of the properties that report on the lock's state. is_locked is ambiguous in this regard so I think we should rename it. (We don't have to code anything to perform that query if we don't need it but we should make it obvious which of these we provide so that a future coder understands whether the lock status we currently provide is what they need or they need to implement a second piece of status).

              jochapma@redhat.com Joseph Chapman (Inactive)
              tkuratom@redhat.com Toshio Kuratomi
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

                Created:
                Updated: