[KOGITO-9982] Add Persistence field in SonataFlow CRD #322

jordigilh · 2023-12-12T18:57:40Z

Adds the Persistence field in the SonataFlow CRD
Adds the implementation to handle PostgreSQL persistence type
If Persistence field is empty or nil, no persistence is defined.

@tchughesiv @wmedvede @ricardozanini PTAL.

controllers/profiles/common/persistence.go

api/v1alpha08/sonataflow_types.go

jakubschwan

Changes looks good to me. Please also check and resolve failing PR checks.

domhanak · 2024-01-15T08:00:05Z

@jordigilh another rebase should solve the test issues.

domhanak · 2024-01-16T12:23:05Z

Looks like the added test case is not working here on CI:
Needs to be stable to merge, I am trying to reproduce the test failure locally.

Validate the persistence  when deploying a SonataFlow CR with PostgreSQL persistence [It] defined in the workflow from an existing kubernetes service as a reference
/home/runner/work/incubator-kie-kogito-serverless-operator/incubator-kie-kogito-serverless-operator/test/e2e/persistence_test.go:123

  [FAILED] Timed out after 600.001s.
  Expected success, but got an error:
      <*errors.errorString | 0xc00007a2a0>: 
      kubectl wait pod -n test-546 -l sonataflow.org/workflow-app=callbackstatetimeouts --for condition=Ready --timeout=5s failed with error: (exit status 1) error: timed out waiting for the condition on pods/callbackstatetimeouts-7bf57f9968-5nfsl
      
      {
          s: "kubectl wait pod -n test-546 -l sonataflow.org/workflow-app=callbackstatetimeouts --for condition=Ready --timeout=5s failed with error: (exit status 1) error: timed out waiting for the condition on pods/callbackstatetimeouts-7bf57f9968-5nfsl\n",
      }

jordigilh · 2024-01-18T15:06:12Z

Looks like the added test case is not working here on CI: Needs to be stable to merge, I am trying to reproduce the test failure locally.

Validate the persistence  when deploying a SonataFlow CR with PostgreSQL persistence [It] defined in the workflow from an existing kubernetes service as a reference
/home/runner/work/incubator-kie-kogito-serverless-operator/incubator-kie-kogito-serverless-operator/test/e2e/persistence_test.go:123

  [FAILED] Timed out after 600.001s.
  Expected success, but got an error:
      <*errors.errorString | 0xc00007a2a0>: 
      kubectl wait pod -n test-546 -l sonataflow.org/workflow-app=callbackstatetimeouts --for condition=Ready --timeout=5s failed with error: (exit status 1) error: timed out waiting for the condition on pods/callbackstatetimeouts-7bf57f9968-5nfsl
      
      {
          s: "kubectl wait pod -n test-546 -l sonataflow.org/workflow-app=callbackstatetimeouts --for condition=Ready --timeout=5s failed with error: (exit status 1) error: timed out waiting for the condition on pods/callbackstatetimeouts-7bf57f9968-5nfsl\n",
      }

Fixed 😄

jordigilh · 2024-01-19T00:59:57Z

@ricardozanini ready to be merged. Please note that I had to remove the DEBUG=true env value in the job because it caused the kubectl commands to dump extra log entries and that caused errors when parsing the output as it did not match the expected output.

ricardozanini · 2024-01-19T01:25:10Z

@jordigilh many thanks! we Will wait for QE verification first with @domhanak

ricardozanini · 2024-01-19T11:20:48Z

Please note that I had to remove the DEBUG=true env value in the job because it caused the kubectl commands to dump extra log entries and that caused errors when parsing the output as it did not match the expected output.

Thanks! I've removed this env var too in an opened PR! 😄

ricardozanini

I just left some minor comments if you don't mind. Otherwise looks good! Many thanks!

controllers/platform/services/services.go

ricardozanini · 2024-01-19T11:24:30Z

controllers/profiles/common/object_creators_test.go

+}
+
+var (
+	postgreSQLPort = 5432


nit; are you using this var outside the TestMergePodSpec_OverrideContainers_WithPostgreSQL_and_ServiceRef test scope?

I'd keep it within the test scope then.

controllers/profiles/common/persistence/postgreSQL.go

test/e2e/persistence_test.go

test/e2e/workflow_test.go

ricardozanini · 2024-01-19T11:27:51Z

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

+    template:
+      buildArgs:
+      - name: QUARKUS_EXTENSIONS
+        value: org.kie.kogito:kogito-addons-quarkus-jobs-knative-eventing:999-SNAPSHOT,org.kie.kogito:kogito-addons-quarkus-persistence-jdbc:999-SNAPSHOT,io.quarkus:quarkus-jdbc-postgresql:3.2.9.Final,io.quarkus:quarkus-agroal:3.2.9.Final


@wmedvede I think it's worth adding those to the images, so we keep them in the cache to facilitate persistence use cases. wdyt?

@jordigilh @ricardozanini @tchughesiv @masayag
I think we need to assess here, because the persistence is a sort of all or nothing.

I mean, if we add the org.kie.kogito:kogito-addons-quarkus-persistence-jdbc:999-SNAPSHOT to the kogito-swf-builder, then, every workflow image we produce must use persistence, otherwise we'll have runtime start-up failures.
Additionally, we have the following scenarios:

1) The images is produced externally:
The workflow must come already built with the proper extensions incorporated in the produced image, and users might decide in a per-workflow basis to include the persistence related addons.
The operator just relies on the workflow CRD con connect with the DB, and just produce the ENV vars as Jordi is doing.
If the image is not ok, it's not the operator problem. Which make sense in my opinion.

(right now, AFAIK Parodos team is using the kogito-swf-builder series, however they might eventually produce the images by adding the persistence related extensions in a per-workflow basis,
or always add them if they always want persistence)

2) The Image is produced by the operator, and thus, managed by Kaniko or OpenShift based builds, etc.
In these cases, we could eventually manage to add the persistence extensions somehow dynamically, since in the end, for example with the Kaniko build we are producing a sonataflow-my-workflow-builder CM and here we can maybe manage to add these extensions.

But, since Parodos is working with 1), I think they should add those extensions in their images build system.

test/testdata/persistence/workflow/by_service/03-sonataflow_callbackstatetimeouts.sw.yaml

ricardozanini · 2024-01-19T11:42:54Z

@tchughesiv can you please review it too?

tchughesiv

lgtm

wmedvede

LGTM.

I have added a general comment regarding the images generation that I think make sense, take a look please

test/testdata/persistence/workflow/by_service/03-sonataflow_callbackstatetimeouts.sw.yaml

wmedvede · 2024-01-23T08:18:51Z

Hi @jordigilh , it looks like we have some merge conflicts now, would you mind take a look please.

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

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

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

…r worfklows that end very quickly, like callbackstatetimeouts Signed-off-by: Jordi Gil <[email protected]>

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

jordigilh · 2024-01-25T14:49:03Z

@ricardozanini PR is green. Can we merge it?

ricardozanini · 2024-01-25T15:24:42Z

Let's wait for @wmedvede approval. I'll also take a look on it tomorrow.

wmedvede · 2024-01-25T16:44:46Z

Let's wait for @wmedvede approval. I'll also take a look on it tomorrow.

@jordigilh @ricardozanini
This PR was already approved from my side guys.

ricardozanini · 2024-01-26T13:48:24Z

@jordigilh can you please open a task to update the docs too? Thank you!

* Added Persistence field in SonataFlow CRD Signed-off-by: Jordi Gil <[email protected]> * Added implementation to manage PostgreSQL persistence type in the SonataFlow CRD Signed-off-by: Jordi Gil <[email protected]> * Added end to end tests Signed-off-by: Jordi Gil <[email protected]> * Retry health endpoints Signed-off-by: Jordi Gil <[email protected]> * Changes based on Ricardo's feedback Signed-off-by: Jordi Gil <[email protected]> * Reorganized persistence e2e test and tweaked health check to cover for worfklows that end very quickly, like callbackstatetimeouts Signed-off-by: Jordi Gil <[email protected]> * Changes after merge conflicts with platform e2e tests Signed-off-by: Jordi Gil <[email protected]> * Added additional check for failing test in workflowproj Signed-off-by: Jordi Gil <[email protected]> --------- Signed-off-by: Jordi Gil <[email protected]>

jordigilh requested a review from ricardozanini as a code owner December 12, 2023 18:57

ricardozanini requested review from wmedvede and jakubschwan December 13, 2023 17:05

ricardozanini reviewed Dec 13, 2023

View reviewed changes

controllers/profiles/common/persistence.go Outdated Show resolved Hide resolved

masayag reviewed Dec 14, 2023

View reviewed changes

api/v1alpha08/sonataflow_types.go Show resolved Hide resolved

jordigilh force-pushed the kogito_9982_add_persistence_field_sonataflow_crd branch from a3b93f8 to 317038d Compare January 2, 2024 22:07

jakubschwan reviewed Jan 15, 2024

View reviewed changes

jordigilh force-pushed the kogito_9982_add_persistence_field_sonataflow_crd branch from 317038d to 4391cad Compare January 15, 2024 18:22

jordigilh force-pushed the kogito_9982_add_persistence_field_sonataflow_crd branch from 4391cad to ce93deb Compare January 18, 2024 15:03

jordigilh force-pushed the kogito_9982_add_persistence_field_sonataflow_crd branch 5 times, most recently from 5a721cd to 81e5814 Compare January 19, 2024 00:39

domhanak approved these changes Jan 19, 2024

View reviewed changes

jordigilh mentioned this pull request Jan 19, 2024

[KOGITO-9940] Add E2E test cases for platform configured with Job Service and Data Index in a combination of scenarios with ephemeral and postgreSQL persistence in dev and production profiles #337

Merged

ricardozanini reviewed Jan 19, 2024

View reviewed changes

tchughesiv approved these changes Jan 19, 2024

View reviewed changes

jordigilh force-pushed the kogito_9982_add_persistence_field_sonataflow_crd branch 2 times, most recently from 1d717ba to 0af9418 Compare January 19, 2024 19:36

wmedvede approved these changes Jan 22, 2024

View reviewed changes

test/testdata/persistence/workflow/by_service/03-sonataflow_callbackstatetimeouts.sw.yaml Show resolved Hide resolved

jordigilh force-pushed the kogito_9982_add_persistence_field_sonataflow_crd branch from e7d5100 to 76116ce Compare January 22, 2024 19:05

jordigilh force-pushed the kogito_9982_add_persistence_field_sonataflow_crd branch 5 times, most recently from 811f961 to 1086910 Compare January 24, 2024 23:39

jordigilh added 6 commits January 24, 2024 19:40

Added Persistence field in SonataFlow CRD

6750d79

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

Added implementation to manage PostgreSQL persistence type in the Son…

7dc1b8a

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

Added end to end tests

6e50c7f

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

Retry health endpoints

a9327b7

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

Changes based on Ricardo's feedback

210bd55

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

Reorganized persistence e2e test and tweaked health check to cover fo…

617e2b9

…r worfklows that end very quickly, like callbackstatetimeouts Signed-off-by: Jordi Gil <[email protected]>

jordigilh force-pushed the kogito_9982_add_persistence_field_sonataflow_crd branch from 1086910 to fa42ecd Compare January 25, 2024 13:58

jordigilh added 2 commits January 25, 2024 09:14

Changes after merge conflicts with platform e2e tests

1383333

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

Added additional check for failing test in workflowproj

89ef882

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

jordigilh force-pushed the kogito_9982_add_persistence_field_sonataflow_crd branch from fa42ecd to 89ef882 Compare January 25, 2024 14:23

ricardozanini merged commit 2b5056d into apache:main Jan 26, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KOGITO-9982] Add Persistence field in SonataFlow CRD #322

[KOGITO-9982] Add Persistence field in SonataFlow CRD #322

jordigilh commented Dec 12, 2023

jakubschwan left a comment

domhanak commented Jan 15, 2024 •

edited

Loading

domhanak commented Jan 16, 2024

jordigilh commented Jan 18, 2024

jordigilh commented Jan 19, 2024

ricardozanini commented Jan 19, 2024

ricardozanini commented Jan 19, 2024

ricardozanini left a comment

ricardozanini Jan 19, 2024

jordigilh Jan 22, 2024

ricardozanini Jan 22, 2024

ricardozanini Jan 19, 2024

wmedvede Jan 22, 2024

ricardozanini commented Jan 19, 2024

tchughesiv left a comment

wmedvede left a comment

wmedvede commented Jan 23, 2024

jordigilh commented Jan 25, 2024

ricardozanini commented Jan 25, 2024

wmedvede commented Jan 25, 2024

ricardozanini commented Jan 26, 2024

[KOGITO-9982] Add Persistence field in SonataFlow CRD #322

[KOGITO-9982] Add Persistence field in SonataFlow CRD #322

Conversation

jordigilh commented Dec 12, 2023

jakubschwan left a comment

Choose a reason for hiding this comment

domhanak commented Jan 15, 2024 • edited Loading

domhanak commented Jan 16, 2024

jordigilh commented Jan 18, 2024

jordigilh commented Jan 19, 2024

ricardozanini commented Jan 19, 2024

ricardozanini commented Jan 19, 2024

ricardozanini left a comment

Choose a reason for hiding this comment

ricardozanini Jan 19, 2024

Choose a reason for hiding this comment

jordigilh Jan 22, 2024

Choose a reason for hiding this comment

ricardozanini Jan 22, 2024

Choose a reason for hiding this comment

ricardozanini Jan 19, 2024

Choose a reason for hiding this comment

wmedvede Jan 22, 2024

Choose a reason for hiding this comment

ricardozanini commented Jan 19, 2024

tchughesiv left a comment

Choose a reason for hiding this comment

wmedvede left a comment

Choose a reason for hiding this comment

wmedvede commented Jan 23, 2024

jordigilh commented Jan 25, 2024

ricardozanini commented Jan 25, 2024

wmedvede commented Jan 25, 2024

ricardozanini commented Jan 26, 2024

domhanak commented Jan 15, 2024 •

edited

Loading