kie-issues-314: Operator driven service discovery API Phase3 #316

wmedvede · 2023-12-04T11:32:33Z

Description of the change:

Motivation for the change:

Checklist

Add or Modify a unit test for your change
Have you verified that tall the tests are passing?

How to backport a pull request to a different branch?

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.

wmedvede · 2023-12-04T11:38:37Z

Closes #314

ricardozanini

Hi Walter! Many thanks for this work! I think we need to adjust a little bit the Knative client builder and we are ready to go. :)

ricardozanini · 2023-12-08T12:34:37Z

bundle/manifests/sonataflow-operator.clusterserviceversion.yaml

@@ -536,6 +536,42 @@ spec:
          - subjectaccessreviews
          verbs:
          - create
+        - apiGroups:


Nice catch!

ricardozanini · 2023-12-08T12:35:46Z

controllers/discovery/discovery.go

@@ -84,10 +89,10 @@ type sonataFlowServiceCatalog struct {
 }

 // NewServiceCatalog returns a new ServiceCatalog configured to resolve kubernetes, knative, and openshift resource addresses.
-func NewServiceCatalog(cli client.Client) ServiceCatalog {
+func NewServiceCatalog(cli client.Client, knServingClient clientservingv1.ServingV1Interface, knEventingClient clienteventingv1.EventingV1Interface) ServiceCatalog {


We can have a DiscoveryClient struct to hold all this and share it here as a pointer. Wdyt?

ricardozanini · 2023-12-08T12:37:52Z

controllers/discovery/discovery_test.go

@@ -116,7 +116,7 @@ func doTesQueryKubernetesDeploymentWithService(t *testing.T, outputFormat string
 	service.Spec.Type = corev1.ServiceTypeNodePort

 	cli := fake.NewClientBuilder().WithRuntimeObjects(deployment, service).Build()
-	ctg := NewServiceCatalog(cli)
+	ctg := NewServiceCatalog(cli, nil, nil)


Having a DiscoveryClient you can get rid of passing nil here.

NewKnativeDiscoveryClient(cli, knativeEven, knativeSvc) and NewDiscoveryClient(cli). Alternatively, the NewServiceCatalog could create this knative clients internally if it only depends on client.

ricardozanini · 2023-12-08T12:39:52Z

controllers/profiles/common/app_properties.go

@@ -267,6 +268,35 @@ func generateDiscoveryProperties(ctx context.Context, catalog discovery.ServiceC
 			}
 		}
 	}
+
+	for _, function := range workflow.Spec.Flow.Functions {


This method is getting quite large. Maybe a little refactoring here to an app_properties_discovery.go file that would wrap al of this? wdyt?

ricardozanini · 2023-12-08T12:46:19Z

main.go

+				os.Exit(1)
+			}
+		}
+	}


Here's not the best place to add this. If I install this operator first and then knative, I won't have these clients configured. We have to carry the RestConfig to the controllers and create the Knative clients during the reconciliation.

Alternatively, we can have a factory for these clients to hold them in a singleton pattern. In every reconciliation cycle, we can simply check if Knative is still installed in the cluster and return the client we have.

This accessor then can be used in discovery and we avoid passing all this construct ProfileExtensions around.

Something like knative.GetClient() will check if there's an instance ready, if not we create one, cache, and return.

Also, this is not a required state, meaning we can't os.Exit(1) if we can't create the client.

ricardozanini · 2023-12-08T12:47:28Z

utils/cluster.go

+// SetIsKnative sets the global flags isKnativeServing and isKnativeEventing by the controller manager.
+func SetIsKnative(cfg *rest.Config) {
+	if cli, err := discovery.NewDiscoveryClientForConfig(cfg); err != nil {
+		panic("Impossible to verify if the knative system is installed in the cluster: " + err.Error())


Like I said in the previous comment, if we can't get a Knative client, we disable discovery for their objects or any future feature we may have.

ricardozanini · 2023-12-08T12:48:09Z

utils/knative/common.go

+ * under the License.
+ */
+
+package knative


We can have a topmost knative package to implement everything I'm proposing in the other comments.

I think you can move this code to the new module controllers/knative

ricardozanini · 2023-12-08T12:48:55Z

utils/cluster.go

@@ -37,6 +41,36 @@ func SetIsOpenShift(cfg *rest.Config) {
 	var err error
 	isOpenShift, err = openshift.IsOpenShift(cfg)
 	if err != nil {
-		panic("Impossible to verify if the cluster is OpenShift or not" + err.Error())
+		panic("Impossible to verify if the cluster is OpenShift or not: " + err.Error())


Here it's fine to have a terminal state since being or not OpenShift is deterministic.

wmedvede · 2023-12-11T17:43:11Z

@ricardozanini your suggestions were added, would you mind take a second look?

wmedvede · 2023-12-12T08:58:00Z

btw, E3E test is failing because of /home/kogito/launch/build-app.sh file permissions.
We should have this fixed when this apache/incubator-kie-kogito-images#1715 is merged and the new quay.io/kiegroup/kogito-swf-builder-nightly:latest image is produced.

INFO[0019] Args: [-c /home/kogito/launch/build-app.sh ./resources]
INFO[0019] Util.Lookup returned: &{Uid:1001 Gid:0 Username:kogito Name:Kogito user HomeDir:/home/kogito}
INFO[0019] Performing slow lookup of group ids for kogito
INFO[0019] Running: [/bin/sh -c /home/kogito/launch/build-app.sh ./resources]
/bin/sh: /home/kogito/launch/build-app.sh: Permission denied
error building image: error building stage: failed to execute command: waiting for process to exit: exit status 126

ricardozanini

Only details now Walter! Great work, clean and concise. I like it a lot. Many thanks!

controllers/discovery/discovery.go

controllers/discovery/knative_catalog.go

controllers/knative/knative.go

controllers/sonataflow_controller.go

ricardozanini · 2023-12-12T14:40:52Z

utils/knative/common.go

+ * under the License.
+ */
+
+package knative


I think you can move this code to the new module controllers/knative

wmedvede · 2023-12-13T15:18:38Z

@ricardozanini your additional suggestions are in, have a look please.

kie-issues-314: Operator driven service discovery API Phase3 - Adds the processing of workflow functions to generate the service discovery for knative functions invokations kie-issues-314: Operator driven service discovery API Phase3 - Code review suggestions 1 kie-issues-314: Operator driven service discovery API Phase3 - Code review suggestions 2

wmedvede · 2023-12-21T18:05:35Z

Hi @ricardozanini , this is ready to merge

)

wmedvede requested a review from ricardozanini as a code owner December 4, 2023 11:32

wmedvede requested review from ricardozanini and removed request for ricardozanini December 4, 2023 11:32

wmedvede force-pushed the kie-issues-314 branch from 4981872 to 51c6a1a Compare December 4, 2023 11:46

wmedvede requested a review from jakubschwan December 4, 2023 11:49

ricardozanini linked an issue Dec 4, 2023 that may be closed by this pull request

Operator driven service discovery API Phase3 #314

Closed

ricardozanini reviewed Dec 8, 2023

View reviewed changes

ricardozanini reviewed Dec 12, 2023

View reviewed changes

wmedvede requested a review from ricardozanini December 13, 2023 15:17

ricardozanini approved these changes Dec 13, 2023

View reviewed changes

jakubschwan approved these changes Dec 13, 2023

View reviewed changes

wmedvede force-pushed the kie-issues-314 branch from ae8ab89 to 5733069 Compare December 21, 2023 17:59

ricardozanini merged commit f0d9068 into apache:main Dec 21, 2023
4 checks passed

rgdoliveira pushed a commit to rgdoliveira/kogito-serverless-operator that referenced this pull request Jan 29, 2024

kie-issues-314: Operator driven service discovery API Phase3 (apache#316

ca09a4b

)

rgdoliveira pushed a commit to rgdoliveira/kogito-serverless-operator that referenced this pull request Jan 29, 2024

kie-issues-314: Operator driven service discovery API Phase3 (apache#316

70977e4

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kie-issues-314: Operator driven service discovery API Phase3 #316

kie-issues-314: Operator driven service discovery API Phase3 #316

wmedvede commented Dec 4, 2023

wmedvede commented Dec 4, 2023 •

edited

Loading

ricardozanini left a comment

ricardozanini Dec 8, 2023

ricardozanini Dec 8, 2023

ricardozanini Dec 8, 2023

ricardozanini Dec 8, 2023

ricardozanini Dec 8, 2023

ricardozanini Dec 8, 2023

ricardozanini Dec 8, 2023

ricardozanini Dec 12, 2023

ricardozanini Dec 8, 2023

wmedvede commented Dec 11, 2023

wmedvede commented Dec 12, 2023

ricardozanini left a comment

ricardozanini Dec 12, 2023

wmedvede commented Dec 13, 2023

wmedvede commented Dec 21, 2023

kie-issues-314: Operator driven service discovery API Phase3 #316

kie-issues-314: Operator driven service discovery API Phase3 #316

Conversation

wmedvede commented Dec 4, 2023

wmedvede commented Dec 4, 2023 • edited Loading

ricardozanini 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmedvede commented Dec 11, 2023

wmedvede commented Dec 12, 2023

ricardozanini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmedvede commented Dec 13, 2023

wmedvede commented Dec 21, 2023

wmedvede commented Dec 4, 2023 •

edited

Loading