feat: keeps sub resource reconcilers in a slice

With this approach there is no need for extract code in main reconcilers (i.e. KServe and ModelMesh) to handle reconcile/delete/cleanup logic of related resources. The only requirement now is to add instance of the subresource reconciler to the slice, the rest is handled uniformly. It also process all subreconcilers now instead of failing fast on the first occured error, following the eventual consistency approach. This is handled by using go-multierror library which is also found in odh-operator. Signed-off-by: bartoszmajsak <[email protected]>
opendatahub-io · Apr 3, 2024 · 45ac5fc · 45ac5fc
1 parent 1ab78df
commit 45ac5fc
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 308 deletions.
diff --git a/controllers/reconcilers/kserve_serverless_inferenceservice_reconciler.go b/controllers/reconcilers/kserve_serverless_inferenceservice_reconciler.go
@@ -17,6 +17,7 @@ package reconcilers
 
 import (
 	"context"
+	"github.com/hashicorp/go-multierror"
 
 	"github.com/go-logr/logr"
 	kservev1beta1 "github.com/kserve/kserve/pkg/apis/serving/v1beta1"
@@ -27,99 +28,47 @@ import (
 var _ Reconciler = (*KserveServerlessInferenceServiceReconciler)(nil)
 
 type KserveServerlessInferenceServiceReconciler struct {
-	client                            client.Client
-	routeReconciler                   *KserveRouteReconciler
-	metricsServiceReconciler          *KserveMetricsServiceReconciler
-	metricsServiceMonitorReconciler   *KserveMetricsServiceMonitorReconciler
-	prometheusRoleBindingReconciler   *KservePrometheusRoleBindingReconciler
-	istioSMMRReconciler               *KserveIstioSMMRReconciler
-	istioTelemetryReconciler          *KserveIstioTelemetryReconciler
-	istioServiceMonitorReconciler     *KserveIstioServiceMonitorReconciler
-	istioPodMonitorReconciler         *KserveIstioPodMonitorReconciler
-	istioPeerAuthenticationReconciler *KserveIstioPeerAuthenticationReconciler
-	networkPolicyReconciler           *KserveNetworkPolicyReconciler
-	authConfigReconciler              *KserveAuthConfigReconciler
+	client                 client.Client
+	subResourceReconcilers []SubResourceReconciler
 }
 
 func NewKServeServerlessInferenceServiceReconciler(client client.Client) *KserveServerlessInferenceServiceReconciler {
+	subResourceReconciler := []SubResourceReconciler{
+		NewKServeIstioSMMRReconciler(client),
+		NewKserveRouteReconciler(client),
+		NewKServeMetricsServiceReconciler(client),
+		NewKServeMetricsServiceMonitorReconciler(client),
+		NewKServePrometheusRoleBindingReconciler(client),
+		NewKServeIstioTelemetryReconciler(client),
+		NewKServeIstioServiceMonitorReconciler(client),
+		NewKServeIstioPodMonitorReconciler(client),
+		NewKServeIstioPeerAuthenticationReconciler(client),
+		NewKServeNetworkPolicyReconciler(client),
+		NewKserveAuthConfigReconciler(client),
+	}
+
 	return &KserveServerlessInferenceServiceReconciler{
-		client:                            client,
-		istioSMMRReconciler:               NewKServeIstioSMMRReconciler(client),
-		routeReconciler:                   NewKserveRouteReconciler(client),
-		metricsServiceReconciler:          NewKServeMetricsServiceReconciler(client),
-		metricsServiceMonitorReconciler:   NewKServeMetricsServiceMonitorReconciler(client),
-		prometheusRoleBindingReconciler:   NewKServePrometheusRoleBindingReconciler(client),
-		istioTelemetryReconciler:          NewKServeIstioTelemetryReconciler(client),
-		istioServiceMonitorReconciler:     NewKServeIstioServiceMonitorReconciler(client),
-		istioPodMonitorReconciler:         NewKServeIstioPodMonitorReconciler(client),
-		istioPeerAuthenticationReconciler: NewKServeIstioPeerAuthenticationReconciler(client),
-		networkPolicyReconciler:           NewKServeNetworkPolicyReconciler(client),
-		authConfigReconciler:              NewKserveAuthConfigReconciler(client),
+		client:                 client,
+		subResourceReconcilers: subResourceReconciler,
 	}
 }
 
-// TODO(reconciler): make it a slice. keep order
 func (r *KserveServerlessInferenceServiceReconciler) Reconcile(ctx context.Context, log logr.Logger, isvc *kservev1beta1.InferenceService) error {
-	//  Resource created per namespace
-
-	if err := r.istioSMMRReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-
-	if err := r.prometheusRoleBindingReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-
-	if err := r.istioTelemetryReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-
-	if err := r.istioServiceMonitorReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-
-	if err := r.istioPodMonitorReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-
-	if err := r.istioPeerAuthenticationReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
+	var reconcileErrors *multierror.Error
+	for _, reconciler := range r.subResourceReconcilers {
+		reconcileErrors = multierror.Append(reconcileErrors, reconciler.Reconcile(ctx, log, isvc))
 	}
 
-	if err := r.networkPolicyReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-
-	if err := r.authConfigReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-
-	if err := r.routeReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-
-	if err := r.metricsServiceReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-
-	if err := r.metricsServiceMonitorReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-
-	return nil
+	return reconcileErrors.ErrorOrNil()
 }
 
 func (r *KserveServerlessInferenceServiceReconciler) OnDeletionOfKserveInferenceService(ctx context.Context, log logr.Logger, isvc *kservev1beta1.InferenceService) error {
-
-	// TODO(reconciler): shouldnt we iterate over all deletes?
-	if err := r.routeReconciler.Delete(ctx, log, isvc); err != nil {
-		return err
+	var deleteErrors *multierror.Error
+	for _, reconciler := range r.subResourceReconcilers {
+		deleteErrors = multierror.Append(deleteErrors, reconciler.Delete(ctx, log, isvc))
 	}
 
-	if err := r.authConfigReconciler.Delete(ctx, log, isvc); err != nil {
-		return err
-	}
-	return nil
+	return deleteErrors.ErrorOrNil()
 }
 
 func (r *KserveServerlessInferenceServiceReconciler) DeleteKserveMetricsResourcesIfNoKserveIsvcExists(ctx context.Context, log logr.Logger, isvcNamespace string) error {
@@ -140,35 +89,12 @@ func (r *KserveServerlessInferenceServiceReconciler) DeleteKserveMetricsResource
 	}
 
 	// If there are no Kserve InferenceServices in the namespace, delete namespace-scoped resources needed for Kserve Metrics
+	var cleanupErrors *multierror.Error
 	if len(inferenceServiceList.Items) == 0 {
-
-		if err := r.istioSMMRReconciler.Cleanup(ctx, log, isvcNamespace); err != nil {
-			return err
-		}
-
-		if err := r.prometheusRoleBindingReconciler.Cleanup(ctx, log, isvcNamespace); err != nil {
-			return err
-		}
-
-		if err := r.istioTelemetryReconciler.Cleanup(ctx, log, isvcNamespace); err != nil {
-			return err
-		}
-
-		if err := r.istioServiceMonitorReconciler.Cleanup(ctx, log, isvcNamespace); err != nil {
-			return err
-		}
-
-		if err := r.istioPodMonitorReconciler.Cleanup(ctx, log, isvcNamespace); err != nil {
-			return err
-		}
-
-		if err := r.istioPeerAuthenticationReconciler.Cleanup(ctx, log, isvcNamespace); err != nil {
-			return err
-		}
-
-		if err := r.networkPolicyReconciler.Cleanup(ctx, log, isvcNamespace); err != nil {
-			return err
+		for _, reconciler := range r.subResourceReconcilers {
+			cleanupErrors = multierror.Append(cleanupErrors, reconciler.Cleanup(ctx, log, isvcNamespace))
 		}
 	}
-	return nil
+
+	return cleanupErrors.ErrorOrNil()
 }
diff --git a/controllers/reconcilers/mm_inferenceservice_reconciler.go b/controllers/reconcilers/mm_inferenceservice_reconciler.go
@@ -17,6 +17,7 @@ package reconcilers
 
 import (
 	"context"
+	"github.com/hashicorp/go-multierror"
 
 	"github.com/go-logr/logr"
 	kservev1beta1 "github.com/kserve/kserve/pkg/apis/serving/v1beta1"
@@ -28,35 +29,28 @@ var _ Reconciler = (*ModelMeshInferenceServiceReconciler)(nil)
 
 type ModelMeshInferenceServiceReconciler struct {
 	SingleResourcePerNamespace
-	client                       client.Client
-	routeReconciler              *ModelMeshRouteReconciler
-	serviceAccountReconciler     *ModelMeshServiceAccountReconciler
-	clusterRoleBindingReconciler *ModelMeshClusterRoleBindingReconciler
+	client                 client.Client
+	subResourceReconcilers []SubResourceReconciler
 }
 
 func NewModelMeshInferenceServiceReconciler(client client.Client) *ModelMeshInferenceServiceReconciler {
 	return &ModelMeshInferenceServiceReconciler{
-		client:                       client,
-		routeReconciler:              NewModelMeshRouteReconciler(client),
-		serviceAccountReconciler:     NewModelMeshServiceAccountReconciler(client),
-		clusterRoleBindingReconciler: NewModelMeshClusterRoleBindingReconciler(client),
+		client: client,
+		subResourceReconcilers: []SubResourceReconciler{
+			NewModelMeshRouteReconciler(client),
+			NewModelMeshServiceAccountReconciler(client),
+			NewModelMeshClusterRoleBindingReconciler(client),
+		},
 	}
 }
 
 func (r *ModelMeshInferenceServiceReconciler) Reconcile(ctx context.Context, log logr.Logger, isvc *kservev1beta1.InferenceService) error {
-
-	if err := r.routeReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
+	var reconcileErrors *multierror.Error
+	for _, reconciler := range r.subResourceReconcilers {
+		reconcileErrors = multierror.Append(reconcileErrors, reconciler.Reconcile(ctx, log, isvc))
 	}
 
-	if err := r.serviceAccountReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-
-	if err := r.clusterRoleBindingReconciler.Reconcile(ctx, log, isvc); err != nil {
-		return err
-	}
-	return nil
+	return reconcileErrors.ErrorOrNil()
 }
 
 func (r *ModelMeshInferenceServiceReconciler) DeleteModelMeshResourcesIfNoMMIsvcExists(ctx context.Context, log logr.Logger, isvcNs string) error {
@@ -77,15 +71,12 @@ func (r *ModelMeshInferenceServiceReconciler) DeleteModelMeshResourcesIfNoMMIsvc
 	}
 
 	// If there are no ModelMesh InferenceServices in the namespace, delete namespace-scoped resources needed for ModelMesh
+	var cleanupErrors *multierror.Error
 	if len(inferenceServiceList.Items) == 0 {
-
-		if err := r.serviceAccountReconciler.Cleanup(ctx, log, isvcNs); err != nil {
-			return err
-		}
-
-		if err := r.clusterRoleBindingReconciler.Cleanup(ctx, log, isvcNs); err != nil {
-			return err
+		for _, reconciler := range r.subResourceReconcilers {
+			cleanupErrors = multierror.Append(cleanupErrors, reconciler.Cleanup(ctx, log, isvcNs))
 		}
 	}
-	return nil
+
+	return cleanupErrors.ErrorOrNil()
 }
diff --git a/go.mod b/go.mod
@@ -4,6 +4,7 @@ go 1.19
 
 require (
 	github.com/go-logr/logr v1.2.4
+	github.com/hashicorp/go-multierror v1.1.1
 	github.com/kserve/kserve v0.11.0
 	github.com/kuadrant/authorino v0.15.0
 	github.com/onsi/ginkgo v1.16.4
@@ -24,7 +25,6 @@ require (
 	knative.dev/serving v0.37.1
 	maistra.io/api v0.0.0-20230417135504-0536f6c22b1c
 	sigs.k8s.io/controller-runtime v0.14.6
-	sigs.k8s.io/yaml v1.3.0
 )
 
 require (
@@ -58,6 +58,7 @@ require (
 	github.com/googleapis/enterprise-certificate-proxy v0.3.2 // indirect
 	github.com/googleapis/gax-go/v2 v2.12.0 // indirect
 	github.com/googleapis/google-cloud-go-testing v0.0.0-20210719221736-1c9a4c676720 // indirect
+	github.com/hashicorp/errwrap v1.1.0 // indirect
 	github.com/imdario/mergo v0.3.15 // indirect
 	github.com/jmespath/go-jmespath v0.4.0 // indirect
 	github.com/josharian/intern v1.0.0 // indirect
@@ -106,6 +107,7 @@ require (
 	knative.dev/networking v0.0.0-20231017124814-2a7676e912b7 // indirect
 	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect
+	sigs.k8s.io/yaml v1.3.0 // indirect
 )
 
 replace (