Fix the generation init-tasks by database migration on Kubernetes #33116

Sgitario · 2023-05-04T05:58:16Z

These changes include:
- Don't generate the init task for db migration if migrate at start is off:

The quarkus.[flyway or liquibase or liquibase-mongodb].enabled and quarkus.[flyway or liquibase or liquibase-mongodb].migrate-at-start properties are runtime properties, so I have to create the following build time properties to read these values and allow users configurating it:

quarkus.[flyway or liquibase or liquibase-mongodb].generate-init-task with default value quarkus.[flyway or liquibase or liquibase-mongodb].enabled
quarkus.[flyway or liquibase or liquibase-mongodb].migrate-with-init-task with default value quarkus.[flyway or liquibase or liquibase-mongodb].migrate-at-start

Plus, I've extended the coverage of these extensions with the K8s tests.

- Allow overriding generated init-container of extensions from user config (related to this comment)
- Change default image for init container waiters (necessary for OpenShift deployments) (related to this comment)

Each of these changes is included in an individual commit on purpose to ease up the review.

Fix #33097

Sgitario · 2023-05-04T05:59:03Z

Moreover, there is an outstanding issue that is fixed by #33086.

The `quarkus.[flyway or liquibase or liquibase-mongodb].enabled` and `quarkus.[flyway or liquibase or liquibase-mongodb].migrate-at-start` properties are runtime properties, so I have to create the following build time properties to read these values and allow users configurating it: - `quarkus.[flyway or liquibase or liquibase-mongodb].generate-init-task` with default value `quarkus.[flyway or liquibase or liquibase-mongodb].enabled` - `quarkus.[flyway or liquibase or liquibase-mongodb].migrate-with-init-task` with default value `quarkus.[flyway or liquibase or liquibase-mongodb].migrate-at-start` Plus, I've extended the coverage of these extensions with the K8s tests. Fix quarkusio#33097

Related to quarkusio#33097 (comment)

To groundnuty/k8s-wait-for:no-root-v1.7. Related to quarkusio#33097 (comment)

quarkus-bot · 2023-05-04T18:57:01Z

Failing Jobs - Building `a536cb9`

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✖	JVM Tests - JDK 17	`Build`	Failures	Logs	Raw logs
⌛	JVM Tests - JDK 17 Windows	`Build`	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 19	`Build`	⚠️ Check →	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/elytron-security-jdbc/deployment 
! Skipped: integration-tests/elytron-security-jdbc

📦 extensions/elytron-security-jdbc/deployment

✖ io.quarkus.elytron.security.jdbc.CustomRoleDecoderDevModeTest.testConfigChange line 66 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

✖ io.quarkus.elytron.security.jdbc.CustomRoleDecoderTest. - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:704)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$12(ClassBasedTestDescriptor.java:395)

✖ io.quarkus.elytron.security.jdbc.MinimalConfigurationTest. - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:704)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$12(ClassBasedTestDescriptor.java:395)

✖ io.quarkus.elytron.security.jdbc.MultipleDataSourcesTest. - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:704)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$12(ClassBasedTestDescriptor.java:395)

✖ io.quarkus.elytron.security.jdbc.MultipleQueriesConfigurationTest. - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:704)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$12(ClassBasedTestDescriptor.java:395)

Sgitario · 2023-05-05T05:00:44Z

I think the test failures are unrelated.

gsmet · 2023-05-05T07:30:12Z

@Sgitario is it something that should be backported to 3.0?

Sgitario · 2023-05-05T07:32:24Z

@Sgitario is it something that should be backported to 3.0?

The issue was introduced in #29026 which is part of Quarkus 3. So, yes, I'm adding the backport label.

iocanel · 2023-05-06T10:39:36Z

...lla/deployment/src/main/java/io/quarkus/kubernetes/deployment/AddInitContainerDecorator.java

+        matching.endInitContainer();
+    }
+
+    private boolean hasInitContainer(ContainerBuilder containerBuilder) {


This predicate does not test the existence of a container, but wether a container has matching name. The name should probably change to match the functionality.

will rename it

iocanel

I have a few objections that I'll summarize here.

Introduced configuration is more fine grained than it needs to.
We are defaulting build time configuration to runtime config values, which is weird.
We implement multiple versions of decorators for not clear reason.

iocanel · 2023-05-06T10:53:13Z

...lla/deployment/src/main/java/io/quarkus/kubernetes/deployment/AddInitContainerDecorator.java

+import io.fabric8.kubernetes.api.model.ObjectMeta;
+import io.fabric8.kubernetes.api.model.PodSpecBuilder;
+
+public abstract class AddInitContainerDecorator extends NamedResourceDecorator<PodSpecBuilder> {


I don't see a reason for creating a new decorator, let alone 3.
If there is a technical reason why we do so, it should definitely need to be part of a comment.

This is necessary to overwrite the hardcoded properties (image, command, args) of the init-containers using the standard properties: #33097 (comment)

We don't need new decorators to do that. Existing decorators for setting image, command and args do already exist and should be used instead. We should limit the amount of overlapping decorators as its complicates predicting the order of things.

iocanel · 2023-05-06T11:02:42Z

...time/src/main/java/io/quarkus/liquibase/mongodb/runtime/LiquibaseMongodbBuildTimeConfig.java

+     *
+     * The default value is `quarkus.liquibase-mongodb.migrate-at-start`.
+     */
+    @ConfigItem(defaultValue = "${quarkus.liquibase-mongodb.migrate-at-start:false}")


I don't think we need two properties for doing pretty much the same thing.
One should be enough.

Also, I am not a big fan for having build time configuration defaulting to runtime values. This seems weird if not wrong.

Last, I am wondering if generateInitTask will is descriptive enough to let people understand this refers to kubernetes jobs.

Also, I am not a big fan for having build time configuration defaulting to runtime values. This seems weird if not wrong.

I'm not super happy with this either. Though, we're generating build time resources (the init-task resources) that is actually disabled by default (migrate-at-start property is disabled by default), and can be enabled/disabled at runtime.

Therefore, if users enable the migrate-at-start property at build time, then it means that they are likely to need the init-task when running the app in K8s, so I think it makes sense to me.

About having two properties, this extension can be enabled/disabled using two properties enabled and migrate-at-start, so having only one property is probably not enough. Also, adding a totally new build-time property will be unlikely to be used by users (because they are used to use the existing and well-known enabled and migrate-at-start properties).

The migrate-at-start property is a runtime property and does not affect in any way the generation of kubernetes resources, so the property is redundant and needs to be removed. What the init container does should not be a concern of the part that is generating the Job.

About the default value I don't have a super strong opinion so let's see what others think.
@geoand @gsmet: We need your expert opinion: Is it ok to have a build time property with a default value that points to a runtime property?

I am pretty sure that won't work, but I'll have a look at this tomorrow

The migration is enabled/disabled by runtime properties, and the generated Job to perform the migration has to be created/or not created at build time. So, here is the clash.

Is it ok to have a build time property with a default value that points to a runtime property?

It works as you can see with the new test cases I added as part of this pull request.

The migration is enabled/disabled by runtime properties, and the generated Job to perform the migration has to be created/or not created at build time. So, here is the clash.

Agree. There is a clash so we shouldn't tie the two together.

It works as you can see with the new test cases I added as part of this pull request.

The fact that this is indeed working is irrelevant. There is a clash as you already pointed out and we should better avoid it.

It works as you can see with the new test cases I added as part of this pull request.

The fact that this is indeed working is irrelevant. There is a clash as you already pointed out and we should better avoid it.

I don't see your point here: the test cases probe that the new build properties whose default values are taken from runtime properties work fine, but you say it's irrelevant?

The current pull request does address and avoids the current linked issue when using flyway / liquibase plus K8s/OpenShift extensions by using well-known properties. It's perfectly fine if you all don't like this solution and/or prefer adding a new build time property that users will need to learn to generate the init-task resources. But don't agree with you saying that the test cases are irrelevant or that the clashing problem is not addressed.

Sgitario · 2023-05-07T08:58:33Z

I have a few objections that I'll summarize here.

Introduced configuration is more fine grained than it needs to.

We are defaulting build time configuration to runtime config values, which is weird.

We implement multiple versions of decorators for not clear reason.

I hope I've clarified and justified all the points here for further discussion.
I still think the requirement is valid because the initial solution introduces breaking changes. And the solution is correct enough, but let me know if you have something else in mind to overcome the linked issue.

Sgitario · 2023-05-08T05:12:01Z

Closing as per comment in #33097 (comment)

Sgitario requested a review from iocanel May 4, 2023 05:58

quarkus-bot bot added area/flyway area/kubernetes area/liquibase labels May 4, 2023

Sgitario mentioned this pull request May 4, 2023

Quarkus with liquibase on Kubernetes #33097

Closed

Sgitario added 3 commits May 4, 2023 14:13

Allow overriding generated init-container of extensions from user config

66a2834

Related to quarkusio#33097 (comment)

Change default image for init container waiters

a536cb9

To groundnuty/k8s-wait-for:no-root-v1.7. Related to quarkusio#33097 (comment)

Sgitario force-pushed the 33097 branch from 273f772 to a536cb9 Compare May 4, 2023 12:13

Sgitario added the triage/backport? label May 5, 2023

iocanel reviewed May 6, 2023

View reviewed changes

iocanel requested changes May 6, 2023

View reviewed changes

Sgitario closed this May 8, 2023

quarkus-bot bot added triage/invalid This doesn't seem right and removed triage/backport? labels May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the generation init-tasks by database migration on Kubernetes #33116

Fix the generation init-tasks by database migration on Kubernetes #33116

Sgitario commented May 4, 2023 •

edited

Loading

Sgitario commented May 4, 2023

quarkus-bot bot commented May 4, 2023

Sgitario commented May 5, 2023

gsmet commented May 5, 2023

Sgitario commented May 5, 2023

iocanel May 6, 2023

Sgitario May 7, 2023

iocanel left a comment

iocanel May 6, 2023

Sgitario May 7, 2023

iocanel May 7, 2023

iocanel May 6, 2023

Sgitario May 7, 2023

iocanel May 7, 2023

geoand May 7, 2023

Sgitario May 8, 2023

iocanel May 8, 2023

Sgitario May 8, 2023

Sgitario commented May 7, 2023

Sgitario commented May 8, 2023

Fix the generation init-tasks by database migration on Kubernetes #33116

Fix the generation init-tasks by database migration on Kubernetes #33116

Conversation

Sgitario commented May 4, 2023 • edited Loading

Sgitario commented May 4, 2023

quarkus-bot bot commented May 4, 2023

Failing Jobs - Building a536cb9

Failures

⚙️ JVM Tests - JDK 17 #

📦 extensions/elytron-security-jdbc/deployment

Sgitario commented May 5, 2023

gsmet commented May 5, 2023

Sgitario commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iocanel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sgitario commented May 7, 2023

Sgitario commented May 8, 2023

Sgitario commented May 4, 2023 •

edited

Loading

Failing Jobs - Building `a536cb9`