kie-issues#2659: [SonataFlow] Migrate DBMigration Image from SonataFlow operator repository to kie-tools

[rhkp]

Closes #2659
Add kogito-db-migrator-tool and kogito-db-migrator-tool-image packages for use by the SonataFlow Operator.

[ricardozanini]

I've left some comments to adjust the PR. Thank you!

[ricardozanini]

Please follow the same directory structure we already have in the other images: https://github.com/apache/incubator-kie-tools/tree/main/packages/kogito-data-index-ephemeral-image

[tiagobento]

Some more inline comments. Thanks for the changes so far @rhkp!

[ricardozanini]

We are almost there!

By the way, have you successfully built this image locally using this package?

[rhkp]

We are almost there!

By the way, have you successfully built this image locally using this package?

Yes. All build:prod and docker run are successful, locally, as in screenshots.

[tiagobento]

@rhkp Not sure this is WIP, but having these new env vars on root-env is not really the way to go. Can you please share your reasoning? Not sure if there are things missing on MANUAL.md that could help. Thanks!

[rhkp]

@rhkp Not sure this is WIP, but having these new env vars on root-env is not really the way to go. Can you please share your reasoning? Not sure if there are things missing on MANUAL.md that could help. Thanks!

Hi @tiagobento,
We have used a better approach using maven dependency plugin and changes to the root-env are rolled back.

[wmedvede]

Hi @rhkp , I have added some comments, mostly nitpicks.

Great work!

[wmedvede]

I have built the image locally and executed the migration, it worked well 💪 :

see: https://gist.github.com/wmedvede/549ecb613e2c27d09f4978624c43253b

The only weird thing is that in the logs I can see this:

2024-12-17 17:24:01,255 WARN [io.qua.run.con.ConfigRecorder] (main) Build time property cannot be changed at runtime:

• quarkus.platform.version is set to '3.8.6' but it is build time fixed to '999-SNAPSHOT'. Did you change the property quarkus.platform.version after building the application?
  2024-12-17 17:24:01,476 INFO [io.quarkus] (main) sonataflow-db-migrator on JVM (powered by Quarkus 3.8.6) started in 0.443s.

The quarkus.platform.version should never have been 999-SNAPSHOT 🤔

[rhkp]

This indicates some issue with the env you may have used. May be some old image is lingering there. The latest logs can be found here and it nowhere refers to 999-SNAPSHOT.

https://gist.github.com/rhkp/bbee2c6654068ad7009cff1319280312

I built the image using:
pnpm --stream=true -F kogito-db-migrator-tool-image... build:prod

And executed with (please change params based on your needs)
docker run --network=host --env MIGRATE_DB_DATAINDEX=true --env QUARKUS_DATASOURCE_DATAINDEX_JDBC_URL=jdbc:postgresql://127.0.0.1:5432/di --env QUARKUS_DATASOURCE_DATAINDEX_USERNAME=postgres --env QUARKUS_DATASOURCE_DATAINDEX_PASSWORD=postgres --env QUARKUS_FLYWAY_DATAINDEX_SCHEMAS=di --env MIGRATE_DB_JOBSSERVICE=true --env QUARKUS_DATASOURCE_JOBSSERVICE_JDBC_URL=jdbc:postgresql://127.0.0.1:5432/js --env QUARKUS_DATASOURCE_JOBSSERVICE_USERNAME=postgres --env QUARKUS_DATASOURCE_JOBSSERVICE_PASSWORD=postgres --env QUARKUS_FLYWAY_JOBSSERVICE_SCHEMAS=js docker.io/apache/incubator-kie-kogito-db-migrator-tool:main

[wmedvede]

I took the latest commits
I have deleted previous image
I have re-build this image

I can still see:

2024-12-18 11:39:32,878 WARN [io.qua.run.con.ConfigRecorder] (main) Build time property cannot be changed at runtime:

• quarkus.platform.version is set to '3.8.6' but it is build time fixed to '999-SNAPSHOT'. Did you change the property quarkus.platform.version after building the application?

https://gist.github.com/wmedvede/aff0a7afa3ed948bce894fdc6e9f5854

[rhkp]

This is what I found, when building the DB Migrator Image:
if the QUARKUS_PLATFORM_VERSION env var in terminal is not set, the 999-SNAPSHOT version appears.

However when the QUARKUS_PLATFORM_VERSION env var in terminal is set when the image is built with a value such as 3.8.6 this does not appear.

However not sure where and why the 999-SNAPSHOT version is being specifically picked up, as the DB migrator tool and/or image do not specifically use these versions.

A quick search of this string in sources shows 999-SNAPSHOT is not used in the tool e.g.

[rhkp]

Hi @wmedvede and @ricardozanini,
I spent more time on this one.

EXECUTE DB MIGRATOR JAVA JAR IN TERMINAL(WITHOUT IMAGE I MEAN)

• The execution of the DB migrator with java -jar target/quarkus-app/quarkus-run.jar gives identical results whether the export QUARKUS_PLATFORM_VERSION=3.8.6 variable is set or not.
• Meaning in any of the situation above I am not seeing 999-SNAPSHOTS log.
• Please note the WARN message for 999-SNAPSHOT comes from [io.qua.run.con.ConfigRecorder] package, when we are using image and I do not at all see this package in the logs in these scenarios.

COMPARE DB MIGRATOR IMAGE build FOLDERS

• I compared TWO directories one built with env vars set and another without setting one.
• The files are identical in this case too.
• And not sure where the 999-SNAPSHOT appears from in the logs when the image is built without setting export QUARKUS_PLATFORM_VERSION=3.8.6.

[wmedvede]

Hi @wmedvede and @ricardozanini, I spent more time on this one.

EXECUTE DB MIGRATOR JAVA JAR IN TERMINAL(WITHOUT IMAGE I MEAN)

• The execution of the DB migrator with java -jar target/quarkus-app/quarkus-run.jar gives identical results whether the export QUARKUS_PLATFORM_VERSION=3.8.6 variable is set or not.
• Meaning in any of the situation above I am not seeing 999-SNAPSHOTS log.
• Please note the WARN message for 999-SNAPSHOT comes from [io.qua.run.con.ConfigRecorder] package, when we are using image and I do not at all see this package in the logs in these scenarios.

COMPARE DB MIGRATOR IMAGE build FOLDERS

• I compared TWO directories one built with env vars set and another without setting one.
• The files are identical in this case too.
• And not sure where the 999-SNAPSHOT appears from in the logs when the image is built without setting export QUARKUS_PLATFORM_VERSION=3.8.6.

I have built again locally and executed both scenarios:

java -jar target/quarkus-app/quarkus-run.jar:

Produce NO Warnings:

see full execution log here: https://gist.github.com/wmedvede/b3631a99b041bd66afdda5e9cb3120b1

--/ __ / / / / _ | / _ / /// / / / __/
-/ // / // / __ |/ , / ,< / // /\ \
--___// |//|//||_//
2024-12-20 13:03:31,609 INFO [io.quarkus] (main) kogito-db-migrator-tool 0.0.0 on JVM (powered by Quarkus 3.8.6) started in 0.469s.
2024-12-20 13:03:31,615 INFO [io.quarkus] (main) Profile prod activated.
2024-12-20 13:03:31,615 INFO [io.quarkus] (main) Installed features: [agroal, cdi, flyway, jdbc-postgresql, narayana-jta, smallrye-context-propagation]
2024-12-20 13:03:31,626 INFO [org.kie.kog.pos.mig.DBConnectionChecker] (main) Checking DB connection: jdbc:postgresql://localhost:5432/data-index - success

Image execution: docker run docker.io/apache/incubator-kie-kogito-db-migrator-tool:main bla bla

Produce warnings

see full execution log here: https://gist.github.com/wmedvede/f7ad4a7ca9df7201438ab053b96dd38b

---

--/ __ / / / / _ | / _ / /// / / / __/
-/ // / // / __ |/ , / ,< / // /\ \
--___// |//|//||_//
2024-12-20 12:07:29,367 WARN [io.qua.run.con.ConfigRecorder] (main) Build time property cannot be changed at runtime:

• quarkus.platform.version is set to '3.8.6' but it is build time fixed to '999-SNAPSHOT'. Did you change the property quarkus.platform.version after building the application?
  2024-12-20 12:07:29,599 INFO [io.quarkus] (main) kogito-db-migrator-tool 0.0.0 on JVM (powered by Quarkus 3.8.6) started in 0.476s.

@rhkp @ricardozanini I conclude that the issue is introduced during the image build process, while the quarkus-run.jar is fine. Apart of doing the review, I have spent some time trying to see where issue comes from, but couldn't find.

Maybe someone else with fresh eyes can take a look and identify, I think it should be a silly nitpick.

@rgdoliveira @domhanak or maybe @fantonangeli , would you mind guys take look?

[wmedvede]

Another general comment that I have just noted:

1. In the module Readme, we say that the migration tool is intended only for use in the context of the Sontaflow operator, etc.
2. The produced artifact, is sonataflow-db-migrator

   <modelVersion>4.0.0</modelVersion>
  <groupId>org.kie.kogito</groupId>
  <artifactId>sonataflow-db-migrator</artifactId>

however, the package is: kogito-db-migrator-tool

For me it doesn't look consistent 🤔

wdyt?
@rgdoliveira @ricardozanini @domhanak @rhkp

Then, in the case of the image:

The package is: kogito-db-migrator-tool-image and we produce: apache/incubator-kie-kogito-db-migrator-tool, looks consistent.

[ricardozanini]

@rhkp @wmedvede

however, the package is: kogito-db-migrator-tool

That should be the name of the package, it's not sonataflow related.

[wmedvede]

I have added a couple of new comments, take a look please.

[wmedvede]

@rhkp thanks for adding all the suggestions.
One last nitpick from my side, other than this LGTM.

If we look here:

incubator-kie-tools/README.md

Line 113 in 82b42be

> - To enable container images build: `export KIE_TOOLS_BUILD__buildContainerImages=true`

The formal production of the different images is conditioned to the env var KIE_TOOLS_BUILD__buildContainerImages=true

If we do for example pnpm -F kogito-data-index-postgresql-image... build:prod and the env var is not set, the image is not produced.

I think the DB migrator image should follow the same pattern, but now is always produced independently of the env var.

would you mind take a look at this last nitpick?

[ricardozanini]

LGTM!

[wmedvede]

LGTM, great work!

[wmedvede]

Hi @tiagobento , when you got some time, would you mind take a look if your suggestions where addressed?

[tiagobento]

Will take a look during this week.

[tiagobento]

Looking good! Thanks @rhkp for all the effort you put into this, and for all the reviewers too. In general, I think we should make two things more cohesive:

1. If we want to include "postgresql" in the name of the packages; and
2. Always refer to PostgreSQL as its official name, so calling things "postgres" or "psql" or whatever it is I think only adds friction for users.

Other than, please consider my comments inline.

Thanks!

[tiagobento]

Do we still need this? I thought this file was only used when we still had the build happening inside the images @ricardozanini

[ricardozanini]

No, we do not. This file should be gone as well as any reference to it. To my knowledge, this package here (image) should rely on the db-migrator one to have the application packaged into the image. That's why both packages live together in the same repo.

[rhkp]

@ricardozanini @tiagobento

• The initial/original implementation was independent of sonataflow-image-common.
• However on Dec 09, 2024 based on this comment by @ricardozanini we started using sonataflow-image-common.
• Thus sonataflow-image-common package is vital for the current implementation and we cannot remove this file.

Please advise.

PS: If I understand it right other images are making use of this too and we are in-line with established practice?

[wmedvede]

1. If we want to include "postgresql" in the name of the packages; and

@tiagobento @rhkp @ricardozanini
Even while now the db-migrator only incorporate the sql files for postgresql, it is very close to be a multi database migrator 🚀. In fact, I was about to suggest that in my reviews, by incorporating an additional parameter "db-type"...., but I didn't want to add more changes to not frustrate the PR author with more suggestions :).

But I'm pretty sure that in the future... more db-types will come.

So I'd vote for keeping the naming as is.

I'd forgot to mention :), some renamings had happened already....
and the produced image is: apache/incubator-kie-kogito-db-migrator-tool

[tiagobento]

I'm fine with either approach @wmedvede, just as long as the name of package is clear and the intentions and scope of the new packages are well defined.

If you want to evolve a single package into a multi-db migration tool, that's fine for me. As well as if you want to have one package per db migration tool. Finding the right granularity is more of an art than a science, so even if the strategy chosen today doesn't evolve well, we can always change. The package abstraction is flexible enough to accommodate for changes like that, so please feel free to proceed in the way you feel is best now.

My request is just that the names and descriptions align with this initial strategy. Thx!

[rhkp]

I'm fine with either approach @wmedvede, just as long as the name of package is clear and the intentions and scope of the new packages are well defined.

If you want to evolve a single package into a multi-db migration tool, that's fine for me. As well as if you want to have one package per db migration tool. Finding the right granularity is more of an art than a science, so even if the strategy chosen today doesn't evolve well, we can always change. The package abstraction is flexible enough to accommodate for changes like that, so please feel free to proceed in the way you feel is best now.

My request is just that the names and descriptions align with this initial strategy. Thx!

Hi @tiagobento @wmedvede @ricardozanini
I think in this wave of implementation we are focussing on postgreSQL so I would suggest to keep it that way: keep postgreSQL in the package name.
In future when we do expand this functionality we can remove the postgreSQL from the name.
WDYT?

[wmedvede]

I think in this wave of implementation we are focussing on postgreSQL so I would suggest to keep it that way:

I'd personally prefer to keep the package names kogito-db-migrator-tool and kogito-db-migrator-tool-image, considering that as commented before, this work is very close to be a multi db migrator. Potential DB candidate to add in the short term could be H2, see: https://github.com/apache/incubator-kie-kogito-apps/blob/538f33c4a6a6e9c564cd172b32d7632cd792b1a0/data-index/data-index-storage/data-index-storage-jpa/src/test/resources/application.properties

[rhkp]

I think in this wave of implementation we are focussing on postgreSQL so I would suggest to keep it that way:

I'd personally prefer to keep the package names kogito-db-migrator-tool and kogito-db-migrator-tool-image, considering that as commented before, this work is very close to be a multi db migrator. Potential DB candidate to add in the short term could be H2, see: https://github.com/apache/incubator-kie-kogito-apps/blob/538f33c4a6a6e9c564cd172b32d7632cd792b1a0/data-index/data-index-storage/data-index-storage-jpa/src/test/resources/application.properties

Reverted back to original names.