Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Health check on io.quarkus.agroal.runtime.UnconfiguredDataSource always reports healthy #36666

Closed
jtama opened this issue Oct 24, 2023 · 9 comments · Fixed by #41929
Closed

Health check on io.quarkus.agroal.runtime.UnconfiguredDataSource always reports healthy #36666

jtama opened this issue Oct 24, 2023 · 9 comments · Fixed by #41929
Labels
area/agroal area/health area/smallrye kind/bug Something isn't working
Milestone
3.17.0.CR1

Comments

@jtama
Copy link
Contributor

jtama commented Oct 24, 2023 •
edited
Loading

Describe the bug

An application using only agroal extension can start even if no datasource has been configured.

This is because https://github.com/quarkusio/quarkus/blob/main/extensions/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/DataSources.java#L147C9-L153C1

An the same application healthCheck wil lreport healthy as UnconfiguredDatasource doesn't overwrite AgroalDatasource.isHealthy which defaults to true.

That means the error is only discovered once someone tries to use the datasource, which is far too late.

Expected behavior

I would expect the application not to start or at least report as not ready.

Actual behavior

The application runs without any errors.

How to Reproduce?

Simply create an application using only agroal datasource (no orm), start (not in dev mode) the application without configuring anything

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

main

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

I can provide a MR but I am quite sure what the rigth way would be as even if the behaviour is expected on some cases (as per Datasources comment) it's not always the case.
@jtama jtama added the kind/bug Something isn't working label Oct 24, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 24, 2023

/cc @Sanne (agroal), @barreiro (agroal), @jmartisk (health), @xstefank (health), @yrodiere (agroal)

@yrodiere
Copy link
Member

Thanks for reporting.

I'm not sure how we could solve this since an UnconfiguredDataSource may exist without being used, in which case it's perfectly fine and should be considered healthy.

IMO this whole UnconfiguredDataSource stuff should just be removed and we just shouldn't create the bean if there's no configuration. But I may be missing something...

@jtama
Copy link
Contributor Author

jtama commented Oct 25, 2023

I'm all for removing this UnconfiguredDataSource, which would prevent the application from starting. I'm just not sure about the impacts on other extensions.

I can provide a PR if you'd like.

@yrodiere
Copy link
Member

yrodiere commented Oct 25, 2023 •
edited
Loading

I can provide a PR if you'd like.

That would be appreciated, but I can't guarantee it will get merged since the git history suggests this was needed for Dev Services.

Maybe the best way to find out would be for you to send a PR removing UnconfiguredDataSource, and see which tests fail? From there we might be able to find an alternative solution (e.g. only return UnconfiguredDataSource in dev mode, fail immediately otherwise).

@jtama jtama mentioned this issue Oct 25, 2023
Closed
@gsmet
Copy link
Member

gsmet commented Oct 25, 2023

Maybe the best way to find out would be for you to send a PR removing UnconfiguredDataSource, and see which tests fail?

Ideally, a draft PR to avoid overloading our CI and have it run in a fork. I will make the PR a draft one.

yrodiere added a commit to yrodiere/quarkus that referenced this issue Dec 6, 2023
@yrodiere
Make DataSource beans application-scoped
3df41c7 
So that we'll be able to postpone initialization to first access
in some cases, instead of doing it on startup.

This could be useful in particular for deactivated datasources:
we don't want to initialize those on startup, but we do want them
to fail on first use.

An alternative would have been to represent deactivated datasources
with a custom implementation of AgroalDataSource, like we currently do
with UnconfiguredDataSource, but that solution has serious problems,
in particular when we "forget" to implement some methods:
see quarkusio#36666
yrodiere added a commit to yrodiere/quarkus that referenced this issue Dec 7, 2023
@yrodiere
Make DataSource beans application-scoped
b4569fe 
So that we'll be able to postpone initialization to first access
in some cases, instead of doing it on startup.

This could be useful in particular for deactivated datasources:
we don't want to initialize those on startup, but we do want them
to fail on first use.

An alternative would have been to represent deactivated datasources
with a custom implementation of AgroalDataSource, like we currently do
with UnconfiguredDataSource, but that solution has serious problems,
in particular when we "forget" to implement some methods:
see quarkusio#36666
yrodiere added a commit to yrodiere/quarkus that referenced this issue Dec 7, 2023
@yrodiere
Make DataSource beans application-scoped
eb3dde7 
So that we'll be able to postpone initialization to first access
in some cases, instead of doing it on startup.

This could be useful in particular for deactivated datasources:
we don't want to initialize those on startup, but we do want them
to fail on first use.

An alternative would have been to represent deactivated datasources
with a custom implementation of AgroalDataSource, like we currently do
with UnconfiguredDataSource, but that solution has serious problems,
in particular when we "forget" to implement some methods:
see quarkusio#36666
yrodiere added a commit to yrodiere/quarkus that referenced this issue Dec 8, 2023
@yrodiere
Make DataSource beans application-scoped
c3597f3 
So that we'll be able to postpone initialization to first access
in some cases, instead of doing it on startup.

This could be useful in particular for deactivated datasources:
we don't want to initialize those on startup, but we do want them
to fail on first use.

An alternative would have been to represent deactivated datasources
with a custom implementation of AgroalDataSource, like we currently do
with UnconfiguredDataSource, but that solution has serious problems,
in particular when we "forget" to implement some methods:
see quarkusio#36666
@yrodiere
Copy link
Member

See https://groups.google.com/g/quarkus-dev/c/enMgpOrb61o/m/cRKwiWmGAgAJ for the discussion to change the current behavior of allowing a datasource to be "unconfigured".

yrodiere added a commit to yrodiere/quarkus that referenced this issue Dec 12, 2023
@yrodiere
Make DataSource beans application-scoped
f560e77 
So that we'll be able to postpone initialization to first access
in some cases, instead of doing it on startup.

This could be useful in particular for deactivated datasources:
we don't want to initialize those on startup, but we do want them
to fail on first use.

An alternative would have been to represent deactivated datasources
with a custom implementation of AgroalDataSource, like we currently do
with UnconfiguredDataSource, but that solution has serious problems,
in particular when we "forget" to implement some methods:
see quarkusio#36666
@jtama jtama changed the title Health check on io.quarkus.agroal.runtime.UnconfiguredDataSource alwaysd reports healthy Health check on io.quarkus.agroal.runtime.UnconfiguredDataSource always reports healthy Dec 13, 2023
@yrodiere yrodiere mentioned this issue Dec 15, 2023
Closed
yrodiere added a commit to yrodiere/quarkus that referenced this issue Dec 22, 2023
@yrodiere
Make DataSource beans application-scoped
dac1380 
So that we'll be able to postpone initialization to first access
in some cases, instead of doing it on startup.

This could be useful in particular for deactivated datasources:
we don't want to initialize those on startup, but we do want them
to fail on first use.

An alternative would have been to represent deactivated datasources
with a custom implementation of AgroalDataSource, like we currently do
with UnconfiguredDataSource, but that solution has serious problems,
in particular when we "forget" to implement some methods:
see quarkusio#36666
@yrodiere yrodiere mentioned this issue Dec 22, 2023
Merged
yrodiere added a commit to yrodiere/quarkus that referenced this issue Jan 8, 2024
@yrodiere
Make DataSource beans application-scoped
c4dcbf3 
So that we'll be able to postpone initialization to first access
in some cases, instead of doing it on startup.

This could be useful in particular for deactivated datasources:
we don't want to initialize those on startup, but we do want them
to fail on first use.

An alternative would have been to represent deactivated datasources
with a custom implementation of AgroalDataSource, like we currently do
with UnconfiguredDataSource, but that solution has serious problems,
in particular when we "forget" to implement some methods:
see quarkusio#36666
@yrodiere
Copy link
Member

yrodiere commented Jun 12, 2024 •
edited
Loading

See https://groups.google.com/g/quarkus-dev/c/enMgpOrb61o/m/cRKwiWmGAgAJ for the discussion to change the current behavior of allowing a datasource to be "unconfigured".

Outcome of that conversation, for future reference:

  • We will add checks that datasources are injected (statically) in user beans through @Inject, or used by other extensions (e.g. Hibernate ORM/Reactive).
  • If a datasource is active and unconfigured (~= missing URL):
    • if we know it's statically injected somewhere, we'd fail on startup
    • if we don't know of any static injection, we'd deactivate the datasource automatically. That would disable healthchecks for that datasource, and would throw an exception on any attempt to retrieve the datasource

Here are hints regarding how to detect static injection:

BeanDiscoveryFinishedBuildItem#getInjectionPoints() if you're interested
in "class-based" injection points or
SynthesisFinishedBuildItem#getInjectionPoints() if you also need
synthetic injection points as well.

And for both build items there's also the
#getBeanResolver().resolveBeans() method that can help with type-safe
resolution rules.

@yrodiere yrodiere mentioned this issue Jun 12, 2024
Closed
@yrodiere
Copy link
Member

Related discussion: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/.22Activate.22.2F.22Deactivate.22.20beans.20at.20runtime

@yrodiere yrodiere mentioned this issue Jun 26, 2024
Closed
@yrodiere
Copy link
Member

Opened #41466 to try to generalize the solution.

@yrodiere yrodiere mentioned this issue Jul 22, 2024
Merged
@yrodiere yrodiere mentioned this issue Jul 31, 2024
Open
@yrodiere yrodiere mentioned this issue Oct 7, 2024
Merged
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/agroal area/health area/smallrye kind/bug Something isn't working
Projects
None yet
Milestone
3.17.0.CR1
3 participants
@yrodiere @gsmet @jtama