chore(reconciler): refactors Reconciler to simplify handling dependent resources #185
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,26 +27,25 @@ import ( | |
| "github.com/opendatahub-io/odh-model-controller/controllers/processors" | ||
| "github.com/opendatahub-io/odh-model-controller/controllers/resources" | ||
| k8serror "k8s.io/apimachinery/pkg/api/errors" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| ctrl "sigs.k8s.io/controller-runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| var _ SubResourceReconciler = (*KserveAuthConfigReconciler)(nil) | ||
|
|
||
| type KserveAuthConfigReconciler struct { | ||
| client client.Client | ||
| deltaProcessor processors.DeltaProcessor | ||
| detector resources.AuthTypeDetector | ||
| store resources.AuthConfigStore | ||
| templateLoader resources.AuthConfigTemplateLoader | ||
| hostExtractor resources.InferenceServiceHostExtractor | ||
| } | ||
|
|
||
| func NewKserveAuthConfigReconciler(client client.Client) *KserveAuthConfigReconciler { | ||
| return &KserveAuthConfigReconciler{ | ||
| client: client, | ||
| deltaProcessor: processors.NewDeltaProcessor(), | ||
| detector: resources.NewKServeAuthTypeDetector(client), | ||
| store: resources.NewClientAuthConfigStore(client), | ||
|
|
@@ -56,6 +55,7 @@ func NewKserveAuthConfigReconciler(client client.Client, scheme *runtime.Scheme) | |
| } | ||
|
|
||
| func (r *KserveAuthConfigReconciler) Reconcile(ctx context.Context, log logr.Logger, isvc *kservev1beta1.InferenceService) error { | ||
| log.V(1).Info("Reconciling Authorino AuthConfig for InferenceService") | ||
|
|
||
| if isvc.Status.URL == nil { | ||
| log.V(1).Info("Inference Service not ready yet, waiting for URL") | ||
|
|
@@ -77,7 +77,20 @@ func (r *KserveAuthConfigReconciler) Reconcile(ctx context.Context, log logr.Log | |
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (r *KserveAuthConfigReconciler) Delete(ctx context.Context, log logr.Logger, isvc *kservev1beta1.InferenceService) error { | ||
|
There was a problem hiding this comment. Why do we need explicit Delete function implementation for There was a problem hiding this comment. Yes, I thought about it too. It's been like that before and I wonder if we need it. We have an internal store of processed templates, that's why there's this hook here, though it's not really about Deleting stuff in the cluster. @aslakknutsen can this be simplified somehow? There was a problem hiding this comment. The store is just a wrapper around Client and has no logic per say. We could leave clean up to Kubernetes GC as suggested. There was a problem hiding this comment. Ok then it would only leave route reconciler left with Delete, as it's a special case as route sits in istio ns. I will clean it up. That's this piece in odh-model-controller/controllers/reconcilers/kserve_route_reconciler.go Lines 202 to 204 in 5db4925 |
||
| log.V(1).Info("Deleting Kserve inference service authorino authconfig entry") | ||
| typeName := types.NamespacedName{ | ||
| Name: isvc.GetName(), | ||
| Namespace: isvc.GetNamespace(), | ||
| } | ||
| return r.store.Remove(ctx, typeName) | ||
| } | ||
|
|
||
| func (r *KserveAuthConfigReconciler) Cleanup(_ context.Context, _ logr.Logger, _ string) error { | ||
| // NOOP | ||
| return nil | ||
| } | ||
|
|
||
| func (r *KserveAuthConfigReconciler) createDesiredResource(ctx context.Context, isvc *kservev1beta1.InferenceService) (*authorinov1beta2.AuthConfig, error) { | ||
|
|
@@ -103,7 +116,7 @@ func (r *KserveAuthConfigReconciler) createDesiredResource(ctx context.Context, | |
| } | ||
| template.Labels[constants.LabelAuthGroup] = "default" | ||
|
|
||
| ctrl.SetControllerReference(isvc, &template, r.client.Scheme()) | ||
|
|
||
| return &template, nil | ||
| } | ||
|
|
@@ -152,11 +165,3 @@ func (r *KserveAuthConfigReconciler) processDelta(ctx context.Context, log logr. | |
| } | ||
| return nil | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KserveAuthConfigReconcilershould implementNoResourceRemovalinterface as it have owner relationship with inference resource.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #185 (comment)