Repository navigation
Deprecate DBLockProvider and replace it with new LockingMechanismProvider - #14751
Conversation
There was a problem hiding this comment.
Thank you for the draft PR. I think the overall direction is right, other than that please see inline.
Do we need releaseAllLocks method? Or we want this only for dev purposes and can be handled only on level of Factory in each implementation?
Yes, this is needed in exceptional case when Keycloak leaves a lock record. I'd even incline to renaming this method to unlockAllLockForcibly.
There was a problem hiding this comment.
Replace literals with constants
There was a problem hiding this comment.
Do we want to have all lock (besides some variable names like Model name) names we use in Keycloak defined in constants? The reason why I used it this way is to make sure it is clear from the interface that it is possible to use any String as the lock name. I didn't want to make any dependency between the old DBLock and new LockingMechanism so I used just Strings.
There was a problem hiding this comment.
Using Strings is correct approach. We should support the common use cases by providing appropriate String constants.
There was a problem hiding this comment.< 8000 /p>
I agree that acquireLock should return an AutoCloseable instance, say of a type GlobalLock? Then releaseLock would be made unnecessary and locking safe:
try (GlobalLock l = ...acquireLock(INITIAL_REALM_CREATION)) {
// do the job....
}Also I think there should be a time-bound variant:
void acquireLock(String lockName, long time, TimeUnit unit);There was a problem hiding this comment.
I knew I forgot one more thing in my initial comment 🤣 Can we use Duration in this case? Should we have also the variant that is not time bounded? In that case is it waiting forever? Or there is some default timeout?
There was a problem hiding this comment.
I knew I forgot one more thing in my initial comment rofl Can we use
Durationin this case?
Yes.
Should we have also the variant that is not time bounded? In that case is it waiting forever? Or there is some default timeout?
I think there should be a default timeout as a property.
Do we want to use the words |
We should not have
|
b5ae80f to
a654a1c
Compare
There was a problem hiding this comment.
No constants in an interface please. For example, you can introduce a GlobalLock.Constants class for declaring the constants.
There was a problem hiding this comment.
Using Strings is correct approach. We should support the common use cases by providing appropriate String constants.
There was a problem hiding this comment.
timeToWaitForLock should not be used here.
a42cf62 to
454d6df
Compare
454d6df to
420f3c1
Compare
420f3c1 to
95559d2
Compare
|
I introduced a new discussion to let others express their opinions on this PR. |
There was a problem hiding this comment.
What are the types of server-wide locks we have? I can think about a lock during boot (keycloak-boot). Any other?
The reference to database there reads a bit weird as it is more an implementation detail on how the lock is implemented, right?
There was a problem hiding this comment.
What are the types of server-wide locks we have? I can think about a lock during boot (keycloak-boot). Any other?
For example, we need to lock specific area when initializing new map storage jpa, see: https://github.com/keycloak/keycloak/blob/main/model/map-jpa/src/main/java/org/keycloak/models/map/storage/jpa/JpaMapStorageProviderFactory.java#L436
Another example is locking HotRod storage when the proto schema is registered with the Infinispan server. Having more granular locks allows us to leave part of Keycloak operational while some other part is initialized.
The reference to database there reads a bit weird as it is more an implementation detail on how the lock is implemented, right?
The constant database in this case doesn't mean the locks are provided by the database. The constant serves for marking all blocks of the code that should not be visited by more threads/nodes at the same time.
So for example, if Liquibase update is running we will acquire lock with name database, this means that all other threads/nodes know someone else is doing Liquibase update so they wait until the lock is released.
There was a problem hiding this comment.
For example, we need to lock specific area when initializing new map storage jpa, see: https://github.com/keycloak/keycloak/blob/main/model/map-jpa/src/main/java/org/keycloak/models/map/storage/jpa/JpaMapStorageProviderFactory.java#L436
Another example is locking HotRod storage when the proto schema is registered with the Infinispan server. Having more granular locks allows us to leave part of Keycloak operational while some other part is initialized.
If both locks you mentioned are related to migration/schema operations, looks like we have a specific type of lock similarly to what you did to keycloak-boot. Perhaps keycloak-initor something. Or even different ones specific for keycloak-migration and keycloak-init.
The reference to database there reads a bit weird as it is more an implementation detail on how the lock is implemented, right?
The constant
databasein this case doesn't mean the locks are provided by the database. The constant serves for marking all blocks of the code that should not be visited by more threads/nodes at the same time.
But database implies it is somehow related to a database. The whole purpose of having a "global" lock is do to what you mentioned so it is still not clear why we need database. Sorry, if I'm missing something.
So for example, if Liquibase update is running we will acquire lock with name
database, this means that all other threads/nodes know someone else is doing Liquibase update so they wait until the lock is released.
I think a more generic approach and also more aligned with the different usages of locks is to not use database. In this example, you are talking about Liquibase and we could just have a constant similarly to keycloak-boot but specific for schema init/migration, as mentioned before.
If you are using database or hot rod locks, it does not matter. But that we are using a locking mechanism for boot, init, or migration.
There was a problem hiding this comment.
If both locks you mentioned are related to migration/schema operations, looks like we have a specific type of lock similarly to what you did to keycloak-boot. Perhaps keycloak-initor something. Or even different ones specific for keycloak-migration and keycloak-init.
The locks I mentioned are not one specific lock, it can be lock per storage area. You can lock only realms or only users. You don't need to lock the whole database when initializing, for example, authz services, other areas can continue working.
But database implies it is somehow related to a database. The whole purpose of having a "global" lock is do to what you mentioned so it is still not clear why we need database. Sorry, if I'm missing something.
Maybe I am missing something as well, what is your suggestion here?
I think a more generic approach and also more aligned with the different usages of locks is to not use database. In this example, you are talking about Liquibase and we could just have a constant similarly to keycloak-boot but specific for schema init/migration, as mentioned before.
If you are using database or hot rod locks, it does not matter. But that we are using a locking mechanism for boot, init, or migration.
This is exactly what is this PR about, we changed the locks to not provide only three specific locks as it was before. Now it is possible to create a lock with any String identifier, so you can create, for example, lock for only one user, or for only one storage area, or whatever you currently need to synchronize across your deployment.
These constants here are not meant to somehow restrict what lock names can be used, it is there just to make sure we use the same String in all places we want to use this specific lock and also to easily find all places that are synchronized for this String.
There was a problem hiding this comment.
But database implies it is somehow related to a database. The whole purpose of having a "global" lock is do to what you mentioned so it is still not clear why we need database. Sorry, if I'm missing something.
Maybe I am missing something as well, what is your suggestion here?
I'm proposing removing those constants. Mainly database as it is not aligned with what you are saying. For instance, database is an implementation detail, and having it on an interface does not make much sense.
Have specific methods on the provider interface to return a specific type of lock such as a boot, init migration, user, etc, lock. For me, it makes sense to make explicit what types of locks you can use.
Something like:
LockProvider provider = // get provider
Lock lock1 = provider.getServerBootLock();
Lock lock2 = provider.getInitLock();
Lock lock3 = provider.getMigrationLock();
StorageLock storageLock = provider.getStorageLock();
Lock userLock = storageLock.getUserLock(id);
No reference to impl details (like database) but making the possible types of locks more explicit in the provider contract.
There was a problem hiding this comment.
I believe the DATABASE lock can be moved fully into legacy-private, correct?
Then the only lock string which should remain is the KEYCLOAK_BOOT lock.
As for individual methods per lock type, these go against the purpose of this PR. Various storages need various locks (e.g. DATABASE and MIGRATION are only relevant for legacy store; there are also additional upgrading lock types anticipated in map storage), and they both cannot be listed in advance and cannot be limited by API. This is the main reason for having the lock ID as a free String parameter.
The same applies to future background tasks which will also need locks for their ability to operate, and we do not know the their full list as they will be created upon demand.
For both reasons, the API should not be fixed by method names.
There was a problem hiding this comment.
I believe the
DATABASElock can be moved fully into legacy-private, correct? Then the only lock string which should remain is theKEYCLOAK_BOOTlock.
Sounds better so we make the new SPI more aligned with the expected design.
For both reasons, the API should not be fixed by method names.
Ok.
There was a problem hiding this comment.
@hmlnarik It seems DATABASE constant can be moved to the legacy module. But we would need to change the code here:
https://github.com/keycloak/keycloak/blob/main/model/map-jpa/src/main/java/org/keycloak/models/map/storage/jpa/JpaMapStorageProviderFactory.java#L438
however, it is probably the correct thing to do in this PR anyway. (replace DATABASE with modelType)
95559d2 to
9b1a33c
Compare
|
@pedroigor @hmlnarik I moved |
There was a problem hiding this comment.
| * <p /> | |
| * <p /> | |
| * Lifespan of this lock is limited by the transaction lifespan in the session which acquired this lock. | |
| * However it may be shorter if the lock implementation supports it, in which case it would | |
| * be released by the {@link GlobalLock#close} method. | |
| * <p/> |
There was a problem hiding this comment.
cc @ahus1
I see how the updated version could be interpreted in multiple ways. We need a better description then:
| * <p /> | |
| * <p /> | |
| * The lock is guaranteed to be kept until the returned {@link GlobalLock} is closed | |
| * using the {@link GlobalLock#close} method. | |
| * <p /> | |
| * Some implementations may benefit from locks that are released at the end of transaction. | |
| * For this purpose, the lifespan of the returned lock is limited by the transaction lifespan | |
| * of the session which acquired this lock. | |
| * <p/> |
|
Reading the previous comment from @hmlnarik:
This addition sentence fortifies a leaky abstraction, so the caller will not know how the implementation will perform. At the same time, there is no implementation for this interface yet. If we keep this description, I'd like to revisit it again once there are the JPA + Infinispan implementations and narrow down the behavior. |
There was a problem hiding this comment.
| * Acquires a new global lock that is visible to all Keycloak nodes. | |
| * Acquires a new global lock that is visible to all Keycloak nodes. The lock is non-reentrant. |
9b1a33c to
a55bf36
Compare
|
Thank you for the PR! |
This is an initial work to introduce a new LockingMechanism provider.
The interface
LockingMechanismProvideris not yet final, I just added some simple methods so we can use it for implementing the DBLock compatible Implementation, but I would like to gather some feedback on what we need from that interface. Some ideas:acquireLockcould throw an exception (for exampleTimeoutException) when locking times outacquireLockcould return someAutoCloseableso we can acquire/release lock in try-with-resources blockreleaseLockreleaseAllLocksmethod? Or we want this only for dev purposes and can be handled only on level of Factory in each implementation?