ricardozanini commented on code in PR #540: URL: https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/540#discussion_r1797101269
########## internal/controller/profiles/monitoring/monitoring.go: ########## @@ -0,0 +1,65 @@ +// 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. + +package monitoring + +import ( + "context" + + operatorapi "github.com/apache/incubator-kie-kogito-serverless-operator/api/v1alpha08" + "github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/monitoring" + "github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/common" + "github.com/apache/incubator-kie-kogito-serverless-operator/log" + "k8s.io/klog/v2" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var _ MonitoringEventingHandler = &monitoringObjectManager{} + +type monitoringObjectManager struct { + serviceMonitor common.ObjectEnsurer + *common.StateSupport +} + +func NewMonitoringHandler(support *common.StateSupport) MonitoringEventingHandler { + return &monitoringObjectManager{ + serviceMonitor: common.NewObjectEnsurer(support.C, common.ServiceMonitorCreator), + StateSupport: support, + } +} + +type MonitoringEventingHandler interface { + Ensure(ctx context.Context, workflow *operatorapi.SonataFlow) ([]client.Object, error) +} + +func (k monitoringObjectManager) Ensure(ctx context.Context, workflow *operatorapi.SonataFlow) ([]client.Object, error) { + var objs []client.Object + monitoringAvail, err := monitoring.GetPrometheusAvailability(k.Cfg) + if err != nil { + klog.V(log.I).InfoS("Error checking Prometheus availability: %v", err) + return nil, err + } + if !monitoringAvail { + klog.V(log.I).InfoS("Prometheus is not installed") Review Comment: In the platform controller, if the `.spec.monitoring.enabled` is true and Prometheus is not available, we should emit a warning event. So we can bring this check there. Here, you can skip logging it. ########## internal/controller/profiles/preview/profile_preview.go: ########## @@ -58,7 +58,13 @@ type ObjectEnsurers struct { // kservice Knative Serving deployment for this ensurer. Don't call it directly, use DeploymentByDeploymentModel instead kservice common.ObjectEnsurerWithPlatform // service for this ensurer. Don't call it directly, use ServiceByDeploymentModel instead - service common.ObjectEnsurer + service common.ObjectEnsurer + // kMetricsService for this ensurer. Don't call it directly, use ServiceByDeploymentModel instead + kMetricsService common.ObjectEnsurer + // serviceMonitor for this ensurer. Don't call it directly, use ServiceMonitorByDeploymentModel instead + serviceMonitor common.ObjectEnsurer + // kServiceMonitor for this ensurer. Don't call it directly, use ServiceMonitorByDeploymentModel instead + kServiceMonitor common.ObjectEnsurer Review Comment: Haven't we removed the knative support? ########## api/v1alpha08/sonataflowplatform_types.go: ########## @@ -63,6 +63,10 @@ type SonataFlowPlatformSpec struct { // These properties MAY NOT be propagated to a SonataFlowClusterPlatform since PropertyVarSource can only refer local context sources. // +optional Properties *PropertyPlatformSpec `json:"properties,omitempty"` + // Settings for Prometheus monitoring + // +optional + // +default: false Review Comment: ```suggestion ``` ########## test/e2e/workflow_test.go: ########## @@ -168,111 +168,206 @@ var _ = Describe("Workflow Persistence Use Cases :: ", Label("flows-persistence" } }) + /* + DescribeTable("when deploying a SonataFlow CR with PostgreSQL persistence", func(testcaseDir string, withPersistence bool, waitKSinkInjection bool) { + By("Deploy the CR") + var manifests []byte + EventuallyWithOffset(1, func() error { + var err error + cmd := exec.Command("kubectl", "kustomize", testcaseDir) + manifests, err = utils.Run(cmd) + return err + }, time.Minute, time.Second).Should(Succeed()) + cmd := exec.Command("kubectl", "create", "-n", ns, "-f", "-") + cmd.Stdin = bytes.NewBuffer(manifests) + _, err := utils.Run(cmd) + Expect(err).NotTo(HaveOccurred()) Review Comment: Can you remove the comment? ########## internal/controller/profiles/dev/states_dev.go: ########## @@ -111,6 +112,14 @@ func (e *ensureRunningWorkflowState) Do(ctx context.Context, workflow *operatora } objs = append(objs, service) + if monitoring.IsMonitoringEnabled(pl) { Review Comment: Can you isolate in a function to avoid conditionals in this func? Otherwise, it will grow in complexity. -- 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]
