From 5ccb9c498efb80a2e1cb28617c6953293981a7b5 Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Thu, 7 Nov 2024 12:09:44 +0100 Subject: [PATCH] Cleanup kustomize manifests rendering action This commit is meant to cleanup the kustomize manifest redering by: - removing a useless parameter related to the caching mechanism - moving the code to the kustomize package to accomodate other rendering engines - introduce metrics tro track how many resources have been rendered and to perform blackbox testing against the action --- .../dashboard/dashboard_controller.go | 10 +-- .../modelregistry/modelregistry_controller.go | 10 +-- controllers/components/ray/ray_controller.go | 10 +-- .../action_render_manifests.go | 23 +++---- .../action_render_manifests_support.go | 2 +- .../action_render_manifests_test.go | 68 ++++++++++--------- .../actions/render/render_metrics.go | 32 +++++++++ pkg/metadata/annotations/annotations.go | 1 - 8 files changed, 92 insertions(+), 64 deletions(-) rename pkg/controller/actions/render/{ => kustomize}/action_render_manifests.go (88%) rename pkg/controller/actions/render/{ => kustomize}/action_render_manifests_support.go (98%) rename pkg/controller/actions/render/{ => kustomize}/action_render_manifests_test.go (77%) create mode 100644 pkg/controller/actions/render/render_metrics.go diff --git a/controllers/components/dashboard/dashboard_controller.go b/controllers/components/dashboard/dashboard_controller.go index a7cd0bbf8bd..fcd25c18b24 100644 --- a/controllers/components/dashboard/dashboard_controller.go +++ b/controllers/components/dashboard/dashboard_controller.go @@ -31,7 +31,7 @@ import ( componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/deploy" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render/kustomize" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/security" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/updatestatus" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/predicates/resources" @@ -74,8 +74,8 @@ func NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { WithAction(devFlags). WithAction(configureDependencies). WithAction(security.NewUpdatePodSecurityRoleBindingAction(serviceAccounts)). - WithAction(render.NewAction( - render.WithCache(true, render.DefaultCachingKeyFn), + WithAction(kustomize.NewAction( + kustomize.WithCache(kustomize.DefaultCachingKeyFn), // Those are the default labels added by the legacy deploy method // and should be preserved as the original plugin were affecting // deployment selectors that are immutable once created, so it won't @@ -84,8 +84,8 @@ func NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { // // Additional labels/annotations MUST be added by the deploy action // so they would affect only objects metadata without side effects - render.WithLabel(labels.ODH.Component(componentName), "true"), - render.WithLabel(labels.K8SCommon.PartOf, componentName), + kustomize.WithLabel(labels.ODH.Component(componentName), "true"), + kustomize.WithLabel(labels.K8SCommon.PartOf, componentName), )). WithAction(customizeResources). WithAction(deploy.NewAction( diff --git a/controllers/components/modelregistry/modelregistry_controller.go b/controllers/components/modelregistry/modelregistry_controller.go index 29c5d185c12..231e5574e08 100644 --- a/controllers/components/modelregistry/modelregistry_controller.go +++ b/controllers/components/modelregistry/modelregistry_controller.go @@ -30,7 +30,7 @@ import ( componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/deploy" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render/kustomize" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/updatestatus" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/handlers" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/predicates/resources" @@ -74,10 +74,10 @@ func NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { WithAction(checkPreConditions). WithAction(initialize). WithAction(configureDependencies). - WithAction(render.NewAction( - render.WithCache(true, render.DefaultCachingKeyFn), - render.WithLabel(labels.ODH.Component(ComponentName), "true"), - render.WithLabel(labels.K8SCommon.PartOf, ComponentName), + WithAction(kustomize.NewAction( + kustomize.WithCache(kustomize.DefaultCachingKeyFn), + kustomize.WithLabel(labels.ODH.Component(ComponentName), "true"), + kustomize.WithLabel(labels.K8SCommon.PartOf, ComponentName), )). WithAction(customizeResources). WithAction(deploy.NewAction( diff --git a/controllers/components/ray/ray_controller.go b/controllers/components/ray/ray_controller.go index 5fce1587653..b0562d99f46 100644 --- a/controllers/components/ray/ray_controller.go +++ b/controllers/components/ray/ray_controller.go @@ -29,7 +29,7 @@ import ( componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/deploy" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render/kustomize" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/updatestatus" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/predicates/resources" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/reconciler" @@ -56,10 +56,10 @@ func NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { // Add Ray-specific actions WithAction(initialize). WithAction(devFlags). - WithAction(render.NewAction( - render.WithCache(true, render.DefaultCachingKeyFn), - render.WithLabel(labels.ODH.Component(ComponentName), "true"), - render.WithLabel(labels.K8SCommon.PartOf, ComponentName), + WithAction(kustomize.NewAction( + kustomize.WithCache(kustomize.DefaultCachingKeyFn), + kustomize.WithLabel(labels.ODH.Component(ComponentName), "true"), + kustomize.WithLabel(labels.K8SCommon.PartOf, ComponentName), )). WithAction(deploy.NewAction( deploy.WithFieldOwner(componentsv1.RayInstanceName), diff --git a/pkg/controller/actions/render/action_render_manifests.go b/pkg/controller/actions/render/kustomize/action_render_manifests.go similarity index 88% rename from pkg/controller/actions/render/action_render_manifests.go rename to pkg/controller/actions/render/kustomize/action_render_manifests.go index 248d9a6ed33..717b1c327e7 100644 --- a/pkg/controller/actions/render/action_render_manifests.go +++ b/pkg/controller/actions/render/kustomize/action_render_manifests.go @@ -1,21 +1,23 @@ -package render +package kustomize import ( "bytes" "context" - "encoding/base64" "fmt" + "strings" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/kustomize/kyaml/filesys" "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/types" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/manifests/kustomize" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) +const RendererEngine = "kustomize" + type CachingKeyFn func(_ context.Context, rr *types.ReconciliationRequest) ([]byte, error) // Action takes a set of manifest locations and render them as Unstructured resources for @@ -25,7 +27,6 @@ type Action struct { keOpts []kustomize.EngineOptsFn ke *kustomize.Engine - cacheAnnotation bool cachingKeyFn CachingKeyFn cachingKey []byte cachedResources resources.UnstructuredList @@ -69,9 +70,8 @@ func WithManifestsOptions(values ...kustomize.EngineOptsFn) ActionOpts { } } -func WithCache(addHashAnnotation bool, value CachingKeyFn) ActionOpts { +func WithCache(value CachingKeyFn) ActionOpts { return func(action *Action) { - action.cacheAnnotation = addHashAnnotation action.cachingKeyFn = value } } @@ -101,20 +101,15 @@ func (a *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error return fmt.Errorf("unable to render reconciliation object: %w", err) } - // Add a letter at the beginning and use URL safe encoding - digest := "v" + base64.RawURLEncoding.EncodeToString(cachingKey) result = res if len(cachingKey) != 0 { - if a.cacheAnnotation { - for i := range result { - resources.SetAnnotation(&result[i], annotations.ComponentHash, digest) - } - } - a.cachingKey = cachingKey a.cachedResources = result } + + controllerName := strings.ToLower(rr.Instance.GetObjectKind().GroupVersionKind().Kind) + render.RenderedResourcesTotal.WithLabelValues(controllerName, RendererEngine).Add(float64(len(result))) } // deep copy object so changes done in the pipelines won't diff --git a/pkg/controller/actions/render/action_render_manifests_support.go b/pkg/controller/actions/render/kustomize/action_render_manifests_support.go similarity index 98% rename from pkg/controller/actions/render/action_render_manifests_support.go rename to pkg/controller/actions/render/kustomize/action_render_manifests_support.go index 0fff58816df..8011d43ec19 100644 --- a/pkg/controller/actions/render/action_render_manifests_support.go +++ b/pkg/controller/actions/render/kustomize/action_render_manifests_support.go @@ -1,4 +1,4 @@ -package render +package kustomize import ( "context" diff --git a/pkg/controller/actions/render/action_render_manifests_test.go b/pkg/controller/actions/render/kustomize/action_render_manifests_test.go similarity index 77% rename from pkg/controller/actions/render/action_render_manifests_test.go rename to pkg/controller/actions/render/kustomize/action_render_manifests_test.go index 6b02856fc9a..4f81008f87b 100644 --- a/pkg/controller/actions/render/action_render_manifests_test.go +++ b/pkg/controller/actions/render/kustomize/action_render_manifests_test.go @@ -1,10 +1,11 @@ -package render_test +package kustomize_test import ( "context" "path" "testing" + "github.com/prometheus/client_golang/prometheus/testutil" "github.com/rs/xid" "sigs.k8s.io/kustomize/kyaml/filesys" @@ -13,9 +14,9 @@ import ( 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/controller/actions/render" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render/kustomize" "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/metadata/annotations" + mk "github.com/opendatahub-io/opendatahub-operator/v2/pkg/manifests/kustomize" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/fakeclient" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/matchers/jq" @@ -104,8 +105,8 @@ func TestRenderResourcesAction(t *testing.T) { id := xid.New().String() fs := filesys.MakeFsInMemory() - _ = fs.MkdirAll(path.Join(id, kustomize.DefaultKustomizationFilePath)) - _ = fs.WriteFile(path.Join(id, kustomize.DefaultKustomizationFileName), []byte(testRenderResourcesKustomization)) + _ = fs.MkdirAll(path.Join(id, mk.DefaultKustomizationFilePath)) + _ = fs.WriteFile(path.Join(id, mk.DefaultKustomizationFileName), []byte(testRenderResourcesKustomization)) _ = fs.WriteFile(path.Join(id, "test-resources-cm.yaml"), []byte(testRenderResourcesConfigMap)) _ = fs.WriteFile(path.Join(id, "test-resources-deployment-managed.yaml"), []byte(testRenderResourcesManaged)) _ = fs.WriteFile(path.Join(id, "test-resources-deployment-unmanaged.yaml"), []byte(testRenderResourcesUnmanaged)) @@ -114,14 +115,14 @@ func TestRenderResourcesAction(t *testing.T) { cl, err := fakeclient.New(ctx) g.Expect(err).ShouldNot(HaveOccurred()) - action := render.NewAction( - render.WithLabel("component.opendatahub.io/name", "foo"), - render.WithLabel("platform.opendatahub.io/namespace", ns), - render.WithAnnotation("platform.opendatahub.io/release", "1.2.3"), - render.WithAnnotation("platform.opendatahub.io/type", "managed"), + action := kustomize.NewAction( + kustomize.WithLabel("component.opendatahub.io/name", "foo"), + kustomize.WithLabel("platform.opendatahub.io/namespace", ns), + kustomize.WithAnnotation("platform.opendatahub.io/release", "1.2.3"), + kustomize.WithAnnotation("platform.opendatahub.io/type", "managed"), // for testing - render.WithManifestsOptions( - kustomize.WithEngineFS(fs), + kustomize.WithManifestsOptions( + mk.WithEngineFS(fs), ), ) @@ -184,32 +185,32 @@ func TestRenderResourcesWithCacheAction(t *testing.T) { id := xid.New().String() fs := filesys.MakeFsInMemory() - _ = fs.MkdirAll(path.Join(id, kustomize.DefaultKustomizationFilePath)) - _ = fs.WriteFile(path.Join(id, kustomize.DefaultKustomizationFileName), []byte(testRenderResourcesWithCacheKustomization)) + _ = fs.MkdirAll(path.Join(id, mk.DefaultKustomizationFilePath)) + _ = fs.WriteFile(path.Join(id, mk.DefaultKustomizationFileName), []byte(testRenderResourcesWithCacheKustomization)) _ = fs.WriteFile(path.Join(id, "test-resources-deployment.yaml"), []byte(testRenderResourcesWithCacheDeployment)) cl, err := fakeclient.New(ctx) g.Expect(err).ShouldNot(HaveOccurred()) - action := render.NewAction( - render.WithCache(true, render.DefaultCachingKeyFn), - render.WithLabel(labels.ComponentPartOf, "foo"), - render.WithLabel("platform.opendatahub.io/namespace", ns), - render.WithAnnotation("platform.opendatahub.io/release", "1.2.3"), - render.WithAnnotation("platform.opendatahub.io/type", "managed"), + action := kustomize.NewAction( + kustomize.WithCache(kustomize.DefaultCachingKeyFn), + kustomize.WithLabel(labels.ComponentPartOf, "foo"), + kustomize.WithLabel("platform.opendatahub.io/namespace", ns), + kustomize.WithAnnotation("platform.opendatahub.io/release", "1.2.3"), + kustomize.WithAnnotation("platform.opendatahub.io/type", "managed"), // for testing - render.WithManifestsOptions( - kustomize.WithEngineFS(fs), + kustomize.WithManifestsOptions( + mk.WithEngineFS(fs), ), ) - hash := "" + render.RenderedResourcesTotal.Reset() for i := int64(0); i < 3; i++ { d := componentsv1.Dashboard{} - if i == 2 { - d.Generation = i + if i >= 1 { + d.Generation = 1 } rr := types.ReconciliationRequest{ @@ -235,14 +236,15 @@ func TestRenderResourcesWithCacheAction(t *testing.T) { )), )) - newHash := rr.Resources[0].GetAnnotations()[annotations.ComponentHash] - if i == 1 { - g.Expect(newHash).Should(Equal(hash)) - } - if i == 2 { - g.Expect(newHash).ShouldNot(Equal(hash)) - } + rc := testutil.ToFloat64(render.RenderedResourcesTotal) - hash = newHash + switch i { + case 0: + g.Expect(rc).Should(BeNumerically("==", 1)) + case 1: + g.Expect(rc).Should(BeNumerically("==", 2)) + case 2: + g.Expect(rc).Should(BeNumerically("==", 2)) + } } } diff --git a/pkg/controller/actions/render/render_metrics.go b/pkg/controller/actions/render/render_metrics.go new file mode 100644 index 00000000000..761308708fa --- /dev/null +++ b/pkg/controller/actions/render/render_metrics.go @@ -0,0 +1,32 @@ +package render + +import ( + "github.com/prometheus/client_golang/prometheus" + "sigs.k8s.io/controller-runtime/pkg/metrics" +) + +var ( + // RenderedResourcesTotal is a prometheus counter metrics which holds the total + // number of resource rendered by the action per controller and rendering type. + // It has two labels. + // controller label refers to the controller name. + // engine label refers to the rendering engine. + RenderedResourcesTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "action_renderer_manifests_total", + Help: "Number of rendered resources", + }, + []string{ + "controller", + "engine", + }, + ) +) + +// init register metrics to the global registry from controller-runtime/pkg/metrics. +// see https://book.kubebuilder.io/reference/metrics#publishing-additional-metrics +// +//nolint:gochecknoinits +func init() { + metrics.Registry.MustRegister(RenderedResourcesTotal) +} diff --git a/pkg/metadata/annotations/annotations.go b/pkg/metadata/annotations/annotations.go index 0b5377c05b2..1ef2dc24ac2 100644 --- a/pkg/metadata/annotations/annotations.go +++ b/pkg/metadata/annotations/annotations.go @@ -19,7 +19,6 @@ const ManagementStateAnnotation = "component.opendatahub.io/management-state" const ( ComponentGeneration = "components.opendatahub.io/generation" - ComponentHash = "components.opendatahub.io/hash" PlatformVersion = "platform.opendatahub.io/version" PlatformType = "platform.opendatahub.io/type" )