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.

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

Use health check to detect mariadb startup and skip volume modifier on M1 #25805

Merged
merged 1 commit into from
May 27, 2022

Conversation

holly-cummins
Copy link
Contributor

@holly-cummins holly-cummins commented May 26, 2022

Partial resolution of #25428. See also discussion in #25648. This PR gets modules which use mariadb running clean with -Dstart-containers on M1 with podman as the container implementation. I know some of the files in this change have mysql-* names, but they’re actually using mariadb, so I included them.

To test these changes, this sequence should be sufficient. It runs clean for me on my M1 after podman machine start:

TESTCONTAINERS_RYUK_DISABLED="true" ./mvnw -Dquickly -DskipTests=false -Dstart-containers -f extensions/reactive-mysql-client/deployment/pom.xml 
TESTCONTAINERS_RYUK_DISABLED="true" ./mvnw -Dquickly -DskipTests=false -Dstart-containers -f integration-tests/hibernate-orm-tenancy/connection-resolver-legacy-qualifiers/pom.xml 
TESTCONTAINERS_RYUK_DISABLED="true" ./mvnw -Dquickly -DskipTests=false -Dstart-containers -f integration-tests/hibernate-orm-tenancy/connection-resolver/pom.xml 
TESTCONTAINERS_RYUK_DISABLED="true" ./mvnw -Dquickly -DskipTests=false -Dstart-containers -f integration-tests/hibernate-orm-tenancy/datasource/pom.xml 
TESTCONTAINERS_RYUK_DISABLED="true" ./mvnw -Dquickly -DskipTests=false -Dstart-containers -f integration-tests/hibernate-reactive-mysql/pom.xml 
TESTCONTAINERS_RYUK_DISABLED="true" ./mvnw -Dquickly -DskipTests=false -Dstart-containers -f integration-tests/jpa-mariadb/pom.xml 
TESTCONTAINERS_RYUK_DISABLED="true" ./mvnw -Dquickly -DskipTests=false -Dstart-containers -f integration-tests/reactive-mysql-client/pom.xml

(and it should run much faster than before the changes, on all platforms).

Note that I do still see some errors in quarkus-integration-test-hibernate-orm-tenancy-connection-resolver-legacy-qualifiers with -Dstart-containers -Dtest-containers. Fixing those is outside the scope of this changeset, which is only looking at getting -Dstart-containers going.

There is some whitespace in the diffs, but I think most of the changes are ‘correct’. Ideally, those changes wouldn’t be contaminating this changeset, but see https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/IntelliJ.20formatting.20setup/near/283912797. Getting our xml formatting consistent will need a bigger effort.

What’s changed?

Readiness checks

I had a problem with tcp-ping based readiness checks for mariadb. Only one test used it, so we can switch to using an sqladmin ping check. Several tests do a ping inside a in the . However, a inside a isn’t a true readiness check. The plugin will always wait for the and then run the`. A better (but more verbose) option is to use a container health check.

This was only strictly necessary where the tests were using a tcp ping to check for readiness (which did not work on podman), but I think it’s an improvement elsewhere.

This sped up the tests quite a bit for me since it will cut the wait short if the database is ready before the timeout pops. I saw a factor of two improvement on tests which used mariadb, but on slower hardware the effect may not be so noticeable.

Be aware when testing healthcheck scripts, caching of named images can cause apparently non-deterministic behaviour. Change the name if making changes.

I also found that trying to extract common code to parent poms caused tests not to be able to connect to the containers. I don’t understand the reason - perhaps some crucial fabric8 state ends up in a target directory at the parent level?

Volume mounts access modifier

The ':Z' SELinux access label seems to cause problems on M1. I think this is related to containers/podman#13631. We mount config files to speed up mariadb, so I’ve parameterised the access modifier so that the mount works on M1.

Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, it's indeed a bit faster for me as well: typical module is now tested in ~19 seconds rather than ~30s.

Also I was a bit worried about this step to build a custom image each time, but it seems that takes just some milliseconds.

There's a little inconsistency in how the custom image builds are named though: I see some are named healthcheck-${mariadb.image}, others init-healthcheck-${mariadb.image}; is there a reason for that?

This seems to cause a problem in my environment: after a module is run, often the instance is leaked (as in it's still running after the previous module completed); this is normally not a problem as when starting a new module we stop existing ones - but the image name must match.

I don't know why they are leaked occasionally but this seems an existing issue - so don't worry about it, assuming we can name them all the same.

@holly-cummins holly-cummins force-pushed the mariadb-m1-podman-fabric8 branch from 3b720fa to 348c2ff Compare May 26, 2022 13:43
@holly-cummins
Copy link
Contributor Author

I'd noticed images sometimes didn't shut down properly too, but I hadn't worked out that inconsistent naming would make the symptoms worse, thanks. There was no reason for the variation in image names, so I've updated to make them consistent.

I wished it wasn't necessary to make the images each time, both because it's a lot of clutter in the build scripts, and it seems like repeated work. I tried pulling things up to the parent poms and things started failing, so I pushed everything back down. With persistence it might be possible to make it work, but since the image build is quick, it's probably not worth it.

More recent mariadb images include a healthcheck script, but it's not wired into the dockerfile, so it doesn't directly help us.

@holly-cummins holly-cummins requested a review from Sanne May 26, 2022 14:00
@holly-cummins holly-cummins changed the title Use health check to detect startup and skip volume modifier on M1 Use health check to detect mariadb startup and skip volume modifier on M1 May 26, 2022
@Sanne Sanne added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 26, 2022
@geoand geoand merged commit 94a53ba into quarkusio:main May 27, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone May 27, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 27, 2022
@holly-cummins holly-cummins deleted the mariadb-m1-podman-fabric8 branch May 31, 2022 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/hibernate-orm Hibernate ORM area/persistence OBSOLETE, DO NOT USE area/reactive-sql-clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants