diff --git a/controllers/routingctrl/controller.go b/controllers/routingctrl/controller.go index b7e7b6d..b4b49cf 100644 --- a/controllers/routingctrl/controller.go +++ b/controllers/routingctrl/controller.go @@ -98,8 +98,6 @@ func (r *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu errs = append(errs, reconciler(ctx, sourceRes)) } - errs = append(errs, unstruct.Patch(ctx, r.Client, sourceRes)) - return ctrl.Result{}, errors.Join(errs...) } diff --git a/controllers/routingctrl/delete_resources.go b/controllers/routingctrl/delete_resources.go index c501686..b32414b 100644 --- a/controllers/routingctrl/delete_resources.go +++ b/controllers/routingctrl/delete_resources.go @@ -6,6 +6,7 @@ import ( "github.com/opendatahub-io/odh-platform/pkg/metadata/labels" "github.com/opendatahub-io/odh-platform/pkg/routing" + "github.com/opendatahub-io/odh-platform/pkg/unstruct" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" k8slabels "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" @@ -88,10 +89,13 @@ func (r *Controller) deleteOwnedResources(ctx context.Context, // removeFinalizer is called after a successful cleanup, it removes the finalizer from the resource in the cluster. func removeFinalizer(ctx context.Context, cli client.Client, sourceRes *unstructured.Unstructured) error { if controllerutil.ContainsFinalizer(sourceRes, finalizerName) { - controllerutil.RemoveFinalizer(sourceRes, finalizerName) + if err := unstruct.PatchWithRetry(ctx, cli, sourceRes, func() error { + controllerutil.RemoveFinalizer(sourceRes, finalizerName) - if err := cli.Update(ctx, sourceRes); err != nil { - return fmt.Errorf("failed to remove finalizer: %w", err) + return nil + }); err != nil { + return fmt.Errorf("failed to remove finalizer from %s (in %s): %w", + sourceRes.GroupVersionKind().String(), sourceRes.GetNamespace(), err) } } diff --git a/controllers/routingctrl/reconcile_resources.go b/controllers/routingctrl/reconcile_resources.go index 5615d92..c86106a 100644 --- a/controllers/routingctrl/reconcile_resources.go +++ b/controllers/routingctrl/reconcile_resources.go @@ -23,12 +23,8 @@ func (r *Controller) createRoutingResources(ctx context.Context, target *unstruc if len(exportModes) == 0 { r.log.Info("No export mode found for target") - metadata.ApplyMetaOptions(target, - annotations.Remove(annotations.RoutingAddressesExternal("")), - annotations.Remove(annotations.RoutingAddressesPublic("")), - ) - return nil + return r.propagateHostsToWatchedCR(ctx, target, nil, nil) } r.log.Info("Reconciling resources for target", "target", target) @@ -97,38 +93,49 @@ func (r *Controller) exportService(ctx context.Context, target *unstructured.Uns } } - return r.propagateHostsToWatchedCR(target, publicHosts, externalHosts) + return r.propagateHostsToWatchedCR(ctx, target, publicHosts, externalHosts) } -func (r *Controller) propagateHostsToWatchedCR(target *unstructured.Unstructured, publicHosts, externalHosts []string) error { - // Remove all existing routing addresses - metaOptions := []metadata.Option{ - annotations.Remove(annotations.RoutingAddressesExternal("")), - annotations.Remove(annotations.RoutingAddressesPublic("")), - } +func (r *Controller) propagateHostsToWatchedCR(ctx context.Context, target *unstructured.Unstructured, publicHosts, externalHosts []string) error { + err := unstruct.PatchWithRetry(ctx, r.Client, target, func() error { + // Always remove the annotations first + annotations.Remove(annotations.RoutingAddressesExternal(""))(target) + annotations.Remove(annotations.RoutingAddressesPublic(""))(target) - if len(publicHosts) > 0 { - metaOptions = append(metaOptions, annotations.RoutingAddressesPublic(strings.Join(publicHosts, ";"))) - } + var metaOptions []metadata.Option - if len(externalHosts) > 0 { - metaOptions = append(metaOptions, annotations.RoutingAddressesExternal(strings.Join(externalHosts, ";"))) - } + if len(publicHosts) > 0 { + metaOptions = append(metaOptions, annotations.RoutingAddressesPublic(strings.Join(publicHosts, ";"))) + } + + if len(externalHosts) > 0 { + metaOptions = append(metaOptions, annotations.RoutingAddressesExternal(strings.Join(externalHosts, ";"))) + } - metadata.ApplyMetaOptions(target, metaOptions...) + metadata.ApplyMetaOptions(target, metaOptions...) + + return nil + }) + + if err != nil { + return fmt.Errorf("failed to propagate hosts to watched CR %s/%s: %w", target.GetNamespace(), target.GetName(), err) + } return nil } func (r *Controller) ensureResourceHasFinalizer(ctx context.Context, target *unstructured.Unstructured) error { if !controllerutil.ContainsFinalizer(target, finalizerName) { - if err := unstruct.PatchMutate(ctx, r.Client, target, func() error { + if err := unstruct.PatchWithRetry(ctx, r.Client, target, func() error { controllerutil.AddFinalizer(target, finalizerName) + return nil }); err != nil { - return fmt.Errorf("failed to patch finalizer to resource %s/%s: %w", target.GetNamespace(), target.GetName(), err) + return fmt.Errorf("failed to patch finalizer to %s (in %s): %w", + target.GroupVersionKind().String(), target.GetNamespace(), err) } } + return nil } diff --git a/pkg/unstruct/funcs.go b/pkg/unstruct/funcs.go index 73984cf..bd3cfde 100644 --- a/pkg/unstruct/funcs.go +++ b/pkg/unstruct/funcs.go @@ -64,38 +64,13 @@ func IsMarkedForDeletion(target *unstructured.Unstructured) bool { return !target.GetDeletionTimestamp().IsZero() } -// Patch updates the specified Kubernetes resource by applying changes from the provided target object. -// In case of conflicts, it will retry using default strategy. -func PatchMutate(ctx context.Context, cli client.Client, target *unstructured.Unstructured, mutate controllerutil.MutateFn) error { +// PatchWithRetry applies changes to the specified resource using the provided mutate function. +// It uses a retry mechanism to handle potential conflicts during the update process. +func PatchWithRetry(ctx context.Context, cli client.Client, target *unstructured.Unstructured, mutate controllerutil.MutateFn) error { err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { _, err := controllerutil.CreateOrPatch(ctx, cli, target, mutate) - return err - }) - - if err != nil { - return fmt.Errorf("failed to patchmutate resource metadata with retry: %w", err) - } - - return nil -} - -// Patch updates the specified Kubernetes resource by applying changes from the provided target object. -// In case of conflicts, it will retry using default strategy. -func Patch(ctx context.Context, cli client.Client, target *unstructured.Unstructured) error { - err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { - currentRes := &unstructured.Unstructured{} - currentRes.SetGroupVersionKind(target.GroupVersionKind()) - - if err := cli.Get(ctx, client.ObjectKeyFromObject(target), currentRes); err != nil { - return err //nolint:wrapcheck // Return unwrapped error for retry logic - } - - patch := client.MergeFrom(target) - if errPatch := cli.Patch(ctx, currentRes, patch); errPatch != nil { - return errPatch //nolint:wrapcheck // Return unwrapped error for retry logic - } - return nil + return err //nolint:wrapcheck // Return unwrapped error per RetryOnConflict godoc }) if err != nil {