Fix the generation init-tasks by database migration on Kubernetes #33116
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| package io.quarkus.kubernetes.deployment; | ||
|
|
||
| import io.dekorate.kubernetes.adapter.ContainerAdapter; | ||
| import io.dekorate.kubernetes.config.Container; | ||
| import io.dekorate.kubernetes.decorator.NamedResourceDecorator; | ||
| import io.fabric8.kubernetes.api.model.ContainerBuilder; | ||
| import io.fabric8.kubernetes.api.model.ObjectMeta; | ||
| import io.fabric8.kubernetes.api.model.PodSpecBuilder; | ||
|
|
||
| public abstract class AddInitContainerDecorator extends NamedResourceDecorator<PodSpecBuilder> { | ||
|
|
||
| private final Container container; | ||
|
|
||
| public AddInitContainerDecorator(String deployment, Container container) { | ||
| super(deployment); | ||
| this.container = container; | ||
| } | ||
|
|
||
| @Override | ||
| public void andThenVisit(PodSpecBuilder podSpec, ObjectMeta resourceMeta) { | ||
| var resource = ContainerAdapter.adapt(container); | ||
| if (podSpec.hasMatchingInitContainer(this::hasInitContainer)) { | ||
| update(podSpec, resource); | ||
| } else { | ||
| add(podSpec, resource); | ||
| } | ||
| } | ||
|
|
||
| private void add(PodSpecBuilder podSpec, io.fabric8.kubernetes.api.model.Container resource) { | ||
| podSpec.addToInitContainers(resource); | ||
| } | ||
|
|
||
| private void update(PodSpecBuilder podSpec, io.fabric8.kubernetes.api.model.Container resource) { | ||
| var matching = podSpec.editMatchingInitContainer(this::hasInitContainer); | ||
| if (resource.getImage() != null) { | ||
| matching.withImage(resource.getImage()); | ||
| } | ||
|
|
||
| if (resource.getImage() != null) { | ||
| matching.withImage(resource.getImage()); | ||
| } | ||
|
|
||
| if (resource.getWorkingDir() != null) { | ||
| matching.withWorkingDir(resource.getWorkingDir()); | ||
| } | ||
|
|
||
| if (resource.getCommand() != null && !resource.getCommand().isEmpty()) { | ||
| matching.withCommand(resource.getCommand()); | ||
| } | ||
|
|
||
| if (resource.getArgs() != null && !resource.getArgs().isEmpty()) { | ||
| matching.withArgs(resource.getArgs()); | ||
| } | ||
|
|
||
| if (resource.getReadinessProbe() != null) { | ||
| matching.withReadinessProbe(resource.getReadinessProbe()); | ||
| } | ||
|
|
||
| if (resource.getLivenessProbe() != null) { | ||
| matching.withLivenessProbe(resource.getLivenessProbe()); | ||
| } | ||
|
|
||
| matching.addAllToEnv(resource.getEnv()); | ||
| if (resource.getPorts() != null && !resource.getPorts().isEmpty()) { | ||
| matching.withPorts(resource.getPorts()); | ||
| } | ||
|
|
||
| matching.endInitContainer(); | ||
| } | ||
|
|
||
| private boolean hasInitContainer(ContainerBuilder containerBuilder) { | ||
|
There was a problem hiding this comment. 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. |
||
| return containerBuilder.getName().equals(container.getName()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package io.quarkus.kubernetes.deployment; | ||
|
|
||
| import io.dekorate.kubernetes.config.Container; | ||
| import io.dekorate.kubernetes.decorator.Decorator; | ||
|
|
||
| public class AddInitContainerFromExtensionsDecorator extends AddInitContainerDecorator { | ||
|
|
||
| public AddInitContainerFromExtensionsDecorator(String deployment, Container container) { | ||
| super(deployment, container); | ||
| } | ||
|
|
||
| public Class<? extends Decorator>[] before() { | ||
| return new Class[] { AddInitContainerFromUserConfigDecorator.class }; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| package io.quarkus.kubernetes.deployment; | ||
|
|
||
| import io.dekorate.kubernetes.config.Container; | ||
|
|
||
| public class AddInitContainerFromUserConfigDecorator extends AddInitContainerDecorator { | ||
|
|
||
| public AddInitContainerFromUserConfigDecorator(String name, Container container) { | ||
| super(name, container); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,4 +15,22 @@ public class LiquibaseMongodbBuildTimeConfig { | |
| */ | ||
| @ConfigItem(defaultValue = "db/changeLog.xml") | ||
| public String changeLog; | ||
|
|
||
| /** | ||
| * Flag to enable / disable the generation of the init task Kubernetes resources. | ||
| * This property is only relevant if the Quarkus Kubernetes/OpenShift extensions are present. | ||
| * | ||
| * The default value is `quarkus.liquibase-mongodb.enabled`. | ||
| */ | ||
| @ConfigItem(defaultValue = "${quarkus.liquibase-mongodb.enabled:true}") | ||
| public boolean generateInitTask; | ||
|
|
||
| /** | ||
| * Flag to enable / disable the migration using the generated init task Kubernetes resources. | ||
| * This property is only relevant if the Quarkus Kubernetes/OpenShift extensions are present. | ||
| * | ||
| * The default value is `quarkus.liquibase-mongodb.migrate-at-start`. | ||
| */ | ||
| @ConfigItem(defaultValue = "${quarkus.liquibase-mongodb.migrate-at-start:false}") | ||
|
There was a problem hiding this comment. I don't think we need two properties for doing pretty much the same thing. 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 There was a problem hiding this comment.
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 ( Therefore, if users enable the About having two properties, this extension can be enabled/disabled using two properties There was a problem hiding this comment. The About the default value I don't have a super strong opinion so let's see what others think. There was a problem hiding this comment. I am pretty sure that won't work, but I'll have a look at this tomorrow There was a problem hiding this comment. 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.
It works as you can see with the new test cases I added as part of this pull request. There was a problem hiding this comment.
Agree. There is a clash so we shouldn't tie the two together.
The fact that this is indeed working is irrelevant. There is a clash as you already pointed out and we should better avoid it. There was a problem hiding this comment.
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. |
||
| public boolean migrateWithInitTask; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary to overwrite the hardcoded properties (
image,command,args) of the init-containers using the standard properties: #33097 (comment)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.