From 83f716deb076021ee3d2d0c9a787aa5687e8a307 Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Wed, 12 Feb 2025 18:41:24 +0100 Subject: [PATCH 1/6] Improve gc service/action (#1656) * Improve gc service/action * Remove useless lock * Fix typos * Improve error handling --- pkg/controller/actions/gc/action_gc.go | 43 +++-- .../actions/gc/action_gc_support.go | 7 +- pkg/controller/actions/gc/action_gc_test.go | 148 ++++++++++++++- pkg/services/gc/gc.go | 178 +++++++++++------- pkg/services/gc/gc_support.go | 8 - 5 files changed, 293 insertions(+), 91 deletions(-) diff --git a/pkg/controller/actions/gc/action_gc.go b/pkg/controller/actions/gc/action_gc.go index b39dc705bd26..081323b49edb 100644 --- a/pkg/controller/actions/gc/action_gc.go +++ b/pkg/controller/actions/gc/action_gc.go @@ -17,15 +17,17 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/services/gc" ) -type PredicateFn func(*odhTypes.ReconciliationRequest, unstructured.Unstructured) (bool, error) +type ObjectPredicateFn func(*odhTypes.ReconciliationRequest, unstructured.Unstructured) (bool, error) +type TypePredicateFn func(*odhTypes.ReconciliationRequest, schema.GroupVersionKind) (bool, error) type ActionOpts func(*Action) type Action struct { - labels map[string]string - selector labels.Selector - unremovables []schema.GroupVersionKind - gc *gc.GC - predicateFn PredicateFn + labels map[string]string + selector labels.Selector + unremovables []schema.GroupVersionKind + gc *gc.GC + objectPredicateFn ObjectPredicateFn + typePredicateFn TypePredicateFn } func WithLabel(name string, value string) ActionOpts { @@ -56,13 +58,22 @@ func WithUnremovables(items ...schema.GroupVersionKind) ActionOpts { } } -func WithPredicate(value PredicateFn) ActionOpts { +func WithObjectPredicate(value ObjectPredicateFn) ActionOpts { return func(action *Action) { if value == nil { return } - action.predicateFn = value + action.objectPredicateFn = value + } +} +func WithTypePredicate(value TypePredicateFn) ActionOpts { + return func(action *Action) { + if value == nil { + return + } + + action.typePredicateFn = value } } @@ -102,13 +113,20 @@ func (a *Action) run(ctx context.Context, rr *odhTypes.ReconciliationRequest) er deleted, err := a.gc.Run( ctx, selector, - func(ctx context.Context, obj unstructured.Unstructured) (bool, error) { + gc.WithTypeFilter(func(ctx context.Context, kind schema.GroupVersionKind) (bool, error) { + if slices.Contains(a.unremovables, kind) { + return false, nil + } + + return a.typePredicateFn(rr, kind) + }), + gc.WithObjectFilter(func(ctx context.Context, obj unstructured.Unstructured) (bool, error) { if slices.Contains(a.unremovables, obj.GroupVersionKind()) { return false, nil } - return a.predicateFn(rr, obj) - }, + return a.objectPredicateFn(rr, obj) + }), ) if err != nil { @@ -124,7 +142,8 @@ func (a *Action) run(ctx context.Context, rr *odhTypes.ReconciliationRequest) er func NewAction(opts ...ActionOpts) actions.Fn { action := Action{} - action.predicateFn = DefaultPredicate + action.objectPredicateFn = DefaultObjectPredicate + action.typePredicateFn = DefaultTypePredicate action.unremovables = make([]schema.GroupVersionKind, 0) for _, opt := range opts { diff --git a/pkg/controller/actions/gc/action_gc_support.go b/pkg/controller/actions/gc/action_gc_support.go index 6ee3da9ebff8..ab8babbcfa09 100644 --- a/pkg/controller/actions/gc/action_gc_support.go +++ b/pkg/controller/actions/gc/action_gc_support.go @@ -5,13 +5,14 @@ import ( "strconv" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" odhTypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" odhAnnotations "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) -func DefaultPredicate(rr *odhTypes.ReconciliationRequest, obj unstructured.Unstructured) (bool, error) { +func DefaultObjectPredicate(rr *odhTypes.ReconciliationRequest, obj unstructured.Unstructured) (bool, error) { if obj.GetAnnotations() == nil { return false, nil } @@ -44,3 +45,7 @@ func DefaultPredicate(rr *odhTypes.ReconciliationRequest, obj unstructured.Unstr return rr.Instance.GetGeneration() != int64(g), nil } + +func DefaultTypePredicate(_ *odhTypes.ReconciliationRequest, _ schema.GroupVersionKind) (bool, error) { + return true, nil +} diff --git a/pkg/controller/actions/gc/action_gc_test.go b/pkg/controller/actions/gc/action_gc_test.go index 2020c7ee3b27..8f5528ee434a 100644 --- a/pkg/controller/actions/gc/action_gc_test.go +++ b/pkg/controller/actions/gc/action_gc_test.go @@ -13,6 +13,7 @@ import ( appsv1 "k8s.io/api/apps/v1" authorizationv1 "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" k8serr "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -128,7 +129,7 @@ func TestGcAction(t *testing.T) { generated: true, matcher: Not(HaveOccurred()), metricsMatcher: BeNumerically("==", 1), - options: []gc.ActionOpts{gc.WithPredicate( + options: []gc.ActionOpts{gc.WithObjectPredicate( func(request *types.ReconciliationRequest, unstructured unstructured.Unstructured) (bool, error) { return unstructured.GroupVersionKind() != gvk.ConfigMap, nil }, @@ -253,3 +254,148 @@ func TestGcAction(t *testing.T) { }) } } + +func TestGcActionCluster(t *testing.T) { + g := NewWithT(t) + + s := runtime.NewScheme() + ctx := context.Background() + id := xid.New().String() + nsn := xid.New().String() + + utilruntime.Must(corev1.AddToScheme(s)) + utilruntime.Must(appsv1.AddToScheme(s)) + utilruntime.Must(apiextensionsv1.AddToScheme(s)) + utilruntime.Must(authorizationv1.AddToScheme(s)) + utilruntime.Must(rbacv1.AddToScheme(s)) + + envTest := &envtest.Environment{ + CRDInstallOptions: envtest.CRDInstallOptions{ + Scheme: s, + CleanUpAfterUse: true, + }, + } + + t.Cleanup(func() { + _ = envTest.Stop() + }) + + cfg, err := envTest.Start() + g.Expect(err).NotTo(HaveOccurred()) + + envTestClient, err := ctrlCli.New(cfg, ctrlCli.Options{Scheme: s}) + g.Expect(err).NotTo(HaveOccurred()) + + cli, err := client.NewFromConfig(cfg, envTestClient) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cli).NotTo(BeNil()) + + gci := gcSvc.New( + cli, + nsn, + // This is required as there are no kubernetes controller running + // with the envtest, hence we can't use the foreground deletion + // policy (default) + gcSvc.WithPropagationPolicy(metav1.DeletePropagationBackground), + ) + + ns := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: nsn, + }, + } + + g.Expect(cli.Create(ctx, &ns)). + NotTo(HaveOccurred()) + g.Expect(gci.Start(ctx)). + NotTo(HaveOccurred()) + + rr := types.ReconciliationRequest{ + Client: cli, + DSCI: &dsciv1.DSCInitialization{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, + }, + Instance: &componentApi.Dashboard{ + TypeMeta: metav1.TypeMeta{ + APIVersion: componentApi.GroupVersion.String(), + Kind: componentApi.DashboardKind, + }, + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + UID: apytypes.UID(id), + }, + }, + Release: common.Release{ + Name: cluster.OpenDataHub, + Version: version.OperatorVersion{ + Version: semver.Version{Major: 0, Minor: 2, Patch: 0}, + }, + }, + Generated: true, + } + + om := metav1.ObjectMeta{ + Namespace: nsn, + Annotations: map[string]string{ + annotations.InstanceGeneration: "1", + annotations.InstanceUID: id, + annotations.PlatformType: string(cluster.OpenDataHub), + }, + Labels: map[string]string{ + labels.PlatformPartOf: strings.ToLower(componentApi.DashboardKind), + }, + } + + cm1 := corev1.ConfigMap{ObjectMeta: *om.DeepCopy()} + cm1.Name = xid.New().String() + cm1.Annotations[annotations.PlatformVersion] = "0.1.0" + + cm2 := corev1.ConfigMap{ObjectMeta: *om.DeepCopy()} + cm2.Name = xid.New().String() + cm2.Annotations[annotations.PlatformVersion] = rr.Release.Version.String() + + cr1 := rbacv1.ClusterRole{ObjectMeta: *om.DeepCopy()} + cr1.Name = xid.New().String() + cr1.Annotations[annotations.PlatformVersion] = "0.1.0" + + cr2 := rbacv1.ClusterRole{ObjectMeta: *om.DeepCopy()} + cr2.Name = xid.New().String() + cr2.Annotations[annotations.PlatformVersion] = rr.Release.Version.String() + + g.Expect(cli.Create(ctx, &cm1)). + NotTo(HaveOccurred()) + + g.Expect(cli.Create(ctx, &cm2)). + NotTo(HaveOccurred()) + + g.Expect(cli.Create(ctx, &cr1)). + NotTo(HaveOccurred()) + + g.Expect(cli.Create(ctx, &cr2)). + NotTo(HaveOccurred()) + + a := gc.NewAction(gc.WithGC(gci)) + + gc.DeletedTotal.Reset() + gc.DeletedTotal.WithLabelValues("dashboard").Add(0) + + err = a(ctx, &rr) + g.Expect(err).NotTo(HaveOccurred()) + + err = cli.Get(ctx, ctrlCli.ObjectKeyFromObject(&cm1), &corev1.ConfigMap{}) + g.Expect(err).To(MatchError(k8serr.IsNotFound, "IsNotFound")) + + err = cli.Get(ctx, ctrlCli.ObjectKeyFromObject(&cm2), &corev1.ConfigMap{}) + g.Expect(err).ToNot(HaveOccurred()) + + err = cli.Get(ctx, ctrlCli.ObjectKeyFromObject(&cr1), &rbacv1.ClusterRole{}) + g.Expect(err).To(MatchError(k8serr.IsNotFound, "IsNotFound")) + + err = cli.Get(ctx, ctrlCli.ObjectKeyFromObject(&cr2), &rbacv1.ClusterRole{}) + g.Expect(err).ToNot(HaveOccurred()) + + ct := testutil.ToFloat64(gc.DeletedTotal) + g.Expect(ct).Should(BeNumerically("==", 2)) +} diff --git a/pkg/services/gc/gc.go b/pkg/services/gc/gc.go index 284a2da3dd7b..de80a46f49c4 100644 --- a/pkg/services/gc/gc.go +++ b/pkg/services/gc/gc.go @@ -22,13 +22,12 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/client" ) -// Instance a global instance of the GC service. -// // TODO: since the GC service is quite heavy, as it has to discover -// -// resources that can be subject to GC, we share single global -// instance, however as long term, we should find a better way -// to let consumer of the service to access it. +// resources that can be subject to GC, we share single global +// instance, however as long term, we should find a better way +// to let consumer of the service to access it. + +// Instance a global instance of the GC service. var Instance *GC const ( @@ -39,14 +38,16 @@ const ( type options struct { propagationPolicy ctrlCli.PropagationPolicy - unremovables []schema.GroupVersionKind + unremovables map[schema.GroupVersionKind]struct{} } type OptsFn func(*options) func WithUnremovables(items ...schema.GroupVersionKind) OptsFn { return func(o *options) { - o.unremovables = append(o.unremovables, items...) + for _, i := range items { + o.unremovables[i] = struct{}{} + } } } @@ -62,7 +63,7 @@ func New(cli *client.Client, ns string, opts ...OptsFn) *GC { ns: ns, options: options{ propagationPolicy: ctrlCli.PropagationPolicy(metav1.DeletePropagationForeground), - unremovables: make([]schema.GroupVersionKind, 0), + unremovables: make(map[schema.GroupVersionKind]struct{}), }, resources: Resources{ @@ -100,48 +101,97 @@ func (gc *GC) Start(ctx context.Context) error { return nil } +type runOptions struct { + typePredicate func(context.Context, schema.GroupVersionKind) (bool, error) + objectPredicate func(context.Context, unstructured.Unstructured) (bool, error) +} + +type RunOptionsFn func(*runOptions) + +func WithObjectFilter(fn func(context.Context, unstructured.Unstructured) (bool, error)) RunOptionsFn { + return func(o *runOptions) { + o.objectPredicate = fn + } +} +func WithTypeFilter(fn func(context.Context, schema.GroupVersionKind) (bool, error)) RunOptionsFn { + return func(o *runOptions) { + o.typePredicate = fn + } +} + +func (gc *GC) listResources( + ctx context.Context, + res Resource, + opts metav1.ListOptions, +) ([]unstructured.Unstructured, error) { + items, err := gc.client.Dynamic().Resource(res.GroupVersionResource()).Namespace("").List(ctx, opts) + switch { + case k8serr.IsForbidden(err) || k8serr.IsMethodNotSupported(err): + gc.log(ctx).V(3).Info( + "cannot list resource", + "reason", err.Error(), + "gvk", res.GroupVersionKind(), + ) + + return nil, nil + case err != nil: + return nil, err + default: + return items.Items, nil + } +} + func (gc *GC) Run( ctx context.Context, selector labels.Selector, - predicate func(context.Context, unstructured.Unstructured) (bool, error), + opts ...RunOptionsFn, ) (int, error) { - l := gc.log(ctx) + ro := runOptions{ + typePredicate: func(_ context.Context, _ schema.GroupVersionKind) (bool, error) { + return true, nil + }, + objectPredicate: func(_ context.Context, _ unstructured.Unstructured) (bool, error) { + return true, nil + }, + } + + for _, opt := range opts { + opt(&ro) + } deleted := 0 resources := gc.resources.Get() - - dc := gc.client.Dynamic() lo := metav1.ListOptions{LabelSelector: selector.String()} - for r := range resources { - items, err := dc.Resource(resources[r].GroupVersionResource()).Namespace("").List(ctx, lo) + for _, res := range resources { + canBeDeleted, err := ro.typePredicate(ctx, res.GroupVersionKind()) if err != nil { - if k8serr.IsForbidden(err) { - l.V(3).Info( - "cannot list resource", - "reason", err.Error(), - "gvk", resources[r].GroupVersionKind(), - ) - - continue - } + return 0, fmt.Errorf("cannot determine if resource %s can be deleted: %w", res.String(), err) + } - if k8serr.IsNotFound(err) { - continue - } + if !canBeDeleted { + continue + } - return 0, fmt.Errorf("cannot list child resources %s: %w", resources[r].String(), err) + items, err := gc.listResources(ctx, res, lo) + if err != nil { + return 0, fmt.Errorf("cannot list child resources %s: %w", res.String(), err) } - for i := range items.Items { - ok, err := gc.delete(ctx, items.Items[i], predicate) + for i := range items { + canBeDeleted, err = ro.objectPredicate(ctx, items[i]) if err != nil { return 0, err } + if !canBeDeleted { + continue + } - if ok { - deleted++ + if err := gc.delete(ctx, items[i]); err != nil { + return 0, err } + + deleted++ } } @@ -151,21 +201,7 @@ func (gc *GC) Run( func (gc *GC) delete( ctx context.Context, resource unstructured.Unstructured, - predicate func(context.Context, unstructured.Unstructured) (bool, error), -) (bool, error) { - if slices.Contains(gc.options.unremovables, resource.GroupVersionKind()) { - return false, nil - } - - canBeDeleted, err := predicate(ctx, resource) - if err != nil { - return false, err - } - - if !canBeDeleted { - return false, err - } - +) error { gc.log(ctx).Info( "delete", "gvk", resource.GroupVersionKind(), @@ -173,14 +209,9 @@ func (gc *GC) delete( "name", resource.GetName(), ) - err = gc.client.Delete(ctx, &resource, gc.options.propagationPolicy) - if err != nil { - // The resource may have already been deleted - if k8serr.IsNotFound(err) { - return true, nil - } - - return false, fmt.Errorf( + err := gc.client.Delete(ctx, &resource, gc.options.propagationPolicy) + if err != nil && !k8serr.IsNotFound(err) { + return fmt.Errorf( "cannot delete resources gvk:%s, namespace: %s, name: %s, reason: %w", resource.GroupVersionKind().String(), resource.GetNamespace(), @@ -189,22 +220,31 @@ func (gc *GC) delete( ) } - return true, nil + return nil } -func (gc *GC) computeDeletableTypes( - ctx context.Context, -) ([]Resource, error) { - // We rely on the discovery API to retrieve all the resources GVK, - // that results in an unbounded set that can impact garbage collection - // latency when scaling up. - items, err := gc.client.Discovery().ServerPreferredNamespacedResources() +func (gc *GC) discoverResources() ([]*metav1.APIResourceList, error) { + // We rely on the discovery API to retrieve all the resources GVK, that + // results in an unbounded set that can impact garbage collection latency + // when scaling up. + items, err := gc.client.Discovery().ServerPreferredResources() - // Swallow group discovery errors, e.g., Knative serving exposes - // an aggregated API for custom.metrics.k8s.io that requires special + // Swallow group discovery errors, e.g., Knative serving exposes an + // aggregated API for custom.metrics.k8s.io that requires special // authentication scheme while discovering preferred resources. if err != nil && !discovery.IsGroupDiscoveryFailedError(err) { - return nil, fmt.Errorf("failure retrieving supported namespaced resources: %w", err) + return nil, fmt.Errorf("failure retrieving supported resources: %w", err) + } + + return items, nil +} + +func (gc *GC) computeDeletableTypes( + ctx context.Context, +) ([]Resource, error) { + items, err := gc.discoverResources() + if err != nil { + return nil, fmt.Errorf("failure discovering resources: %w", err) } // We only take types that support the "delete" verb, @@ -220,13 +260,13 @@ func (gc *GC) computeDeletableTypes( // Get the permissions of the service account in the specified namespace. rules, err := gc.retrieveResourceRules(ctx) if err != nil { - return nil, fmt.Errorf("failure retiring resource rules: %w", err) + return nil, fmt.Errorf("failure retrieving resource rules: %w", err) } // Collect deletable resources. resources, err := gc.collectDeletableResources(apiResourceLists, rules) if err != nil { - return nil, fmt.Errorf("failure retiring deletable resources: %w", err) + return nil, fmt.Errorf("failure retrieving deletable resources: %w", err) } return resources, nil @@ -320,7 +360,7 @@ func (gc *GC) collectDeletableResources( gvr.Scope = meta.RESTScopeRoot } - if slices.Contains(gc.options.unremovables, gvr.GroupVersionKind()) { + if _, ok := gc.options.unremovables[gvr.GroupVersionKind()]; ok { continue } diff --git a/pkg/services/gc/gc_support.go b/pkg/services/gc/gc_support.go index dc445b1452d2..a11bb8128ca5 100644 --- a/pkg/services/gc/gc_support.go +++ b/pkg/services/gc/gc_support.go @@ -2,7 +2,6 @@ package gc import ( "slices" - "sync" authorizationv1 "k8s.io/api/authorization/v1" "k8s.io/apimachinery/pkg/api/meta" @@ -37,20 +36,13 @@ func (r Resource) IsNamespaced() bool { // We may want to introduce iterators (https://pkg.go.dev/iter) once moved to go 1.23 type Resources struct { - lock sync.RWMutex items []Resource } func (r *Resources) Set(resources []Resource) { - r.lock.Lock() - defer r.lock.Unlock() - r.items = resources } func (r *Resources) Get() []Resource { - r.lock.RLock() - defer r.lock.RUnlock() - return slices.Clone(r.items) } From a9c1c57955331e5d7cc38adb105bfba09cffbb26 Mon Sep 17 00:00:00 2001 From: Wen Zhou Date: Thu, 13 Feb 2025 08:28:30 +0100 Subject: [PATCH 2/6] update: remove ondelete not used for cleanup steps (#1621) * update: remove ondelete not used for cleanup steps - we do not unpatch auth in smcp since we do not support unmanaged case --------- Signed-off-by: Wen Zhou --- .../dscinitialization/servicemesh_setup.go | 6 -- pkg/feature/servicemesh/cleanup.go | 68 ------------------- .../features/servicemesh_feature_test.go | 32 --------- 3 files changed, 106 deletions(-) delete mode 100644 pkg/feature/servicemesh/cleanup.go diff --git a/controllers/dscinitialization/servicemesh_setup.go b/controllers/dscinitialization/servicemesh_setup.go index ed3e5a9424b2..1008f685889a 100644 --- a/controllers/dscinitialization/servicemesh_setup.go +++ b/controllers/dscinitialization/servicemesh_setup.go @@ -205,12 +205,6 @@ func (r *DSCInitializationReconciler) authorizationFeatures(instance *dsciv1.DSC ). PostConditions( feature.WaitForPodsToBeReady(serviceMeshSpec.ControlPlane.Namespace), - ). - OnDelete( - servicemesh.RemoveExtensionProvider( - instance.Spec.ServiceMesh.ControlPlane, - instance.Spec.ApplicationsNamespace+"-auth-provider", - ), ), // We do not have the control over deployment resource creation. diff --git a/pkg/feature/servicemesh/cleanup.go b/pkg/feature/servicemesh/cleanup.go deleted file mode 100644 index a8c4a61dcd41..000000000000 --- a/pkg/feature/servicemesh/cleanup.go +++ /dev/null @@ -1,68 +0,0 @@ -package servicemesh - -import ( - "context" - - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/client-go/util/retry" - "sigs.k8s.io/controller-runtime/pkg/client" - - infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/infrastructure/v1" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" -) - -func RemoveExtensionProvider(controlPlane infrav1.ControlPlaneSpec, extensionName string) feature.CleanupFunc { - return func(ctx context.Context, cli client.Client) error { - smcp := &unstructured.Unstructured{} - smcp.SetGroupVersionKind(gvk.ServiceMeshControlPlane) - - if err := cli.Get(ctx, client.ObjectKey{ - Namespace: controlPlane.Namespace, - Name: controlPlane.Name, - }, smcp); err != nil { - return client.IgnoreNotFound(err) - } - - extensionProviders, found, err := unstructured.NestedSlice(smcp.Object, "spec", "techPreview", "meshConfig", "extensionProviders") - if err != nil { - return err - } - if !found { - return nil - } - - removed := false - - for i, v := range extensionProviders { - extensionProvider, ok := v.(map[string]interface{}) - if !ok { - continue - } - - currentExtensionName, isString := extensionProvider["name"].(string) - if !isString { - continue - } - if currentExtensionName == extensionName { - extensionProviders = append(extensionProviders[:i], extensionProviders[i+1:]...) - err = unstructured.SetNestedSlice(smcp.Object, extensionProviders, "spec", "techPreview", "meshConfig", "extensionProviders") - if err != nil { - return err - } - removed = true - break - } - } - - if removed { - // Update the ServiceMeshControlPlane with the removed extension provider. - // As it could have been updated by another controller in the meantime, we need to retry on conflict. - return retry.RetryOnConflict(retry.DefaultBackoff, func() error { - return cli.Update(ctx, smcp) - }) - } - - return nil - } -} diff --git a/tests/integration/features/servicemesh_feature_test.go b/tests/integration/features/servicemesh_feature_test.go index 6cb5ec5cbf2f..5fd39825e11c 100644 --- a/tests/integration/features/servicemesh_feature_test.go +++ b/tests/integration/features/servicemesh_feature_test.go @@ -6,7 +6,6 @@ import ( corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - k8serr "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/yaml" "sigs.k8s.io/controller-runtime/pkg/client" @@ -262,12 +261,6 @@ var _ = Describe("Service Mesh setup", func() { ). WithData( servicemesh.FeatureData.ControlPlane.Define(&dsci.Spec).AsAction(), - ). - OnDelete( - servicemesh.RemoveExtensionProvider( - dsci.Spec.ServiceMesh.ControlPlane, - dsci.Spec.ApplicationsNamespace+"-auth-provider", - ), )) }) @@ -293,31 +286,6 @@ var _ = Describe("Service Mesh setup", func() { } Expect(envoyExtAuthzGrpc["service"]).To(Equal("authorino-authorino-authorization.auth-provider.svc.cluster.local")) }) - - // then - By("verifying that extension provider has been removed and namespace is gone too", func() { - Expect(handler.Delete(ctx, envTestClient)).To(Succeed()) - Eventually(func(ctx context.Context) []any { - - serviceMeshControlPlane, err := getServiceMeshControlPlane(ctx, namespace, name) - Expect(err).ToNot(HaveOccurred()) - - extensionProviders, found, err := unstructured.NestedSlice(serviceMeshControlPlane.Object, "spec", "techPreview", "meshConfig", "extensionProviders") - Expect(err).ToNot(HaveOccurred()) - Expect(found).To(BeTrue()) - - _, err = fixtures.GetNamespace(ctx, envTestClient, serviceMeshSpec.Auth.Namespace) - Expect(k8serr.IsNotFound(err)).To(BeTrue()) - - return extensionProviders - - }). - WithTimeout(fixtures.Timeout). - WithPolling(fixtures.Interval). - WithContext(ctx). - Should(BeEmpty()) - }) - }) }) From 7e467587a962ee83dec0d5d8bae64051c13e213d Mon Sep 17 00:00:00 2001 From: Yauheni Kaliuta Date: Thu, 13 Feb 2025 12:42:10 +0200 Subject: [PATCH 3/6] actions: refactor to move template and manifest rendering to a helper (#1563) * actions: abstract caching functionality into cacher The caching code for all caching Actions is pretty much the same 1) hash the key 2) if the hash is the same as the stored one, return stored resource 3) otherwise render the resource 4) store the key and the resource 5) return the resource Abstract this logic into Cacher[T] generic type, where T is the type of the resource, which is supposed to be included as a composition in on the higher levels. The core engine is implemented as Render method which accepts actual renderer (function) and return boolean value in addition to the resources indicating if it performed rendering. It supposed to be used in resource rendering actions for accounting. The renderer should return (error, T). Put renderer as the last argument to make more convenient to call with a closure. Copy CachingKeyFn type. In future it can be extented to accept context for possible debug logging. Define contract, CachingKeyFn should not return empty hash. (!!!) The code originally is taken pretty much from the existing kustomize rendering Action. But taking into account the contract, refactor the code to avoid comparation of the result (and nil assignments) since with generics to work with both nilable (maps, arrays, etc) and non-nilable (ints, strings) types it requires some extra handling of Zero values. Zero values are still required for return values in error case, so implement Zero[T] generic function. With the assumption make "empty key" a error condition, it removes need of extra comparation. Move actual rendering and cache update into separate function, it makes main Render function more clear without extra indentation. Possible usage supposed to be ``` type Action struct { cacher.Cacher[resources.UnstructuredList] // action data } type ActionOpts func(*Action) func WithCache() ActionOpts { return func(a *Action) { a.Cacher.SetKeyFn(types.Hash) // or own } } func (s *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error { //... res, acted, err := a.Cacher.Render(ctx, rr, s.render) if acted { /* may be do something */ } // use resources } func (s *Action) render(ctx context.Context, rr *types.ReconciliationRequest) (resources.UnstructuredList, error) { result := make(resources.UnstructuredList) // make some resources return result, nil } func NewAction(opts ...ActionOpts) actions.Fn { for _, opt := range opts { opt(&action) } return action.run } ``` with reconciler creation ``` func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { _, err := reconciler.ReconcilerFor( mgr, &componentApi.Type{}, ). // ... WithAction(foobar.NewAction( foobar.WithCache(), )) } ``` Pay attention to signature of render() and call to Cacher.Render() from run(). Signed-off-by: Yauheni Kaliuta * actions: abstract resource caching into resourcecacher Make an abstract wrapper on the raw cacher.Cacher[T] to work on actions whose Renderer produces resources (resources.UnstructuredList) to be recorded into ReconciliationRequest.Manifests and later deployed (deleted) to the cluster. Performs common tasks: - invalidate caching if devflags enabled; - account resources if they were generated (and skip cached ones); - add resources to the ReconciliationRequest. Created constructor which accepts mandatory name. Since it is used in prometheus, it's recommended to make it lower case. At this moment this is the only part which interested in the origin of resources (cached or not) and it's supposed to be used in actions whose final goal is to add resources to the request (which is done by this helper), so change signature of its Render() method (comparing to cacher.Cacher) to return only error. The code is basically copied from manifest/template rendering actions run() method. Usage (changed run() comparing to cacher.Cacher): ``` type Action struct { resourcecacher.ResourceCacher // action data } type ActionOpts func(*Action) func WithCache() ActionOpts { return func(a *Action) { a.ResourceCacher.SetKeyFn(types.Hash) // or own } } func (s *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error { //... return a.ResourceCacher.Render(ctx, rr, s.render) } func (s *Action) render(ctx context.Context, rr *types.ReconciliationRequest) (resources.UnstructuredList, error) { result := make(resources.UnstructuredList) // make some resources return result, nil } func NewAction(opts ...ActionOpts) actions.Fn { action := Action{ ResourceCacher: resourcecacher.NewResourceCacher("mycoolaction"), } for _, opt := range opts { opt(&action) } return action.run } ``` Signed-off-by: Yauheni Kaliuta * actions: use resourcecacher for manifests rendering All the run() functionality now implemented by the ResourceCacher, so just call its Render() in run(). Amend WithCache() as required by ResourceCacher to set its CachingKeyFn and remove CachingKeyFn initialization from NewAction. Change render() signature to return resources.UnstructuredList RendererEngine is used as a field initializer now, so no need to export, rename to rendererEngine. Signed-off-by: Yauheni Kaliuta * actions: use resourcecacher for template rendering The same changes as to manifests rendering. Just keep `data` field initialization in NewAction, used in render(). Signed-off-by: Yauheni Kaliuta --------- Signed-off-by: Yauheni Kaliuta --- go.mod | 1 + pkg/controller/actions/cacher/cacher.go | 86 +++++++ pkg/controller/actions/cacher/cacher_test.go | 201 +++++++++++++++ .../kustomize/action_render_manifests.go | 71 +----- .../template/action_render_templates.go | 72 +----- .../actions/resourcecacher/resourcecacher.go | 65 +++++ .../resourcecacher/resourcecacher_test.go | 235 ++++++++++++++++++ 7 files changed, 610 insertions(+), 121 deletions(-) create mode 100644 pkg/controller/actions/cacher/cacher.go create mode 100644 pkg/controller/actions/cacher/cacher_test.go create mode 100644 pkg/controller/actions/resourcecacher/resourcecacher.go create mode 100644 pkg/controller/actions/resourcecacher/resourcecacher_test.go diff --git a/go.mod b/go.mod index b0de9b689cfc..a2024dc7d336 100644 --- a/go.mod +++ b/go.mod @@ -80,6 +80,7 @@ require ( github.com/rhobs/obo-prometheus-operator/pkg/apis/monitoring v0.61.1-rhobs1 // indirect github.com/sirupsen/logrus v1.9.2 // indirect github.com/spf13/pflag v1.0.5 // indirect + github.com/stretchr/objx v0.5.2 // indirect github.com/xlab/treeprint v1.2.0 // indirect go.starlark.net v0.0.0-20200306205701-8dd3e2ee1dd5 // indirect go.uber.org/multierr v1.11.0 // indirect diff --git a/pkg/controller/actions/cacher/cacher.go b/pkg/controller/actions/cacher/cacher.go new file mode 100644 index 000000000000..c1da9d4adf48 --- /dev/null +++ b/pkg/controller/actions/cacher/cacher.go @@ -0,0 +1,86 @@ +package cacher + +import ( + "bytes" + "context" + "errors" + "fmt" + + logf "sigs.k8s.io/controller-runtime/pkg/log" + + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" +) + +type CachingKeyFn func(rr *types.ReconciliationRequest) ([]byte, error) + +type Cacher[T any] struct { + cachingKeyFn CachingKeyFn + cachingKey []byte + cachedResources T +} + +func Zero[T any]() T { + return *new(T) +} + +// SetKeyFn installs the function which calculates hash of resource sources, +// taking ReconciliationRequest as the argument and returning []byte +// The returned hash MUST NOT be empty by the contract. +func (s *Cacher[T]) SetKeyFn(key CachingKeyFn) { + s.cachingKeyFn = key +} + +// InvalidateCache invalidates both caching key and cached resources +// what forces Render() to regenerate them. +func (s *Cacher[T]) InvalidateCache() { + s.cachingKey = nil +} + +func (s *Cacher[T]) reRender(ctx context.Context, cachingKey []byte, rr *types.ReconciliationRequest, + r func(ctx context.Context, rr *types.ReconciliationRequest) (T, error)) (T, bool, error) { + var err error + log := logf.FromContext(ctx) + + log.V(4).Info("cache is not valid, rendering resources") + res, err := r(ctx, rr) + if err != nil { + return Zero[T](), false, err + } + + // does not matter at this point if it's empty or not, because the next run: + // if the keyFn is nil, anyway rerender + // if the keyFn returns empty, it's a error + // otherwise comparing non-empty new key with empty will cause rerender. + s.cachingKey = cachingKey + s.cachedResources = res + + return res, true, nil +} + +func (s *Cacher[T]) Render(ctx context.Context, rr *types.ReconciliationRequest, + r func(ctx context.Context, rr *types.ReconciliationRequest) (T, error)) (T, bool, error) { + var err error + var cachingKey []byte + log := logf.FromContext(ctx) + + if s.cachingKeyFn == nil { + return s.reRender(ctx, nil, rr, r) + } + + cachingKey, err = s.cachingKeyFn(rr) + if err != nil { + return Zero[T](), false, fmt.Errorf("unable to calculate caching key: %w", err) + } + // Contract + if len(cachingKey) == 0 { + return Zero[T](), false, errors.New("calculated empty hash") + } + + // s.cachingKey can be empty (invalidated) causing cache rebuild + if bytes.Equal(cachingKey, s.cachingKey) { + log.V(4).Info("using cached resources") + return s.cachedResources, false, nil + } + + return s.reRender(ctx, cachingKey, rr, r) +} diff --git a/pkg/controller/actions/cacher/cacher_test.go b/pkg/controller/actions/cacher/cacher_test.go new file mode 100644 index 000000000000..d42dfb6721cf --- /dev/null +++ b/pkg/controller/actions/cacher/cacher_test.go @@ -0,0 +1,201 @@ +package cacher_test + +import ( + "context" + "errors" + "testing" + + "github.com/rs/xid" + "github.com/stretchr/testify/mock" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/cacher" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" + + . "github.com/onsi/gomega" +) + +type testCacher struct { + mock.Mock + cacher *cacher.Cacher[resources.UnstructuredList] + rr *types.ReconciliationRequest + ctx context.Context //nolint:containedctx + r resources.UnstructuredList +} + +func newHash() []byte { + return xid.New().Bytes() +} + +func newResources() []unstructured.Unstructured { + return []unstructured.Unstructured{ + { + Object: map[string]any{ + xid.New().String(): xid.New().String(), + }, + }, + } +} + +func newTestCacher() *testCacher { + c := &testCacher{ + ctx: context.Background(), + r: newResources(), + rr: &types.ReconciliationRequest{}, + } + c.cacher = newCacher(c.hash) + return c +} + +func (s *testCacher) hash(rr *types.ReconciliationRequest) ([]byte, error) { + args := s.Called(rr) + return args.Get(0).([]byte), args.Error(1) //nolint:errcheck,forcetypeassert +} + +func (s *testCacher) render(ctx context.Context, rr *types.ReconciliationRequest) (resources.UnstructuredList, error) { + args := s.Called(ctx, rr) + return args.Get(0).(resources.UnstructuredList), args.Error(1) //nolint:errcheck,forcetypeassert +} + +func newCacher(key cacher.CachingKeyFn) *cacher.Cacher[resources.UnstructuredList] { + cacher := &cacher.Cacher[resources.UnstructuredList]{} + cacher.SetKeyFn(key) + return cacher +} +func TestCacherShouldRenderFirstRun(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() // mock + + m.On("hash", m.rr).Return(newHash(), nil).Once() + m.On("render", m.ctx, m.rr).Return(m.r, nil).Once() + + res, acted, err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(res).Should(Equal(m.r)) + g.Expect(acted).Should(BeTrue()) + + m.AssertExpectations(t) +} +func TestCacherShouldNotRenderSecondRun(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + m.On("hash", m.rr).Return(newHash(), nil).Twice() + m.On("render", m.ctx, m.rr).Return(m.r, nil).Once() + + m.cacher.Render(m.ctx, m.rr, m.render) //nolint:errcheck + res, acted, err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(res).Should(Equal(m.r)) + g.Expect(acted).Should(BeFalse()) + + m.AssertExpectations(t) +} + +func TestCacherShouldRenderDifferentKey(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + m. + On("hash", m.rr).Return(newHash(), nil).Once(). + On("hash", m.rr).Return(newHash(), nil).Once() + m.On("render", m.ctx, m.rr).Return(m.r, nil).Twice() + + m.cacher.Render(m.ctx, m.rr, m.render) //nolint:errcheck + res, acted, err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(res).Should(Equal(m.r)) + g.Expect(acted).Should(BeTrue()) + + m.AssertExpectations(t) +} + +func TestCacherShouldRenderAfterInvalidation(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + m.On("hash", m.rr).Return(newHash(), nil).Twice() + m.On("render", m.ctx, m.rr).Return(m.r, nil).Twice() + + m.cacher.Render(m.ctx, m.rr, m.render) //nolint:errcheck + m.cacher.InvalidateCache() + res, acted, err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(res).Should(Equal(m.r)) + g.Expect(acted).Should(BeTrue()) + + m.AssertExpectations(t) +} + +func TestCacherShouldRenderIfKeyUnset(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + m.cacher.SetKeyFn(nil) + + m.On("render", m.ctx, m.rr).Return(m.r, nil).Twice() + + m.cacher.Render(m.ctx, m.rr, m.render) //nolint:errcheck + res, acted, err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(res).Should(Equal(m.r)) + g.Expect(acted).Should(BeTrue()) + + m.AssertExpectations(t) +} + +func TestCacherShouldErrorIfKeyError(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + keyErr := errors.New("hashing error") + + m.On("hash", m.rr).Return(newHash(), keyErr).Once() + + res, acted, err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).Should(HaveOccurred()) + g.Expect(res).Should(BeNil()) + g.Expect(acted).Should(BeFalse()) + + m.AssertExpectations(t) +} + +func TestCacherShouldErrorIfRenderError(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + renderErr := errors.New("hashing error") + + m.On("hash", m.rr).Return(newHash(), nil).Once() + m.On("render", m.ctx, m.rr).Return(cacher.Zero[resources.UnstructuredList](), renderErr).Once() + + res, acted, err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).Should(HaveOccurred()) + g.Expect(res).Should(BeNil()) + g.Expect(acted).Should(BeFalse()) + + m.AssertExpectations(t) +} + +func TestCacherShouldErrorIfKeyEmpty(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + m.On("hash", m.rr).Return([]byte{}, nil).Once() + + res, acted, err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).Should(HaveOccurred()) + g.Expect(res).Should(BeNil()) + g.Expect(acted).Should(BeFalse()) + + m.AssertExpectations(t) +} diff --git a/pkg/controller/actions/render/kustomize/action_render_manifests.go b/pkg/controller/actions/render/kustomize/action_render_manifests.go index b6007aa53254..30f44eb33795 100644 --- a/pkg/controller/actions/render/kustomize/action_render_manifests.go +++ b/pkg/controller/actions/render/kustomize/action_render_manifests.go @@ -1,34 +1,27 @@ package kustomize import ( - "bytes" "context" - "fmt" - "strings" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/kustomize/kyaml/filesys" - "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/resourcecacher" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/manifests/kustomize" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) -const RendererEngine = "kustomize" +const rendererEngine = "kustomize" // Action takes a set of manifest locations and render them as Unstructured resources for // further processing. The Action can eventually cache the results in memory to avoid doing // a full manifest rendering when not needed. type Action struct { + resourcecacher.ResourceCacher + keOpts []kustomize.EngineOptsFn ke *kustomize.Engine - - cachingKeyFn render.CachingKeyFn - cachingKey []byte - cachedResources resources.UnstructuredList } type ActionOpts func(*Action) @@ -71,58 +64,16 @@ func WithManifestsOptions(values ...kustomize.EngineOptsFn) ActionOpts { func WithCache() ActionOpts { return func(action *Action) { - action.cachingKeyFn = types.Hash + action.ResourceCacher.SetKeyFn(types.Hash) } } -func (a *Action) run(_ context.Context, rr *types.ReconciliationRequest) error { - var err error - var cachingKey []byte - - inst, ok := rr.Instance.(common.WithDevFlags) - if ok && inst.GetDevFlags() != nil { - // if dev flags are enabled, caching is disabled as dev flags are meant for - // development time only where caching is not relevant - a.cachingKey = nil - } else { - cachingKey, err = a.cachingKeyFn(rr) - if err != nil { - return fmt.Errorf("unable to calculate checksum of reconciliation object: %w", err) - } - } - - var result resources.UnstructuredList - - if len(cachingKey) != 0 && bytes.Equal(cachingKey, a.cachingKey) && len(a.cachedResources) != 0 { - result = a.cachedResources - } else { - res, err := a.render(rr) - if err != nil { - return fmt.Errorf("unable to render reconciliation object: %w", err) - } - - result = res - - if len(cachingKey) != 0 { - a.cachingKey = cachingKey - a.cachedResources = result - } - - controllerName := strings.ToLower(rr.Instance.GetObjectKind().GroupVersionKind().Kind) - render.RenderedResourcesTotal.WithLabelValues(controllerName, RendererEngine).Add(float64(len(result))) - - rr.Generated = true - } - - // deep copy object so changes done in the pipelines won't - // alter them - rr.Resources = append(rr.Resources, result.Clone()...) - - return nil +func (a *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error { + return a.ResourceCacher.Render(ctx, rr, a.render) } -func (a *Action) render(rr *types.ReconciliationRequest) ([]unstructured.Unstructured, error) { - result := make([]unstructured.Unstructured, 0) +func (a *Action) render(ctx context.Context, rr *types.ReconciliationRequest) (resources.UnstructuredList, error) { + result := make(resources.UnstructuredList, 0) for i := range rr.Manifests { renderedResources, err := a.ke.Render( @@ -142,9 +93,7 @@ func (a *Action) render(rr *types.ReconciliationRequest) ([]unstructured.Unstruc func NewAction(opts ...ActionOpts) actions.Fn { action := Action{ - cachingKeyFn: func(rr *types.ReconciliationRequest) ([]byte, error) { - return nil, nil - }, + ResourceCacher: resourcecacher.NewResourceCacher(rendererEngine), } for _, opt := range opts { diff --git a/pkg/controller/actions/render/template/action_render_templates.go b/pkg/controller/actions/render/template/action_render_templates.go index 51c276007dbf..99cc1fb03347 100644 --- a/pkg/controller/actions/render/template/action_render_templates.go +++ b/pkg/controller/actions/render/template/action_render_templates.go @@ -6,21 +6,18 @@ import ( "fmt" "io/fs" "maps" - "strings" gt "text/template" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/serializer" - "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/resourcecacher" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) const ( - RendererEngine = "template" + rendererEngine = "template" ComponentKey = "Component" DSCIKey = "DSCI" ) @@ -29,18 +26,17 @@ const ( // further processing. The Action can eventually cache the results in memory to avoid doing // a full manifest rendering when not needed. type Action struct { - cachingKeyFn render.CachingKeyFn - cachingKey []byte - cachedResources resources.UnstructuredList - data map[string]any - dataFn []func(context.Context, *types.ReconciliationRequest) (map[string]any, error) + resourcecacher.ResourceCacher + + data map[string]any + dataFn []func(context.Context, *types.ReconciliationRequest) (map[string]any, error) } type ActionOpts func(*Action) func WithCache() ActionOpts { return func(action *Action) { - action.cachingKeyFn = types.Hash + action.ResourceCacher.SetKeyFn(types.Hash) } } @@ -59,52 +55,10 @@ func WithDataFn(fns ...func(context.Context, *types.ReconciliationRequest) (map[ } func (a *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error { - var err error - var cachingKey []byte - - inst, ok := rr.Instance.(common.WithDevFlags) - if ok && inst.GetDevFlags() != nil { - // if dev flags are enabled, caching is disabled as dev flags are meant for - // development time only where caching is not relevant - a.cachingKey = nil - } else { - cachingKey, err = a.cachingKeyFn(rr) - if err != nil { - return fmt.Errorf("unable to calculate checksum of reconciliation object: %w", err) - } - } - - var result resources.UnstructuredList - - if len(cachingKey) != 0 && bytes.Equal(cachingKey, a.cachingKey) && len(a.cachedResources) != 0 { - result = a.cachedResources - } else { - res, err := a.render(ctx, rr) - if err != nil { - return fmt.Errorf("unable to render reconciliation object: %w", err) - } - - result = res - - if len(cachingKey) != 0 { - a.cachingKey = cachingKey - a.cachedResources = result - } - - controllerName := strings.ToLower(rr.Instance.GetObjectKind().GroupVersionKind().Kind) - render.RenderedResourcesTotal.WithLabelValues(controllerName, RendererEngine).Add(float64(len(result))) - - rr.Generated = true - } - - // deep copy object so changes done in the pipelines won't - // alter them - rr.Resources = append(rr.Resources, result.Clone()...) - - return nil + return a.ResourceCacher.Render(ctx, rr, a.render) } -func (a *Action) render(ctx context.Context, rr *types.ReconciliationRequest) ([]unstructured.Unstructured, error) { +func (a *Action) render(ctx context.Context, rr *types.ReconciliationRequest) (resources.UnstructuredList, error) { decoder := serializer.NewCodecFactory(rr.Client.Scheme()).UniversalDeserializer() data := maps.Clone(a.data) @@ -121,7 +75,7 @@ func (a *Action) render(ctx context.Context, rr *types.ReconciliationRequest) ([ data[ComponentKey] = rr.Instance data[DSCIKey] = rr.DSCI - result := make([]unstructured.Unstructured, 0) + result := make(resources.UnstructuredList, 0) var buffer bytes.Buffer @@ -158,10 +112,8 @@ func (a *Action) render(ctx context.Context, rr *types.ReconciliationRequest) ([ func NewAction(opts ...ActionOpts) actions.Fn { action := Action{ - cachingKeyFn: func(rr *types.ReconciliationRequest) ([]byte, error) { - return nil, nil - }, - data: make(map[string]any), + data: make(map[string]any), + ResourceCacher: resourcecacher.NewResourceCacher(rendererEngine), } for _, opt := range opts { diff --git a/pkg/controller/actions/resourcecacher/resourcecacher.go b/pkg/controller/actions/resourcecacher/resourcecacher.go new file mode 100644 index 000000000000..71245ddaf0e9 --- /dev/null +++ b/pkg/controller/actions/resourcecacher/resourcecacher.go @@ -0,0 +1,65 @@ +package resourcecacher + +import ( + "context" + "strings" + + logf "sigs.k8s.io/controller-runtime/pkg/log" + + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/cacher" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" +) + +// Renderer is the actual function received from upper layers +// which generates resources based on the ReconciliationRequest. +type Renderer func(ctx context.Context, rr *types.ReconciliationRequest) (resources.UnstructuredList, error) +type ResourceCacher struct { + cacher.Cacher[resources.UnstructuredList] + name string +} + +func (s *ResourceCacher) SetKeyFn(key cacher.CachingKeyFn) { + s.Cacher.SetKeyFn(key) +} + +func (s *ResourceCacher) Render(ctx context.Context, rr *types.ReconciliationRequest, r Renderer) error { + log := logf.FromContext(ctx) + inst, ok := rr.Instance.(common.WithDevFlags) + if ok && inst.GetDevFlags() != nil { + // if dev flags are enabled, caching is disabled as dev flags are meant for + // development time only where caching is not relevant + log.V(4).Info("devFlags enabled, invalidating resource cache") + s.InvalidateCache() + } + + res, acted, err := s.Cacher.Render(ctx, rr, r) + if err != nil { + return err + } + + resLen := len(res) + + if acted { + log.V(4).Info("accounted rendered resources", "count", resLen) + + controllerName := strings.ToLower(rr.Instance.GetObjectKind().GroupVersionKind().Kind) + render.RenderedResourcesTotal.WithLabelValues(controllerName, s.name).Add(float64(resLen)) + + // flag new resources, used by GC to avoid useless run + rr.Generated = true + } + + // deep copy object so changes done in the pipelines won't + // alter them + rr.Resources = append(rr.Resources, res.Clone()...) + log.V(4).Info("added resources to the request", "count", resLen) + + return nil +} + +func NewResourceCacher(name string) ResourceCacher { + return ResourceCacher{name: name} +} diff --git a/pkg/controller/actions/resourcecacher/resourcecacher_test.go b/pkg/controller/actions/resourcecacher/resourcecacher_test.go new file mode 100644 index 000000000000..bc757177db2b --- /dev/null +++ b/pkg/controller/actions/resourcecacher/resourcecacher_test.go @@ -0,0 +1,235 @@ +package resourcecacher_test + +import ( + "context" + "errors" + "testing" + + "github.com/rs/xid" + "github.com/stretchr/testify/mock" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" + componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/cacher" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/resourcecacher" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" + + . "github.com/onsi/gomega" +) + +type testCacher struct { + mock.Mock + cacher *resourcecacher.ResourceCacher + rr *types.ReconciliationRequest + ctx context.Context //nolint:containedctx + r resources.UnstructuredList + doubleRes resources.UnstructuredList +} + +func newHash() []byte { + return xid.New().Bytes() +} + +func newResources() []unstructured.Unstructured { + return []unstructured.Unstructured{ + { + Object: map[string]any{ + xid.New().String(): xid.New().String(), + }, + }, + } +} + +func newTestCacher() *testCacher { + c := &testCacher{ + ctx: context.Background(), + r: newResources(), + rr: &types.ReconciliationRequest{}, + } + c.cacher = newCacher(c.hash) + c.rr.Instance = &componentApi.Dashboard{} + c.doubleRes = append(c.doubleRes, c.r[0], c.r[0]) + + return c +} + +func (s *testCacher) resetGenerated() { + s.rr.Generated = false +} + +func (s *testCacher) setResources(r []unstructured.Unstructured) { + s.rr.Resources = r +} + +func (s *testCacher) setDevFlags(f *common.DevFlags) { + d := s.rr.Instance.(*componentApi.Dashboard) //nolint:errcheck,forcetypeassert + d.Spec.DevFlags = f +} + +func (s *testCacher) GetDevFlags() common.DevFlags { + args := s.Called() + return args.Get(0).(common.DevFlags) //nolint:errcheck,forcetypeassert +} + +func (s *testCacher) hash(rr *types.ReconciliationRequest) ([]byte, error) { + args := s.Called(rr) + return args.Get(0).([]byte), args.Error(1) //nolint:errcheck,forcetypeassert +} + +func (s *testCacher) render(ctx context.Context, rr *types.ReconciliationRequest) (resources.UnstructuredList, error) { + args := s.Called(ctx, rr) + return args.Get(0).(resources.UnstructuredList), args.Error(1) //nolint:errcheck,forcetypeassert +} + +func newCacher(key cacher.CachingKeyFn) *resourcecacher.ResourceCacher { + cacher := &resourcecacher.ResourceCacher{} + cacher.SetKeyFn(key) + return cacher +} +func TestCacherShouldRenderFirstRun(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() // mock + + m.On("hash", m.rr).Return(newHash(), nil).Once() + m.On("render", m.ctx, m.rr).Return(m.r, nil).Once() + + err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(m.rr.Resources).Should(BeEquivalentTo(m.r)) + g.Expect(m.rr.Generated).Should(BeTrue()) + + m.AssertExpectations(t) +} +func TestCacherShouldNotRenderSecondRun(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + m.On("hash", m.rr).Return(newHash(), nil).Twice() + m.On("render", m.ctx, m.rr).Return(m.r, nil).Once() + + _ = m.cacher.Render(m.ctx, m.rr, m.render) + m.resetGenerated() + err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(m.rr.Resources).Should(BeEquivalentTo(m.doubleRes)) + g.Expect(m.rr.Generated).Should(BeFalse()) + + m.AssertExpectations(t) +} + +func TestCacherShouldRenderDifferentKey(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + m. + On("hash", m.rr).Return(newHash(), nil).Once(). + On("hash", m.rr).Return(newHash(), nil).Once() + m.On("render", m.ctx, m.rr).Return(m.r, nil).Twice() + + _ = m.cacher.Render(m.ctx, m.rr, m.render) + m.resetGenerated() + err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(m.rr.Resources).Should(BeEquivalentTo(m.doubleRes)) + g.Expect(m.rr.Generated).Should(BeTrue()) + + m.AssertExpectations(t) +} + +func TestCacherShouldRenderIfKeyUnset(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + m.cacher.SetKeyFn(nil) + + m.On("render", m.ctx, m.rr).Return(m.r, nil).Twice() + + _ = m.cacher.Render(m.ctx, m.rr, m.render) + m.resetGenerated() + err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(m.rr.Resources).Should(BeEquivalentTo(m.doubleRes)) + g.Expect(m.rr.Generated).Should(BeTrue()) + + m.AssertExpectations(t) +} + +func TestCacherShouldErrorIfKeyError(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + keyErr := errors.New("hashing error") + + m.On("hash", m.rr).Return(newHash(), keyErr).Once() + + err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).Should(HaveOccurred()) + g.Expect(m.rr.Resources).Should(BeEmpty()) + g.Expect(m.rr.Generated).Should(BeFalse()) + + m.AssertExpectations(t) +} + +func TestCacherShouldErrorIfRenderError(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + renderErr := errors.New("hashing error") + + m.On("hash", m.rr).Return(newHash(), nil).Once() + m.On("render", m.ctx, m.rr).Return(cacher.Zero[resources.UnstructuredList](), renderErr).Once() + + err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).Should(HaveOccurred()) + g.Expect(m.rr.Resources).Should(BeEmpty()) + g.Expect(m.rr.Generated).Should(BeFalse()) + + m.AssertExpectations(t) +} + +func TestCacherShouldRenderIfDevFlags(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + devFlags := common.DevFlags{} + + m.On("hash", m.rr).Return(newHash(), nil).Twice() + m.On("render", m.ctx, m.rr).Return(m.r, nil).Twice() + + m.setDevFlags(&devFlags) + _ = m.cacher.Render(m.ctx, m.rr, m.render) + m.resetGenerated() + err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(m.rr.Resources).Should(BeEquivalentTo(m.doubleRes)) + g.Expect(m.rr.Generated).Should(BeTrue()) + + m.AssertExpectations(t) +} +func TestCacherShouldAddResources(t *testing.T) { + g := NewWithT(t) + m := newTestCacher() + + orig := append(newResources(), newResources()...) + m.setResources(orig) + expected := append([]unstructured.Unstructured{}, orig...) + expected = append(expected, m.r...) + + m.On("hash", m.rr).Return(newHash(), nil).Once() + m.On("render", m.ctx, m.rr).Return(m.r, nil).Once() + + err := m.cacher.Render(m.ctx, m.rr, m.render) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(m.rr.Resources).Should(BeEquivalentTo(expected)) + + m.AssertExpectations(t) +} From fd104d48b0662581de3202efc27827554a929445 Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Wed, 29 Jan 2025 18:57:55 +0100 Subject: [PATCH 4/6] chore: move Release strcuts from pkg/cluster to apis/common (#1580) Structs that are part of the public APIs should be in the apis packages for consistency and to avoid cyclic import issues (cherry picked from commit 1760dee4e85485a93b6322246374b2cf15be1bd0) --- apis/common/types.go | 10 +++++ apis/common/zz_generated.deepcopy.go | 16 ++++++++ .../v1/datasciencecluster_types.go | 4 +- .../v1/dscinitialization_types.go | 4 +- controllers/components/codeflare/codeflare.go | 3 +- controllers/components/dashboard/dashboard.go | 3 +- .../components/dashboard/dashboard_support.go | 13 ++++--- .../datasciencepipelines.go | 2 +- .../datasciencepipelines_support.go | 5 ++- controllers/components/kserve/kserve.go | 3 +- controllers/components/kueue/kueue.go | 3 +- .../modelcontroller/modelcontroller.go | 3 +- .../modelmeshserving/modelmeshserving.go | 3 +- .../components/modelregistry/modelregistry.go | 3 +- controllers/components/ray/ray.go | 3 +- .../trainingoperator/trainingoperator.go | 3 +- controllers/components/trustyai/trustyai.go | 3 +- .../components/trustyai/trustyai_support.go | 5 ++- .../components/workbenches/workbenches.go | 3 +- controllers/dscinitialization/utils.go | 9 +++-- main.go | 9 +++-- pkg/cluster/cluster_config.go | 26 +++++-------- pkg/cluster/const.go | 10 +++-- pkg/cluster/zz_generated.deepcopy.go | 39 ------------------- pkg/componentsregistry/componentsregistry.go | 3 +- .../action_delete_resources_test.go | 3 +- .../deploy/action_deploy_cache_test.go | 5 ++- .../actions/deploy/action_deploy_test.go | 13 ++++--- pkg/controller/actions/gc/action_gc_test.go | 3 +- .../kustomize/action_render_manifests_test.go | 5 ++- .../template/action_render_templates_test.go | 9 +++-- .../updatestatus/action_update_status_test.go | 9 +++-- pkg/controller/reconciler/reconciler.go | 4 +- pkg/controller/types/types.go | 3 +- pkg/upgrade/uninstallation.go | 3 +- pkg/upgrade/upgrade.go | 22 +++++------ tests/e2e/controller_test.go | 3 +- 37 files changed, 126 insertions(+), 144 deletions(-) delete mode 100644 pkg/cluster/zz_generated.deepcopy.go diff --git a/apis/common/types.go b/apis/common/types.go index 44542d158957..473f288e0c85 100644 --- a/apis/common/types.go +++ b/apis/common/types.go @@ -2,6 +2,7 @@ package common import ( operatorv1 "github.com/openshift/api/operator/v1" + "github.com/operator-framework/api/pkg/lib/version" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -82,3 +83,12 @@ type PlatformObject interface { client.Object WithStatus } + +type Platform string + +// Release includes information on operator version and platform +// +kubebuilder:object:generate=true +type Release struct { + Name Platform `json:"name,omitempty"` + Version version.OperatorVersion `json:"version,omitempty"` +} diff --git a/apis/common/zz_generated.deepcopy.go b/apis/common/zz_generated.deepcopy.go index b55b115b3541..6c981a98ca36 100644 --- a/apis/common/zz_generated.deepcopy.go +++ b/apis/common/zz_generated.deepcopy.go @@ -79,6 +79,22 @@ func (in *ManagementSpec) DeepCopy() *ManagementSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Release) DeepCopyInto(out *Release) { + *out = *in + in.Version.DeepCopyInto(&out.Version) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Release. +func (in *Release) DeepCopy() *Release { + if in == nil { + return nil + } + out := new(Release) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Status) DeepCopyInto(out *Status) { *out = *in diff --git a/apis/datasciencecluster/v1/datasciencecluster_types.go b/apis/datasciencecluster/v1/datasciencecluster_types.go index fd5956758a2f..18bb445f0d27 100644 --- a/apis/datasciencecluster/v1/datasciencecluster_types.go +++ b/apis/datasciencecluster/v1/datasciencecluster_types.go @@ -17,12 +17,12 @@ limitations under the License. package v1 import ( + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" ) // DataScienceClusterSpec defines the desired state of the cluster. @@ -134,7 +134,7 @@ type DataScienceClusterStatus struct { Components ComponentsStatus `json:"components"` // Version and release type - Release cluster.Release `json:"release,omitempty"` + Release common.Release `json:"release,omitempty"` } // +kubebuilder:object:root=true diff --git a/apis/dscinitialization/v1/dscinitialization_types.go b/apis/dscinitialization/v1/dscinitialization_types.go index 9e3e1bbb7a45..762f9ebdfd6f 100644 --- a/apis/dscinitialization/v1/dscinitialization_types.go +++ b/apis/dscinitialization/v1/dscinitialization_types.go @@ -17,6 +17,7 @@ limitations under the License. package v1 import ( + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" serviceApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/services/v1alpha1" operatorv1 "github.com/openshift/api/operator/v1" conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" @@ -24,7 +25,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/infrastructure/v1" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" ) // +operator-sdk:csv:customresourcedefinitions:order=1 @@ -109,7 +109,7 @@ type DSCInitializationStatus struct { ErrorMessage string `json:"errorMessage,omitempty"` // Version and release type - Release cluster.Release `json:"release,omitempty"` + Release common.Release `json:"release,omitempty"` } //+kubebuilder:object:root=true diff --git a/controllers/components/codeflare/codeflare.go b/controllers/components/codeflare/codeflare.go index 0920a8b45b17..1406097be72a 100644 --- a/controllers/components/codeflare/codeflare.go +++ b/controllers/components/codeflare/codeflare.go @@ -15,7 +15,6 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" cr "github.com/opendatahub-io/opendatahub-operator/v2/pkg/componentsregistry" odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" @@ -56,7 +55,7 @@ func (s *componentHandler) NewCRObject(dsc *dscv1.DataScienceCluster) common.Pla } } -func (s *componentHandler) Init(_ cluster.Platform) error { +func (s *componentHandler) Init(_ common.Platform) error { if err := odhdeploy.ApplyParams(paramsPath, imageParamMap); err != nil { return fmt.Errorf("failed to update images on path %s: %w", paramsPath, err) } diff --git a/controllers/components/dashboard/dashboard.go b/controllers/components/dashboard/dashboard.go index 7070c87d4897..1913f7fe6d36 100644 --- a/controllers/components/dashboard/dashboard.go +++ b/controllers/components/dashboard/dashboard.go @@ -15,7 +15,6 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" cr "github.com/opendatahub-io/opendatahub-operator/v2/pkg/componentsregistry" odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" @@ -38,7 +37,7 @@ func (s *componentHandler) GetManagementState(dsc *dscv1.DataScienceCluster) ope return operatorv1.Removed } -func (s *componentHandler) Init(platform cluster.Platform) error { +func (s *componentHandler) Init(platform common.Platform) error { mi := defaultManifestInfo(platform) if err := odhdeploy.ApplyParams(mi.String(), imagesMap); err != nil { diff --git a/controllers/components/dashboard/dashboard_support.go b/controllers/components/dashboard/dashboard_support.go index 70c59cd22648..8f14f34dbded 100644 --- a/controllers/components/dashboard/dashboard_support.go +++ b/controllers/components/dashboard/dashboard_support.go @@ -7,6 +7,7 @@ import ( conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" @@ -29,28 +30,28 @@ const ( ) var ( - adminGroups = map[cluster.Platform]string{ + adminGroups = map[common.Platform]string{ cluster.SelfManagedRhoai: "rhods-admins", cluster.ManagedRhoai: "dedicated-admins", cluster.OpenDataHub: "odh-admins", cluster.Unknown: "odh-admins", } - sectionTitle = map[cluster.Platform]string{ + sectionTitle = map[common.Platform]string{ cluster.SelfManagedRhoai: "OpenShift Self Managed Services", cluster.ManagedRhoai: "OpenShift Managed Services", cluster.OpenDataHub: "OpenShift Open Data Hub", cluster.Unknown: "OpenShift Open Data Hub", } - baseConsoleURL = map[cluster.Platform]string{ + baseConsoleURL = map[common.Platform]string{ cluster.SelfManagedRhoai: "https://rhods-dashboard-", cluster.ManagedRhoai: "https://rhods-dashboard-", cluster.OpenDataHub: "https://odh-dashboard-", cluster.Unknown: "https://odh-dashboard-", } - overlaysSourcePaths = map[cluster.Platform]string{ + overlaysSourcePaths = map[common.Platform]string{ cluster.SelfManagedRhoai: "/rhoai/onprem", cluster.ManagedRhoai: "/rhoai/addon", cluster.OpenDataHub: "/odh", @@ -62,7 +63,7 @@ var ( } ) -func defaultManifestInfo(p cluster.Platform) odhtypes.ManifestInfo { +func defaultManifestInfo(p common.Platform) odhtypes.ManifestInfo { return odhtypes.ManifestInfo{ Path: odhdeploy.DefaultManifestPath, ContextDir: ComponentName, @@ -70,7 +71,7 @@ func defaultManifestInfo(p cluster.Platform) odhtypes.ManifestInfo { } } -func computeKustomizeVariable(ctx context.Context, cli client.Client, platform cluster.Platform, dscispec *dsciv1.DSCInitializationSpec) (map[string]string, error) { +func computeKustomizeVariable(ctx context.Context, cli client.Client, platform common.Platform, dscispec *dsciv1.DSCInitializationSpec) (map[string]string, error) { consoleLinkDomain, err := cluster.GetDomain(ctx, cli) if err != nil { return nil, fmt.Errorf("error getting console route URL %s : %w", consoleLinkDomain, err) diff --git a/controllers/components/datasciencepipelines/datasciencepipelines.go b/controllers/components/datasciencepipelines/datasciencepipelines.go index 02a2318423e7..275085be0351 100644 --- a/controllers/components/datasciencepipelines/datasciencepipelines.go +++ b/controllers/components/datasciencepipelines/datasciencepipelines.go @@ -38,7 +38,7 @@ func (s *componentHandler) GetManagementState(dsc *dscv1.DataScienceCluster) ope return operatorv1.Removed } -func (s *componentHandler) Init(_ cluster.Platform) error { +func (s *componentHandler) Init(_ common.Platform) error { release := cluster.GetRelease() extraParams := map[string]string{ platformVersionParamsKey: release.Version.String(), diff --git a/controllers/components/datasciencepipelines/datasciencepipelines_support.go b/controllers/components/datasciencepipelines/datasciencepipelines_support.go index aea02c19cf6f..f01c3c6d90c9 100644 --- a/controllers/components/datasciencepipelines/datasciencepipelines_support.go +++ b/controllers/components/datasciencepipelines/datasciencepipelines_support.go @@ -5,6 +5,7 @@ import ( conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" @@ -39,7 +40,7 @@ var ( "IMAGES_PIPELINESRUNTIMEGENERIC": "RELATED_IMAGE_ODH_ML_PIPELINES_RUNTIME_GENERIC_IMAGE", } - overlaysSourcePaths = map[cluster.Platform]string{ + overlaysSourcePaths = map[common.Platform]string{ cluster.SelfManagedRhoai: "overlays/rhoai", cluster.ManagedRhoai: "overlays/rhoai", cluster.OpenDataHub: "overlays/odh", @@ -48,7 +49,7 @@ var ( paramsPath = path.Join(odhdeploy.DefaultManifestPath, ComponentName, "base") ) -func manifestPath(p cluster.Platform) types.ManifestInfo { +func manifestPath(p common.Platform) types.ManifestInfo { return types.ManifestInfo{ Path: odhdeploy.DefaultManifestPath, ContextDir: ComponentName, diff --git a/controllers/components/kserve/kserve.go b/controllers/components/kserve/kserve.go index 8c1ddb51bc81..e52a0a107583 100644 --- a/controllers/components/kserve/kserve.go +++ b/controllers/components/kserve/kserve.go @@ -15,7 +15,6 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" cr "github.com/opendatahub-io/opendatahub-operator/v2/pkg/componentsregistry" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" ) @@ -42,7 +41,7 @@ func init() { //nolint:gochecknoinits } // Init for set images. -func (s *componentHandler) Init(platform cluster.Platform) error { +func (s *componentHandler) Init(platform common.Platform) error { return nil } diff --git a/controllers/components/kueue/kueue.go b/controllers/components/kueue/kueue.go index 23287e9898fd..af285c3a0b03 100644 --- a/controllers/components/kueue/kueue.go +++ b/controllers/components/kueue/kueue.go @@ -15,7 +15,6 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" cr "github.com/opendatahub-io/opendatahub-operator/v2/pkg/componentsregistry" odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" @@ -56,7 +55,7 @@ func (s *componentHandler) NewCRObject(dsc *dscv1.DataScienceCluster) common.Pla } } -func (s *componentHandler) Init(platform cluster.Platform) error { +func (s *componentHandler) Init(platform common.Platform) error { if err := odhdeploy.ApplyParams(manifestsPath().String(), imageParamMap); err != nil { return fmt.Errorf("failed to update images on path %s: %w", manifestsPath(), err) } diff --git a/controllers/components/modelcontroller/modelcontroller.go b/controllers/components/modelcontroller/modelcontroller.go index 234281939efc..5ca91c61aea2 100644 --- a/controllers/components/modelcontroller/modelcontroller.go +++ b/controllers/components/modelcontroller/modelcontroller.go @@ -15,7 +15,6 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/componentsregistry" odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" @@ -76,7 +75,7 @@ func (s *componentHandler) NewCRObject(dsc *dscv1.DataScienceCluster) common.Pla } // Init for set images. -func (s *componentHandler) Init(_ cluster.Platform) error { +func (s *componentHandler) Init(_ common.Platform) error { // Update image parameters if err := odhdeploy.ApplyParams(manifestsPath().String(), imageParamMap); err != nil { return fmt.Errorf("failed to update images on path %s: %w", manifestsPath(), err) diff --git a/controllers/components/modelmeshserving/modelmeshserving.go b/controllers/components/modelmeshserving/modelmeshserving.go index 22bbb6aceea8..bc0374cf182f 100644 --- a/controllers/components/modelmeshserving/modelmeshserving.go +++ b/controllers/components/modelmeshserving/modelmeshserving.go @@ -15,7 +15,6 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/componentsregistry" odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" @@ -38,7 +37,7 @@ func (s *componentHandler) GetManagementState(dsc *dscv1.DataScienceCluster) ope return operatorv1.Removed } -func (s *componentHandler) Init(_ cluster.Platform) error { +func (s *componentHandler) Init(_ common.Platform) error { // Update image parameters if err := odhdeploy.ApplyParams(manifestsPath().String(), imageParamMap); err != nil { return fmt.Errorf("failed to update images on path %s: %w", manifestsPath(), err) diff --git a/controllers/components/modelregistry/modelregistry.go b/controllers/components/modelregistry/modelregistry.go index 31b13a1ed2cb..98f72e2c26e0 100644 --- a/controllers/components/modelregistry/modelregistry.go +++ b/controllers/components/modelregistry/modelregistry.go @@ -15,7 +15,6 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" cr "github.com/opendatahub-io/opendatahub-operator/v2/pkg/componentsregistry" odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" @@ -38,7 +37,7 @@ func (s *componentHandler) GetManagementState(dsc *dscv1.DataScienceCluster) ope return operatorv1.Removed } -func (s *componentHandler) Init(_ cluster.Platform) error { +func (s *componentHandler) Init(_ common.Platform) error { mi := baseManifestInfo(BaseManifestsSourcePath) if err := odhdeploy.ApplyParams(mi.String(), imagesMap, extraParamsMap); err != nil { diff --git a/controllers/components/ray/ray.go b/controllers/components/ray/ray.go index 551961a84ab9..47777d8f48b4 100644 --- a/controllers/components/ray/ray.go +++ b/controllers/components/ray/ray.go @@ -15,7 +15,6 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" cr "github.com/opendatahub-io/opendatahub-operator/v2/pkg/componentsregistry" odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" @@ -56,7 +55,7 @@ func (s *componentHandler) NewCRObject(dsc *dscv1.DataScienceCluster) common.Pla } } -func (s *componentHandler) Init(_ cluster.Platform) error { +func (s *componentHandler) Init(_ common.Platform) error { if err := odhdeploy.ApplyParams(manifestPath().String(), imageParamMap); err != nil { return fmt.Errorf("failed to update images on path %s: %w", manifestPath(), err) } diff --git a/controllers/components/trainingoperator/trainingoperator.go b/controllers/components/trainingoperator/trainingoperator.go index 316d686b0c20..46c57fd38fac 100644 --- a/controllers/components/trainingoperator/trainingoperator.go +++ b/controllers/components/trainingoperator/trainingoperator.go @@ -15,7 +15,6 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" cr "github.com/opendatahub-io/opendatahub-operator/v2/pkg/componentsregistry" odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" @@ -55,7 +54,7 @@ func (s *componentHandler) NewCRObject(dsc *dscv1.DataScienceCluster) common.Pla } } -func (s *componentHandler) Init(platform cluster.Platform) error { +func (s *componentHandler) Init(platform common.Platform) error { if err := odhdeploy.ApplyParams(manifestPath().String(), imageParamMap); err != nil { return fmt.Errorf("failed to update images on path %s: %w", manifestPath(), err) } diff --git a/controllers/components/trustyai/trustyai.go b/controllers/components/trustyai/trustyai.go index 2dd4c138eb15..31cdbc9b67be 100644 --- a/controllers/components/trustyai/trustyai.go +++ b/controllers/components/trustyai/trustyai.go @@ -15,7 +15,6 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" cr "github.com/opendatahub-io/opendatahub-operator/v2/pkg/componentsregistry" odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" @@ -56,7 +55,7 @@ func (s *componentHandler) NewCRObject(dsc *dscv1.DataScienceCluster) common.Pla } } -func (s *componentHandler) Init(platform cluster.Platform) error { +func (s *componentHandler) Init(platform common.Platform) error { mp := manifestsPath(platform) if err := odhdeploy.ApplyParams(mp.String(), imageParamMap); err != nil { diff --git a/controllers/components/trustyai/trustyai_support.go b/controllers/components/trustyai/trustyai_support.go index f758e443c576..6f85425f13e3 100644 --- a/controllers/components/trustyai/trustyai_support.go +++ b/controllers/components/trustyai/trustyai_support.go @@ -3,6 +3,7 @@ package trustyai import ( conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" @@ -27,7 +28,7 @@ var ( "trustyaiOperatorImage": "RELATED_IMAGE_ODH_TRUSTYAI_SERVICE_OPERATOR_IMAGE", } - overlaysSourcePaths = map[cluster.Platform]string{ + overlaysSourcePaths = map[common.Platform]string{ cluster.SelfManagedRhoai: "/overlays/rhoai", cluster.ManagedRhoai: "/overlays/rhoai", cluster.OpenDataHub: "/overlays/odh", @@ -35,7 +36,7 @@ var ( } ) -func manifestsPath(p cluster.Platform) types.ManifestInfo { +func manifestsPath(p common.Platform) types.ManifestInfo { return types.ManifestInfo{ Path: odhdeploy.DefaultManifestPath, ContextDir: ComponentName, diff --git a/controllers/components/workbenches/workbenches.go b/controllers/components/workbenches/workbenches.go index 9e2f95cc8f32..a3087819ce4d 100644 --- a/controllers/components/workbenches/workbenches.go +++ b/controllers/components/workbenches/workbenches.go @@ -15,7 +15,6 @@ import ( componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" cr "github.com/opendatahub-io/opendatahub-operator/v2/pkg/componentsregistry" odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" @@ -56,7 +55,7 @@ func (s *componentHandler) NewCRObject(dsc *dscv1.DataScienceCluster) common.Pla } } -func (s *componentHandler) Init(platform cluster.Platform) error { +func (s *componentHandler) Init(platform common.Platform) error { nbcManifestInfo := notebookControllerManifestInfo(notebookControllerManifestSourcePath) if err := odhdeploy.ApplyParams(nbcManifestInfo.String(), map[string]string{ "odh-notebook-controller-image": "RELATED_IMAGE_ODH_NOTEBOOK_CONTROLLER_IMAGE", diff --git a/controllers/dscinitialization/utils.go b/controllers/dscinitialization/utils.go index 5c1325e84d5c..70c0b053ef08 100644 --- a/controllers/dscinitialization/utils.go +++ b/controllers/dscinitialization/utils.go @@ -22,6 +22,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logf "sigs.k8s.io/controller-runtime/pkg/log" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" @@ -43,7 +44,7 @@ var ( // - 2. Patch monitoring namespace // - 3. Network Policies 'opendatahub' that allow traffic between the ODH namespaces // - 4. ConfigMap 'odh-common-config'. -func (r *DSCInitializationReconciler) createOperatorResource(ctx context.Context, dscInit *dsciv1.DSCInitialization, platform cluster.Platform) error { +func (r *DSCInitializationReconciler) createOperatorResource(ctx context.Context, dscInit *dsciv1.DSCInitialization, platform common.Platform) error { log := logf.FromContext(ctx) if err := r.appNamespaceHandler(ctx, dscInit, platform); err != nil { @@ -75,7 +76,7 @@ func (r *DSCInitializationReconciler) createOperatorResource(ctx context.Context return nil } -func (r *DSCInitializationReconciler) appNamespaceHandler(ctx context.Context, dscInit *dsciv1.DSCInitialization, platform cluster.Platform) error { +func (r *DSCInitializationReconciler) appNamespaceHandler(ctx context.Context, dscInit *dsciv1.DSCInitialization, platform common.Platform) error { log := logf.FromContext(ctx) nsList := &corev1.NamespaceList{} @@ -115,7 +116,7 @@ func (r *DSCInitializationReconciler) appNamespaceHandler(ctx context.Context, d } } -func (r *DSCInitializationReconciler) createAppNamespace(ctx context.Context, nsName string, platform cluster.Platform, extraLabel ...map[string]string) error { +func (r *DSCInitializationReconciler) createAppNamespace(ctx context.Context, nsName string, platform common.Platform, extraLabel ...map[string]string) error { desiredDefaultNS := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{Name: nsName}, } @@ -172,7 +173,7 @@ func (r *DSCInitializationReconciler) patchMonitoringNS(ctx context.Context, dsc func (r *DSCInitializationReconciler) reconcileDefaultNetworkPolicy( ctx context.Context, dscInit *dsciv1.DSCInitialization, - platform cluster.Platform, + platform common.Platform, ) error { log := logf.FromContext(ctx) name := dscInit.Spec.ApplicationsNamespace diff --git a/main.go b/main.go index c1e3d9f0a1e1..c7fba3fb4d1a 100644 --- a/main.go +++ b/main.go @@ -60,6 +60,7 @@ import ( ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics/server" ctrlwebhook "sigs.k8s.io/controller-runtime/pkg/webhook" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" @@ -133,7 +134,7 @@ func init() { //nolint:gochecknoinits utilruntime.Must(templatev1.Install(scheme)) } -func initComponents(_ context.Context, p cluster.Platform) error { +func initComponents(_ context.Context, p common.Platform) error { return cr.ForEach(func(ch cr.ComponentHandler) error { return ch.Init(p) }) @@ -454,7 +455,7 @@ func main() { //nolint:funlen,maintidx,gocyclo } } -func getCommonCache(ctx context.Context, cli client.Client, platform cluster.Platform) (map[string]cache.Config, error) { +func getCommonCache(ctx context.Context, cli client.Client, platform common.Platform) (map[string]cache.Config, error) { namespaceConfigs := map[string]cache.Config{} // newtowkrpolicy need operator namespace operatorNs, err := cluster.GetOperatorNamespace() @@ -495,7 +496,7 @@ func getCommonCache(ctx context.Context, cli client.Client, platform cluster.Pla return namespaceConfigs, nil } -func createSecretCacheConfig(ctx context.Context, cli client.Client, platform cluster.Platform) (map[string]cache.Config, error) { +func createSecretCacheConfig(ctx context.Context, cli client.Client, platform common.Platform) (map[string]cache.Config, error) { namespaceConfigs := map[string]cache.Config{ "istio-system": {}, // for both knative-serving-cert and default-modelregistry-cert, as an easy workarond, to watch both in this namespace "openshift-ingress": {}, @@ -511,7 +512,7 @@ func createSecretCacheConfig(ctx context.Context, cli client.Client, platform cl return namespaceConfigs, nil } -func createODHGeneralCacheConfig(ctx context.Context, cli client.Client, platform cluster.Platform) (map[string]cache.Config, error) { +func createODHGeneralCacheConfig(ctx context.Context, cli client.Client, platform common.Platform) (map[string]cache.Config, error) { namespaceConfigs := map[string]cache.Config{ "istio-system": {}, // for serivcemonitor: data-science-smcp-pilot-monitor "openshift-operators": {}, // for dependent operators installed namespace diff --git a/pkg/cluster/cluster_config.go b/pkg/cluster/cluster_config.go index 2afbbdf1a637..a9b7172d4726 100644 --- a/pkg/cluster/cluster_config.go +++ b/pkg/cluster/cluster_config.go @@ -20,18 +20,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" ) -type Platform string - -// Release includes information on operator version and platform -// +kubebuilder:object:generate=true -type Release struct { - Name Platform `json:"name,omitempty"` - Version version.OperatorVersion `json:"version,omitempty"` -} - type ClusterInfo struct { Type string `json:"type,omitempty"` // openshift , TODO: can be other value if we later support other type Version version.OperatorVersion `json:"version,omitempty"` @@ -39,7 +31,7 @@ type ClusterInfo struct { var clusterConfig struct { Namespace string - Release Release + Release common.Release ClusterInfo ClusterInfo } @@ -84,7 +76,7 @@ func GetOperatorNamespace() (string, error) { return clusterConfig.Namespace, nil } -func GetRelease() Release { +func GetRelease() common.Release { return clusterConfig.Release } @@ -169,8 +161,8 @@ func GetClusterServiceVersion(ctx context.Context, c client.Client, namespace st } // detectSelfManaged detects if it is Self Managed Rhoai or OpenDataHub. -func detectSelfManaged(ctx context.Context, cli client.Client) (Platform, error) { - variants := map[string]Platform{ +func detectSelfManaged(ctx context.Context, cli client.Client) (common.Platform, error) { + variants := map[string]common.Platform{ "opendatahub-operator": OpenDataHub, "rhods-operator": SelfManagedRhoai, } @@ -189,7 +181,7 @@ func detectSelfManaged(ctx context.Context, cli client.Client) (Platform, error) } // detectManagedRhoai checks if catsrc CR add-on exists ManagedRhoai. -func detectManagedRhoai(ctx context.Context, cli client.Client) (Platform, error) { +func detectManagedRhoai(ctx context.Context, cli client.Client) (common.Platform, error) { catalogSource := &ofapiv1alpha1.CatalogSource{} operatorNs, err := GetOperatorNamespace() if err != nil { @@ -202,7 +194,7 @@ func detectManagedRhoai(ctx context.Context, cli client.Client) (Platform, error return ManagedRhoai, nil } -func getPlatform(ctx context.Context, cli client.Client) (Platform, error) { +func getPlatform(ctx context.Context, cli client.Client) (common.Platform, error) { switch os.Getenv("ODH_PLATFORM_TYPE") { case "OpenDataHub": return OpenDataHub, nil @@ -223,8 +215,8 @@ func getPlatform(ctx context.Context, cli client.Client) (Platform, error) { } } -func getRelease(ctx context.Context, cli client.Client) (Release, error) { - initRelease := Release{ +func getRelease(ctx context.Context, cli client.Client) (common.Release, error) { + initRelease := common.Release{ // dummy version set to name "", version 0.0.0 Version: version.OperatorVersion{ Version: semver.Version{}, diff --git a/pkg/cluster/const.go b/pkg/cluster/const.go index 68c6434b6b11..844c139438c1 100644 --- a/pkg/cluster/const.go +++ b/pkg/cluster/const.go @@ -1,14 +1,16 @@ package cluster +import "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" + const ( // ManagedRhoai defines expected addon catalogsource. - ManagedRhoai Platform = "OpenShift AI Cloud Service" + ManagedRhoai common.Platform = "OpenShift AI Cloud Service" // SelfManagedRhoai defines display name in csv. - SelfManagedRhoai Platform = "OpenShift AI Self-Managed" + SelfManagedRhoai common.Platform = "OpenShift AI Self-Managed" // OpenDataHub defines display name in csv. - OpenDataHub Platform = "Open Data Hub" + OpenDataHub common.Platform = "Open Data Hub" // Unknown indicates that operator is not deployed using OLM. - Unknown Platform = "" + Unknown common.Platform = "" // DefaultNotebooksNamespace defines default namespace for notebooks. DefaultNotebooksNamespace = "rhods-notebooks" diff --git a/pkg/cluster/zz_generated.deepcopy.go b/pkg/cluster/zz_generated.deepcopy.go deleted file mode 100644 index 0bc3e0f557d8..000000000000 --- a/pkg/cluster/zz_generated.deepcopy.go +++ /dev/null @@ -1,39 +0,0 @@ -//go:build !ignore_autogenerated - -/* -Copyright 2023. - -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. -*/ - -// Code generated by controller-gen. DO NOT EDIT. - -package cluster - -import () - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *Release) DeepCopyInto(out *Release) { - *out = *in - in.Version.DeepCopyInto(&out.Version) -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Release. -func (in *Release) DeepCopy() *Release { - if in == nil { - return nil - } - out := new(Release) - in.DeepCopyInto(out) - return out -} diff --git a/pkg/componentsregistry/componentsregistry.go b/pkg/componentsregistry/componentsregistry.go index cd9c07b64026..3c74821fde57 100644 --- a/pkg/componentsregistry/componentsregistry.go +++ b/pkg/componentsregistry/componentsregistry.go @@ -12,13 +12,12 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" ) // ComponentHandler is an interface to manage a component // Every method should accept ctx since it contains the logger. type ComponentHandler interface { - Init(platform cluster.Platform) error + Init(platform common.Platform) error // GetName and GetManagementState sound like pretty much the same across // all components, but I could not find a way to avoid it GetName() string diff --git a/pkg/controller/actions/deleteresource/action_delete_resources_test.go b/pkg/controller/actions/deleteresource/action_delete_resources_test.go index ecf6b540e04c..20fc19291a64 100644 --- a/pkg/controller/actions/deleteresource/action_delete_resources_test.go +++ b/pkg/controller/actions/deleteresource/action_delete_resources_test.go @@ -9,6 +9,7 @@ import ( appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" @@ -65,7 +66,7 @@ func TestDeleteResourcesAction(t *testing.T) { Client: cl, Instance: nil, DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}}, - Release: cluster.Release{Name: cluster.OpenDataHub}, + Release: common.Release{Name: cluster.OpenDataHub}, }) g.Expect(err).ShouldNot(HaveOccurred()) diff --git a/pkg/controller/actions/deploy/action_deploy_cache_test.go b/pkg/controller/actions/deploy/action_deploy_cache_test.go index 4ee26b6b31d0..297d23f95c07 100644 --- a/pkg/controller/actions/deploy/action_deploy_cache_test.go +++ b/pkg/controller/actions/deploy/action_deploy_cache_test.go @@ -20,6 +20,7 @@ import ( ctrlCli "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" @@ -152,7 +153,7 @@ func testResourceNotReDeployed(t *testing.T, cli *client.Client, obj ctrlCli.Obj Generation: 1, }, }, - Release: cluster.Release{ + Release: common.Release{ Name: cluster.OpenDataHub, Version: version.OperatorVersion{Version: semver.Version{ Major: 1, Minor: 2, Patch: 3, @@ -225,7 +226,7 @@ func testCacheTTL(t *testing.T, cli *client.Client, obj ctrlCli.Object) { Generation: 1, }, }, - Release: cluster.Release{ + Release: common.Release{ Name: cluster.OpenDataHub, Version: version.OperatorVersion{Version: semver.Version{ Major: 1, Minor: 2, Patch: 3, diff --git a/pkg/controller/actions/deploy/action_deploy_test.go b/pkg/controller/actions/deploy/action_deploy_test.go index 16e80c50270e..1928ee5e98c6 100644 --- a/pkg/controller/actions/deploy/action_deploy_test.go +++ b/pkg/controller/actions/deploy/action_deploy_test.go @@ -24,6 +24,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/envtest" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" @@ -79,7 +80,7 @@ func TestDeployAction(t *testing.T) { Generation: 1, }, }, - Release: cluster.Release{ + Release: common.Release{ Name: cluster.OpenDataHub, Version: version.OperatorVersion{Version: semver.Version{ Major: 1, Minor: 2, Patch: 3, @@ -165,7 +166,7 @@ func TestDeployNotOwnedSkip(t *testing.T) { Generation: 1, }, }, - Release: cluster.Release{ + Release: common.Release{ Name: cluster.OpenDataHub, Version: version.OperatorVersion{Version: semver.Version{ Major: 1, Minor: 2, Patch: 3, @@ -231,7 +232,7 @@ func TestDeployNotOwnedCreate(t *testing.T) { Generation: 1, }, }, - Release: cluster.Release{ + Release: common.Release{ Name: cluster.OpenDataHub, Version: version.OperatorVersion{Version: semver.Version{ Major: 1, Minor: 2, Patch: 3, @@ -358,7 +359,7 @@ func deployClusterRoles(t *testing.T, ctx context.Context, cli *odhCli.Client, r UID: apimachinery.UID(xid.New().String()), }, }, - Release: cluster.Release{ + Release: common.Release{ Name: cluster.OpenDataHub, Version: version.OperatorVersion{Version: semver.Version{ Major: 1, Minor: 2, Patch: 3, @@ -425,7 +426,7 @@ func TestDeployCRD(t *testing.T) { UID: apimachinery.UID(id), }, }, - Release: cluster.Release{ + Release: common.Release{ Name: cluster.OpenDataHub, Version: version.OperatorVersion{Version: semver.Version{ Major: 1, Minor: 2, Patch: 3, @@ -598,7 +599,7 @@ func TestDeployOwnerRef(t *testing.T) { ApplicationsNamespace: id, }}, Instance: instance, - Release: cluster.Release{ + Release: common.Release{ Name: cluster.OpenDataHub, Version: version.OperatorVersion{Version: semver.Version{ Major: 1, Minor: 2, Patch: 3, diff --git a/pkg/controller/actions/gc/action_gc_test.go b/pkg/controller/actions/gc/action_gc_test.go index 8f5528ee434a..94efe54fe7a3 100644 --- a/pkg/controller/actions/gc/action_gc_test.go +++ b/pkg/controller/actions/gc/action_gc_test.go @@ -24,6 +24,7 @@ import ( ctrlCli "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" @@ -200,7 +201,7 @@ func TestGcAction(t *testing.T) { UID: apytypes.UID(id), }, }, - Release: cluster.Release{ + Release: common.Release{ Name: cluster.OpenDataHub, Version: version.OperatorVersion{ Version: tt.version, diff --git a/pkg/controller/actions/render/kustomize/action_render_manifests_test.go b/pkg/controller/actions/render/kustomize/action_render_manifests_test.go index 01a7a0f7f4d3..2d5ab6eecc99 100644 --- a/pkg/controller/actions/render/kustomize/action_render_manifests_test.go +++ b/pkg/controller/actions/render/kustomize/action_render_manifests_test.go @@ -9,6 +9,7 @@ import ( "github.com/rs/xid" "sigs.k8s.io/kustomize/kyaml/filesys" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" @@ -129,7 +130,7 @@ func TestRenderResourcesAction(t *testing.T) { Client: cl, Instance: &componentApi.Dashboard{}, DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}}, - Release: cluster.Release{Name: cluster.OpenDataHub}, + Release: common.Release{Name: cluster.OpenDataHub}, Manifests: []types.ManifestInfo{{Path: id}}, } @@ -215,7 +216,7 @@ func TestRenderResourcesWithCacheAction(t *testing.T) { Client: cl, Instance: &d, DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}}, - Release: cluster.Release{Name: cluster.OpenDataHub}, + Release: common.Release{Name: cluster.OpenDataHub}, Manifests: []types.ManifestInfo{{Path: id}}, } diff --git a/pkg/controller/actions/render/template/action_render_templates_test.go b/pkg/controller/actions/render/template/action_render_templates_test.go index 0839fbe7fb8b..e9e5484bb31a 100644 --- a/pkg/controller/actions/render/template/action_render_templates_test.go +++ b/pkg/controller/actions/render/template/action_render_templates_test.go @@ -11,6 +11,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apytypes "k8s.io/apimachinery/pkg/types" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/infrastructure/v1" @@ -56,7 +57,7 @@ func TestRenderTemplate(t *testing.T) { }, }, }, - Release: cluster.Release{Name: cluster.OpenDataHub}, + Release: common.Release{Name: cluster.OpenDataHub}, Templates: []types.TemplateInfo{{FS: testFS, Path: "resources/smm.tmpl.yaml"}}, } @@ -120,7 +121,7 @@ func TestRenderTemplateWithData(t *testing.T) { }, }, }, - Release: cluster.Release{Name: cluster.OpenDataHub}, + Release: common.Release{Name: cluster.OpenDataHub}, Templates: []types.TemplateInfo{{FS: testFS, Path: "resources/smm-data.tmpl.yaml"}}, } @@ -165,7 +166,7 @@ func TestRenderTemplateWithDataErr(t *testing.T) { }, }, DSCI: &dsciv1.DSCInitialization{}, - Release: cluster.Release{Name: cluster.OpenDataHub}, + Release: common.Release{Name: cluster.OpenDataHub}, Templates: []types.TemplateInfo{{FS: testFS, Path: "resources/smm-data.tmpl.yaml"}}, } @@ -216,7 +217,7 @@ func TestRenderTemplateWithCache(t *testing.T) { Client: cl, Instance: &d, DSCI: &dsci, - Release: cluster.Release{Name: cluster.OpenDataHub}, + Release: common.Release{Name: cluster.OpenDataHub}, Templates: []types.TemplateInfo{{FS: testFS, Path: "resources/smm.tmpl.yaml"}}, } diff --git a/pkg/controller/actions/updatestatus/action_update_status_test.go b/pkg/controller/actions/updatestatus/action_update_status_test.go index 7eb47d60e926..e52c47a60112 100644 --- a/pkg/controller/actions/updatestatus/action_update_status_test.go +++ b/pkg/controller/actions/updatestatus/action_update_status_test.go @@ -11,6 +11,7 @@ import ( appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" @@ -78,7 +79,7 @@ func TestUpdateStatusActionNotReady(t *testing.T) { Client: cl, Instance: &componentApi.Dashboard{}, DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}}, - Release: cluster.Release{Name: cluster.OpenDataHub}, + Release: common.Release{Name: cluster.OpenDataHub}, } err = action(ctx, &rr) @@ -148,7 +149,7 @@ func TestUpdateStatusActionReady(t *testing.T) { Client: cl, Instance: &componentApi.Dashboard{}, DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}}, - Release: cluster.Release{Name: cluster.OpenDataHub}, + Release: common.Release{Name: cluster.OpenDataHub}, } err = action(ctx, &rr) @@ -217,7 +218,7 @@ func TestUpdateStatusActionReadyAutoSelector(t *testing.T) { Client: cl, Instance: &componentApi.Dashboard{}, DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}}, - Release: cluster.Release{Name: cluster.OpenDataHub}, + Release: common.Release{Name: cluster.OpenDataHub}, } err = action(ctx, &rr) @@ -286,7 +287,7 @@ func TestUpdateStatusActionNotReadyNotFound(t *testing.T) { Client: cl, Instance: &componentApi.Dashboard{}, DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}}, - Release: cluster.Release{Name: cluster.OpenDataHub}, + Release: common.Release{Name: cluster.OpenDataHub}, } err = action(ctx, &rr) diff --git a/pkg/controller/reconciler/reconciler.go b/pkg/controller/reconciler/reconciler.go index 565738e3a7b7..8256614c7d14 100644 --- a/pkg/controller/reconciler/reconciler.go +++ b/pkg/controller/reconciler/reconciler.go @@ -35,7 +35,7 @@ type Reconciler struct { Log logr.Logger Controller controller.Controller Recorder record.EventRecorder - Release cluster.Release + Release common.Release name string m *odhManager.Manager @@ -71,7 +71,7 @@ func NewReconciler[T common.PlatformObject](mgr manager.Manager, name string, ob return &cc, nil } -func (r *Reconciler) GetRelease() cluster.Release { +func (r *Reconciler) GetRelease() common.Release { return r.Release } diff --git a/pkg/controller/types/types.go b/pkg/controller/types/types.go index d46b5c6d1452..d0165805cae0 100644 --- a/pkg/controller/types/types.go +++ b/pkg/controller/types/types.go @@ -13,7 +13,6 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" odhClient "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/client" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/manager" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" @@ -59,7 +58,7 @@ type ReconciliationRequest struct { Manager *manager.Manager Instance common.PlatformObject DSCI *dsciv1.DSCInitialization - Release cluster.Release + Release common.Release Manifests []ManifestInfo // diff --git a/pkg/upgrade/uninstallation.go b/pkg/upgrade/uninstallation.go index 5f300091f1d8..f5395719cb7c 100644 --- a/pkg/upgrade/uninstallation.go +++ b/pkg/upgrade/uninstallation.go @@ -12,6 +12,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" @@ -26,7 +27,7 @@ const ( // OperatorUninstall deletes all the externally generated resources. // This includes DSCI, namespace created by operator (but not workbench or MR's), subscription and CSV. -func OperatorUninstall(ctx context.Context, cli client.Client, platform cluster.Platform) error { +func OperatorUninstall(ctx context.Context, cli client.Client, platform common.Platform) error { log := logf.FromContext(ctx) if err := removeDSC(ctx, cli); err != nil { diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 4ebb8ca0417f..7ddcd4be8092 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -106,7 +106,7 @@ func CreateDefaultDSC(ctx context.Context, cli client.Client) error { // CreateDefaultDSCI creates a default instance of DSCI // If there exists default-dsci instance already, it will not update DSCISpec on it. // Note: DSCI CR modifcations are not supported, as it is the initial prereq setting for the components. -func CreateDefaultDSCI(ctx context.Context, cli client.Client, _ cluster.Platform, monNamespace string) error { +func CreateDefaultDSCI(ctx context.Context, cli client.Client, _ common.Platform, monNamespace string) error { log := logf.FromContext(ctx) defaultDsciSpec := &dsciv1.DSCInitializationSpec{ Monitoring: serviceApi.DSCMonitoring{ @@ -204,9 +204,9 @@ func getDashboardWatsonResources(ns string) []ResourceSpec { // TODO: remove function once we have a generic solution across all components. func CleanupExistingResource(ctx context.Context, cli client.Client, - platform cluster.Platform, + platform common.Platform, dscMonitoringNamespace string, - oldReleaseVersion cluster.Release, + oldReleaseVersion common.Release, ) error { var multiErr *multierror.Error // get DSCI CR to get application namespace @@ -368,7 +368,7 @@ func removOdhApplicationsCR(ctx context.Context, cli client.Client, gvk schema.G // 1. unset ownerreference for CR odh-dashboard-config // 2. flip TrustyAI BiasMetrics to false (.spec.dashboardConfig.disableBiasMetrics) if it is lower release version than input 'release'. // 3. flip ModelRegistry to false (.spec.dashboardConfig.disableModelRegistry) if it is lower release version than input 'release'. -func upgradeODCCR(ctx context.Context, cli client.Client, instanceName string, applicationNS string, release cluster.Release) error { +func upgradeODCCR(ctx context.Context, cli client.Client, instanceName string, applicationNS string, release common.Release) error { crd := &apiextv1.CustomResourceDefinition{} if err := cli.Get(ctx, client.ObjectKey{Name: "odhdashboardconfigs.opendatahub.io"}, crd); err != nil { return client.IgnoreNotFound(err) @@ -404,7 +404,7 @@ func unsetOwnerReference(ctx context.Context, cli client.Client, instanceName st return nil } -func updateODCBiasMetrics(ctx context.Context, cli client.Client, instanceName string, oldRelease cluster.Release, odhObject *unstructured.Unstructured) error { +func updateODCBiasMetrics(ctx context.Context, cli client.Client, instanceName string, oldRelease common.Release, odhObject *unstructured.Unstructured) error { log := logf.FromContext(ctx) // "from version" as oldRelease, if return "0.0.0" meaning running on 2.10- release/dummy CI build // if oldRelease is lower than 2.14.0(e.g 2.13.x-a), flip disableBiasMetrics to false (even the field did not exist) @@ -421,7 +421,7 @@ func updateODCBiasMetrics(ctx context.Context, cli client.Client, instanceName s return nil } -func updateODCModelRegistry(ctx context.Context, cli client.Client, instanceName string, oldRelease cluster.Release, odhObject *unstructured.Unstructured) error { +func updateODCModelRegistry(ctx context.Context, cli client.Client, instanceName string, oldRelease common.Release, odhObject *unstructured.Unstructured) error { log := logf.FromContext(ctx) // "from version" as oldRelease, if return "0.0.0" meaning running on 2.10- release/dummy CI build // if oldRelease is lower than 2.14.0(e.g 2.13.x-a), flip disableModelRegistry to false (even the field did not exist) @@ -469,10 +469,10 @@ func removeRBACProxyModelRegistry(ctx context.Context, cli client.Client, compon return nil } -func GetDeployedRelease(ctx context.Context, cli client.Client) (cluster.Release, error) { +func GetDeployedRelease(ctx context.Context, cli client.Client) (common.Release, error) { dsciInstance := &dsciv1.DSCInitializationList{} if err := cli.List(ctx, dsciInstance); err != nil { - return cluster.Release{}, err + return common.Release{}, err } if len(dsciInstance.Items) == 1 { // found one DSCI CR found // can return a valid Release or 0.0.0 @@ -481,16 +481,16 @@ func GetDeployedRelease(ctx context.Context, cli client.Client) (cluster.Release // no DSCI CR found, try with DSC CR dscInstances := &dscv1.DataScienceClusterList{} if err := cli.List(ctx, dscInstances); err != nil { - return cluster.Release{}, err + return common.Release{}, err } if len(dscInstances.Items) == 1 { // one DSC CR found return dscInstances.Items[0].Status.Release, nil } // could be a clean installation or both CRs are deleted already - return cluster.Release{}, nil + return common.Release{}, nil } -func cleanupNimIntegration(ctx context.Context, cli client.Client, oldRelease cluster.Release, applicationNS string) error { +func cleanupNimIntegration(ctx context.Context, cli client.Client, oldRelease common.Release, applicationNS string) error { var errs *multierror.Error log := logf.FromContext(ctx) diff --git a/tests/e2e/controller_test.go b/tests/e2e/controller_test.go index 3fe562f773ff..5dfb15410b19 100644 --- a/tests/e2e/controller_test.go +++ b/tests/e2e/controller_test.go @@ -27,6 +27,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" + "github.com/opendatahub-io/opendatahub-operator/v2/apis/common" componentApi "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1alpha1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" @@ -101,7 +102,7 @@ type testContext struct { // test DSCI CR because we do not create it in ODH by default testDSCI *dsciv1.DSCInitialization // test platform - platform cluster.Platform + platform common.Platform // context for accessing resources //nolint:containedctx //reason: legacy v1 test setup ctx context.Context From 0e24eb39e35b1a3d88ace7f1ffded1447fadab72 Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Tue, 11 Feb 2025 21:18:06 +0100 Subject: [PATCH 5/6] feat(test): support for Succeed and MatchError in testf framework (#1628) (cherry picked from commit a77476765c9fb6b9f16f578362f4b0c2ba7c4a21) --- pkg/utils/test/testf/testf_assertions.go | 57 ++++++++++++++++++++---- pkg/utils/test/testf/testf_witht_test.go | 42 +++++++++++++++++ tests/e2e/components_test.go | 8 ++-- tests/e2e/dashboard_test.go | 2 +- tests/e2e/kueue_test.go | 2 +- tests/e2e/modelcontroller_test.go | 7 ++- tests/e2e/modelregistry_test.go | 2 +- tests/e2e/trustyai_test.go | 4 +- 8 files changed, 102 insertions(+), 22 deletions(-) diff --git a/pkg/utils/test/testf/testf_assertions.go b/pkg/utils/test/testf/testf_assertions.go index 5ff981c389f1..a03f9d8ce010 100644 --- a/pkg/utils/test/testf/testf_assertions.go +++ b/pkg/utils/test/testf/testf_assertions.go @@ -7,6 +7,7 @@ import ( "time" "github.com/onsi/gomega" + "github.com/onsi/gomega/matchers" "github.com/onsi/gomega/types" ) @@ -83,7 +84,7 @@ func (a *Assertion[T]) WithContext(ctx context.Context) *Assertion[T] { return a } -func (a *Assertion[T]) build(f func(ctx context.Context) (T, error)) gomega.AsyncAssertion { +func (a *Assertion[T]) build(f interface{}) gomega.AsyncAssertion { var aa gomega.AsyncAssertion switch a.m { @@ -107,14 +108,33 @@ func (a *Assertion[T]) build(f func(ctx context.Context) (T, error)) gomega.Asyn return aa } +//nolint:dupl func (a *Assertion[T]) Should(matcher types.GomegaMatcher, optionalDescription ...interface{}) T { var res atomic.Value + var wrapper interface{} - wrapper := func(ctx context.Context) (T, error) { - v, err := a.f(ctx) - res.Store(v) + switch matcher.(type) { + case *matchers.SucceedMatcher: + wrapper = func(ctx context.Context) error { + v, err := a.f(ctx) + res.Store(v) - return v, err + return err + } + case *matchers.MatchErrorMatcher: + wrapper = func(ctx context.Context) error { + v, err := a.f(ctx) + res.Store(v) + + return err + } + default: + wrapper = func(ctx context.Context) (T, error) { + v, err := a.f(ctx) + res.Store(v) + + return v, err + } } a.build(wrapper).Should(matcher, optionalDescription...) @@ -123,14 +143,33 @@ func (a *Assertion[T]) Should(matcher types.GomegaMatcher, optionalDescription . return res.Load().(T) } +//nolint:dupl func (a *Assertion[T]) ShouldNot(matcher types.GomegaMatcher, optionalDescription ...interface{}) T { var res atomic.Value + var wrapper interface{} - wrapper := func(ctx context.Context) (T, error) { - v, err := a.f(ctx) - res.Store(v) + switch matcher.(type) { + case *matchers.SucceedMatcher: + wrapper = func(ctx context.Context) error { + v, err := a.f(ctx) + res.Store(v) - return v, err + return err + } + case *matchers.MatchErrorMatcher: + wrapper = func(ctx context.Context) error { + v, err := a.f(ctx) + res.Store(v) + + return err + } + default: + wrapper = func(ctx context.Context) (T, error) { + v, err := a.f(ctx) + res.Store(v) + + return v, err + } } a.build(wrapper).ShouldNot(matcher, optionalDescription...) diff --git a/pkg/utils/test/testf/testf_witht_test.go b/pkg/utils/test/testf/testf_witht_test.go index 60ab8629c4c8..13e097cfeb15 100644 --- a/pkg/utils/test/testf/testf_witht_test.go +++ b/pkg/utils/test/testf/testf_witht_test.go @@ -61,12 +61,26 @@ func TestGet(t *testing.T) { g.Expect(v).ShouldNot(BeNil()) }) + t.Run("Eventually Succeed", func(t *testing.T) { + wt := tc.NewWithT(t) + + v := wt.Get(gvk.ConfigMap, key).Eventually().Should(Succeed()) + g.Expect(v).ShouldNot(BeNil()) + }) + t.Run("Consistently", func(t *testing.T) { wt := tc.NewWithT(t) v := wt.Get(gvk.ConfigMap, key).Consistently().WithTimeout(1 * time.Second).Should(matchMetadata) g.Expect(v).ShouldNot(BeNil()) }) + + t.Run("Consistently Succeed", func(t *testing.T) { + wt := tc.NewWithT(t) + + v := wt.Get(gvk.ConfigMap, key).Consistently().WithTimeout(1 * time.Second).Should(Succeed()) + g.Expect(v).ShouldNot(BeNil()) + }) } func TestList(t *testing.T) { @@ -114,12 +128,26 @@ func TestList(t *testing.T) { g.Expect(v).ShouldNot(BeNil()) }) + t.Run("Eventually Succeed", func(t *testing.T) { + wt := tc.NewWithT(t) + + v := wt.List(gvk.ConfigMap).Eventually().Should(Succeed()) + g.Expect(v).ShouldNot(BeNil()) + }) + t.Run("Consistently", func(t *testing.T) { wt := tc.NewWithT(t) v := wt.List(gvk.ConfigMap).Consistently().WithTimeout(1 * time.Second).Should(matchMetadata) g.Expect(v).ShouldNot(BeNil()) }) + + t.Run("Consistently Succeed", func(t *testing.T) { + wt := tc.NewWithT(t) + + v := wt.List(gvk.ConfigMap).Consistently().WithTimeout(1 * time.Second).Should(Succeed()) + g.Expect(v).ShouldNot(BeNil()) + }) } func TestUpdate(t *testing.T) { @@ -168,12 +196,26 @@ func TestUpdate(t *testing.T) { g.Expect(v).Should(matchMetadataAndData) }) + t.Run("Eventually Succeed", func(t *testing.T) { + wt := tc.NewWithT(t) + + v := wt.Update(gvk.ConfigMap, key, transformer).Eventually().Should(Succeed()) + g.Expect(v).Should(matchMetadataAndData) + }) + t.Run("Consistently", func(t *testing.T) { wt := tc.NewWithT(t) v := wt.Update(gvk.ConfigMap, key, transformer).Consistently().WithTimeout(1 * time.Second).Should(matchMetadataAndData) g.Expect(v).Should(matchMetadataAndData) }) + + t.Run("Consistently Succeed", func(t *testing.T) { + wt := tc.NewWithT(t) + + v := wt.Update(gvk.ConfigMap, key, transformer).Consistently().WithTimeout(1 * time.Second).Should(Succeed()) + g.Expect(v).Should(matchMetadataAndData) + }) } func TestDelete(t *testing.T) { diff --git a/tests/e2e/components_test.go b/tests/e2e/components_test.go index 134229b49af1..79584480759e 100644 --- a/tests/e2e/components_test.go +++ b/tests/e2e/components_test.go @@ -90,7 +90,7 @@ func (c *ComponentTestCtx) ValidateComponentEnabled(t *testing.T) { c.DSCName, testf.Transform(`.spec.components.%s.managementState = "%s"`, strings.ToLower(c.GVK.Kind), operatorv1.Managed), ).Eventually().Should( - jq.Match(`.spec.components.%s.managementState == "%s"`, strings.ToLower(c.GVK.Kind), operatorv1.Managed), + Succeed(), ) g.List(gvk.DataScienceCluster).Eventually().Should(And( @@ -185,7 +185,7 @@ func (c *ComponentTestCtx) ValidateComponentDisabled(t *testing.T) { c.DSCName, testf.Transform(`.spec.components.%s.managementState = "%s"`, strings.ToLower(c.GVK.Kind), operatorv1.Removed), ).Eventually().Should( - jq.Match(`.spec.components.%s.managementState == "%s"`, strings.ToLower(c.GVK.Kind), operatorv1.Removed), + Succeed(), ) g.List(c.GVK).Eventually().Should( @@ -222,7 +222,7 @@ func (c *ComponentTestCtx) ValidateCRDReinstated(t *testing.T, name string, vers c.DSCName, testf.Transform(`.spec.components.%s.managementState = "%s"`, strings.ToLower(c.GVK.Kind), operatorv1.Removed), ).Eventually().Should( - jq.Match(`.spec.components.%s.managementState == "%s"`, strings.ToLower(c.GVK.Kind), operatorv1.Removed), + Succeed(), ) g.List(c.GVK).Eventually().Should( @@ -249,7 +249,7 @@ func (c *ComponentTestCtx) ValidateCRDReinstated(t *testing.T, name string, vers c.DSCName, testf.Transform(`.spec.components.%s.managementState = "%s"`, strings.ToLower(c.GVK.Kind), operatorv1.Managed), ).Eventually().Should( - jq.Match(`.spec.components.%s.managementState == "%s"`, strings.ToLower(c.GVK.Kind), operatorv1.Managed), + Succeed(), ) g.List(c.GVK).Eventually().Should( diff --git a/tests/e2e/dashboard_test.go b/tests/e2e/dashboard_test.go index 56d6f1007a36..9e0bc3a7bcef 100644 --- a/tests/e2e/dashboard_test.go +++ b/tests/e2e/dashboard_test.go @@ -56,7 +56,7 @@ func (c *DashboardTestCtx) validateOperandsDynamicallyWatchedResources(t *testin return nil }, ).Eventually().Should( - jq.Match(`.metadata.annotations."%s" == "%s"`, annotations.PlatformType, newPt), + Succeed(), ) g.List( diff --git a/tests/e2e/kueue_test.go b/tests/e2e/kueue_test.go index 4a2307794b58..2347cc596b61 100644 --- a/tests/e2e/kueue_test.go +++ b/tests/e2e/kueue_test.go @@ -131,7 +131,7 @@ func (tc *KueueTestCtx) validateKueuePreCheck(t *testing.T) { tc.DSCName, testf.Transform(`.spec.components.%s.managementState = "%s"`, strings.ToLower(tc.GVK.Kind), operatorv1.Managed), ).Eventually().Should( - jq.Match(`.spec.components.%s.managementState == "%s"`, strings.ToLower(tc.GVK.Kind), operatorv1.Managed), + Succeed(), ) g.List(gvk.DataScienceCluster).Eventually().Should(And( diff --git a/tests/e2e/modelcontroller_test.go b/tests/e2e/modelcontroller_test.go index fe58578d1052..1ad44a6b6323 100644 --- a/tests/e2e/modelcontroller_test.go +++ b/tests/e2e/modelcontroller_test.go @@ -75,10 +75,9 @@ func (c *ModelControllerTestCtx) validateComponentDeployed( testf.Transform(`.spec.components.%s.managementState = "%s"`, componentApi.ModelMeshServingComponentName, modelMeshState), testf.Transform(`.spec.components.%s.managementState = "%s"`, componentApi.KserveComponentName, kserveState), ), - ).Eventually().WithTimeout(30 * time.Second).WithPolling(1 * time.Second).Should(And( - jq.Match(`.spec.components.%s.managementState == "%s"`, componentApi.ModelMeshServingComponentName, modelMeshState), - jq.Match(`.spec.components.%s.managementState == "%s"`, componentApi.KserveComponentName, kserveState), - )) + ).Eventually().WithTimeout(30 * time.Second).WithPolling(1 * time.Second).Should( + Succeed(), + ) if status == metav1.ConditionTrue { g.List(gvk.ModelController).Eventually().Should(And( diff --git a/tests/e2e/modelregistry_test.go b/tests/e2e/modelregistry_test.go index 413226ef94a8..155c2bf92977 100644 --- a/tests/e2e/modelregistry_test.go +++ b/tests/e2e/modelregistry_test.go @@ -95,7 +95,7 @@ func (c *ModelRegistryTestCtx) validateOperandsDynamicallyWatchedResources(t *te oldPt = resources.SetAnnotation(obj, annotations.PlatformType, newPt) return nil }).Eventually().Should( - jq.Match(`.metadata.annotations."%s" == "%s"`, annotations.PlatformType, newPt), + Succeed(), ) g.List( diff --git a/tests/e2e/trustyai_test.go b/tests/e2e/trustyai_test.go index cb372875aa2e..6803377352f2 100644 --- a/tests/e2e/trustyai_test.go +++ b/tests/e2e/trustyai_test.go @@ -55,7 +55,7 @@ func (c *TrustyAITestCtx) enableKserve(t *testing.T) { c.DSCName, testf.Transform(`.spec.components.%s.managementState = "%s"`, componentApi.KserveComponentName, operatorv1.Managed), ).Eventually().Should( - jq.Match(`.spec.components.%s.managementState == "%s"`, componentApi.KserveComponentName, operatorv1.Managed), + Succeed(), ) g.List(gvk.Kserve).Eventually().Should(And( @@ -74,7 +74,7 @@ func (c *TrustyAITestCtx) disableKserve(t *testing.T) { c.DSCName, testf.Transform(`.spec.components.%s.managementState = "%s"`, componentApi.KserveComponentName, operatorv1.Removed), ).Eventually().Should( - jq.Match(`.spec.components.%s.managementState == "%s"`, componentApi.KserveComponentName, operatorv1.Removed), + Succeed(), ) g.List(gvk.Kserve).Eventually().Should( From 82adec0a26174a3ca59fb60de5f8125a7c2684c8 Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Tue, 11 Feb 2025 22:51:34 +0100 Subject: [PATCH 6/6] chore(kserve): wrap error returned by FromUnstructured (#1646) (cherry picked from commit 9402f6aff43d5381cc32d0055eabdc84dc70a10f) --- controllers/components/kserve/kserve_support.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/components/kserve/kserve_support.go b/controllers/components/kserve/kserve_support.go index 528cdbdceea6..3a374eaaeeaf 100644 --- a/controllers/components/kserve/kserve_support.go +++ b/controllers/components/kserve/kserve_support.go @@ -213,7 +213,7 @@ func getIndexedResource(rs []unstructured.Unstructured, obj any, g schema.GroupV err := runtime.DefaultUnstructuredConverter.FromUnstructured(rs[idx].Object, obj) if err != nil { - return idx, fmt.Errorf("failed converting to %T from Unstructured.Object: %v", obj, rs[idx].Object) + return idx, fmt.Errorf("failed converting to %T from unstructured %v: %w", obj, rs[idx].Object, err) } return idx, nil