From 10dd5543ca098e17d8265d29d3ba365867ec5b0a Mon Sep 17 00:00:00 2001 From: Bartosz Majsak Date: Fri, 13 Sep 2024 11:54:06 +0200 Subject: [PATCH] feat(tracker): allows to define ownerReferences for FeatureTrackers (#1228) * feat(tracker): feature tracker can have ownerReferences Introduces ability to define owner reference for FeatureTracker (internal custom resource). This enables garbage collection of resources belonging to the given Feature when its owner such as DSC or DSCI has been removed. When Features are grouped through means of `FeatureHandler`s, the ownership is defined under the hood, linking it to DSCI or DSC, depending on the type used. In addition, this PR simplifies how Feature relies on `client.Client` instance. Instead of creating its own instance it expects to have one passed as part of the builder call chain. It creates a default one otherwise. With that we can rely on a client configured for controllers with the right schemas, caching rules etc. As a consequence the change of retry logic for status needed to adjusted. Besides a conflict it needs to be triggered on NotFound error, as shared client's cache might not have yet an instance of FeatureTracker created just before the condition is reported. The reason why it was working before is the fact that Feature has its own simple client instance w/o caching. > [!IMPORTANT] > > With DSCI and DSC becoming owners of particular FeatureTrackers > `func (c *Component) Cleanup` called in finalizer block becomes > somewhat speculative. The only use case where custom function is > invoked is unpatching SMCP authorization provider. This was based > on early assumption that we might want to plug into customer's > existing SMCP instance. It's unclear to me if this is still long-term > thinking so we might want to revisit need for this function. > > From the completness point of view, if we allow to create/manipulate > resources programatically as part of the Feature DSL, we should be able > to register cleanup counter-part functions, especially if we cannot simply > remove them (as this is handled through ownership of associated > FeatureTracker). > > There is, however, a need to perform cleanup when particular > component is "Removed" (not managed anymore). Right now this is > handled inside its Reconcile function. Fixes [RHOAIENG-8629](https://issues.redhat.com/browse/RHOAIENG-8629) Relates to https://github.com/opendatahub-io/opendatahub-operator/pull/1192#issuecomment-2314308317 Co-authored-by: Bartosz Majsak * fix: no need to return dsci --------- Co-authored-by: Cameron Garrison --- components/component.go | 4 +- components/kserve/kserve.go | 12 +-- components/kserve/kserve_config_handler.go | 11 +-- components/kserve/servicemesh_setup.go | 11 +-- .../datasciencecluster_controller.go | 4 +- .../dscinitialization/servicemesh_setup.go | 4 +- controllers/status/reporter.go | 10 ++- pkg/cluster/gvk/gvk.go | 5 ++ pkg/cluster/meta.go | 2 + pkg/feature/builder.go | 85 ++++++++++--------- pkg/feature/feature.go | 1 + pkg/feature/feature_tracker_handler.go | 12 ++- pkg/feature/handler.go | 17 +++- pkg/feature/servicemesh/cleanup.go | 7 +- .../integration/features/cleanup_int_test.go | 19 ++--- .../features/features_suite_int_test.go | 2 + .../fixtures/cluster_test_fixtures.go | 43 +++++++--- .../features/manifests_int_test.go | 11 +-- .../features/preconditions_int_test.go | 10 +-- .../features/resources_int_test.go | 14 ++- .../features/serverless_feature_test.go | 47 +++++----- .../features/servicemesh_feature_test.go | 21 ++--- .../integration/features/tracker_int_test.go | 78 ++++++++++++----- 23 files changed, 260 insertions(+), 170 deletions(-) diff --git a/components/component.go b/components/component.go index 213ca9b8152..37f8151aa2a 100644 --- a/components/component.go +++ b/components/component.go @@ -43,7 +43,7 @@ func (c *Component) GetManagementState() operatorv1.ManagementState { return c.ManagementState } -func (c *Component) Cleanup(_ context.Context, _ client.Client, _ *dsciv1.DSCInitializationSpec) error { +func (c *Component) Cleanup(_ context.Context, _ client.Client, _ metav1.Object, _ *dsciv1.DSCInitializationSpec) error { // noop return nil } @@ -80,7 +80,7 @@ type ManifestsConfig struct { type ComponentInterface interface { ReconcileComponent(ctx context.Context, cli client.Client, logger logr.Logger, owner metav1.Object, DSCISpec *dsciv1.DSCInitializationSpec, platform cluster.Platform, currentComponentStatus bool) error - Cleanup(ctx context.Context, cli client.Client, DSCISpec *dsciv1.DSCInitializationSpec) error + Cleanup(ctx context.Context, cli client.Client, owner metav1.Object, DSCISpec *dsciv1.DSCInitializationSpec) error GetComponentName() string GetManagementState() operatorv1.ManagementState OverrideManifests(ctx context.Context, platform cluster.Platform) error diff --git a/components/kserve/kserve.go b/components/kserve/kserve.go index fe2eb2a994d..a04bd17dd9f 100644 --- a/components/kserve/kserve.go +++ b/components/kserve/kserve.go @@ -107,12 +107,12 @@ func (k *Kserve) ReconcileComponent(ctx context.Context, cli client.Client, monitoringEnabled := dscispec.Monitoring.ManagementState == operatorv1.Managed if !enabled { - if err := k.removeServerlessFeatures(ctx, dscispec); err != nil { + if err := k.removeServerlessFeatures(ctx, cli, owner, dscispec); err != nil { return err } } else { // Configure dependencies - if err := k.configureServerless(ctx, cli, l, dscispec); err != nil { + if err := k.configureServerless(ctx, cli, l, owner, dscispec); err != nil { return err } if k.DevFlags != nil { @@ -123,7 +123,7 @@ func (k *Kserve) ReconcileComponent(ctx context.Context, cli client.Client, } } - if err := k.configureServiceMesh(ctx, cli, dscispec); err != nil { + if err := k.configureServiceMesh(ctx, cli, owner, dscispec); err != nil { return fmt.Errorf("failed configuring service mesh while reconciling kserve component. cause: %w", err) } @@ -177,10 +177,10 @@ func (k *Kserve) ReconcileComponent(ctx context.Context, cli client.Client, return nil } -func (k *Kserve) Cleanup(ctx context.Context, cli client.Client, instance *dsciv1.DSCInitializationSpec) error { - if removeServerlessErr := k.removeServerlessFeatures(ctx, instance); removeServerlessErr != nil { +func (k *Kserve) Cleanup(ctx context.Context, cli client.Client, owner metav1.Object, instance *dsciv1.DSCInitializationSpec) error { + if removeServerlessErr := k.removeServerlessFeatures(ctx, cli, owner, instance); removeServerlessErr != nil { return removeServerlessErr } - return k.removeServiceMeshConfigurations(ctx, cli, instance) + return k.removeServiceMeshConfigurations(ctx, cli, owner, instance) } diff --git a/components/kserve/kserve_config_handler.go b/components/kserve/kserve_config_handler.go index 46df279e753..8479e207053 100644 --- a/components/kserve/kserve_config_handler.go +++ b/components/kserve/kserve_config_handler.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/go-multierror" operatorv1 "github.com/openshift/api/operator/v1" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" @@ -118,14 +119,14 @@ func (k *Kserve) setDefaultDeploymentMode(ctx context.Context, cli client.Client return nil } -func (k *Kserve) configureServerless(ctx context.Context, cli client.Client, logger logr.Logger, instance *dsciv1.DSCInitializationSpec) error { +func (k *Kserve) configureServerless(ctx context.Context, cli client.Client, logger logr.Logger, owner metav1.Object, instance *dsciv1.DSCInitializationSpec) error { switch k.Serving.ManagementState { case operatorv1.Unmanaged: // Bring your own CR logger.Info("Serverless CR is not configured by the operator, we won't do anything") case operatorv1.Removed: // we remove serving CR logger.Info("existing Serverless CR (owned by operator) will be removed") - if err := k.removeServerlessFeatures(ctx, instance); err != nil { + if err := k.removeServerlessFeatures(ctx, cli, owner, instance); err != nil { return err } @@ -147,7 +148,7 @@ func (k *Kserve) configureServerless(ctx context.Context, cli client.Client, log return dependOpsErrors } - serverlessFeatures := feature.ComponentFeaturesHandler(k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance)) + serverlessFeatures := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance)) if err := serverlessFeatures.Apply(ctx); err != nil { return err @@ -156,8 +157,8 @@ func (k *Kserve) configureServerless(ctx context.Context, cli client.Client, log return nil } -func (k *Kserve) removeServerlessFeatures(ctx context.Context, instance *dsciv1.DSCInitializationSpec) error { - serverlessFeatures := feature.ComponentFeaturesHandler(k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance)) +func (k *Kserve) removeServerlessFeatures(ctx context.Context, cli client.Client, owner metav1.Object, instance *dsciv1.DSCInitializationSpec) error { + serverlessFeatures := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance)) return serverlessFeatures.Delete(ctx) } diff --git a/components/kserve/servicemesh_setup.go b/components/kserve/servicemesh_setup.go index 6853e69eae3..23e3c5321bb 100644 --- a/components/kserve/servicemesh_setup.go +++ b/components/kserve/servicemesh_setup.go @@ -6,6 +6,7 @@ import ( "path" operatorv1 "github.com/openshift/api/operator/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -16,10 +17,10 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/servicemesh" ) -func (k *Kserve) configureServiceMesh(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) error { +func (k *Kserve) configureServiceMesh(ctx context.Context, cli client.Client, owner metav1.Object, dscispec *dsciv1.DSCInitializationSpec) error { if dscispec.ServiceMesh != nil { if dscispec.ServiceMesh.ManagementState == operatorv1.Managed && k.GetManagementState() == operatorv1.Managed { - serviceMeshInitializer := feature.ComponentFeaturesHandler(k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec)) + serviceMeshInitializer := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec)) return serviceMeshInitializer.Apply(ctx) } if dscispec.ServiceMesh.ManagementState == operatorv1.Unmanaged && k.GetManagementState() == operatorv1.Managed { @@ -27,11 +28,11 @@ func (k *Kserve) configureServiceMesh(ctx context.Context, cli client.Client, ds } } - return k.removeServiceMeshConfigurations(ctx, cli, dscispec) + return k.removeServiceMeshConfigurations(ctx, cli, owner, dscispec) } -func (k *Kserve) removeServiceMeshConfigurations(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) error { - serviceMeshInitializer := feature.ComponentFeaturesHandler(k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec)) +func (k *Kserve) removeServiceMeshConfigurations(ctx context.Context, cli client.Client, owner metav1.Object, dscispec *dsciv1.DSCInitializationSpec) error { + serviceMeshInitializer := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec)) return serviceMeshInitializer.Delete(ctx) } diff --git a/controllers/datasciencecluster/datasciencecluster_controller.go b/controllers/datasciencecluster/datasciencecluster_controller.go index 119c0d63dfd..69181fd4536 100644 --- a/controllers/datasciencecluster/datasciencecluster_controller.go +++ b/controllers/datasciencecluster/datasciencecluster_controller.go @@ -137,7 +137,7 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R } } for _, component := range allComponents { - if err := component.Cleanup(ctx, r.Client, r.DataScienceCluster.DSCISpec); err != nil { + if err := component.Cleanup(ctx, r.Client, instance, r.DataScienceCluster.DSCISpec); err != nil { return ctrl.Result{}, err } } @@ -188,7 +188,7 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R } else { r.Log.Info("Finalization DataScienceCluster start deleting instance", "name", instance.Name, "finalizer", finalizerName) for _, component := range allComponents { - if err := component.Cleanup(ctx, r.Client, r.DataScienceCluster.DSCISpec); err != nil { + if err := component.Cleanup(ctx, r.Client, instance, r.DataScienceCluster.DSCISpec); err != nil { return ctrl.Result{}, err } } diff --git a/controllers/dscinitialization/servicemesh_setup.go b/controllers/dscinitialization/servicemesh_setup.go index 283a0f33570..7b5e6bc0977 100644 --- a/controllers/dscinitialization/servicemesh_setup.go +++ b/controllers/dscinitialization/servicemesh_setup.go @@ -91,7 +91,7 @@ func (r *DSCInitializationReconciler) removeServiceMesh(ctx context.Context, ins func (r *DSCInitializationReconciler) serviceMeshCapability(instance *dsciv1.DSCInitialization, initialCondition *conditionsv1.Condition) *feature.HandlerWithReporter[*dsciv1.DSCInitialization] { //nolint:lll // Reason: generics are long return feature.NewHandlerWithReporter( - feature.ClusterFeaturesHandler(instance, r.serviceMeshCapabilityFeatures(instance)), + feature.ClusterFeaturesHandler(r.Client, instance, r.serviceMeshCapabilityFeatures(instance)), createCapabilityReporter(r.Client, instance, initialCondition), ) } @@ -119,7 +119,7 @@ func (r *DSCInitializationReconciler) authorizationCapability(ctx context.Contex } return feature.NewHandlerWithReporter( - feature.ClusterFeaturesHandler(instance, r.authorizationFeatures(instance)), + feature.ClusterFeaturesHandler(r.Client, instance, r.authorizationFeatures(instance)), createCapabilityReporter(r.Client, instance, condition), ), nil } diff --git a/controllers/status/reporter.go b/controllers/status/reporter.go index de9dbe7e6fc..13f4c5da32d 100644 --- a/controllers/status/reporter.go +++ b/controllers/status/reporter.go @@ -4,6 +4,7 @@ import ( "context" "errors" + k8serr "k8s.io/apimachinery/pkg/api/errors" "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -44,7 +45,7 @@ func UpdateWithRetry[T client.Object](ctx context.Context, cli client.Client, or if !ok { return *new(T), errors.New("failed to deep copy object") } - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + err := retry.OnError(retry.DefaultBackoff, retryOnNotFoundOrConflict, func() error { if err := cli.Get(ctx, client.ObjectKeyFromObject(original), saved); err != nil { return err } @@ -52,9 +53,14 @@ func UpdateWithRetry[T client.Object](ctx context.Context, cli client.Client, or update(saved) // Return err itself here (not wrapped inside another error) - // so that RetryOnConflict can identify it correctly. + // so that Retry can identify it correctly. return cli.Status().Update(ctx, saved) }) return saved, err } + +func retryOnNotFoundOrConflict(err error) bool { + // We are now sharing the client, read/write can occur on delay + return k8serr.IsConflict(err) || k8serr.IsNotFound(err) +} diff --git a/pkg/cluster/gvk/gvk.go b/pkg/cluster/gvk/gvk.go index 836deb687bc..0415304abe2 100644 --- a/pkg/cluster/gvk/gvk.go +++ b/pkg/cluster/gvk/gvk.go @@ -14,6 +14,11 @@ var ( Version: "v1", Kind: "DataScienceCluster", } + DSCInitialization = schema.GroupVersionKind{ + Group: "dscinitialization.opendatahub.io", + Version: "v1", + Kind: "DSCInitialization", + } Deployment = schema.GroupVersionKind{ Group: "apps", diff --git a/pkg/cluster/meta.go b/pkg/cluster/meta.go index c3e8d9a96a2..659aa8112df 100644 --- a/pkg/cluster/meta.go +++ b/pkg/cluster/meta.go @@ -29,6 +29,8 @@ func WithOwnerReference(ownerReferences ...metav1.OwnerReference) MetaOptions { } } +// OwnedBy sets the owner reference for the object being created. It requires scheme to be passed +// as TypeMeta might not be set for the owning object, see: https://github.com/kubernetes-sigs/controller-runtime/issues/1517 func OwnedBy(owner metav1.Object, scheme *runtime.Scheme) MetaOptions { return func(obj metav1.Object) error { return controllerutil.SetOwnerReference(owner, obj, scheme) diff --git a/pkg/feature/builder.go b/pkg/feature/builder.go index e182c716be3..9cb53c58a70 100644 --- a/pkg/feature/builder.go +++ b/pkg/feature/builder.go @@ -5,9 +5,9 @@ import ( "fmt" "github.com/hashicorp/go-multierror" - ofapiv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" "github.com/pkg/errors" apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" "sigs.k8s.io/controller-runtime/pkg/client" @@ -24,9 +24,10 @@ type featureBuilder struct { featureName string managed bool source featurev1.Source + owner metav1.Object targetNs string - config *rest.Config + client client.Client builders []partialBuilder } @@ -94,6 +95,14 @@ func (fb *featureBuilder) Manifests(creators ...resource.Creator) *featureBuilde return fb } +// OwnedBy is optionally used to pass down the owning object in order to set the ownerReference +// in the corresponding feature tracker. +func (fb *featureBuilder) OwnedBy(object metav1.Object) *featureBuilder { + fb.owner = object + + return fb +} + // Managed marks the feature as managed by the operator. This effectively marks all resources which are part of this feature // as those that should be updated on operator reconcile. // Managed marks the feature as managed by the operator. @@ -184,6 +193,12 @@ func (fb *featureBuilder) OnDelete(cleanups ...CleanupFunc) *featureBuilder { return fb } +// UsingClient allows to provide a custom client to the feature. If not called, a default client will be created. +func (fb *featureBuilder) UsingClient(cli client.Client) *featureBuilder { + fb.client = cli + return fb +} + // Create creates a new Feature instance and add it to corresponding FeaturesHandler. // The actual feature creation in the cluster is not performed here. func (fb *featureBuilder) Create() (*Feature, error) { @@ -197,18 +212,16 @@ func (fb *featureBuilder) Create() (*Feature, error) { Enabled: alwaysEnabled, Log: log.Log.WithName("features").WithValues("feature", fb.featureName), source: &fb.source, + owner: fb.owner, } - // UsingConfig builder wasn't called while constructing this feature. - // Get default settings and create needed clients. - if fb.config == nil { - if err := fb.withDefaultClient(); err != nil { + // UsingClient has not been called, so we need to create a new client + if fb.client == nil { + if err := createDefaultClient()(f); err != nil { return nil, err } - } - - if err := createClient(fb.config)(f); err != nil { - return nil, err + } else { + f.Client = fb.client } for i := range fb.builders { @@ -220,46 +233,38 @@ func (fb *featureBuilder) Create() (*Feature, error) { return f, nil } -// UsingConfig allows to pass a custom rest.Config to the feature. Useful for testing. -func (fb *featureBuilder) UsingConfig(config *rest.Config) *featureBuilder { - fb.config = config - return fb -} - -func createClient(config *rest.Config) partialBuilder { +func createDefaultClient() partialBuilder { return func(f *Feature) error { var err error - f.Client, err = client.New(config, client.Options{}) + restCfg, err := config.GetConfig() + if errors.Is(err, rest.ErrNotInCluster) { + // rollback to local kubeconfig - this can be helpful when running the process locally i.e. while debugging + kubeconfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig( + &clientcmd.ClientConfigLoadingRules{ExplicitPath: clientcmd.RecommendedHomeFile}, + &clientcmd.ConfigOverrides{}, + ) + + restCfg, err = kubeconfig.ClientConfig() + if err != nil { + return err + } + } else if err != nil { + return err + } + + f.Client, err = client.New(restCfg, client.Options{}) if err != nil { return errors.WithStack(err) } var multiErr *multierror.Error s := f.Client.Scheme() - multiErr = multierror.Append(multiErr, featurev1.AddToScheme(s), apiextv1.AddToScheme(s), ofapiv1alpha1.AddToScheme(s)) - - return multiErr.ErrorOrNil() - } -} - -func (fb *featureBuilder) withDefaultClient() error { - restCfg, err := config.GetConfig() - if errors.Is(err, rest.ErrNotInCluster) { - // rollback to local kubeconfig - this can be helpful when running the process locally i.e. while debugging - kubeconfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig( - &clientcmd.ClientConfigLoadingRules{ExplicitPath: clientcmd.RecommendedHomeFile}, - &clientcmd.ConfigOverrides{}, + multiErr = multierror.Append(multiErr, + featurev1.AddToScheme(s), + apiextv1.AddToScheme(s), ) - restCfg, err = kubeconfig.ClientConfig() - if err != nil { - return err - } - } else if err != nil { - return err + return multiErr.ErrorOrNil() } - - fb.config = restCfg - return nil } diff --git a/pkg/feature/feature.go b/pkg/feature/feature.go index 967e284cacc..3a16a2b9a40 100644 --- a/pkg/feature/feature.go +++ b/pkg/feature/feature.go @@ -46,6 +46,7 @@ type Feature struct { tracker *featurev1.FeatureTracker source *featurev1.Source + owner metav1.Object data map[string]any diff --git a/pkg/feature/feature_tracker_handler.go b/pkg/feature/feature_tracker_handler.go index 403d87abbea..6ca1f682941 100644 --- a/pkg/feature/feature_tracker_handler.go +++ b/pkg/feature/feature_tracker_handler.go @@ -11,6 +11,7 @@ import ( featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" ) // withConditionReasonError is a wrapper around an error which provides a reason for a feature condition. @@ -41,13 +42,20 @@ func createFeatureTracker(ctx context.Context, f *Feature) error { Source: *f.source, AppNamespace: f.TargetNamespace, } + if f.owner != nil { + ownerRef := cluster.OwnedBy(f.owner, f.Client.Scheme()) + if errMetaOpts := cluster.ApplyMetaOptions(tracker, ownerRef); errMetaOpts != nil { + return fmt.Errorf("failed adding owner to FeatureTracker %s: %w", tracker.Name, errMetaOpts) + } + } + if errCreate := f.Client.Create(ctx, tracker); errCreate != nil { - return errCreate + return fmt.Errorf("failed creating FeatureTracker %s: %w", tracker.Name, errCreate) } } if errGVK := ensureGVKSet(tracker, f.Client.Scheme()); errGVK != nil { - return errGVK + return fmt.Errorf("failed ensuring GVK is set for %s: %w", tracker.Name, errGVK) } f.tracker = tracker diff --git a/pkg/feature/handler.go b/pkg/feature/handler.go index c6434b7d687..834234adee0 100644 --- a/pkg/feature/handler.go +++ b/pkg/feature/handler.go @@ -6,6 +6,7 @@ import ( "fmt" "github.com/hashicorp/go-multierror" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" @@ -27,8 +28,10 @@ var _ featuresHandler = (*FeaturesHandler)(nil) // FeaturesHandler provides a structured way to manage and coordinate the creation, application, // and deletion of features needed in particular Data Science Cluster configuration. type FeaturesHandler struct { - targetNamespace string + cli client.Client source featurev1.Source + owner metav1.Object + targetNamespace string features []*Feature featuresProviders []FeaturesProvider } @@ -42,7 +45,9 @@ func (fh *FeaturesHandler) Add(builders ...*featureBuilder) error { for i := range builders { fb := builders[i] - feature, err := fb.TargetNamespace(fh.targetNamespace). + feature, err := fb.UsingClient(fh.cli). + TargetNamespace(fh.targetNamespace). + OwnedBy(fh.owner). Source(fh.source). Create() multiErr = multierror.Append(multiErr, err) @@ -96,16 +101,20 @@ func (fh *FeaturesHandler) Delete(ctx context.Context) error { // and add them to the handler's registry. type FeaturesProvider func(registry FeaturesRegistry) error -func ClusterFeaturesHandler(dsci *dsciv1.DSCInitialization, def ...FeaturesProvider) *FeaturesHandler { +func ClusterFeaturesHandler(cli client.Client, dsci *dsciv1.DSCInitialization, def ...FeaturesProvider) *FeaturesHandler { return &FeaturesHandler{ + cli: cli, targetNamespace: dsci.Spec.ApplicationsNamespace, + owner: dsci, source: featurev1.Source{Type: featurev1.DSCIType, Name: dsci.Name}, featuresProviders: def, } } -func ComponentFeaturesHandler(componentName, targetNamespace string, def ...FeaturesProvider) *FeaturesHandler { +func ComponentFeaturesHandler(cli client.Client, owner metav1.Object, componentName, targetNamespace string, def ...FeaturesProvider) *FeaturesHandler { return &FeaturesHandler{ + cli: cli, + owner: owner, targetNamespace: targetNamespace, source: featurev1.Source{Type: featurev1.ComponentType, Name: componentName}, featuresProviders: def, diff --git a/pkg/feature/servicemesh/cleanup.go b/pkg/feature/servicemesh/cleanup.go index 73ccc211c49..a8c4a61dcd4 100644 --- a/pkg/feature/servicemesh/cleanup.go +++ b/pkg/feature/servicemesh/cleanup.go @@ -4,6 +4,7 @@ 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" @@ -55,7 +56,11 @@ func RemoveExtensionProvider(controlPlane infrav1.ControlPlaneSpec, extensionNam } if removed { - return cli.Update(ctx, smcp) + // 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/cleanup_int_test.go b/tests/integration/features/cleanup_int_test.go index ea5f7308b10..9e2d6103b03 100644 --- a/tests/integration/features/cleanup_int_test.go +++ b/tests/integration/features/cleanup_int_test.go @@ -21,8 +21,7 @@ import ( var _ = Describe("feature cleanup", func() { - Context("using FeatureTracker and ownership as cleanup strategy", Ordered, func() { - + Context("using FeatureTracker and ownership as cleanup strategy", func() { const ( featureName = "create-secret" secretName = "test-secret" @@ -34,9 +33,9 @@ var _ = Describe("feature cleanup", func() { testFeature *feature.Feature ) - BeforeAll(func() { + BeforeEach(func(ctx context.Context) { namespace = envtestutil.AppendRandomNameTo("test-secret-ownership") - dsci = fixtures.NewDSCInitialization(namespace) + dsci = fixtures.NewDSCInitialization(ctx, envTestClient, namespace) var errSecretCreation error testFeature, errSecretCreation = feature.Define(featureName). TargetNamespace(dsci.Spec.ApplicationsNamespace). @@ -44,7 +43,7 @@ var _ = Describe("feature cleanup", func() { Type: featurev1.DSCIType, Name: dsci.Name, }). - UsingConfig(envTest.Config). + UsingClient(envTestClient). PreConditions( feature.CreateNamespaceIfNotExists(namespace), ). @@ -94,9 +93,9 @@ var _ = Describe("feature cleanup", func() { featuresHandler *feature.FeaturesHandler ) - BeforeAll(func() { + BeforeAll(func(ctx context.Context) { namespace = envtestutil.AppendRandomNameTo("test-conditional-cleanup") - dsci = fixtures.NewDSCInitialization(namespace) + dsci = fixtures.NewDSCInitialization(ctx, envTestClient, namespace) }) It("should create feature, apply resource and create feature tracker", func(ctx context.Context) { @@ -104,9 +103,8 @@ var _ = Describe("feature cleanup", func() { err := fixtures.CreateOrUpdateNamespace(ctx, envTestClient, fixtures.NewNamespace("conditional-ns")) Expect(err).To(Not(HaveOccurred())) - featuresHandler = feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler = feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { return registry.Add(feature.Define(featureName). - UsingConfig(envTest.Config). EnabledWhen(namespaceExists). PreConditions( feature.CreateNamespaceIfNotExists(namespace), @@ -132,9 +130,8 @@ var _ = Describe("feature cleanup", func() { Expect(err).To(Not(HaveOccurred())) // Mimic reconcile by re-loading the feature handler - featuresHandler = feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler = feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { return registry.Add(feature.Define(featureName). - UsingConfig(envTest.Config). EnabledWhen(namespaceExists). PreConditions( feature.CreateNamespaceIfNotExists(namespace), diff --git a/tests/integration/features/features_suite_int_test.go b/tests/integration/features/features_suite_int_test.go index bda9122ed31..1dcf8d68c5a 100644 --- a/tests/integration/features/features_suite_int_test.go +++ b/tests/integration/features/features_suite_int_test.go @@ -15,6 +15,7 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" + dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" "github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil" @@ -51,6 +52,7 @@ var _ = BeforeSuite(func() { utilruntime.Must(featurev1.AddToScheme(testScheme)) utilruntime.Must(apiextensionsv1.AddToScheme(testScheme)) utilruntime.Must(ofapiv1alpha1.AddToScheme(testScheme)) + utilruntime.Must(dsciv1.AddToScheme(testScheme)) envTest = &envtest.Environment{ CRDInstallOptions: envtest.CRDInstallOptions{ diff --git a/tests/integration/features/fixtures/cluster_test_fixtures.go b/tests/integration/features/fixtures/cluster_test_fixtures.go index c6cb8f319ee..5105b5bda89 100644 --- a/tests/integration/features/fixtures/cluster_test_fixtures.go +++ b/tests/integration/features/fixtures/cluster_test_fixtures.go @@ -3,6 +3,7 @@ package fixtures import ( "context" + "github.com/onsi/gomega" ofapiv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -14,6 +15,7 @@ import ( dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" 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" ) @@ -44,6 +46,15 @@ func createOrUpdateSubscription(ctx context.Context, client client.Client, subsc return err } +func CreateOrUpdateDSCI(ctx context.Context, client client.Client, dsci *dsciv1.DSCInitialization) error { + _, err := controllerutil.CreateOrUpdate(ctx, client, dsci, func() error { + return nil + }) + dsci.APIVersion = dsciv1.GroupVersion.String() + dsci.Kind = gvk.DSCInitialization.Kind + return err +} + func NewNamespace(name string) *corev1.Namespace { return &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -52,15 +63,6 @@ func NewNamespace(name string) *corev1.Namespace { } } -func GetConfigMap(client client.Client, namespace, name string) (*corev1.ConfigMap, error) { - cfgMap := &corev1.ConfigMap{} - err := client.Get(context.Background(), types.NamespacedName{ - Name: name, Namespace: namespace, - }, cfgMap) - - return cfgMap, err -} - func GetNamespace(ctx context.Context, client client.Client, namespace string) (*corev1.Namespace, error) { ns := NewNamespace(namespace) err := client.Get(ctx, types.NamespacedName{Name: namespace}, ns) @@ -105,12 +107,19 @@ func GetFeatureTracker(ctx context.Context, cli client.Client, appNamespace, fea return tracker, err } -func NewDSCInitialization(ns string) *dsciv1.DSCInitialization { - return &dsciv1.DSCInitialization{ +func NewDSCInitialization(ctx context.Context, cli client.Client, ns string) *dsciv1.DSCInitialization { + dsci := &dsciv1.DSCInitialization{ + TypeMeta: metav1.TypeMeta{ + APIVersion: gvk.DSCInitialization.Version, + Kind: gvk.DSCInitialization.Kind, + }, ObjectMeta: metav1.ObjectMeta{ Name: "default-dsci", }, - Spec: dsciv1.DSCInitializationSpec{ + } + + _, errCreate := controllerutil.CreateOrUpdate(ctx, cli, dsci, func() error { + dsci.Spec = dsciv1.DSCInitializationSpec{ ApplicationsNamespace: ns, ServiceMesh: &infrav1.ServiceMeshSpec{ ManagementState: "Managed", @@ -120,6 +129,12 @@ func NewDSCInitialization(ns string) *dsciv1.DSCInitialization { MetricsCollection: "Istio", }, }, - }, - } + } + + return nil + }) + + gomega.Expect(errCreate).ToNot(gomega.HaveOccurred()) + + return dsci } diff --git a/tests/integration/features/manifests_int_test.go b/tests/integration/features/manifests_int_test.go index 6f084c29663..196140a98d8 100644 --- a/tests/integration/features/manifests_int_test.go +++ b/tests/integration/features/manifests_int_test.go @@ -35,7 +35,7 @@ var _ = Describe("Applying resources", func() { namespace, err = cluster.CreateNamespace(ctx, envTestClient, nsName) Expect(err).ToNot(HaveOccurred()) - dsci = fixtures.NewDSCInitialization(nsName) + dsci = fixtures.NewDSCInitialization(ctx, envTestClient, nsName) dsci.Spec.ServiceMesh.ControlPlane.Namespace = namespace.Name }) @@ -45,9 +45,8 @@ var _ = Describe("Applying resources", func() { It("should be able to process an embedded YAML file", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { errNsCreate := registry.Add(feature.Define("create-namespaces"). - UsingConfig(envTest.Config). Manifests( manifest.Location(fixtures.TestEmbeddedFiles). Include(path.Join(fixtures.BaseDir, "namespaces.yaml")), @@ -76,9 +75,8 @@ var _ = Describe("Applying resources", func() { It("should be able to process an embedded template file", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { errSvcCreate := registry.Add(feature.Define("create-local-gw-svc"). - UsingConfig(envTest.Config). Manifests( manifest.Location(fixtures.TestEmbeddedFiles). Include(path.Join(fixtures.BaseDir, "local-gateway-svc.tmpl.yaml")), @@ -111,9 +109,8 @@ metadata: Expect(fixtures.CreateFile(tempDir, "namespace.yaml", nsYAML)).To(Succeed()) - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { errSvcCreate := registry.Add(feature.Define("create-namespace"). - UsingConfig(envTest.Config). Manifests( manifest.Location(os.DirFS(tempDir)). Include(path.Join("namespace.yaml")), // must be relative to root DirFS defined above diff --git a/tests/integration/features/preconditions_int_test.go b/tests/integration/features/preconditions_int_test.go index f1310b13f89..dae5aacc5a9 100644 --- a/tests/integration/features/preconditions_int_test.go +++ b/tests/integration/features/preconditions_int_test.go @@ -26,12 +26,12 @@ var _ = Describe("feature preconditions", func() { dsci *dsciv1.DSCInitialization ) - BeforeEach(func() { + BeforeEach(func(ctx context.Context) { objectCleaner = envtestutil.CreateCleaner(envTestClient, envTest.Config, fixtures.Timeout, fixtures.Interval) testFeatureName := "test-ns-creation" namespace = envtestutil.AppendRandomNameTo(testFeatureName) - dsci = fixtures.NewDSCInitialization(namespace) + dsci = fixtures.NewDSCInitialization(ctx, envTestClient, namespace) }) It("should create namespace if it does not exist", func(ctx context.Context) { @@ -41,9 +41,8 @@ var _ = Describe("feature preconditions", func() { defer objectCleaner.DeleteAll(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}}) // when - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("create-new-ns"). - UsingConfig(envTest.Config). PreConditions(feature.CreateNamespaceIfNotExists(namespace)), ) @@ -77,9 +76,8 @@ var _ = Describe("feature preconditions", func() { defer objectCleaner.DeleteAll(ctx, ns) // when - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("create-new-ns"). - UsingConfig(envTest.Config). PreConditions(feature.CreateNamespaceIfNotExists(namespace)), ) diff --git a/tests/integration/features/resources_int_test.go b/tests/integration/features/resources_int_test.go index af778b2348b..3068c511905 100644 --- a/tests/integration/features/resources_int_test.go +++ b/tests/integration/features/resources_int_test.go @@ -42,7 +42,7 @@ var _ = Describe("Applying and updating resources", func() { namespace, err = cluster.CreateNamespace(ctx, envTestClient, testNamespace) Expect(err).ToNot(HaveOccurred()) - dsci = fixtures.NewDSCInitialization(testNamespace) + dsci = fixtures.NewDSCInitialization(ctx, envTestClient, testNamespace) dsci.Spec.ServiceMesh.ControlPlane.Namespace = namespace.Name }) @@ -54,10 +54,9 @@ var _ = Describe("Applying and updating resources", func() { It("should reconcile the resource to its managed state", func(ctx context.Context) { // given managed feature - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { return registry.Add( feature.Define("create-local-gw-svc"). - UsingConfig(envTest.Config). Managed(). Manifests( manifest.Location(fixtures.TestEmbeddedFiles). @@ -91,10 +90,9 @@ var _ = Describe("Applying and updating resources", func() { It("should not reconcile explicitly opt-ed out resource", func(ctx context.Context) { // given managed feature - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { return registry.Add( feature.Define("create-unmanaged-svc"). - UsingConfig(envTest.Config). Managed(). Manifests( manifest.Location(fixtures.TestEmbeddedFiles). @@ -128,10 +126,9 @@ var _ = Describe("Applying and updating resources", func() { It("should not reconcile the resource", func(ctx context.Context) { // given unmanaged feature - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { return registry.Add( feature.Define("create-local-gw-svc"). - UsingConfig(envTest.Config). Manifests( manifest.Location(fixtures.TestEmbeddedFiles). Include(path.Join(fixtures.BaseDir, "local-gateway-svc.tmpl.yaml")), @@ -162,10 +159,9 @@ var _ = Describe("Applying and updating resources", func() { When("a feature is unmanaged but the object is marked as managed", func() { It("should reconcile this resource", func(ctx context.Context) { // given unmanaged feature but object marked with managed annotation - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { return registry.Add( feature.Define("create-managed-svc"). - UsingConfig(envTest.Config). Manifests( manifest.Location(fixtures.TestEmbeddedFiles). Include(path.Join(fixtures.BaseDir, "managed-svc.tmpl.yaml")), diff --git a/tests/integration/features/serverless_feature_test.go b/tests/integration/features/serverless_feature_test.go index d611b9428a1..9258aa78d10 100644 --- a/tests/integration/features/serverless_feature_test.go +++ b/tests/integration/features/serverless_feature_test.go @@ -34,13 +34,13 @@ var _ = Describe("Serverless feature", func() { kserveComponent *kserve.Kserve ) - BeforeEach(func() { + BeforeEach(func(ctx context.Context) { // TODO rework c, err := client.New(envTest.Config, client.Options{}) Expect(err).ToNot(HaveOccurred()) objectCleaner = envtestutil.CreateCleaner(c, envTest.Config, fixtures.Timeout, fixtures.Interval) - dsci = fixtures.NewDSCInitialization("default") + dsci = fixtures.NewDSCInitialization(ctx, envTestClient, "default") kserveComponent = &kserve.Kserve{} }) @@ -50,17 +50,18 @@ var _ = Describe("Serverless feature", func() { It("should fail on precondition check", func(ctx context.Context) { // given - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + featuresProvider := func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add( feature.Define("no-serverless-operator-check"). - UsingConfig(envTest.Config). PreConditions(serverless.EnsureServerlessOperatorInstalled), ) Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil - }) + } + + featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // when applyErr := featuresHandler.Apply(ctx) @@ -97,17 +98,18 @@ var _ = Describe("Serverless feature", func() { It("should succeed checking operator installation using precondition", func(ctx context.Context) { // when - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + featuresProvider := func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add( feature.Define("serverless-operator-check"). - UsingConfig(envTest.Config). PreConditions(serverless.EnsureServerlessOperatorInstalled), ) Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil - }) + } + + featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // then Expect(featuresHandler.Apply(ctx)).To(Succeed()) @@ -115,17 +117,18 @@ var _ = Describe("Serverless feature", func() { It("should succeed if serving is not installed for KNative serving precondition", func(ctx context.Context) { // when - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + featuresProvider := func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add( feature.Define("no-serving-installed-yet"). - UsingConfig(envTest.Config). PreConditions(serverless.EnsureServerlessAbsent), ) Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil - }) + } + + featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // then Expect(featuresHandler.Apply(ctx)).To(Succeed()) @@ -144,17 +147,18 @@ var _ = Describe("Serverless feature", func() { Expect(envTestClient.Create(ctx, knativeServing)).To(Succeed()) // when - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + featuresProvider := func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add( feature.Define("serving-already-installed"). - UsingConfig(envTest.Config). PreConditions(serverless.EnsureServerlessAbsent), ) Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil - }) + } + + featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // then Expect(featuresHandler.Apply(ctx)).ToNot(Succeed()) @@ -279,10 +283,9 @@ var _ = Describe("Serverless feature", func() { kserveComponent.Serving.IngressGateway.Certificate.Type = infrav1.SelfSigned kserveComponent.Serving.IngressGateway.Domain = fixtures.TestDomainFooCom - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + featuresProvider := func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add( feature.Define("tls-secret-creation"). - UsingConfig(envTest.Config). WithData( servicemesh.FeatureData.ControlPlane.Define(&dsci.Spec).AsAction(), serverless.FeatureData.Serving.Define(&kserveComponent.Serving).AsAction(), @@ -295,7 +298,9 @@ var _ = Describe("Serverless feature", func() { Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil - }) + } + + featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // when Expect(featuresHandler.Apply(ctx)).To(Succeed()) @@ -319,10 +324,10 @@ var _ = Describe("Serverless feature", func() { // given kserveComponent.Serving.IngressGateway.Certificate.Type = infrav1.Provided kserveComponent.Serving.IngressGateway.Domain = fixtures.TestDomainFooCom - featuresHandler := feature.ComponentFeaturesHandler(kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, func(registry feature.FeaturesRegistry) error { + + featuresProvider := func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add( feature.Define("tls-secret-creation"). - UsingConfig(envTest.Config). WithData( servicemesh.FeatureData.ControlPlane.Define(&dsci.Spec).AsAction(), serverless.FeatureData.Serving.Define(&kserveComponent.Serving).AsAction(), @@ -335,7 +340,9 @@ var _ = Describe("Serverless feature", func() { Expect(errFeatureAdd).ToNot(HaveOccurred()) return nil - }) + } + + featuresHandler := feature.ComponentFeaturesHandler(envTestClient, dsci, kserveComponent.GetComponentName(), dsci.Spec.ApplicationsNamespace, featuresProvider) // when Expect(featuresHandler.Apply(ctx)).To(Succeed()) diff --git a/tests/integration/features/servicemesh_feature_test.go b/tests/integration/features/servicemesh_feature_test.go index 1c77c0eea7c..45a3bbc62b2 100644 --- a/tests/integration/features/servicemesh_feature_test.go +++ b/tests/integration/features/servicemesh_feature_test.go @@ -32,14 +32,14 @@ var _ = Describe("Service Mesh setup", func() { objectCleaner *envtestutil.Cleaner ) - BeforeEach(func() { + BeforeEach(func(ctx context.Context) { c, err := client.New(envTest.Config, client.Options{}) Expect(err).ToNot(HaveOccurred()) objectCleaner = envtestutil.CreateCleaner(c, envTest.Config, fixtures.Timeout, fixtures.Interval) namespace := envtestutil.AppendRandomNameTo("service-mesh-settings") - dsci = fixtures.NewDSCInitialization(namespace) + dsci = fixtures.NewDSCInitialization(ctx, envTestClient, namespace) Expect(err).ToNot(HaveOccurred()) }) @@ -52,9 +52,8 @@ var _ = Describe("Service Mesh setup", func() { It("should fail using precondition check", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("no-service-mesh-operator-check"). - UsingConfig(envTest.Config). PreConditions(servicemesh.EnsureServiceMeshOperatorInstalled), ) @@ -86,10 +85,9 @@ var _ = Describe("Service Mesh setup", func() { It("should succeed using precondition check", func(ctx context.Context) { // when - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add( feature.Define("service-mesh-operator-check"). - UsingConfig(envTest.Config). WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)). PreConditions(servicemesh.EnsureServiceMeshOperatorInstalled), ) @@ -119,9 +117,8 @@ var _ = Describe("Service Mesh setup", func() { dsci.Spec.ServiceMesh.ControlPlane.Name = "test-name" // when - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("service-mesh-control-plane-check"). - UsingConfig(envTest.Config). WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)). PreConditions(servicemesh.EnsureServiceMeshInstalled), ) @@ -141,10 +138,9 @@ var _ = Describe("Service Mesh setup", func() { dsci.Spec.ServiceMesh.ControlPlane.Namespace = "test-namespace" // when - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("no-service-mesh-control-plane-check"). WithData(feature.Entry("ControlPlane", provider.ValueOf(dsci.Spec.ServiceMesh.ControlPlane).Get)). - UsingConfig(envTest.Config). PreConditions(servicemesh.EnsureServiceMeshInstalled), ) @@ -176,7 +172,7 @@ var _ = Describe("Service Mesh setup", func() { BeforeEach(func(ctx context.Context) { smcpCrdObj = installServiceMeshCRD(ctx) objectCleaner = envtestutil.CreateCleaner(envTestClient, envTest.Config, fixtures.Timeout, fixtures.Interval) - dsci = fixtures.NewDSCInitialization(namespace) + dsci = fixtures.NewDSCInitialization(ctx, envTestClient, namespace) serviceMeshSpec = dsci.Spec.ServiceMesh @@ -198,9 +194,8 @@ var _ = Describe("Service Mesh setup", func() { createServiceMeshControlPlane(ctx, name, namespace) - handler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + handler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { return registry.Add(feature.Define("control-plane-with-external-authz-provider"). - UsingConfig(envTest.Config). Manifests( manifest.Location(fixtures.TestEmbeddedFiles). Include(path.Join("templates", "mesh-authz-ext-provider.patch.tmpl.yaml")), diff --git a/tests/integration/features/tracker_int_test.go b/tests/integration/features/tracker_int_test.go index 3f7af1528d9..6a05096b790 100644 --- a/tests/integration/features/tracker_int_test.go +++ b/tests/integration/features/tracker_int_test.go @@ -26,18 +26,15 @@ var _ = Describe("Feature tracking capability", func() { dsci *dsciv1.DSCInitialization ) - BeforeEach(func() { - dsci = fixtures.NewDSCInitialization("default") + BeforeEach(func(ctx context.Context) { + dsci = fixtures.NewDSCInitialization(ctx, envTestClient, "default") }) Context("Reporting progress when applying Feature", func() { It("should indicate successful installation in FeatureTracker through Status conditions", func(ctx context.Context) { - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { - errFeatureAdd := registry.Add( - feature.Define("always-working-feature"). - UsingConfig(envTest.Config), - ) + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add(feature.Define("always-working-feature")) Expect(errFeatureAdd).ToNot(HaveOccurred()) @@ -62,9 +59,8 @@ var _ = Describe("Feature tracking capability", func() { It("should indicate when failure occurs in preconditions through Status conditions", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("precondition-fail"). - UsingConfig(envTest.Config). PreConditions(func(_ context.Context, _ *feature.Feature) error { return errors.New("during test always fail") }), @@ -93,9 +89,8 @@ var _ = Describe("Feature tracking capability", func() { It("should indicate when failure occurs in post-conditions through Status conditions", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { errFeatureAdd := registry.Add(feature.Define("post-condition-failure"). - UsingConfig(envTest.Config). PostConditions(func(_ context.Context, _ *feature.Feature) error { return errors.New("during test always fail") }), @@ -127,10 +122,8 @@ var _ = Describe("Feature tracking capability", func() { It("should correctly indicate source in the feature tracker", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { - errFeatureAdd := registry.Add(feature.Define("always-working-feature"). - UsingConfig(envTest.Config), - ) + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add(feature.Define("always-working-feature")) Expect(errFeatureAdd).ToNot(HaveOccurred()) @@ -153,10 +146,8 @@ var _ = Describe("Feature tracking capability", func() { It("should correctly indicate app namespace in the feature tracker", func(ctx context.Context) { // given - featuresHandler := feature.ClusterFeaturesHandler(dsci, func(registry feature.FeaturesRegistry) error { - errFeatureAdd := registry.Add(feature.Define("empty-feature"). - UsingConfig(envTest.Config), - ) + featuresHandler := feature.ClusterFeaturesHandler(envTestClient, dsci, func(registry feature.FeaturesRegistry) error { + errFeatureAdd := registry.Add(feature.Define("empty-feature")) Expect(errFeatureAdd).ToNot(HaveOccurred()) @@ -173,4 +164,53 @@ var _ = Describe("Feature tracking capability", func() { }) }) + + Context("adding ownerReferences to feature tracker", func() { + It("should indicate owner in the feature tracker when owner in feature", func(ctx context.Context) { + // given + + // DSCI created in cluster + Expect(fixtures.CreateOrUpdateDSCI(ctx, envTestClient, dsci)).ToNot(HaveOccurred()) + + feature, featErr := feature.Define("empty-feat-with-owner"). + UsingClient(envTestClient). + Source(featurev1.Source{ + Type: featurev1.DSCIType, + Name: dsci.Name, + }). + TargetNamespace(appNamespace). + OwnedBy(dsci). + Create() + + // when + Expect(featErr).ToNot(HaveOccurred()) + Expect(feature.Apply(ctx)).To(Succeed()) + + // then + tracker, err := fixtures.GetFeatureTracker(ctx, envTestClient, appNamespace, "empty-feat-with-owner") + Expect(err).ToNot(HaveOccurred()) + Expect(tracker.OwnerReferences).ToNot(BeEmpty()) + }) + + It("should not indicate owner in the feature tracker when owner not in feature", func(ctx context.Context) { + // given + feature, featErr := feature.Define("empty-feat-no-owner"). + UsingClient(envTestClient). + Source(featurev1.Source{ + Type: featurev1.DSCIType, + Name: dsci.Name, + }). + TargetNamespace(appNamespace). + Create() + + // when + Expect(featErr).ToNot(HaveOccurred()) + Expect(feature.Apply(ctx)).To(Succeed()) + + // then + tracker, err := fixtures.GetFeatureTracker(ctx, envTestClient, appNamespace, "empty-feat-no-owner") + Expect(err).ToNot(HaveOccurred()) + Expect(tracker.OwnerReferences).To(BeEmpty()) + }) + }) })