You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "tchughesiv (via GitHub)" <gi...@apache.org> on 2024/03/01 16:16:57 UTC
[PR] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
tchughesiv opened a new pull request, #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415
<!--
Welcome to the SonataFlow Operator! Before contributing, make sure to:
- Rebase your branch on the latest upstream main
- Link any relevant issues, PR's, or documentation
- Check that the commit message is concise and helpful:
- When fixing an issue, add "Closes #<ISSUE_NUMBER>"
- Follow the below checklist
-->
**Description of the change:**
This PR adds enabled supporting services to the `SonataFlow` status. With this change, we'll start using the `SonataFlow.Status.Services` when generating workflow properties ConfigMaps. This is to ensure `SonataFlow.Status.Services` matches/dictates what is set in the managed application.properties ConfigMap.
This PR also ensures that supporting services changes, whether cluster-wide or local to the workflow, are reflected via ConfigMap reconciliation... which automatically triggers the workflow deployment to rollout and use the new properties.
An example of the new status fields -
```yaml
apiVersion: sonataflow.org/v1alpha08
kind: SonataFlow
metadata:
name: workflow
namespace: <namespace>
spec:
...
status:
...
services:
dataIndexRef:
url: 'http://example-data-index-service.<namespace>'
jobServiceRef:
url: 'http://example-jobs-service.<namespace>'
```
**Motivation for the change:**
To reflect which supporting services are being used by each workflow so the user has a clearer understanding.
To ensure supporting services settings/changes are reconciled to workflows cluster-wide. This is critical. For example, we don't want workflows attempting to communicate against services that have been disabled or deleted. The workflows should always reflect the current state of the supporting services.
**Checklist**
- [ ] Add or Modify a unit test for your change
- [ ] Have you verified that tall the tests are passing?
<details>
<summary>
How to backport a pull request to a different branch?
</summary>
In order to automatically create a **backporting pull request** please add one or more labels having the following format `backport-<branch-name>`, where `<branch-name>` is the name of the branch where the pull request must be backported to (e.g., `backport-7.67.x` to backport the original PR to the `7.67.x` branch).
> **NOTE**: **backporting** is an action aiming to move a change (usually a commit) from a branch (usually the main one) to another one, which is generally referring to a still maintained release branch. Keeping it simple: it is about to move a specific change or a set of them from one branch to another.
Once the original pull request is successfully merged, the automated action will create one backporting pull request per each label (with the previous format) that has been added.
If something goes wrong, the author will be notified and at this point a manual backporting is needed.
> **NOTE**: this automated backporting is triggered whenever a pull request on `main` branch is labeled or closed, but both conditions must be satisfied to get the new PR created.
</details>
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "ricardozanini (via GitHub)" <gi...@apache.org>.
ricardozanini merged PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1520023589
##########
controllers/platform/services/properties.go:
##########
@@ -159,24 +159,25 @@ func generateReactiveURL(postgresSpec *operatorapi.PersistencePostgreSQL, schema
// Never nil.
func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "false")
- props.Set(constants.KogitoProcessInstancesEventsEnabled, "false")
di := NewDataIndexHandler(platform)
- if workflow != nil && !profiles.IsDevProfile(workflow) && di.IsServiceEnabled() {
- props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "true")
- props.Set(constants.KogitoProcessDefinitionsEventsErrorsEnabled, "true")
- props.Set(constants.KogitoProcessInstancesEventsEnabled, "true")
- props.Set(constants.KogitoDataIndexHealthCheckEnabled, "true")
- di := NewDataIndexHandler(platform)
- p, err := di.GenerateWorkflowProperties()
- if err != nil {
- return nil, err
+ if !profiles.IsDevProfile(workflow) && workflow != nil && workflow.Status.Services != nil && workflow.Status.Services.DataIndexRef != nil {
+ serviceBaseUrl := workflow.Status.Services.DataIndexRef.Url
+ if di.IsServiceEnabled() && len(serviceBaseUrl) > 0 {
+ props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "true")
+ props.Set(constants.KogitoProcessDefinitionsEventsErrorsEnabled, "true")
+ props.Set(constants.KogitoProcessInstancesEventsEnabled, "true")
+ props.Set(constants.KogitoDataIndexHealthCheckEnabled, "true")
+ props.Set(constants.KogitoDataIndexURL, serviceBaseUrl)
+ props.Set(constants.KogitoProcessDefinitionsEventsURL, serviceBaseUrl+constants.KogitoProcessDefinitionsEventsPath)
+ props.Set(constants.KogitoProcessInstancesEventsURL, serviceBaseUrl+constants.KogitoProcessInstancesEventsPath)
}
- props.Merge(p)
+ } else {
+ props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "false")
Review Comment:
it'll work either way, I'll change it.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "ricardozanini (via GitHub)" <gi...@apache.org>.
ricardozanini commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1509235232
##########
controllers/profiles/common/reconciler.go:
##########
@@ -46,7 +48,12 @@ type StateSupport struct {
// PerformStatusUpdate updates the SonataFlow Status conditions
func (s StateSupport) PerformStatusUpdate(ctx context.Context, workflow *operatorapi.SonataFlow) (bool, error) {
var err error
+ pl, err := platform.GetActivePlatform(ctx, s.C, workflow.Namespace)
Review Comment:
Don't we already have the platform in the `StateSupport`?
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1520073718
##########
controllers/platform/services/properties.go:
##########
@@ -185,19 +186,21 @@ func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platf
// Never nil.
func GenerateJobServiceWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.JobServiceRequestEventsConnector, constants.QuarkusHTTP)
- props.Set(constants.JobServiceRequestEventsURL, fmt.Sprintf("%s://localhost/v2/jobs/events", constants.JobServiceURLProtocol))
js := NewJobServiceHandler(platform)
- if workflow != nil && !profiles.IsDevProfile(workflow) && js.IsServiceEnabled() {
- if workflowdef.HasTimeouts(workflow) {
- props.Set(constants.KogitoJobServiceHealthCheckEnabled, "true")
- }
- p, err := js.GenerateWorkflowProperties()
- if err != nil {
- return nil, err
+ if !profiles.IsDevProfile(workflow) && workflow != nil && workflow.Status.Services != nil && workflow.Status.Services.JobServiceRef != nil {
+ serviceBaseUrl := workflow.Status.Services.JobServiceRef.Url
Review Comment:
sort of... with an important distinction... we are still using the service handlers to set the urls. There's a new method called `SetServiceUrlsInWorkflowStatus` that handles this in the workflow(s) status -
https://github.com/apache/incubator-kie-kogito-serverless-operator/blob/2325390963e643afd9868174e9818d7dafde4317/controllers/platform/services/services.go#L432-L439
what HAS changed is... during a workflow's configmap properties creation, instead of calling the handler directly, we're using the service url directly from the workflow's status... which has already been properly set by the service handlers.
this is actually LESS error prone because it ensures the accuracy of some things, which, in `main` today can be a bit nebulous -
1. this ensures that a workflow's application properties match the correct service urls. currently a user would have to figure this out for themselves by either checking the config map, or the platform object.
2. this ensures that application properties config maps are properly updated in real-time when service changes are made in the platform/clusterplatform... which, in turn, forces the workflow pod to restart and these new settings to take affect. today, a user may have to remove the existing configmap to force proper properties... which is problematic because they may not know this is required and a workflow may be attempting to send to a non-existent (or incorrect) service.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#issuecomment-1973824635
ok, e2e tests have been added and should be working
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "ricardozanini (via GitHub)" <gi...@apache.org>.
ricardozanini commented on PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#issuecomment-1978876504
@wmedvede @jakubschwan can you please review? @tchughesiv will have to jump into another task.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#issuecomment-1973524439
i need to modify the e2e tests before we merge to account for this change.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1520037462
##########
controllers/platform/services/properties.go:
##########
@@ -185,19 +186,21 @@ func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platf
// Never nil.
func GenerateJobServiceWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.JobServiceRequestEventsConnector, constants.QuarkusHTTP)
Review Comment:
done
##########
controllers/platform/services/properties.go:
##########
@@ -159,24 +159,25 @@ func generateReactiveURL(postgresSpec *operatorapi.PersistencePostgreSQL, schema
// Never nil.
func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "false")
- props.Set(constants.KogitoProcessInstancesEventsEnabled, "false")
di := NewDataIndexHandler(platform)
- if workflow != nil && !profiles.IsDevProfile(workflow) && di.IsServiceEnabled() {
- props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "true")
- props.Set(constants.KogitoProcessDefinitionsEventsErrorsEnabled, "true")
- props.Set(constants.KogitoProcessInstancesEventsEnabled, "true")
- props.Set(constants.KogitoDataIndexHealthCheckEnabled, "true")
- di := NewDataIndexHandler(platform)
- p, err := di.GenerateWorkflowProperties()
- if err != nil {
- return nil, err
+ if !profiles.IsDevProfile(workflow) && workflow != nil && workflow.Status.Services != nil && workflow.Status.Services.DataIndexRef != nil {
+ serviceBaseUrl := workflow.Status.Services.DataIndexRef.Url
+ if di.IsServiceEnabled() && len(serviceBaseUrl) > 0 {
+ props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "true")
+ props.Set(constants.KogitoProcessDefinitionsEventsErrorsEnabled, "true")
+ props.Set(constants.KogitoProcessInstancesEventsEnabled, "true")
+ props.Set(constants.KogitoDataIndexHealthCheckEnabled, "true")
+ props.Set(constants.KogitoDataIndexURL, serviceBaseUrl)
+ props.Set(constants.KogitoProcessDefinitionsEventsURL, serviceBaseUrl+constants.KogitoProcessDefinitionsEventsPath)
+ props.Set(constants.KogitoProcessInstancesEventsURL, serviceBaseUrl+constants.KogitoProcessInstancesEventsPath)
}
- props.Merge(p)
+ } else {
+ props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "false")
Review Comment:
done
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1520099759
##########
controllers/platform/services/properties.go:
##########
@@ -185,19 +186,21 @@ func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platf
// Never nil.
func GenerateJobServiceWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.JobServiceRequestEventsConnector, constants.QuarkusHTTP)
- props.Set(constants.JobServiceRequestEventsURL, fmt.Sprintf("%s://localhost/v2/jobs/events", constants.JobServiceURLProtocol))
js := NewJobServiceHandler(platform)
- if workflow != nil && !profiles.IsDevProfile(workflow) && js.IsServiceEnabled() {
- if workflowdef.HasTimeouts(workflow) {
- props.Set(constants.KogitoJobServiceHealthCheckEnabled, "true")
- }
- p, err := js.GenerateWorkflowProperties()
- if err != nil {
- return nil, err
+ if !profiles.IsDevProfile(workflow) && workflow != nil && workflow.Status.Services != nil && workflow.Status.Services.JobServiceRef != nil {
+ serviceBaseUrl := workflow.Status.Services.JobServiceRef.Url
Review Comment:
one more note on this. this is the final part of a long planned, multi-tasked, implementation of supporting services in the operator. `workflow.status.services` has not existed until now, so we previously had no choice but to call the service handlers directly during workflow properties creation. with this PR, we'll have reached the intended "final" implementation.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "wmedvede (via GitHub)" <gi...@apache.org>.
wmedvede commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1519597852
##########
controllers/platform/services/properties.go:
##########
@@ -159,24 +159,25 @@ func generateReactiveURL(postgresSpec *operatorapi.PersistencePostgreSQL, schema
// Never nil.
func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "false")
- props.Set(constants.KogitoProcessInstancesEventsEnabled, "false")
di := NewDataIndexHandler(platform)
- if workflow != nil && !profiles.IsDevProfile(workflow) && di.IsServiceEnabled() {
- props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "true")
- props.Set(constants.KogitoProcessDefinitionsEventsErrorsEnabled, "true")
- props.Set(constants.KogitoProcessInstancesEventsEnabled, "true")
- props.Set(constants.KogitoDataIndexHealthCheckEnabled, "true")
- di := NewDataIndexHandler(platform)
- p, err := di.GenerateWorkflowProperties()
- if err != nil {
- return nil, err
+ if !profiles.IsDevProfile(workflow) && workflow != nil && workflow.Status.Services != nil && workflow.Status.Services.DataIndexRef != nil {
+ serviceBaseUrl := workflow.Status.Services.DataIndexRef.Url
+ if di.IsServiceEnabled() && len(serviceBaseUrl) > 0 {
+ props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "true")
+ props.Set(constants.KogitoProcessDefinitionsEventsErrorsEnabled, "true")
+ props.Set(constants.KogitoProcessInstancesEventsEnabled, "true")
+ props.Set(constants.KogitoDataIndexHealthCheckEnabled, "true")
+ props.Set(constants.KogitoDataIndexURL, serviceBaseUrl)
+ props.Set(constants.KogitoProcessDefinitionsEventsURL, serviceBaseUrl+constants.KogitoProcessDefinitionsEventsPath)
+ props.Set(constants.KogitoProcessInstancesEventsURL, serviceBaseUrl+constants.KogitoProcessInstancesEventsPath)
}
- props.Merge(p)
+ } else {
+ props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "false")
Review Comment:
I'd suggest moving the setting of this two properties to the begin of the processing as we did before.
```
props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "false")
props.Set(constants.KogitoProcessInstancesEventsEnabled, "false")
```
Because, now, with this change, we have
if (frst level condition) {
if (second level condition) {
//setting to true if second level condition is true, but if not, we still need the workflow to have them set to false.
props.Set(constants.KogitoProcessDefinitionsEventsEnabled, "true")
props.Set(constants.KogitoProcessInstancesEventsEnabled, "true")
}
}
##########
controllers/platform/services/properties.go:
##########
@@ -185,19 +186,21 @@ func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platf
// Never nil.
func GenerateJobServiceWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.JobServiceRequestEventsConnector, constants.QuarkusHTTP)
Review Comment:
Same comment as for the the DataIndex, It's more safe to keep this setting here.
##########
controllers/platform/services/properties.go:
##########
@@ -185,19 +186,21 @@ func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platf
// Never nil.
func GenerateJobServiceWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.JobServiceRequestEventsConnector, constants.QuarkusHTTP)
- props.Set(constants.JobServiceRequestEventsURL, fmt.Sprintf("%s://localhost/v2/jobs/events", constants.JobServiceURLProtocol))
js := NewJobServiceHandler(platform)
- if workflow != nil && !profiles.IsDevProfile(workflow) && js.IsServiceEnabled() {
- if workflowdef.HasTimeouts(workflow) {
- props.Set(constants.KogitoJobServiceHealthCheckEnabled, "true")
- }
- p, err := js.GenerateWorkflowProperties()
- if err != nil {
- return nil, err
+ if !profiles.IsDevProfile(workflow) && workflow != nil && workflow.Status.Services != nil && workflow.Status.Services.JobServiceRef != nil {
+ serviceBaseUrl := workflow.Status.Services.JobServiceRef.Url
Review Comment:
I see something that I believe more difficult to follow now, or potentially error prone. Let me explain:
**Before:**
The jobs-service url was calculated by the JobServiceHandler, and there, it was derived somehow directly from that service, or, from the cluster platform if any.
**Now:**
Get get the jobs-service url from the workflow.Status.Services.JobServiceRef, BUT, the workflow is normally what is being deployed at this point, and that somehow "caused" the execution of all this cycle. So we configure some WF properties from values from the WF itself.
I am correct?
Same reasoning apply for the DI
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1520073718
##########
controllers/platform/services/properties.go:
##########
@@ -185,19 +186,21 @@ func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platf
// Never nil.
func GenerateJobServiceWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.JobServiceRequestEventsConnector, constants.QuarkusHTTP)
- props.Set(constants.JobServiceRequestEventsURL, fmt.Sprintf("%s://localhost/v2/jobs/events", constants.JobServiceURLProtocol))
js := NewJobServiceHandler(platform)
- if workflow != nil && !profiles.IsDevProfile(workflow) && js.IsServiceEnabled() {
- if workflowdef.HasTimeouts(workflow) {
- props.Set(constants.KogitoJobServiceHealthCheckEnabled, "true")
- }
- p, err := js.GenerateWorkflowProperties()
- if err != nil {
- return nil, err
+ if !profiles.IsDevProfile(workflow) && workflow != nil && workflow.Status.Services != nil && workflow.Status.Services.JobServiceRef != nil {
+ serviceBaseUrl := workflow.Status.Services.JobServiceRef.Url
Review Comment:
no, we are still using the service handlers to set the urls. There's a new method called `SetServiceUrlsInWorkflowStatus` that handles this in the workflow(s) status -
https://github.com/apache/incubator-kie-kogito-serverless-operator/blob/2325390963e643afd9868174e9818d7dafde4317/controllers/platform/services/services.go#L432-L439
what HAS changed is... during a workflow's configmap properties creation, instead of calling the handler directly, we're using the service url directly from the workflow's status... which has already been properly set by the service handlers.
this is actually LESS error prone because it ensures the accuracy of some things, which, in `main` today can be a bit nebulous -
1. this ensures that a workflow's application properties match the correct service urls which are set the status of a workflow. currently a user would have to figure this out for themselves by either checking the config map, or the platform object.
2. this ensures that application properties config maps are properly updated in real-time when service changes are made in the platform/clusterplatform... which, in turn, forces the workflow pod to restart and these new settings to take affect. today, a user may have to remove the existing configmap to force proper properties... which is problematic because they may not know this is required and a workflow may be attempting to send to a non-existent (or incorrect) service.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1520080448
##########
controllers/platform/services/properties.go:
##########
@@ -185,19 +186,21 @@ func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platf
// Never nil.
func GenerateJobServiceWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.JobServiceRequestEventsConnector, constants.QuarkusHTTP)
- props.Set(constants.JobServiceRequestEventsURL, fmt.Sprintf("%s://localhost/v2/jobs/events", constants.JobServiceURLProtocol))
js := NewJobServiceHandler(platform)
- if workflow != nil && !profiles.IsDevProfile(workflow) && js.IsServiceEnabled() {
- if workflowdef.HasTimeouts(workflow) {
- props.Set(constants.KogitoJobServiceHealthCheckEnabled, "true")
- }
- p, err := js.GenerateWorkflowProperties()
- if err != nil {
- return nil, err
+ if !profiles.IsDevProfile(workflow) && workflow != nil && workflow.Status.Services != nil && workflow.Status.Services.JobServiceRef != nil {
+ serviceBaseUrl := workflow.Status.Services.JobServiceRef.Url
Review Comment:
its important to keep in mind that, workflows are re-reconciled anytime a related platform object is changed -
https://github.com/apache/incubator-kie-kogito-serverless-operator/blob/fff6361bde07b566a22bc24ec7be84506e7b98ae/controllers/sonataflow_controller.go#L174-L181
this ensures that service-related changes at the platform level will result in a workflow.status change... which will result in a configmap change and workflow pod restart. this has all been thoroughly tested.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1509241369
##########
controllers/profiles/common/reconciler.go:
##########
@@ -46,7 +48,12 @@ type StateSupport struct {
// PerformStatusUpdate updates the SonataFlow Status conditions
func (s StateSupport) PerformStatusUpdate(ctx context.Context, workflow *operatorapi.SonataFlow) (bool, error) {
var err error
+ pl, err := platform.GetActivePlatform(ctx, s.C, workflow.Namespace)
Review Comment:
hmm... i'm not seeing it
https://github.com/apache/incubator-kie-kogito-serverless-operator/blob/76ef90247c695ea6973e7887c794b03a1541ac63/controllers/profiles/common/reconciler.go#L41-L47
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1511683050
##########
controllers/profiles/common/reconciler.go:
##########
@@ -46,7 +48,12 @@ type StateSupport struct {
// PerformStatusUpdate updates the SonataFlow Status conditions
func (s StateSupport) PerformStatusUpdate(ctx context.Context, workflow *operatorapi.SonataFlow) (bool, error) {
var err error
+ pl, err := platform.GetActivePlatform(ctx, s.C, workflow.Namespace)
Review Comment:
the problem is that half of the profile/state reconciles are not retrieving the active platform. i thought it was cleaner to add it once here, rather than several other locations.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "wmedvede (via GitHub)" <gi...@apache.org>.
wmedvede commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1520135464
##########
controllers/platform/services/properties.go:
##########
@@ -185,19 +186,21 @@ func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platf
// Never nil.
func GenerateJobServiceWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.JobServiceRequestEventsConnector, constants.QuarkusHTTP)
- props.Set(constants.JobServiceRequestEventsURL, fmt.Sprintf("%s://localhost/v2/jobs/events", constants.JobServiceURLProtocol))
js := NewJobServiceHandler(platform)
- if workflow != nil && !profiles.IsDevProfile(workflow) && js.IsServiceEnabled() {
- if workflowdef.HasTimeouts(workflow) {
- props.Set(constants.KogitoJobServiceHealthCheckEnabled, "true")
- }
- p, err := js.GenerateWorkflowProperties()
- if err != nil {
- return nil, err
+ if !profiles.IsDevProfile(workflow) && workflow != nil && workflow.Status.Services != nil && workflow.Status.Services.JobServiceRef != nil {
+ serviceBaseUrl := workflow.Status.Services.JobServiceRef.Url
Review Comment:
Ok, many thanks for the explanation.
The part that wasn't clear to me was "during a workflow's configmap properties creation, instead of calling the handler directly, we're using the service url directly from **the workflow's status... which has already been properly set by the service handlers.**"
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "ricardozanini (via GitHub)" <gi...@apache.org>.
ricardozanini commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1511664290
##########
controllers/profiles/common/reconciler.go:
##########
@@ -46,7 +48,12 @@ type StateSupport struct {
// PerformStatusUpdate updates the SonataFlow Status conditions
func (s StateSupport) PerformStatusUpdate(ctx context.Context, workflow *operatorapi.SonataFlow) (bool, error) {
var err error
+ pl, err := platform.GetActivePlatform(ctx, s.C, workflow.Namespace)
Review Comment:
I'm wondering if we can reuse the platform we already get in the states/reconciliation. But it's a nit; I think the PR is fine. 👍
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1511683050
##########
controllers/profiles/common/reconciler.go:
##########
@@ -46,7 +48,12 @@ type StateSupport struct {
// PerformStatusUpdate updates the SonataFlow Status conditions
func (s StateSupport) PerformStatusUpdate(ctx context.Context, workflow *operatorapi.SonataFlow) (bool, error) {
var err error
+ pl, err := platform.GetActivePlatform(ctx, s.C, workflow.Namespace)
Review Comment:
the problem is that half of the profile/state reconciles are not using/retrieving the active platform. i thought it was cleaner to add it once here, rather than several other locations.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1520073718
##########
controllers/platform/services/properties.go:
##########
@@ -185,19 +186,21 @@ func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platf
// Never nil.
func GenerateJobServiceWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.JobServiceRequestEventsConnector, constants.QuarkusHTTP)
- props.Set(constants.JobServiceRequestEventsURL, fmt.Sprintf("%s://localhost/v2/jobs/events", constants.JobServiceURLProtocol))
js := NewJobServiceHandler(platform)
- if workflow != nil && !profiles.IsDevProfile(workflow) && js.IsServiceEnabled() {
- if workflowdef.HasTimeouts(workflow) {
- props.Set(constants.KogitoJobServiceHealthCheckEnabled, "true")
- }
- p, err := js.GenerateWorkflowProperties()
- if err != nil {
- return nil, err
+ if !profiles.IsDevProfile(workflow) && workflow != nil && workflow.Status.Services != nil && workflow.Status.Services.JobServiceRef != nil {
+ serviceBaseUrl := workflow.Status.Services.JobServiceRef.Url
Review Comment:
no, we are still using the service handlers to set the urls. There's a new method called `SetServiceUrlsInWorkflowStatus` that handles this in the workflow(s) status -
https://github.com/apache/incubator-kie-kogito-serverless-operator/blob/2325390963e643afd9868174e9818d7dafde4317/controllers/platform/services/services.go#L432-L439
what HAS changed is... during a workflow's configmap properties creation, instead of calling the handler directly, we're using the service url directly from the workflow's status... which has already been properly set by the service handlers.
this is actually LESS error prone because it ensures the accuracy of some things, which, in `main` today can be a bit nebulous -
1. this ensures that a workflow's application properties match the correct service urls. currently a user would have to figure this out for themselves by either checking the config map, or the platform object.
2. this ensures that application properties config maps are properly updated in real-time when service changes are made in the platform/clusterplatform... which, in turn, forces the workflow pod to restart and these new settings to take affect. today, a user may have to remove the existing configmap to force proper properties... which is problematic because they may not know this is required and a workflow may be attempting to send to a non-existent (or incorrect) service.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [KOGITO-9973] Add `SonataFlow.Status.Services` field and improve workflow properties ConfigMap reconciliation [incubator-kie-kogito-serverless-operator]
Posted by "tchughesiv (via GitHub)" <gi...@apache.org>.
tchughesiv commented on code in PR #415:
URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/415#discussion_r1520080448
##########
controllers/platform/services/properties.go:
##########
@@ -185,19 +186,21 @@ func GenerateDataIndexWorkflowProperties(workflow *operatorapi.SonataFlow, platf
// Never nil.
func GenerateJobServiceWorkflowProperties(workflow *operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform) (*properties.Properties, error) {
props := properties.NewProperties()
- props.Set(constants.JobServiceRequestEventsConnector, constants.QuarkusHTTP)
- props.Set(constants.JobServiceRequestEventsURL, fmt.Sprintf("%s://localhost/v2/jobs/events", constants.JobServiceURLProtocol))
js := NewJobServiceHandler(platform)
- if workflow != nil && !profiles.IsDevProfile(workflow) && js.IsServiceEnabled() {
- if workflowdef.HasTimeouts(workflow) {
- props.Set(constants.KogitoJobServiceHealthCheckEnabled, "true")
- }
- p, err := js.GenerateWorkflowProperties()
- if err != nil {
- return nil, err
+ if !profiles.IsDevProfile(workflow) && workflow != nil && workflow.Status.Services != nil && workflow.Status.Services.JobServiceRef != nil {
+ serviceBaseUrl := workflow.Status.Services.JobServiceRef.Url
Review Comment:
its also important to keep in mind that, workflows are re-reconciled anytime a related platform object is changed -
https://github.com/apache/incubator-kie-kogito-serverless-operator/blob/fff6361bde07b566a22bc24ec7be84506e7b98ae/controllers/sonataflow_controller.go#L174-L181
this ensures that service-related changes at the platform level will result in a workflow.status change... which will result in a configmap change and workflow pod restart. this has all been thoroughly tested.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org