Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add section to configure when operator full knative #247

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gabriel-farache
Copy link
Collaborator

@gabriel-farache gabriel-farache commented Sep 30, 2024

Reopening of #211

Closes https://issues.redhat.com/browse/FLPATH-1406

  • Add section to configure when operator full knative
  • Add broker spec in orchestrator.sonataflowPlatform to populate the OSL operator eventing related spec

Related to parodos-dev/serverless-workflows-config#375

@gabriel-farache
Copy link
Collaborator Author

/hold until apache/incubator-kie-kogito-serverless-operator#467 is merged and chart is updated with operator version with the feature

@gabriel-farache
Copy link
Collaborator Author

/unhold

@@ -79,6 +79,16 @@ spec:
Job Service container image to be used instead of the provided
one by SonataFlow
type: string
broker:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to match the SFP structure instead, i.e:

eventing:
  description: Eventing related properties to set in the SonataFlow Platform
  properties:
    broker:
      description: Broker contains the name and namespace of the broker to use if using Knative eventing for components communications 
      properties:
        name:
          description: name describes the name of the broker to use
          type: string
        namespace:
          description: namespace describes the namespace on which the broker to use is deployed
          type: string
      type: object
type: object

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you considered adding these fields to the sonataflowOperator structure? I'm aware that as it is here keeps consistency with the other fields (dataindex and job service image resource limits), but I find it strange that these fields reside under the orchestrator struct, even though they are specific to sonataflow. If they belong to both orchestrator and sonataflow, what do you think of the broker being its own high level entity, sibling to the orchestrator?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency sake, I think we can leave it at the same level with dataindex and job service images...(children of sonataflowPlatform) under the orchestrator.

Perhaps, for the go-operator, we split the sonataflowOperator (now serverlessLogicOperator) as we did for rhdh, and have the sonataflowPlatform configurations (dataindex, job service, broker etc) as a child since these are all specific to sonataflow. E.g

"serverlessLogic": {
  "installOperator": true
  "resources":
      "limits":
        "cpu": "cpu"
        "memory": "memory"
      "requests":
        "cpu": "cpu"
        "memory": "memory"
  "eventing":
    "broker":
      "name": "name"
      "namespace": "namespace"
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I should introduce the eventing level here as I suggested, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please, that's sounds fine to me.

helm-charts/orchestrator/templates/sonataflows.yaml Outdated Show resolved Hide resolved
docs/main/kafka-knative-broker/README.md Outdated Show resolved Hide resolved
docs/main/kafka-knative-broker/README.md Outdated Show resolved Hide resolved
docs/main/kafka-knative-broker/README.md Outdated Show resolved Hide resolved
## Installation steps

1. Configure and enable kafka broker feature in Knative: https://knative.dev/docs/eventing/brokers/broker-types/kafka-broker/
* i.e:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e meaning in essence? I'm curious why use the abbreviation in a statement that has no other sentences. What do you think of adding more context to this bullet point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e means that is, so, to configure knative with kafka, that is the commands you need to run.
I could have used e.g as the configuration may change and may differ https://www.merriam-webster.com/grammar/ie-vs-eg-abbreviation-meaning-usage-difference

docs/main/kafka-knative-broker/README.md Outdated Show resolved Hide resolved
docs/main/kafka-knative-broker/README.md Outdated Show resolved Hide resolved
docs/main/kafka-knative-broker/README.md Outdated Show resolved Hide resolved
docs/main/kafka-knative-broker/README.md Outdated Show resolved Hide resolved
@@ -79,6 +79,16 @@ spec:
Job Service container image to be used instead of the provided
one by SonataFlow
type: string
broker:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you considered adding these fields to the sonataflowOperator structure? I'm aware that as it is here keeps consistency with the other fields (dataindex and job service image resource limits), but I find it strange that these fields reside under the orchestrator struct, even though they are specific to sonataflow. If they belong to both orchestrator and sonataflow, what do you think of the broker being its own high level entity, sibling to the orchestrator?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants