diff --git a/pkg/cluster/gvk/gvk.go b/pkg/cluster/gvk/gvk.go index 19f113c6183..0801dd50252 100644 --- a/pkg/cluster/gvk/gvk.go +++ b/pkg/cluster/gvk/gvk.go @@ -15,6 +15,12 @@ var ( Kind: "DataScienceCluster", } + Deployment = schema.GroupVersionKind{ + Group: "apps", + Version: "v1", + Kind: "Deployment", + } + KnativeServing = schema.GroupVersionKind{ Group: "operator.knative.dev", Version: "v1beta1", @@ -38,10 +44,12 @@ var ( Version: "v1", Kind: "OdhApplication", } + OdhDocument = schema.GroupVersionKind{ Group: "dashboard.opendatahub.io", Version: "v1", Kind: "OdhDocument", } + OdhQuickStart = schema.GroupVersionKind{ Group: "console.openshift.io", Version: "v1", Kind: "OdhQuickStart", diff --git a/pkg/conversion/conversion.go b/pkg/conversion/conversion.go index ca4cbbd55d7..33d68943f34 100644 --- a/pkg/conversion/conversion.go +++ b/pkg/conversion/conversion.go @@ -5,7 +5,7 @@ import ( "strings" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "sigs.k8s.io/kustomize/api/resmap" + "sigs.k8s.io/kustomize/api/resource" "sigs.k8s.io/yaml" ) @@ -33,20 +33,13 @@ func StrToUnstructured(resources string) ([]*unstructured.Unstructured, error) { return objs, nil } -// ResMapToUnstructured converts a ResMap to a slice of Unstructured objects. -func ResMapToUnstructured(resMap resmap.ResMap) ([]*unstructured.Unstructured, error) { - resources := make([]*unstructured.Unstructured, 0, resMap.Size()) - for _, res := range resMap.Resources() { - u := &unstructured.Unstructured{} - asYAML, errToYAML := res.AsYAML() - if errToYAML != nil { - return nil, errToYAML - } - if errUnmarshal := yaml.Unmarshal(asYAML, u); errUnmarshal != nil { - return nil, errUnmarshal - } - resources = append(resources, u) +// ResourceToUnstructured converts a resource.Resource to an Unstructured object. +func ResourceToUnstructured(res *resource.Resource) (*unstructured.Unstructured, error) { + u := &unstructured.Unstructured{} + + if err := yaml.Unmarshal([]byte(res.MustYaml()), u); err != nil { + return nil, err } - return resources, nil + return u, nil } diff --git a/pkg/deploy/deploy.go b/pkg/deploy/deploy.go index ff800e9e095..745c184e6db 100644 --- a/pkg/deploy/deploy.go +++ b/pkg/deploy/deploy.go @@ -41,6 +41,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/kustomize/api/krusty" "sigs.k8s.io/kustomize/api/resmap" + "sigs.k8s.io/kustomize/api/resource" "sigs.k8s.io/kustomize/kyaml/filesys" "github.com/opendatahub-io/opendatahub-operator/v2/components" @@ -183,13 +184,9 @@ func DeployManifestsFromPath( return fmt.Errorf("failed applying labels plugin when preparing Kustomize resources. %w", err) } - objs, err := conversion.ResMapToUnstructured(resMap) - if err != nil { - return err - } // Create / apply / delete resources in the cluster - for _, obj := range objs { - err = manageResource(ctx, cli, obj, owner, namespace, componentName, componentEnabled) + for _, res := range resMap.Resources() { + err = manageResource(ctx, cli, res, owner, namespace, componentName, componentEnabled) if err != nil { return err } @@ -198,13 +195,13 @@ func DeployManifestsFromPath( return nil } -func manageResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured, owner metav1.Object, applicationNamespace, componentName string, enabled bool) error { - // Skip if resource is of Kind: Namespace and Name: applicationNamespace - if obj.GetKind() == "Namespace" && obj.GetName() == applicationNamespace { +func manageResource(ctx context.Context, cli client.Client, res *resource.Resource, owner metav1.Object, applicationNamespace, componentName string, enabled bool) error { + // Return if resource is of Kind: Namespace and Name: applicationsNamespace + if res.GetKind() == "Namespace" && res.GetName() == applicationNamespace { return nil } - found, err := getResource(ctx, cli, obj) + found, err := getResource(ctx, cli, res) if err != nil { if !k8serr.IsNotFound(err) { @@ -212,7 +209,7 @@ func manageResource(ctx context.Context, cli client.Client, obj *unstructured.Un } // Create resource if it doesn't exist and component enabled if enabled { - return createResource(ctx, cli, obj, owner) + return createResource(ctx, cli, res, owner) } // Skip if resource doesn't exist and component is disabled return nil @@ -226,59 +223,27 @@ func manageResource(ctx context.Context, cli client.Client, obj *unstructured.Un if found.GetAnnotations()[annotations.ManagedByODHOperator] == "false" && componentName == "kserve" { return nil } - return updateResource(ctx, cli, obj, found, owner, componentName) + return updateResource(ctx, cli, res, found, owner) } // Delete resource if it exists and component is disabled return handleDisabledComponent(ctx, cli, found, componentName) } -// removeResourcesFromDeployment checks if the provided resource is a Deployment, -// and if so, removes the resources field from each container in the Deployment. This ensures we do not overwrite the -// resources field when Patch is applied with the returned unstructured resource. -// TODO: remove this function once RHOAIENG-7929 is finished. -func removeResourcesFromDeployment(u *unstructured.Unstructured) error { - // Check if the resource is a Deployment. This can be expanded to other resources as well. - if u.GetKind() != "Deployment" { - return nil - } - // Navigate to the containers array in the Deployment spec - containers, exists, err := unstructured.NestedSlice(u.Object, "spec", "template", "spec", "containers") - if err != nil { - return fmt.Errorf("error when trying to retrieve containers from Deployment: %w", err) - } - // Return if no containers exist - if !exists { - return nil - } - - // Iterate over the containers to remove the resources field - for i := range containers { - container, ok := containers[i].(map[string]interface{}) - // If containers field is not in expected type, return. - if !ok { - return nil - } - // Check and delete the resources field. This can be expanded to any whitelisted field. - delete(container, "resources") - containers[i] = container - } - - // Update the containers in the original unstructured object - if err := unstructured.SetNestedSlice(u.Object, containers, "spec", "template", "spec", "containers"); err != nil { - return fmt.Errorf("failed to update containers in Deployment: %w", err) - } - - return nil -} - -func getResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { +func getResource(ctx context.Context, cli client.Client, obj *resource.Resource) (*unstructured.Unstructured, error) { found := &unstructured.Unstructured{} + residGvk := obj.GetGvk() + gvk := schema.GroupVersionKind{ + Group: residGvk.Group, + Version: residGvk.Version, + Kind: residGvk.Kind, + } // Setting gvk is required to do Get request - found.SetGroupVersionKind(obj.GroupVersionKind()) + found.SetGroupVersionKind(gvk) + err := cli.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, found) if errors.Is(err, &meta.NoKindMatchError{}) { // convert the error to NotFound to handle both the same way in the caller - return nil, k8serr.NewNotFound(schema.GroupResource{Group: obj.GroupVersionKind().Group}, obj.GetName()) + return nil, k8serr.NewNotFound(schema.GroupResource{Group: gvk.Group}, obj.GetName()) } if err != nil { return nil, err @@ -308,7 +273,11 @@ func deleteResource(ctx context.Context, cli client.Client, found *unstructured. return nil } -func createResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured, owner metav1.Object) error { +func createResource(ctx context.Context, cli client.Client, res *resource.Resource, owner metav1.Object) error { + obj, err := conversion.ResourceToUnstructured(res) + if err != nil { + return err + } if obj.GetKind() != "CustomResourceDefinition" && obj.GetKind() != "OdhDashboardConfig" { if err := ctrl.SetControllerReference(owner, metav1.Object(obj), cli.Scheme()); err != nil { return err @@ -317,13 +286,40 @@ func createResource(ctx context.Context, cli client.Client, obj *unstructured.Un return cli.Create(ctx, obj) } -// TODO: cleanup some logic once RHOAIENG-7929 is finished. -func skipUpdateOnWhitelistedFields(obj *unstructured.Unstructured, componentName string) error { - if componentName == "kserve" || componentName == "model-mesh" { - if err := removeResourcesFromDeployment(obj); err != nil { +func updateResource(ctx context.Context, cli client.Client, res *resource.Resource, found *unstructured.Unstructured, owner metav1.Object) error { + // Skip ODHDashboardConfig Update + if found.GetKind() == "OdhDashboardConfig" { + return nil + } + + // only reconcile whiltelistedFields if the existing resource has annoation set to "true" + // all other cases, whiltelistedfields will be skipped by ODH operator + if managed, exists := found.GetAnnotations()[annotations.ManagedByODHOperator]; !exists || managed != "true" { + if err := skipUpdateOnWhitelistedFields(res); err != nil { return err } } + + obj, err := conversion.ResourceToUnstructured(res) + if err != nil { + return err + } + + // Retain existing labels on update + updateLabels(found, obj) + + return performPatch(ctx, cli, obj, found, owner) +} + +// skipUpdateOnWhitelistedFields applies RemoverPlugin to the component's resources +// This ensures that we do not overwrite the fields when Patch is applied later to the resource. +func skipUpdateOnWhitelistedFields(res *resource.Resource) error { + for _, rmPlugin := range plugins.WhitelistedFields { + if err := rmPlugin.TransformResource(res); err != nil { + return err + } + } + return nil } @@ -347,20 +343,4 @@ func performPatch(ctx context.Context, cli client.Client, obj, found *unstructur return cli.Patch(ctx, found, client.RawPatch(types.ApplyPatchType, data), client.ForceOwnership, client.FieldOwner(owner.GetName())) } -func updateResource(ctx context.Context, cli client.Client, obj, found *unstructured.Unstructured, owner metav1.Object, componentName string) error { - // Skip ODHDashboardConfig Update - if found.GetKind() == "OdhDashboardConfig" { - return nil - } - // skip updating whitelisted fields from component - if err := skipUpdateOnWhitelistedFields(obj, componentName); err != nil { - return err - } - - // Retain existing labels on update - updateLabels(found, obj) - - return performPatch(ctx, cli, obj, found, owner) -} - // TODO : Add function to cleanup code created as part of pre install and post install task of a component diff --git a/pkg/plugins/remover_plugin_test.go b/pkg/plugins/remover_plugin_test.go new file mode 100644 index 00000000000..3fba3abc0c6 --- /dev/null +++ b/pkg/plugins/remover_plugin_test.go @@ -0,0 +1,266 @@ +package plugins_test + +import ( + "sigs.k8s.io/kustomize/api/provider" + "sigs.k8s.io/kustomize/api/resource" + + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/plugins" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var factory = provider.NewDefaultDepProvider().GetResourceFactory() + +var _ = Describe("Remover plugin", func() { + var res *resource.Resource + + BeforeEach(func() { + var err error + res, err = factory.FromBytes([]byte(` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: testdeployment +spec: + replicas: 3 + selector: + matchLabels: + control-plane: odh-component + template: + metadata: + labels: + app: odh-component + app.opendatahub.io/odh-component: "true" + control-plane: odh-component + spec: + containers: + - name: conatiner0 + env: + - name: NAMESPACE + value: namespace + image: quay.io/opendatahub/odh-component:latest + ports: + - containerPort: 80 + - name: conatiner1 + env: + - name: NAMESPACE + value: namespace + image: quay.io/opendatahub/odh-component:latest + ports: + - containerPort: 8080 + resources: + limits: + cpu: 500m + memory: 2Gi + requests: + cpu: 10m + memory: 64Mi + +`)) + Expect(err).NotTo(HaveOccurred()) + }) + + It("Should be able to remove replicas from resources", func() { + rmPlug := plugins.RemoverPlugin{ + Gvk: gvk.Deployment, + Path: []string{"spec", "replicas"}, + } + + expected := ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: testdeployment +spec: + selector: + matchLabels: + control-plane: odh-component + template: + metadata: + labels: + app: odh-component + app.opendatahub.io/odh-component: "true" + control-plane: odh-component + spec: + containers: + - name: conatiner0 + env: + - name: NAMESPACE + value: namespace + image: quay.io/opendatahub/odh-component:latest + ports: + - containerPort: 80 + - name: conatiner1 + env: + - name: NAMESPACE + value: namespace + image: quay.io/opendatahub/odh-component:latest + ports: + - containerPort: 8080 + resources: + limits: + cpu: 500m + memory: 2Gi + requests: + cpu: 10m + memory: 64Mi + +` + err := rmPlug.TransformResource(res) + Expect(err).NotTo(HaveOccurred()) + + Expect(res.MustYaml()).To(MatchYAML(expected)) + }) + + It("Should be able to remove resources filed", func() { + rmPlug := plugins.RemoverPlugin{ + Gvk: gvk.Deployment, + Path: []string{"spec", "template", "spec", "containers", "*", "resources"}, + } + + expected := ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: testdeployment +spec: + replicas: 3 + selector: + matchLabels: + control-plane: odh-component + template: + metadata: + labels: + app: odh-component + app.opendatahub.io/odh-component: "true" + control-plane: odh-component + spec: + containers: + - name: conatiner0 + env: + - name: NAMESPACE + value: namespace + image: quay.io/opendatahub/odh-component:latest + ports: + - containerPort: 80 + - name: conatiner1 + env: + - name: NAMESPACE + value: namespace + image: quay.io/opendatahub/odh-component:latest + ports: + - containerPort: 8080 +` + err := rmPlug.TransformResource(res) + Expect(err).NotTo(HaveOccurred()) + + Expect(res.MustYaml()).To(MatchYAML(expected)) + }) + + It("Should be able to remove both replicas and resources", func() { + rmPlugRep := plugins.RemoverPlugin{ + Gvk: gvk.Deployment, + Path: []string{"spec", "replicas"}, + } + rmPlugRes := plugins.RemoverPlugin{ + Gvk: gvk.Deployment, + Path: []string{"spec", "template", "spec", "containers", "*", "resources"}, + } + + expected := ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: testdeployment +spec: + selector: + matchLabels: + control-plane: odh-component + template: + metadata: + labels: + app: odh-component + app.opendatahub.io/odh-component: "true" + control-plane: odh-component + spec: + containers: + - name: conatiner0 + env: + - name: NAMESPACE + value: namespace + image: quay.io/opendatahub/odh-component:latest + ports: + - containerPort: 80 + - name: conatiner1 + env: + - name: NAMESPACE + value: namespace + image: quay.io/opendatahub/odh-component:latest + ports: + - containerPort: 8080 +` + err := rmPlugRep.TransformResource(res) + Expect(err).NotTo(HaveOccurred()) + + err = rmPlugRes.TransformResource(res) + Expect(err).NotTo(HaveOccurred()) + + Expect(res.MustYaml()).To(MatchYAML(expected)) + }) + + It("Should not remove fileds with unexisted path", func() { + rmPlug := plugins.RemoverPlugin{ + Gvk: gvk.Deployment, + Path: []string{"spec", "unexisted"}, + } + + expected := ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: testdeployment +spec: + replicas: 3 + selector: + matchLabels: + control-plane: odh-component + template: + metadata: + labels: + app: odh-component + app.opendatahub.io/odh-component: "true" + control-plane: odh-component + spec: + containers: + - name: conatiner0 + env: + - name: NAMESPACE + value: namespace + image: quay.io/opendatahub/odh-component:latest + ports: + - containerPort: 80 + - name: conatiner1 + env: + - name: NAMESPACE + value: namespace + image: quay.io/opendatahub/odh-component:latest + ports: + - containerPort: 8080 + resources: + limits: + cpu: 500m + memory: 2Gi + requests: + cpu: 10m + memory: 64Mi + +` + err := rmPlug.TransformResource(res) + Expect(err).NotTo(HaveOccurred()) + + Expect(res.MustYaml()).To(MatchYAML(expected)) + }) + +}) diff --git a/pkg/plugins/removerplugin.go b/pkg/plugins/removerplugin.go new file mode 100644 index 00000000000..a991e33c68e --- /dev/null +++ b/pkg/plugins/removerplugin.go @@ -0,0 +1,103 @@ +package plugins + +import ( + "errors" + + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/kustomize/api/resmap" + "sigs.k8s.io/kustomize/api/resource" + "sigs.k8s.io/kustomize/kyaml/kio" + kyaml "sigs.k8s.io/kustomize/kyaml/yaml" + + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" +) + +var ( + WhitelistedFields = []RemoverPlugin{ + // for resources, i.e cpu and memory + { + Gvk: gvk.Deployment, + Path: []string{"spec", "template", "spec", "containers", "*", "resources"}, + }, + // for replicas + { + Gvk: gvk.Deployment, + Path: []string{"spec", "replicas"}, + }, + } +) + +// Removes the field from the resources of ResMap if they match GVK. +type RemoverPlugin struct { + Gvk schema.GroupVersionKind + Path []string +} + +var _ resmap.Transformer = &RemoverPlugin{} + +// Transform removes the field from ResMap if they match filter. +func (p *RemoverPlugin) Transform(m resmap.ResMap) error { + filter := RemoverFilter{ + Gvk: p.Gvk, + Path: p.Path, + } + return m.ApplyFilter(filter) +} + +// TransformResource works only on one resource, not on the whole ResMap. +func (p *RemoverPlugin) TransformResource(r *resource.Resource) error { + filter := RemoverFilter{ + Gvk: p.Gvk, + Path: p.Path, + } + + nodes := []*kyaml.RNode{&r.RNode} + _, err := filter.Filter(nodes) + return err +} + +type RemoverFilter struct { + Gvk schema.GroupVersionKind + Path []string +} + +var _ kio.Filter = RemoverFilter{} + +func (f RemoverFilter) Filter(nodes []*kyaml.RNode) ([]*kyaml.RNode, error) { + return kio.FilterAll(kyaml.FilterFunc(f.run)).Filter(nodes) +} + +func (f RemoverFilter) run(node *kyaml.RNode) (*kyaml.RNode, error) { + pathLen := len(f.Path) + if pathLen == 0 { + return node, errors.New("no field set to remove, path to the field cannot be empty") + } + + typeMeta := kyaml.TypeMeta{ + APIVersion: f.Gvk.GroupVersion().String(), + Kind: f.Gvk.Kind, + } + + meta, err := node.GetMeta() + if err != nil { + return node, err + } + + if meta.TypeMeta != typeMeta { + return node, nil + } + + path := f.Path[:pathLen-1] + name := f.Path[pathLen-1] + + matcher := &kyaml.PathMatcher{Path: path} + result, err := node.Pipe(matcher) + if err != nil { + return node, err + } + + return node, result.VisitElements( + func(node *kyaml.RNode) error { + return node.PipeE(kyaml.FieldClearer{Name: name}) + }) +} diff --git a/pkg/plugins/suite_test.go b/pkg/plugins/suite_test.go new file mode 100644 index 00000000000..de22287bc07 --- /dev/null +++ b/pkg/plugins/suite_test.go @@ -0,0 +1,14 @@ +package plugins_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestPlugins(t *testing.T) { + RegisterFailHandler(Fail) + + RunSpecs(t, "Plugins test suite") +} diff --git a/tests/e2e/dsc_creation_test.go b/tests/e2e/dsc_creation_test.go index ffbba29db55..86d7f98323b 100644 --- a/tests/e2e/dsc_creation_test.go +++ b/tests/e2e/dsc_creation_test.go @@ -435,7 +435,7 @@ func (tc *testContext) testUpdateComponentReconcile() error { } if len(appDeployments.Items) != 0 { testDeployment := appDeployments.Items[0] - expectedReplica := testDeployment.Spec.Replicas + expectedReplica := 3 patchedReplica := &autoscalingv1.Scale{ ObjectMeta: metav1.ObjectMeta{ Name: testDeployment.Name, @@ -446,22 +446,23 @@ func (tc *testContext) testUpdateComponentReconcile() error { }, Status: autoscalingv1.ScaleStatus{}, } - retrievedDep, err := tc.kubeClient.AppsV1().Deployments(tc.applicationsNamespace).UpdateScale(tc.ctx, testDeployment.Name, patchedReplica, metav1.UpdateOptions{}) + updatedDep, err := tc.kubeClient.AppsV1().Deployments(tc.applicationsNamespace).UpdateScale(tc.ctx, testDeployment.Name, patchedReplica, metav1.UpdateOptions{}) if err != nil { return fmt.Errorf("error patching component resources : %w", err) } - if retrievedDep.Spec.Replicas != patchedReplica.Spec.Replicas { - return fmt.Errorf("failed to patch replicas : expect to be %v but got %v", patchedReplica.Spec.Replicas, retrievedDep.Spec.Replicas) + if updatedDep.Spec.Replicas != patchedReplica.Spec.Replicas { + return fmt.Errorf("failed to patch replicas : expect to be %v but got %v", patchedReplica.Spec.Replicas, updatedDep.Spec.Replicas) } // Sleep for 40 seconds to allow the operator to reconcile + // we expect it should not revert back to original value because of whitelist time.Sleep(4 * tc.resourceRetryInterval) - revertedDep, err := tc.kubeClient.AppsV1().Deployments(tc.applicationsNamespace).Get(tc.ctx, testDeployment.Name, metav1.GetOptions{}) + reconciledDep, err := tc.kubeClient.AppsV1().Deployments(tc.applicationsNamespace).Get(tc.ctx, testDeployment.Name, metav1.GetOptions{}) if err != nil { return fmt.Errorf("error getting component resource after reconcile: %w", err) } - if *revertedDep.Spec.Replicas != *expectedReplica { - return fmt.Errorf("failed to revert back replicas : expect to be %v but got %v", *expectedReplica, *revertedDep.Spec.Replicas) + if *reconciledDep.Spec.Replicas != int32(expectedReplica) { + return fmt.Errorf("failed to revert back replicas : expect to be %v but got %v", expectedReplica, *reconciledDep.Spec.Replicas) } }