Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup kustomize manifests rendering action #1353

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Cleanup kustomize manifests rendering action #1353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions controllers/components/dashboard/dashboard_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand Down
10 changes: 5 additions & 5 deletions controllers/components/modelregistry/modelregistry_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(
Expand Down
10 changes: 5 additions & 5 deletions controllers/components/ray/ray_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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),
Expand Down
23 changes: 9 additions & 14 deletions ...actions/render/action_render_manifests.go → ...nder/kustomize/action_render_manifests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -25,7 +27,6 @@ type Action struct {
keOpts []kustomize.EngineOptsFn
ke *kustomize.Engine

cacheAnnotation bool
cachingKeyFn CachingKeyFn
cachingKey []byte
cachedResources resources.UnstructuredList
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion ...render/action_render_manifests_support.go → ...tomize/action_render_manifests_support.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package render
package kustomize

import (
"context"
Expand Down
68 changes: 35 additions & 33 deletions ...ns/render/action_render_manifests_test.go → ...kustomize/action_render_manifests_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"

Expand All @@ -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"
Expand Down Expand Up @@ -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))
Expand All @@ -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),
),
)

Expand Down Expand Up @@ -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{
Expand All @@ -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))
}
Comment on lines +241 to +248
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure i fully understand this case:
so we have 3 replica of dashboard,
for the 1st one, gauge should set be 1
for the 2nd one, then gauge to 2
and the last one, to 3.
or i mistunderstood this

Copy link
Contributor Author

@lburgazzoli lburgazzoli Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is not about replicas, this is about testing the cache so:

  • 1st iteration, the counter should be 1, because nothing did exist before
  • 2nd iteration, it should be 2, because the Generation field of the live object has changed
  • 3rd iteration, it should still 3, because nothing has changed so no rendering should have been performed and values are taken from the cache

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i see, so the test is to generate a 3 times-loop to minic :
i = 0, first time creation, no cache, increase in guage
i= 1, render when generateion already as 1, get from cache, increase in gauge
i= 2, render but with same generation value, no increase in gauge
no need more, till be same as i=2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, case 1,2: that you do not spend effort to compare symbol by symbol equal branches?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in 1,2 is where the action return the previously rendered resources because the source of customization has not changed

}
}
32 changes: 32 additions & 0 deletions pkg/controller/actions/render/render_metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
zdtsw marked this conversation as resolved.
Show resolved Hide resolved
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)
}
1 change: 0 additions & 1 deletion pkg/metadata/annotations/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)