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

kie-issues#2789: [sonataflow-operator] Implement DB Migrator Changes into SonataFlow Operator #2790

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

kie-issues#2789: [sonataflow-operator] Implement DB Migrator Changes into SonataFlow Operator #2790

wants to merge 13 commits into from

Conversation

rhkp
Copy link
Contributor

@rhkp rhkp commented Dec 4, 2024

Closes #2789
Implement DB Migrator Changes into SonataFlow Operator.

@rhkp rhkp requested a review from tiagobento as a code owner December 4, 2024 15:36
ricardozanini
ricardozanini requested changes Dec 5, 2024
View reviewed changes
packages/sonataflow-operator/api/v1alpha08/sonataflowplatform_services_types.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/dbMigratorJob.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/dbMigratorJob.go Outdated
const (
dbMigrationJobName = "sonataflow-db-migrator-job"
dbMigrationContainerName = "db-migration-container"
dbMigratorToolImage = "quay.io/rhkp/incubator-kie-kogito-service-db-migration-postgresql:latest"
Copy link
Member

Choose a reason for hiding this comment

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

This image must come from the image package. So can you bring that here? Or we can merge the image first and then you review this const. I'm about to change this versioning/image naming in the operator package to read this info from ENV/Configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to "docker.io/apache/incubator-kie-kogito-db-migrator-tool:latest" as named in DB Migrator Image package here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, unresolving this and reverting to old image for sake of seeing how CI behaves.

packages/sonataflow-operator/internal/controller/platform/dbMigratorJob.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/dbMigratorJob.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/dbMigratorJob.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/dbMigratorJob.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/services/secrets.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/services/services.go Show resolved Hide resolved
...est/e2e/testdata/platform/persistence/service_based_db_migration/02-sonataflow_platform.yaml Outdated Show resolved Hide resolved
wmedvede
wmedvede reviewed Dec 20, 2024
View reviewed changes
Copy link
Contributor

@wmedvede wmedvede left a comment

Choose a reason for hiding this comment

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

Hi @rhkp I have added some comments, but couldn't finish the review.
Will continue on this after Christmas.

Many thanks!

packages/sonataflow-operator/api/v1alpha08/sonataflow_persistence_types.go Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
@tiagobento
Copy link
Contributor

Can someone please help me understand the relationship between this PR and #2697?

This PR's issue has no description, which makes it hard to follow what's the overall plan for the DB Migrator image.

Thanks!

@rhkp
Copy link
Contributor Author

rhkp commented Dec 23, 2024

Can someone please help me understand the relationship between this PR and #2697?

This PR's issue has no description, which makes it hard to follow what's the overall plan for the DB Migrator image.

Thanks!

In short, the SonataFlow Operator PR#2790 will make use of the image created by PR#2697.

@wmedvede wmedvede requested a review from jakubschwan January 14, 2025 16:12
wmedvede
wmedvede requested changes Jan 16, 2025
View reviewed changes
packages/sonataflow-operator/operator.yaml Show resolved Hide resolved
packages/sonataflow-operator/config/manager/controllers_cfg.yaml Outdated Show resolved Hide resolved
...es/sonataflow-operator/config/manifests/bases/sonataflow-operator.clusterserviceversion.yaml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the dbMigrationStrategy must be added to the DI and JS in this file too.

                      dbMigrationStrategy:
                        default: service
                        description: |-
                          DB Migration approach for service?
                          job: use job based approach
                          service: service itself shall migrate the db
                          none: no db migration needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wmedvede Where are DI and JS in this file? Please advise.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rhkp your observation is good, and mine too 🤔

Rethinking here, It looks like we have a sort of outdated (and not necessarily outdated) specification of the CustomResourceDefinitions in this file... i.e., not all the fields were always added to the respective CRDs.

Some fields were added, and some other don't.

For example:
we have things like path: services.jobService.podTemplate but we don't have things like services.jobService.persistence

Which makes me think that not all the fields for the different CRDs owned by this operator where added to this file along the time. Question is, do we need them all added there? I think that not, see below.

Also, by looking at here: https://olm.operatorframework.io/docs/concepts/crds/clusterserviceversion I think the full specification of each CRD in that list is not needed (that information is not used for creating the respective CRDs), instead we could just only declare file the list of owned resources by this operator....

@ricardozanini is my reasoning correct?

packages/sonataflow-operator/internal/controller/cfg/controllers_cfg.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/services/secrets.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
wmedvede
wmedvede requested changes Jan 16, 2025
View reviewed changes
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/k8s.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/k8s.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/k8s.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/k8s.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/k8s.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/k8s.go Outdated Show resolved Hide resolved
jakubschwan
jakubschwan reviewed Jan 21, 2025
View reviewed changes
packages/sonataflow-operator/config/manager/controllers_cfg.yaml
Comment on lines +33 to +34
# The Kogito PostgreSQL DB Migrator image to use (TBD: to replace with apache image)
dbMigratorToolImageTag: "quay.io/rhkp/incubator-kie-kogito-service-db-migration-postgresql:latest"
Copy link
Contributor

Choose a reason for hiding this comment

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

do not forget to replace with apache image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed as soon as the image PR is approved and image is available, we will do the same.

packages/sonataflow-operator/api/v1alpha08/sonataflow_persistence_types.go Outdated Show resolved Hide resolved
packages/sonataflow-operator/internal/controller/platform/db_migrator_job.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakubschwan jakubschwan jakubschwan left review comments

@wmedvede wmedvede wmedvede requested changes

@ricardozanini ricardozanini ricardozanini requested changes

@tiagobento tiagobento Awaiting requested review from tiagobento tiagobento is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
area:sonataflow
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[sonataflow-operator] Implement DB Migrator Changes into SonataFlow Operator
5 participants
@rhkp @tiagobento @ricardozanini @wmedvede @jakubschwan