wmedvede commented on code in PR #372:
URL:
https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/372#discussion_r1474095567
##########
controllers/platform/initialize.go:
##########
@@ -106,6 +107,13 @@ func (action *initializeAction) Handle(ctx
context.Context, platform *operatorap
}
platform.Status.Version = metadata.SpecVersion
+ // store workflow persistence configuration
+ if platform.Spec.Persistence != nil {
+ persistence.WorkflowConfig.SetConfig(platform.Spec.Persistence)
Review Comment:
I don't understand this. Are we storing globally the Persistence
configuration present in current platform?
And then, we use it in defaultContainer(workflow *operatorapi.SonataFlow)
creation when we create a workflow container?
But what happens if we have two different platforms with different
persistence settings?
##########
controllers/platform/services/services.go:
##########
@@ -138,11 +138,23 @@ func (d DataIndexHandler) MergePodSpec(podSpec
corev1.PodSpec) (corev1.PodSpec,
return *c, err
}
+// hasPostgreSQLConfigured returns true when either the SonataFlow Platform
PostgreSQL CR's structure or the one in the Data Index service specification is
not nil
+func (d DataIndexHandler) hasPostgreSQLConfigured() bool {
+ return (d.platform.Spec.Services.DataIndex.Persistence != nil &&
d.platform.Spec.Services.DataIndex.Persistence.PostgreSql != nil) ||
Review Comment:
silly comment here, that maybe we won't have any more if we move to use the
PersistenceOptions in all places.
But here we have.
d.platform.Spec.Services.DataIndex.Persistence.**PostgreSql** and
d.platform.Spec.Persistence.**PostgreSQL**
I think that we should try to unify the capitalization, at least for the
fields that are part of the API
##########
test/testdata/workflow/persistence/from_platform_overwritten_by_service/02-sonataflow_platform.yaml:
##########
@@ -0,0 +1,37 @@
+# Copyright 2024 Apache Software Foundation (ASF)
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+apiVersion: sonataflow.org/v1alpha08
+kind: SonataFlowPlatform
+metadata:
+ name: sonataflow-platform
+spec:
+ persistence:
+ postgresql:
+ secretRef:
+ name: my-secret
+ userKey: MY_USER
+ passwordKey: MY_PASSWORD
+ serviceRef:
+ name: db_not_defined
+ port: 3456
+ databaseName: db_name
+ build:
+ 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
Review Comment:
same here,
org.kie.kogito:kogito-addons-quarkus-jobs-knative-eventing:999-SNAPSHOT we
don't need.
##########
controllers/profiles/common/persistence/postgresql.go:
##########
@@ -0,0 +1,203 @@
+// Copyright 2023 Apache Software Foundation (ASF)
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package persistence
+
+import (
+ "fmt"
+ "sync"
+
+ corev1 "k8s.io/api/core/v1"
+
+ operatorapi
"github.com/apache/incubator-kie-kogito-serverless-operator/api/v1alpha08"
+
"github.com/apache/incubator-kie-kogito-serverless-operator/controllers/profiles/common/constants"
+)
+
+const (
+ defaultSchemaName = "default"
Review Comment:
Looks like most of this constants are not being used
##########
test/testdata/platform/persistence/generic_from_platform_cr/02-sonataflow_platform.yaml:
##########
@@ -0,0 +1,51 @@
+# Copyright 2024 Apache Software Foundation (ASF)
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+apiVersion: sonataflow.org/v1alpha08
+kind: SonataFlowPlatform
+metadata:
+ name: sonataflow-platform
+spec:
+ build:
+ template:
+ buildArgs:
+ - name: QUARKUS_EXTENSIONS
+ value:
org.kie.kogito:kogito-addons-quarkus-jobs-knative-eventing:2.0.0-SNAPSHOT
Review Comment:
yes, this extension is now included in swf-builder by default, so we can
remove it here.
##########
controllers/platform/services/services.go:
##########
@@ -138,11 +138,23 @@ func (d DataIndexHandler) MergePodSpec(podSpec
corev1.PodSpec) (corev1.PodSpec,
return *c, err
}
+// hasPostgreSQLConfigured returns true when either the SonataFlow Platform
PostgreSQL CR's structure or the one in the Data Index service specification is
not nil
+func (d DataIndexHandler) hasPostgreSQLConfigured() bool {
+ return (d.platform.Spec.Services.DataIndex.Persistence != nil &&
d.platform.Spec.Services.DataIndex.Persistence.PostgreSql != nil) ||
+ (d.platform.Spec.Persistence != nil &&
d.platform.Spec.Persistence.PostgreSQL != nil)
+}
+
func (d DataIndexHandler) ConfigurePersistence(containerSpec
*corev1.Container) *corev1.Container {
- if d.platform.Spec.Services.DataIndex.Persistence != nil &&
d.platform.Spec.Services.DataIndex.Persistence.PostgreSql != nil {
+
+ if d.hasPostgreSQLConfigured() {
+ p := d.platform.Spec.Services.DataIndex.Persistence
c := containerSpec.DeepCopy()
c.Image =
d.GetServiceImageName(constants.PersistenceTypePostgreSQL)
- c.Env = append(c.Env,
persistence.ConfigurePostgreSqlEnv(d.platform.Spec.Services.DataIndex.Persistence.PostgreSql,
d.GetServiceName(), d.platform.Namespace)...)
+ if d.platform.Spec.Services.DataIndex.Persistence != nil &&
d.platform.Spec.Services.DataIndex.Persistence.PostgreSql != nil {
Review Comment:
if we reached this point, maybe we don't need this
d.platform.Spec.Services.DataIndex.Persistence.PostgreSql != nil condition.
Maybe this changes a bit if we move to the PersistenceOptions, but yeah,
just in case.
##########
controllers/profiles/common/object_creators.go:
##########
@@ -218,10 +226,20 @@ func WorkflowPropsConfigMapCreator(workflow
*operatorapi.SonataFlow) (client.Obj
return workflowproj.CreateNewAppPropsConfigMap(workflow, props), nil
}
-func ConfigurePersistence(serviceContainer *corev1.Container, options
*operatorapi.PersistenceOptions, defaultSchema, namespace string)
*corev1.Container {
+func ConfigurePersistence(serviceContainer *corev1.Container, config
*operatorapi.PersistenceOptions, defaultSchema, namespace string)
(*corev1.Container, error) {
+ if config == nil {
+ return serviceContainer, nil
+ }
c := serviceContainer.DeepCopy()
- if options != nil && options.PostgreSql != nil {
- c.Env = append(c.Env,
persistence.ConfigurePostgreSqlEnv(options.PostgreSql, defaultSchema,
namespace)...)
+
+ if config.PostgreSql != nil {
+ c.Env = append(c.Env,
persistence.ConfigurePostgreSQLEnv(config.PostgreSql, defaultSchema,
namespace)...)
+ return c, nil
+ }
+ p := persistence.WorkflowConfig.GetPostgreSQLConfiguration()
Review Comment:
related to my previous comment, when we do persistence.WorkflowConfig are we
getting a global value?
Maybe I'm missing a go lang trick that IDK..., but looks like this value is
globally set.
##########
controllers/profiles/common/persistence/postgresql.go:
##########
@@ -0,0 +1,203 @@
+// Copyright 2023 Apache Software Foundation (ASF)
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package persistence
+
+import (
+ "fmt"
+ "sync"
+
+ corev1 "k8s.io/api/core/v1"
+
+ operatorapi
"github.com/apache/incubator-kie-kogito-serverless-operator/api/v1alpha08"
+
"github.com/apache/incubator-kie-kogito-serverless-operator/controllers/profiles/common/constants"
+)
+
+const (
+ defaultSchemaName = "default"
+ defaultDatabaseName = "sonataflow"
+
+ timeoutSeconds = 3
+ failureThreshold = 5
+ initialPeriodSeconds = 15
+ initialDelaySeconds = 10
+ successThreshold = 1
+
+ postgreSQLCPULimit = "500m"
+ postgreSQLMemoryLimit = "256Mi"
+ postgreSQLMemoryRequest = "256Mi"
+ postgreSQLCPURequest = "100m"
+
+ defaultPostgreSQLUsername = "sonataflow"
+ defaultPostgresSQLPassword = "sonataflow"
+)
+
+var (
+ WorkflowConfig *syncConfig
Review Comment:
Global var where we store the persistence information for a specific
platform, I have asked already about how this works. Happy to learn if there's
a go trick I'm missing.
##########
controllers/platform/services/services.go:
##########
@@ -252,14 +264,24 @@ func (j JobServiceHandler)
MergeContainerSpec(containerSpec *corev1.Container) (
return c, err
}
+// hasPostgreSQLConfigured returns true when either the SonataFlow Platform
PostgreSQL CR's structure or the one in the Job service specification is not nil
+func (j JobServiceHandler) hasPostgreSQLConfigured() bool {
+ return (j.platform.Spec.Services.JobService.Persistence != nil &&
j.platform.Spec.Services.JobService.Persistence.PostgreSql != nil) ||
+ (j.platform.Spec.Persistence != nil &&
j.platform.Spec.Persistence.PostgreSQL != nil)
+}
+
func (j JobServiceHandler) ConfigurePersistence(containerSpec
*corev1.Container) *corev1.Container {
- if j.platform.Spec.Services.JobService.Persistence != nil &&
j.platform.Spec.Services.JobService.Persistence.PostgreSql != nil {
+ if j.hasPostgreSQLConfigured() {
c := containerSpec.DeepCopy()
c.Image =
j.GetServiceImageName(constants.PersistenceTypePostgreSQL)
- c.Env = append(c.Env,
persistence.ConfigurePostgreSqlEnv(j.platform.Spec.Services.JobService.Persistence.PostgreSql,
j.GetServiceName(), j.platform.Namespace)...)
+ if j.platform.Spec.Services.JobService.Persistence != nil &&
j.platform.Spec.Services.JobService.Persistence.PostgreSql != nil {
Review Comment:
same here, j.platform.Spec.Services.JobService.Persistence.PostgreSql don't
needed.
##########
controllers/platform/services/services.go:
##########
@@ -276,14 +298,25 @@ func (j JobServiceHandler) GenerateServiceProperties()
(*properties.Properties,
props.Set(constants.KogitoServiceURLProperty,
generateServiceURL(constants.KogitoServiceURLProtocol, j.platform.Namespace,
j.GetServiceName()))
props.Set(constants.JobServiceKafkaSmallRyeHealthProperty, "false")
// add data source reactive URL
- jspec := j.platform.Spec.Services.JobService
- if jspec != nil && jspec.Persistence != nil &&
jspec.Persistence.PostgreSql != nil {
- dataSourceReactiveURL, err :=
generateReactiveURL(jspec.Persistence.PostgreSql, j.GetServiceName(),
j.platform.Namespace, constants.DefaultDatabaseName,
constants.DefaultPostgreSQLPort)
- if err != nil {
- return nil, err
+ if j.hasPostgreSQLConfigured() {
+ var dataSourceReactiveURL string
+ var err error
+ if j.platform.Spec.Services.JobService.Persistence != nil &&
j.platform.Spec.Services.JobService.Persistence.PostgreSql != nil {
Review Comment:
same here j.platform.Spec.Services.JobService.Persistence.PostgreSql
##########
controllers/platform/services/services.go:
##########
@@ -138,11 +138,23 @@ func (d DataIndexHandler) MergePodSpec(podSpec
corev1.PodSpec) (corev1.PodSpec,
return *c, err
}
+// hasPostgreSQLConfigured returns true when either the SonataFlow Platform
PostgreSQL CR's structure or the one in the Data Index service specification is
not nil
+func (d DataIndexHandler) hasPostgreSQLConfigured() bool {
+ return (d.platform.Spec.Services.DataIndex.Persistence != nil &&
d.platform.Spec.Services.DataIndex.Persistence.PostgreSql != nil) ||
Review Comment:
Here goes an open question guys @ricardozanini @jordigilh @tchughesiv, that
of course can't be included here because the SCP is not merged yet, but in a
flow-up PR.
If we have a configured SCP with persistence, for example Postgresql, should
it be considered here too?
##########
api/v1alpha08/sonataflow_types.go:
##########
@@ -657,8 +657,7 @@ type SonataFlowSpec struct {
// PodTemplate describes the deployment details of this SonataFlow
instance.
//+operator-sdk:csv:customresourcedefinitions:type=spec,displayName="podTemplate"
PodTemplate PodTemplateSpec `json:"podTemplate,omitempty"`
- // Persists service to a datasource of choice. Ephemeral by default.
- // +optional
+ // Persistence defines the database persistence configuration for the
workflow
Persistence *PersistenceOptions `json:"persistence,omitempty"`
Review Comment:
Now that we are going to share the PersistenceOptions type between services
and workflows, and the other Postgresql types too, maybe it could make sense to
have these structs in a separate file, maybe sonataflow_persitence_types.go
@ricardozanini @jordigilh wdyt?
##########
test/testdata/platform/persistence/overwritten_by_services/02-sonataflow_platform.yaml:
##########
@@ -0,0 +1,69 @@
+# Copyright 2024 Apache Software Foundation (ASF)
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+apiVersion: sonataflow.org/v1alpha08
+kind: SonataFlowPlatform
+metadata:
+ name: sonataflow-platform
+spec:
+ build:
+ template:
+ buildArgs:
+ - name: QUARKUS_EXTENSION
+ value:
org.kie.kogito:kogito-addons-quarkus-jobs-knative-eventing:2.0.0-SNAPSHOT
Review Comment:
same here, we can remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
