[KOGITO-9980] Add persistence to SonataflowPlatform CRD #372

jordigilh · 2024-01-26T16:24:25Z

Adds a new field in the SonataflowPlatform Spec struct to allow defining a shareable postgreSQL persistence configuration for workflows and to platform services:
https://github.com/jordigilh/incubator-kie-kogito-serverless-operator/blob/7f96442d27ae773d23cab74e84423b069dfe2f90/api/v1alpha08/sonataflowplatform_types.go#L49-L53

This configuration is inherited by workflows that require persistence when the persistence field in the workflow spec is initialized as empty.
https://github.com/jordigilh/incubator-kie-kogito-serverless-operator/blob/ee06ddd88a03658dace236574108c2f446454186/controllers/profiles/common/object_creators_test.go#L383-L385

When both workflow CR and platform CR contain persistence, the workflow CR specification will take precedence.

Persistence in the workflow can be configured using these scenarios:

If persistence field is not nil, the operator will use its configuration. If the configuration is empty (e.g. {}, then no persistence is set for the workflow)
If the Persistence field is nil, the operator will derive the persistence from the SonataFlowPlatform CR's persistence field, if it exists. Otherwise no persistence is provided.

Extends e2e tests in these cases:

Sonataflow CR where it retrieves the persistence from the platform.
Sonataflow CR where it retrieves the persistence from its Persistence spec, ignoring the one provided by the platform CR.
Sonataflow CR where it's set to discard persistence from the Platform CR by initializing it as {}
Platform services where it retrieves the persistence from the platform.
Platform services where it retrieves the persistence from the platform service's spec in favor of the generic on defined in the platform CR Persistence field in the Spec.

@ricardozanini @wmedvede @tchughesiv @domhanak PTAL.

api/v1alpha08/sonataflowplatform_types.go

controllers/platform/services/services.go

test/testdata/platform/persistence/generic_from_platform_cr/02-sonataflow_platform.yaml

workflowproj/workflowproj_test.go

api/v1alpha08/sonataflowplatform_types.go

wmedvede

I have left some minor comments, good job! thanks.

controllers/platform/initialize.go

controllers/profiles/common/object_creators.go

controllers/platform/services/services.go

controllers/profiles/common/persistence/postgresql.go

test/testdata/platform/persistence/generic_from_platform_cr/02-sonataflow_platform.yaml

test/testdata/platform/persistence/overwritten_by_services/02-sonataflow_platform.yaml

...stdata/workflow/persistence/from_platform_overwritten_by_service/02-sonataflow_platform.yaml

api/v1alpha08/sonataflow_types.go

controllers/platform/services/services.go

ricardozanini · 2024-02-01T16:59:03Z

@jordigilh can you please rebase since we've merged the global platform PR?

wmedvede · 2024-02-02T08:22:06Z

@jordigilh look like we need some love to fix merge conflicts 😿

wmedvede

I have added some more comments, many thanks @jordigilh

api/v1alpha08/sonataflowplatform_types.go

controllers/platform/initialize.go

wmedvede · 2024-02-02T11:05:05Z

controllers/profiles/common/object_creators.go

@@ -157,7 +159,13 @@ func defaultContainer(workflow *operatorapi.SonataFlow) (*corev1.Container, erro
 	if err := mergo.Merge(defaultFlowContainer, workflow.Spec.PodTemplate.Container.ToContainer(), mergo.WithOverride); err != nil {
 		return nil, err
 	}
-	defaultFlowContainer = ConfigurePersistence(defaultFlowContainer, workflow.Spec.Persistence, defaultSchemaName, workflow.Namespace)
+	if workflow.Spec.Persistence != nil {


Please correct me if I'm wrong, but believe this code is not considering the case where the workflow don't have a configured persistence, but the platform has. In that case, we should try to get it from the platform.

I am correct?

There are 3 scenarios managed here:

Persistence field in the workflow spec is nil: If the persistence field is nil, then the workflow will have no persistence at all.

Persistence is empty ({}): Persistence will be pull from the platform if it exists.

Persistence is populated: Use the one in workflow.

Let me know if that makes sense in this case.

It makes sense. But what about adding a label to address this problem sonataflow.org/persistence=platform|workflow|none, defaults to none.

The problem with using this approach, @jordigilh @tchughesiv is that the API is not communicating what will be done. It's an error prune having .spec.persistence: {} meaning that we will pull from the platform.

On the other hand, having .spec.persistence: <data> clears communicate that we are overriding. But .spec.persistence: {} or not having this attribute defined is dubious IMO. One can interpret that it will pull from platform if the attribute is not there.

Actually there is the enabled field in the service that is set to true if the service is defined (not nil) by the operator. It's something we're doing already in that sense: define an empty service for DI and the operator will populate it with the default configuration, in this case enabled=true.

~~I'm fine using labels as well~~. Consistency is important at the end, so if labels/annotations is the path forward for these cases, then this should be the approach accross whenever facing these situations.

I'd look at this as:

No persistence configured: .spec.persistence: {}

Sets explicitly the empty value for persistence.

Use workflow's persistence configuration: .spec.persistence: <data>

Use the system/platform default: .spec.persistence: nil

By omitting this, the decision is propagated to platform/cluster

+1 for consistency (always).

if persistence is nil in the workflow, settings are used from platform CR.

Doesn't that break backwards compatibility? I mean, currently a workflow that doesn't have the persistence field populated is not expecting to have persistence if the platform contains one.

@ricardozanini here's one example https://kubernetes.io/docs/tasks/administer-cluster/manage-resources/cpu-default-namespace/

if persistence is nil in the workflow, settings are used from platform CR.

Doesn't that break backwards compatibility? I mean, currently a workflow that doesn't have the persistence field populated is not expecting to have persistence if the platform contains one.

what do you mean by currently ? isn't this PR introducing this capability and we get to define how it functions? how would this break backwards compatibility?

Be

if persistence is nil in the workflow, settings are used from platform CR.

Doesn't that break backwards compatibility? I mean, currently a workflow that doesn't have the persistence field populated is not expecting to have persistence if the platform contains one.

what do you mean by currently ? isn't this PR introducing this capability and we get to define how it functions? how would this break backwards compatibility?

Because if we define that when persistence is nil, which is the default value, then it should derive the persistence from the platform if it´s defined, then workflows that are currently running without persistence (the ones before merging this PR), will suddently now inherit persistence from the platform even though they don´t require it.

That´s what I mean by backwards compatibility:

Currently persistence=nil means no persistence, it matches the behavior that the workflows expect before this PR is merged

After this PR is merged, then persistence=nil means it might have persistence depending on the platform, whether the workflow requested it or not.

I think that if we have a way to discern if the workflow requires persistence or not, regardless of the value of the persistence struct, we will save ourselves all this trouble and just apply what needs to be applied.

After this PR is merged, then persistence=nil means it might have persistence depending on the platform, whether the workflow requested it or not.

but it won't, because existing platform CRs won't have the spec.persistence field set. this PR is introducing it as a new field. hence, existing workflows won't inherit persistence unless the user decides to enable it either within the workflow or the existing platform. we should be good to go, no backwards compat issues.

wmedvede · 2024-02-02T11:13:17Z

controllers/profiles/common/object_creators.go

+		return nil, fmt.Errorf("no persistence specification available in the workflow or in the platform")
+	}
+	if config.PostgreSQL != nil {
+		p = config.PostgreSQL


If the workflow persistence was set, we need to give the chance to see if a schema was provided, and if it's provided, below we must use that schema instead of the defaultSchema that we receive as parameter.
In this way we give users the chance to fine tune the schema name for a particular workflow.

wmedvede · 2024-02-02T11:16:34Z

controllers/profiles/common/object_creators.go

+	if config.PostgreSQL != nil {
+		p = config.PostgreSQL
+	} else if persistence.WorkflowConfig.GetPostgreSQLConfiguration() != nil {
+		p = persistence.WorkflowConfig.GetPostgreSQLConfiguration()


if we pick the platform configuration, yes, we must use the defaultSchema as it comes, basically the workflow name.

api/v1alpha08/sonataflowplatform_types.go

api/v1alpha08/sonataflowplatform_services_types.go

config/manager/kustomization.yaml

api/v1alpha08/sonataflowplatform_types.go

controllers/platform/platformutils.go

controllers/platform/services/properties_services_test.go

controllers/platform/services/services.go

controllers/profiles/common/properties/application_test.go

controllers/platform/services/services.go

tchughesiv

lgtm

ricardozanini · 2024-02-06T17:33:47Z

controllers/profiles/common/object_creators.go

@@ -157,7 +159,13 @@ func defaultContainer(workflow *operatorapi.SonataFlow) (*corev1.Container, erro
 	if err := mergo.Merge(defaultFlowContainer, workflow.Spec.PodTemplate.Container.ToContainer(), mergo.WithOverride); err != nil {
 		return nil, err
 	}
-	defaultFlowContainer = ConfigurePersistence(defaultFlowContainer, workflow.Spec.Persistence, defaultSchemaName, workflow.Namespace)
+	if workflow.Spec.Persistence != nil {


It makes sense. But what about adding a label to address this problem sonataflow.org/persistence=platform|workflow|none, defaults to none.

The problem with using this approach, @jordigilh @tchughesiv is that the API is not communicating what will be done. It's an error prune having .spec.persistence: {} meaning that we will pull from the platform.

On the other hand, having .spec.persistence: <data> clears communicate that we are overriding. But .spec.persistence: {} or not having this attribute defined is dubious IMO. One can interpret that it will pull from platform if the attribute is not there.

controllers/profiles/dev/states_dev.go

test/testdata/workflow/persistence/by_service/02-sonataflow_platform.yaml

…nataflow CRDs Signed-off-by: Jordi Gil <[email protected]>

…sistence configuration when they don't provide one. Services will always default to use persistence when available either within their spec or defined in the platform's. Workflows, on the other hand, can use the platform's persistence configuration by defining an empty persistence struct in their CR. Otherwise they can populate the structure with the service references Signed-off-by: Jordi Gil <[email protected]>

…wn provided configuration or one derived from the platform Signed-off-by: Jordi Gil <[email protected]>

…void requiting to provide it when the service is co-located to the sonataflow CR Signed-off-by: Jordi Gil <[email protected]>

…e persistence configuration Signed-off-by: Jordi Gil <[email protected]>

Signed-off-by: Jordi Gil <[email protected]>

…dPropsAndSpecAndGeneric fails Signed-off-by: Jordi Gil <[email protected]>

Signed-off-by: Jordi Gil <[email protected]>

…tests Signed-off-by: Jordi Gil <[email protected]>

Signed-off-by: Jordi Gil <[email protected]>

…URL string and dataSchema in the configuration to be inherited by the workflows. Signed-off-by: Jordi Gil <[email protected]>

Signed-off-by: Jordi Gil <[email protected]>

…ect persistence configuration when platform provides one Signed-off-by: Jordi Gil <[email protected]>

… came in by accident Signed-off-by: Jordi Gil <[email protected]>

… before processing the postgreSQL configuration Signed-off-by: Jordi Gil <[email protected]>

Signed-off-by: Jordi Gil <[email protected]>

… in workflow so that when deriving the persistence from the platform, to generate the schema using the workflow's name Signed-off-by: Jordi Gil <[email protected]>

… operator to derive the persistence from the platform if available, and persistence = {} to instruct the operator to discard persistence in the workflow, even when the platform has one Signed-off-by: Jordi Gil <[email protected]>

…n RetrievePersistence()

ricardozanini · 2024-02-28T11:59:31Z

@jordigilh can you check the code gen?

Signed-off-by: Jordi Gil <[email protected]>

jordigilh requested a review from ricardozanini as a code owner January 26, 2024 16:24

ricardozanini requested review from wmedvede and domhanak January 26, 2024 16:38

jordigilh force-pushed the kogito_9980_2_add_persistence_to_platform_crd branch 5 times, most recently from 1ed9a32 to 809bc9b Compare January 29, 2024 14:57

ricardozanini reviewed Jan 29, 2024

View reviewed changes

jordigilh force-pushed the kogito_9980_2_add_persistence_to_platform_crd branch 2 times, most recently from de33edd to 7b43534 Compare January 29, 2024 19:58

tchughesiv suggested changes Jan 30, 2024

View reviewed changes

api/v1alpha08/sonataflowplatform_types.go Outdated Show resolved Hide resolved

wmedvede reviewed Feb 1, 2024

View reviewed changes

jordigilh force-pushed the kogito_9980_2_add_persistence_to_platform_crd branch 2 times, most recently from f0585cf to 22640ef Compare February 1, 2024 13:20

ricardozanini reviewed Feb 1, 2024

View reviewed changes

controllers/platform/services/services.go Outdated Show resolved Hide resolved

wmedvede reviewed Feb 2, 2024

View reviewed changes

jordigilh force-pushed the kogito_9980_2_add_persistence_to_platform_crd branch from 22640ef to e407a8d Compare February 5, 2024 15:18

tchughesiv suggested changes Feb 5, 2024

View reviewed changes

api/v1alpha08/sonataflowplatform_types.go Outdated Show resolved Hide resolved

api/v1alpha08/sonataflowplatform_services_types.go Show resolved Hide resolved

tchughesiv suggested changes Feb 5, 2024

View reviewed changes

jordigilh force-pushed the kogito_9980_2_add_persistence_to_platform_crd branch from e407a8d to 071796c Compare February 5, 2024 15:38

tchughesiv suggested changes Feb 5, 2024

View reviewed changes

jordigilh force-pushed the kogito_9980_2_add_persistence_to_platform_crd branch from f61ee98 to 95b390b Compare February 5, 2024 15:59

tchughesiv reviewed Feb 5, 2024

View reviewed changes

controllers/platform/services/services.go Show resolved Hide resolved

controllers/platform/services/services.go Show resolved Hide resolved

tchughesiv reviewed Feb 5, 2024

View reviewed changes

controllers/platform/services/services.go Outdated Show resolved Hide resolved

controllers/platform/services/services.go Outdated Show resolved Hide resolved

tchughesiv approved these changes Feb 6, 2024

View reviewed changes

ricardozanini reviewed Feb 6, 2024

View reviewed changes

tchughesiv mentioned this pull request Feb 6, 2024

Enhance SonataFlowClusterPlatform.spec so it better communicates which settings are used by workflows cluster-wide #380

Closed

jordigilh added 20 commits February 27, 2024 09:08

[KOGITO-9980] Add persistence structures in sonataflowplatform and so…

4fbbb92

…nataflow CRDs Signed-off-by: Jordi Gil <[email protected]>

Added e2e tests for both platform and workflows leveraging on their o…

55b9cc6

…wn provided configuration or one derived from the platform Signed-off-by: Jordi Gil <[email protected]>

Change namespace in persistence service reference to be optional to a…

4757a09

…void requiting to provide it when the service is co-located to the sonataflow CR Signed-off-by: Jordi Gil <[email protected]>

Extend e2e tests to cover for workflow CR using platform CR to provid…

705846b

…e persistence configuration Signed-off-by: Jordi Gil <[email protected]>

Move image references in test to use ubi9/ubi-micro:latest

11d3123

Signed-off-by: Jordi Gil <[email protected]>

Add descriptive message when unit test Test_Handler_WorkflowMinimalAn…

e49a12b

…dPropsAndSpecAndGeneric fails Signed-off-by: Jordi Gil <[email protected]>

Fix flaky test Test_Handler_WorkflowMinimalAndPropsAndSpecAndGeneric

f7d9b88

Signed-off-by: Jordi Gil <[email protected]>

Collect cluster events and list all namespace/pods after running e2e …

7eba3f2

…tests Signed-off-by: Jordi Gil <[email protected]>

Changes based on latest feedback from Ricardo

5655e61

Signed-off-by: Jordi Gil <[email protected]>

Remove duplicated tests

69d0793

Signed-off-by: Jordi Gil <[email protected]>

Aligned Persistence with Services, including adding support for JDBC …

71799ff

…URL string and dataSchema in the configuration to be inherited by the workflows. Signed-off-by: Jordi Gil <[email protected]>

refactor platform CR retrieval

d235ca1

Signed-off-by: Jordi Gil <[email protected]>

Remove focus test function

3a4a5cc

Signed-off-by: Jordi Gil <[email protected]>

Refactor logic to retrieve platform CR when deploying workflow to inj…

785ad19

…ect persistence configuration when platform provides one Signed-off-by: Jordi Gil <[email protected]>

Updates based on latest changes and revert some unwanted changes that…

77a9134

… came in by accident Signed-off-by: Jordi Gil <[email protected]>

Add additional check for nil values in platform spec and service spec…

f98eb50

… before processing the postgreSQL configuration Signed-off-by: Jordi Gil <[email protected]>

Revert unnecessary change in variable name

e69129c

Signed-off-by: Jordi Gil <[email protected]>

Change platform persistence to remove database schema and update code…

216f826

… in workflow so that when deriving the persistence from the platform, to generate the schema using the workflow's name Signed-off-by: Jordi Gil <[email protected]>

jordigilh force-pushed the kogito_9980_2_add_persistence_to_platform_crd branch from 5c4d907 to 8281d2c Compare February 27, 2024 14:09

Refactored ConfigurePersistence() in JobServiceHandler to use functio…

214e0b2

…n RetrievePersistence()

jordigilh force-pushed the kogito_9980_2_add_persistence_to_platform_crd branch 3 times, most recently from 257bc03 to 4ff3d70 Compare February 28, 2024 03:30

Fix e2e tests and add 'time.Minute' timeout to waitFor condition

9865cca

Signed-off-by: Jordi Gil <[email protected]>

jordigilh force-pushed the kogito_9980_2_add_persistence_to_platform_crd branch from 4ff3d70 to 9865cca Compare February 28, 2024 13:30

ricardozanini merged commit fff6361 into apache:main Feb 28, 2024
4 checks passed

rgdoliveira pushed a commit to rgdoliveira/kogito-serverless-operator that referenced this pull request Mar 11, 2024

[KOGITO-9980] Add persistence to SonataflowPlatform CRD (apache#372)

6d822b4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KOGITO-9980] Add persistence to SonataflowPlatform CRD #372

[KOGITO-9980] Add persistence to SonataflowPlatform CRD #372

jordigilh commented Jan 26, 2024 •

edited

Loading

wmedvede left a comment

ricardozanini commented Feb 1, 2024

wmedvede commented Feb 2, 2024

wmedvede left a comment

wmedvede Feb 2, 2024

jordigilh Feb 5, 2024

ricardozanini Feb 6, 2024

jordigilh Feb 6, 2024 •

edited

Loading

masayag Feb 7, 2024

jordigilh Feb 9, 2024

tchughesiv Feb 9, 2024

tchughesiv Feb 9, 2024

jordigilh Feb 9, 2024

tchughesiv Feb 9, 2024 •

edited

Loading

wmedvede Feb 2, 2024

wmedvede Feb 2, 2024

tchughesiv left a comment

ricardozanini Feb 6, 2024

ricardozanini commented Feb 28, 2024

[KOGITO-9980] Add persistence to SonataflowPlatform CRD #372

[KOGITO-9980] Add persistence to SonataflowPlatform CRD #372

Conversation

jordigilh commented Jan 26, 2024 • edited Loading

wmedvede left a comment

Choose a reason for hiding this comment

ricardozanini commented Feb 1, 2024

wmedvede commented Feb 2, 2024

wmedvede 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

jordigilh Feb 6, 2024 • edited Loading

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

tchughesiv Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchughesiv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardozanini commented Feb 28, 2024

jordigilh commented Jan 26, 2024 •

edited

Loading

jordigilh Feb 6, 2024 •

edited

Loading

tchughesiv Feb 9, 2024 •

edited

Loading