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

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

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

Conversation

rhkp
Copy link
Contributor

@rhkp rhkp commented Dec 4, 2024

Closes #2789
Implement DB Migrator Changes into SonataFlow Operator.

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.

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!

@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
packages/sonataflow-operator/operator.yaml Show resolved Hide resolved
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.

}

if jsJobsBasedDBMigration {
quarkusDataSourceJobService = getQuarkusDataSourceFromPersistence(ctx, platform, platform.Spec.Services.JobService.Persistence, "jobs-service")
Copy link
Contributor

Choose a reason for hiding this comment

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

same as for the DI, see comment above.

Copy link
Contributor

Choose a reason for hiding this comment

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

and same comment for default schema name calculation, see: j.GetServiceName()

return defaultSchemaName
}

func getJdbcUrl(postgresql *operatorapi.PersistencePostgreSQL, defaultDBSchemaName string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this calculations are kind of duplicated here:

I think we should explore moving this calculation postgresql.go and reuse.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file needs tests, the same as we have for other ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @wmedvede!
They are included here e.g.

Please let me know if you are referring to something else.

MigrationCmd string
}

func getDBSchemaName(persistencePostgreSQL *operatorapi.PersistencePostgreSQL, defaultSchemaName string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

tests please

return dataSourceURL
}

func getQuarkusDataSourceFromPersistence(ctx context.Context, platform *operatorapi.SonataFlowPlatform, persistence *operatorapi.PersistenceOptionsSpec, defaultSchemaName string) *QuarkusDataSource {
Copy link
Contributor

Choose a reason for hiding this comment

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

Important:

Persistence configuration for a service can be taken not only from eg. spec.services.jobsService but also from the SPF global persistence configuration.

see:

https://sonataflow.org/serverlessworkflow/main/cloud/operator/supporting-services.html#common-persistence-configuration

https://sonataflow.org/serverlessworkflow/main/cloud/operator/using-persistence.html#configuring-persistence-using-the-sonataflowplatform-cr

Along this PR, we must consider that potential case too.

I.e, the service don't have "persistence", but the SPF do.

Examples:

	apiVersion: sonataflow.org/v1alpha08
	kind: SonataFlowPlatform
	metadata:
	  name: sonataflow-platform
	spec:
	  services:
	    # no persistence is defined for the DI, it will take it from the SFP if any.
	    # and apply the by default DBMigrationStrategy
	    dataIndex:
	      enabled: true

	    jobService:
	      # The only persistence setting we do is the DBMigrationStrategy
	      # So, the service grabs the persistence configuation from the SPF and the given DBMigrationStrategy
	      # from here.
	      
	      enabled: true
	      persistence:
		dbMigrationStrategy: "job"
	  
	  # global SPF persitence configuration.     
	  persistence:
	    postgresql:
	      secretRef:
		name: postgres-secrets
		userKey: POSTGRESQL_USER
		passwordKey: POSTGRESQL_PASSWORD
	      serviceRef:
		name: postgres
		databaseName: sonataflow

@rhkp take look please and propagate in all places where needed.

@ricardozanini see my considerations for this particular case, where a service takes the persistence from the SPF, but we still can mark at the service level the dbMigrationStrategy

diJobsBasedDBMigration := false
jsJobsBasedDBMigration := false

if pshDI.IsPersistenceEnabledtInSpec() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider my comments regarding a potential persistence configuration at the SFP level

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sonataflow-operator] Implement DB Migrator Changes into SonataFlow Operator
4 participants