[KOGITO-9940] Add Job Service to SonataFlowPlatform CRD #311

jordigilh · 2023-11-28T18:43:09Z

Adds a new field to the ServiceSpec in the SonataflowPlatform CRD for a Job Service, reusing the same type as the DataIndex field.
Implements the reconciliation logic for the Job Service.
Adds 2 new unit tests specific for the Job Service and another one for the use case where both DataIndex and Job Service fields are populated with an empty struct.
Refactor platform services to split from profiles and move it inside platform package

@tchughesiv @ricardozanini PTAL.

ricardozanini

Nice work! Many thanks! I just left a few small comments.

controllers/profiles/common/app_properties.go

ricardozanini

Great work! ❤️

ricardozanini · 2023-11-28T20:13:55Z

Can you run:

make generate-all
make vet fmt

Should pass the CI.

tchughesiv · 2023-11-28T20:28:57Z

lgtm

tchughesiv

missed this earlier

controllers/profiles/common/app_properties_test.go

controllers/profiles/common/app_properties.go

api/v1alpha08/sonataflowplatform_services_types.go

wmedvede

I've added some comments guys at the time we continue working in parallel to make the service work by using the PR sources.

good job in general!

wmedvede · 2023-11-30T11:41:53Z

controllers/profiles/common/app_properties.go

+
+// GetServiceImageName returns the image name of the service's container. It takes in the service and persistence types and returns a string
+// that contains the FQDN of the image, including the tag.
+func GetServiceImageName(persistenceName string, stype ServiceType) string {


Let me add guys a general comment regarding the app_properties.go. I think that here, we originally started to manage the application.properties related to the workflows, but the application.properties related to the services are a different stuff. Now I think this is starting to become a sort of all in one.... Not sure it's good.

now we have things like this:

incubator-kie-kogito-serverless-operator/controllers/profiles/common/app_properties.go

Line 256 in 9a1e05d

handler := &appPropertyHandler{

with a common.NewServiceAppPropertyHandler that uses the original handler := &appPropertyHandler{
platform: platform,
isService: true,
}

and that we use when we need to create the applicaton.properties for a service, in places like this:

incubator-kie-kogito-serverless-operator/controllers/platform/services.go

Line 404 in b017c4b

workflowproj.ApplicationPropertiesFileName: common.NewServiceAppPropertyHandler(platform).Build(),

IDK, just thinking aloud, but maybe this deserves a piece of refactoring.

wmedvede · 2023-11-30T12:00:56Z

controllers/platform/services.go

@@ -58,30 +60,72 @@ func (action *serviceAction) Handle(ctx context.Context, platform *operatorapi.S
 		return nil, err
 	}

-	if err := createDataIndexComponents(ctx, action.client, platform); err != nil {
-		return nil, err
+	if platform.Spec.Services.DataIndex != nil {


I have question here, maybe I'm missing something or not understanding well, but my understanding is that when we execute this Handle, is because the platform.Status.IsReady().

So, when the platform is ready, we start to create the services. All good, but on the other hand, if the platform is ready, the workflows might have been started the creation procedure too.
if worklfows and services re being created at the same time, what happens if a workflow needs a service?

does it make sense?

return platform.Status.IsReady()

agreed we should enhance the SonataFlowPlatform's status to account for service deployments.

KOGITO-9972 will require changes to SonataFlowPlatform.status and, i expect, will be a good time to make some enhancements.

controllers/profiles/common/app_properties.go

controllers/platform/services.go

ricardozanini

Thank you for the refactoring, but can you kindly consider my comments? Many thanks!

controllers/platform/services.go

controllers/profiles/common/app_properties.go

jordigilh · 2023-12-05T15:08:06Z

@masayag PTAL

masayag · 2023-12-05T17:02:08Z

controllers/platform/services.go

@@ -58,30 +55,32 @@ func (action *serviceAction) Handle(ctx context.Context, platform *operatorapi.S
 		return nil, err
 	}

-	if err := createDataIndexComponents(ctx, action.client, platform); err != nil {
-		return nil, err
+	if platform.Spec.Services.DataIndex != nil {


Should we get into this block if platform.Spec.Services.DataIndex.Enabled == false?

it's correct as it is. the Enabled field does not control whether a service is deployed. instead, it controls whether its used by the "prod" workflows -

incubator-kie-kogito-serverless-operator/api/v1alpha08/sonataflowplatform_services_types.go

Lines 27 to 29 in a96add5

// Determines whether "prod" profile workflows should be configured to use this service

// +optional

Enabled *bool `json:"enabled,omitempty"`

Thank you for the reference.
In which case should this be set to false, assuming the workflow's profile is set to prod?

no, this flag only affects "prod" workflows. if it's a dev workflow, this flag won't even be checked by the operator. when a user elects to set platform.spec.services.dataIndex.enabled to true (which is the default), they are saying they want "prod" workflows to use this data index.

My question is when would platform.spec.services.dataIndex.enabled be set to false?
Which business use case is served by disabling the data-index (or job-service) for prod workflows?

I also think this all sounds reasonable, with RollingUpdate as the default when this new "flag" is not set. @ricardozanini I don't think the cluster-scoped changes would need to wait for this though... what is your thinking with that?

Won't we replicate the spec in the cluster scope? I'd rather have the change once and propagate to the cluster-scoped requirements.

no, we will not replicate the spec in cluster-scope. we previously discussed this and decided to only reference the chosen platform in spec and leave it at that. the referenced SonataFlowPlatform will be the single source of truth.

Fair, I remembered now sorry for the noise. @masayag can you open an issue here to implement this?

controllers/platform/services.go

ricardozanini

@wmedvede can you please take a second look too?

controllers/platform/services/properties.go

tchughesiv · 2023-12-08T13:51:03Z

@jordigilh does your PR descrip still match the changes you're making here? if not, could you update it? thanks!

jordigilh · 2023-12-08T14:01:28Z

@jordigilh does your PR descrip still match the changes you're making here? if not, could you update it? thanks!

Updated.

controllers/platform/services/properties.go

wmedvede

LGTM, I've added some comments, thanks!

wmedvede · 2023-12-12T16:38:12Z

controllers/platform/services/properties.go

+// service's spec field `Enabled` set to true
+func GenerateJobServiceApplicationProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) *properties.Properties {
+	props := properties.NewProperties()
+	if profiles.IsProdProfile(workflow) && jobServiceEnabled(platform) {


maybe this condition explains what we chatted, note that if no workflow is present and we create a platform with the jobs-service it's created.

@jordigilh @ricardozanini In my opinion to instanciate the services even with no workflows is valid.

btw, @jordigilh @ricardozanini the comment above regarding the IsProdProfile check apply also for the data-index

Yes, that should work independently.

Removed references to the workflow in both data index and job service application properties generation.

@jordigilh can you explain what was changed here and why?

@tchughesiv This change is because in order for the workflows to work, (when configured) we need the jobs-service and data-index to be deployed first, but also, it might occur, that we push the platform into the namespace, and then, we push the workflows. So, what we are looking for with this change is that the deployment of the services are not dependent on any particular workflow.
That's basically the aim of this change.

controllers/platform/services/services.go

controllers/profiles/common/constants/platform_services.go

controllers/profiles/common/properties/application.go

controllers/platform/services/services.go

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

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

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

…r jobs-service Signed-off-by: Jordi Gil <[email protected]>

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

…nts and avoid duplications between packages Signed-off-by: Jordi Gil <[email protected]>

…tionProperties and GenerateDataIndexApplicationProperties Signed-off-by: Jordi Gil <[email protected]>

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

This reverts commit 2b3638a.

…generate the application service properties. Reactive URL for job service won't include a default schema if it's not provided in the JDBC URL for PostgreSQL Hardcoded Job Service replica count to 1. Signed-off-by: Jordi Gil <[email protected]>

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

…gresql port value Signed-off-by: Jordi Gil <[email protected]>

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

…cover the two cases of deploying the service with its unique properties and the ones generated specific for workflow deployments Signed-off-by: Jordi Gil <[email protected]>

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

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

wmedvede

After the final local tests on openshift I can see the following scenarios working

1) jobs-service and data-index deployed with postgresql + a later workflow deployment
Services are started well and gets positive health checks, and has the expected configurations. Similar results for the workflow.

2) jobs-service postgresql + a later workflow deployment
Job service is started well and gets positive health checks, and has the expected configurations. Similar results for the workflow.

3) data-index deployed with postgresql + a later workflow deployment
Data index service is started well getting positive health checks and, and has expected configurations, similar for the workflows.

I conclude that we can merge, other fine tunnings or missing tests can be done in a follow-up PR.

minkube deployment checks where was done by @jordigilh

Good job!

@ricardozanini let's merge!

tchughesiv

lgtm

* [KOGITO-9940] Added job service field in platform CRD Signed-off-by: Jordi Gil <[email protected]> * Add reconciliation logic for Job Service Signed-off-by: Jordi Gil <[email protected]> * Added ServiceType enum type and changed the image tag based on operator version Signed-off-by: Jordi Gil <[email protected]> * Make generate-all * Add application.properties entry for jobService URL Property Signed-off-by: Jordi Gil <[email protected]> * Renaming serviceSpec field 'job' to 'jobService' Signed-off-by: Jordi Gil <[email protected]> * Add missing job service properties Signed-off-by: Jordi Gil <[email protected]> * Add reactive property when postgreSQL is configured as persistence for jobs-service Signed-off-by: Jordi Gil <[email protected]> * Refactor to use interface Signed-off-by: Jordi Gil <[email protected]> * Refactoring services to extract from profiles/common Signed-off-by: Jordi Gil <[email protected]> * Moved platform services and reconciliation constants to common/constants and avoid duplications between packages Signed-off-by: Jordi Gil <[email protected]> * Updated golang doc function description for GenerateJobServiceApplicationProperties and GenerateDataIndexApplicationProperties Signed-off-by: Jordi Gil <[email protected]> * Updated ginkgo to v2.13.0 and gomega to v1.30.0 Signed-off-by: Jordi Gil <[email protected]> * Added unit tests for data index and job service application properties * Changes based on feedback from Walter Signed-off-by: Jordi Gil <[email protected]> * Revert "Changes based on feedback from Walter" This reverts commit 2b3638a. * Check for not dev profile instead of production one for condition to generate the application service properties. Reactive URL for job service won't include a default schema if it's not provided in the JDBC URL for PostgreSQL Hardcoded Job Service replica count to 1. Signed-off-by: Jordi Gil <[email protected]> * Include the platform service application properties in each service's configmap Signed-off-by: Jordi Gil <[email protected]> * Fix sonataflowplatform_controller_test.go failing due to invalid postgresql port value Signed-off-by: Jordi Gil <[email protected]> * Fix tests due to changes in schema and additional properties for the job service Signed-off-by: Jordi Gil <[email protected]> * Extended logic in Job Service for application property generation to cover the two cases of deploying the service with its unique properties and the ones generated specific for workflow deployments Signed-off-by: Jordi Gil <[email protected]> * Updated generated properties based on feedback Signed-off-by: Jordi Gil <[email protected]> * Refactored unit tests for properties and rebased to fix e2e test Signed-off-by: Jordi Gil <[email protected]> * Add JS generated property for deployment and cleaning chores on property tests Signed-off-by: Jordi Gil <[email protected]> --------- Signed-off-by: Jordi Gil <[email protected]>

jordigilh requested a review from ricardozanini as a code owner November 28, 2023 18:43

jordigilh changed the title ~~Kogito 9940 add job service sonataflowplatform~~ [KOGITO-9940] Add Job Service to SonataFlowPlatform CRD Nov 28, 2023

jordigilh force-pushed the kogito_9940_add_job_service_sonataflowplatform branch from 8dac349 to cba7063 Compare November 28, 2023 18:46

ricardozanini requested a review from wmedvede November 28, 2023 18:50

ricardozanini reviewed Nov 28, 2023

View reviewed changes

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

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

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

ricardozanini approved these changes Nov 28, 2023

View reviewed changes

tchughesiv suggested changes Nov 29, 2023

View reviewed changes

tchughesiv reviewed Nov 29, 2023

View reviewed changes

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

tchughesiv suggested changes Nov 29, 2023

View reviewed changes

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

tchughesiv suggested changes Nov 29, 2023

View reviewed changes

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

wmedvede reviewed Nov 30, 2023

View reviewed changes

jordigilh force-pushed the kogito_9940_add_job_service_sonataflowplatform branch from 0688e86 to fdd9ef4 Compare December 5, 2023 00:24

ricardozanini requested changes Dec 5, 2023

View reviewed changes

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

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

controllers/profiles/common/app_properties.go Show resolved Hide resolved

masayag reviewed Dec 5, 2023

View reviewed changes

controllers/platform/services.go Show resolved Hide resolved

masayag reviewed Dec 5, 2023

View reviewed changes

controllers/platform/services.go Show resolved Hide resolved

jordigilh force-pushed the kogito_9940_add_job_service_sonataflowplatform branch 3 times, most recently from e1bd42c to 320ab56 Compare December 7, 2023 14:50

ricardozanini reviewed Dec 7, 2023

View reviewed changes

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

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

jordigilh force-pushed the kogito_9940_add_job_service_sonataflowplatform branch from 439c96b to 1e0f2f5 Compare December 7, 2023 18:45

masayag mentioned this pull request Dec 11, 2023

Define workflows behavior when a SonataflowPlatform CR is changed. #321

Open

wmedvede reviewed Dec 12, 2023

View reviewed changes

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

wmedvede reviewed Dec 12, 2023

View reviewed changes

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

jordigilh added 22 commits December 19, 2023 15:26

Add reconciliation logic for Job Service

8e08bc1

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

Added ServiceType enum type and changed the image tag based on operat…

8b89b6d

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

Make generate-all

6a8ddba

Add application.properties entry for jobService URL Property

e0e6526

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

Renaming serviceSpec field 'job' to 'jobService'

e930e77

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

Add missing job service properties

c14e19b

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

Add reactive property when postgreSQL is configured as persistence fo…

efc47d2

…r jobs-service Signed-off-by: Jordi Gil <[email protected]>

Refactor to use interface

b945d3e

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

Refactoring services to extract from profiles/common

5b2a610

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

Moved platform services and reconciliation constants to common/consta…

381fc5a

…nts and avoid duplications between packages Signed-off-by: Jordi Gil <[email protected]>

Updated golang doc function description for GenerateJobServiceApplica…

f3085d5

…tionProperties and GenerateDataIndexApplicationProperties Signed-off-by: Jordi Gil <[email protected]>

Updated ginkgo to v2.13.0 and gomega to v1.30.0

7f11c92

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

Added unit tests for data index and job service application properties

b287490

Changes based on feedback from Walter

b5fccdf

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

Revert "Changes based on feedback from Walter"

66b8da8

This reverts commit 2b3638a.

Include the platform service application properties in each service's…

8900730

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

Fix sonataflowplatform_controller_test.go failing due to invalid post…

c77e89a

…gresql port value Signed-off-by: Jordi Gil <[email protected]>

Fix tests due to changes in schema and additional properties for the …

67b8b00

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

Extended logic in Job Service for application property generation to …

98352ec

…cover the two cases of deploying the service with its unique properties and the ones generated specific for workflow deployments Signed-off-by: Jordi Gil <[email protected]>

Updated generated properties based on feedback

23fd53f

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

Refactored unit tests for properties and rebased to fix e2e test

dc887d7

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

jordigilh force-pushed the kogito_9940_add_job_service_sonataflowplatform branch from eb9d750 to dc887d7 Compare December 20, 2023 02:43

Add JS generated property for deployment and cleaning chores on prope…

6d0ebcf

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

wmedvede approved these changes Dec 20, 2023

View reviewed changes

tchughesiv approved these changes Dec 20, 2023

View reviewed changes

ricardozanini approved these changes Dec 20, 2023

View reviewed changes

ricardozanini merged commit afc7684 into apache:main Dec 20, 2023
7 checks passed

	// Determines whether "prod" profile workflows should be configured to use this service
	// +optional
	Enabled *bool `json:"enabled,omitempty"`

[KOGITO-9940] Add Job Service to SonataFlowPlatform CRD #311

[KOGITO-9940] Add Job Service to SonataFlowPlatform CRD #311

Conversation

jordigilh commented Nov 28, 2023 • edited Loading

ricardozanini left a comment

Choose a reason for hiding this comment

ricardozanini left a comment

Choose a reason for hiding this comment

ricardozanini commented Nov 28, 2023

tchughesiv commented Nov 28, 2023

tchughesiv left a comment • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

ricardozanini left a comment

Choose a reason for hiding this comment

jordigilh commented Dec 5, 2023

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 Dec 11, 2023 • 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

ricardozanini left a comment

Choose a reason for hiding this comment

tchughesiv commented Dec 8, 2023

jordigilh commented Dec 8, 2023 • edited Loading

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

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

wmedvede left a comment

Choose a reason for hiding this comment

tchughesiv left a comment

Choose a reason for hiding this comment

jordigilh commented Nov 28, 2023 •

edited

Loading

tchughesiv left a comment •

edited

Loading

tchughesiv Dec 11, 2023 •

edited

Loading

jordigilh commented Dec 8, 2023 •

edited

Loading