Warnings in log during normal startup

[stianst]

Before reporting an issue

• I have read and understood the above terms for submitting issues, and I understand that my issue may be closed without action if I do not follow them.

Area

dist/quarkus

Describe the bug

When starting Keycloak there are warnings in the log:

bin/kc.sh start-dev --log-level warn
Updating the configuration and installing your custom providers, if any. Please wait.
2024-02-27 11:47:21,678 WARN  [io.quarkus.agroal.runtime.DataSources] (main) Datasource <default> enables XA but transaction recovery is not enabled. Please enable transaction recovery by setting quarkus.transaction-manager.enable-recovery=true, otherwise data may be lost if the application is terminated abruptly
2024-02-27 11:47:21,958 WARN  [org.infinispan.PERSISTENCE] (keycloak-cache-init) ISPN000554: jboss-marshalling is deprecated and planned for removal
2024-02-27 11:47:21,993 WARN  [org.infinispan.CONFIG] (keycloak-cache-init) ISPN000569: Unable to persist Infinispan internal caches as no global state enabled
2024-02-27 11:47:23,119 WARN  [org.keycloak.quarkus.runtime.KeycloakMain] (main) Running the server in development mode. DO NOT use this configuration in production.

The last entry is fine, but the others should not be present when I'm starting Keycloak with default configuration.

Version

23.0.6

Regression

• The issue is a regression

Expected behavior

No unexpected warnings in logs with default configuration

Actual behavior

Outputs several warnings that users won't know what to do with

How to Reproduce?

bin/kc.sh start-dev --log-level warn

Anything else?

No response

[shawkins]

2024-02-27 11:47:21,678 WARN [io.quarkus.agroal.runtime.DataSources] (main) Datasource enables XA but transaction recovery is not enabled. Please enable transaction recovery by setting quarkus.transaction-manager.enable-recovery=true, otherwise data may be lost if the application is terminated abruptly

We should default kc.transaction-xa-enabled=false in dev mode.

2024-02-27 11:47:21,958 WARN [org.infinispan.PERSISTENCE] (keycloak-cache-init) ISPN000554: jboss-marshalling is deprecated and planned for removal

Resolved in infinispan 15: infinispan/infinispan@3a98601

2024-02-27 11:47:21,993 WARN [org.infinispan.CONFIG] (keycloak-cache-init) ISPN000569: Unable to persist Infinispan internal caches as no global state enabled

Neither of our built-in cache configs have global-state defined, but the ClusterPermissionMapper and the ClusterRoleMapper create internal caches that expect persistence. For dev mode there doesn't seem to be a great way to ignore this, and it doesn't like we should be configuing a global-store in that case. @mhajas @mabartos @vmuzikar is there any benefit to users configuring a global-state when not in dev mode? If not, then it seems like we'd want the upstream to switch this to a debug message instead. -- moved to #27718 since the XA warning took over this issue.

[shawkins]

We should default kc.transaction-xa-enabled=false in dev mode.

I guess that actually just kicks the can down the road a bit - the warning will come back for the prod profile. The only way to resolve it then is to either to not use xa or to use quarkus configuration to enable recovery. #15255 was resolved without addressing whether quarkus.transaction-manager.enable-recovery should be settable via a keycloak option, or even just default to true. cc @vmuzikar @keycloak/store what is your preference here?

[ahus1]

We should default kc.transaction-xa-enabled=false in dev mode.

I vote against having different default between dev and prod mode, as it is source for lots of tears. It will bite those who write extensions, and then Keycloak would behave differently out-of-the-blue.
I would even go as far as vetoing such a difference if the only reason is a log output.

About the matter of transactions: I remember that the container needs to have a writable folder anyway, and we're setting the quarkus.transaction-manager.object-store.directory in the application properties already.

If we can't settle for enabling it, we can still choose to configure the logging for that class to swallow the warning.

[shawkins]

#15255 was resolved without addressing whether quarkus.transaction-manager.enable-recovery should be settable via a keycloak option, or even just default to true. cc @vmuzikar @keycloak/store what is your preference here?

Switching the tagging to @keycloak/core

If we can't settle for enabling it, we can still choose to configure the logging for that class to swallow the warning.

I would opt against just swallowing the warning.

To expand a little bit on the impluse to change the default for dev mode - unless I'm missing something users shouldn't be trying to coordinate across multiple transactional resources in dev mode correct?

If we can't agree to enable recovery by default, then we can extend this reasoning about a principled default a little further - for example only default to xa when multiple datasources are configured. Then the burden would be on the user to specifically enable xa in single source scenarios where they were trying to enlist some other kind of resource.

[ahus1]

I'd be ok to make the default non-XA in all setups as Keycloak is usually targeting only a single database. I know that the transaction manager behaves identical as long as only one datasource joins the transaction, still this IMHO can't be determined at startup by how many datasources there are. When implementing map storage, Infinispan was "joining" the transaction, and then the transaction manager did a full XA, so it might not only be the databases you expect. In addition, we officially support custom user storage, which might come with its own JDBC connection. LDAP doesn't support it, and Infinispan in the current store doesn't support it for performance and throughput reason.

When doing XA, it should have transaction recovery, but I never understood that fully. To set this up with containers would be a real challenge (nightmare?) to figure out where to store that information. The https://quarkus.io/guides/transaction describes on how to store this information in a database using JDBC, but also that it is not simple in a cloud environment:

However, in cloud-native apps, using a database to store transaction logs has additional requirements. The narayana-jta extension, which manages these transactions, requires stable storage, a unique reusable node identifier, and a steady IP address to work correctly. While the JDBC object store provides a stable storage, users must still plan how to meet the other two requirements.

So I think I would be ok with:

• making non-XA the default for all setups
• document how to enable XA, and make all XA necessary options configurable via regular Keycloak options (which would include quarkus.transaction-manager.enable-recovery, and possibly quarkus.transaction-manager.object-store.directory and others.

[shawkins]

so it might not only be the databases you expect.

Yes that is what I meant by other resources.

a unique reusable node identifier, and a steady IP address to work correctly

The statefulset pod names can provide the former, but the latter seems like it would require creating a service (probably with a static ip) for each replica.

There is also a mention of a workaround with setting quarkus.datasource.TX_LOG.jdbc.transactions=disabled

It seems a little easier to just use StatefulSet stable storage.

document how to enable XA, and make all XA necessary options configurable via regular Keycloak options (which would include quarkus.transaction-manager.enable-recovery, and possibly quarkus.transaction-manager.object-store.directory and others.

Would the additional support be treated as a separate enhancement, or would you want that in place prior to flipping the default? It seems like you could end up needing to add quite a few properties.

[ahus1]

Would the additional support be treated as a separate enhancement, or would you want that in place prior to flipping the default? It seems like you could end up needing to add quite a few properties.

Some thoughts about keeping the exposed options at a minimum:

In the past the goal was to write documentation which didn't reference Quarkus configuration options directly. To keep that bar, and given the warning message above, I assume quarkus.transaction-manager.enable-recovery would either need to be exposed, or set to enabled by default if we expect users not to change it.

We could choose to support only file based XA for now (as before), and ignore JDBC until requested. If that is the case, there wouldn't be the need for those properties to be exposed. We would still document where the transaction logs are kept in the file system (which is data/transaction-logs as configured in the application.properties). That's something we didn't document yet, but people would need to know about those to keep them available for Keycloak between restarts when using XA.

TL;DR: So it could be no additional options exposed if we set quarkus.transaction-manager.enable-recovery to true and stick to file based XA only. Still we should document data/transaction-logs in a new XA paragraph in the docs.

WDYT?

[shawkins]

In addition, we officially support custom user storage, which might come with its own JDBC connection.

Which we would be able to detect as long as it is configured via quarkus properties - that should be the most common scenario. However you probably have a concern that users should knowingly set XA support rather than it being inferred. So ignoring the case of making the kc.transaction-xa-enabled default more principled, we have roughly an options matrix like this:

kc.transaction-xa-enabled default	default quarkus.transaction-manager.enable-recovery=true	default quarkus.transaction-manager.enable-recovery=false
true	Warning goes away. We'll still need to document the recovery support (the storage requirements seem minimal) and add stable storage in the operator.	Current state
false	Warning goes away. XA in general, and recovery in particular could be treated as an advanced case - proper documentation and operator support could come later.	Warning goes away. XA in general, and recovery in particular could be treated as an advanced case requiring additional quarkus properties for recovery to function.

To make sure, are you advocating initially for the top left box - kc.transaction-xa-enabled=true and quarkus.transaction-manager.enable-recovery=true, or are you still open for defaulting kc.transaction-xa-enabled=false as well?

[ahus1]

@shawkins - my main concern in my first response was to have dev and prod mode aligned.

After this discussion, I'd be ok (and possibly prefer if there are no arguments against it) the defaults in any mode and independent of any other configuration as follows: kc.transaction-xa-enabled=false + quarkus.transaction-manager.enable-recovery=true

A little bit of documentation about the folder data/transaction-logs and transaction recovery in general would be essential IMHO. No changes to the Operator would be needed for that setup.