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. I also process all subreconcilers now instead of failing fast on the first occured error, following eventual consistency approach. This is handled by using go-multierror library which is also found in odh-operator.
opendatahub-io · Apr 3, 2024 · 0ab3d19 · 0ab3d19
1 parent c60f7cd
commit 0ab3d19
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 (