feat: Avoid Redundant SSA for Manifest Patching (#1620)

* feat: avoid redundant ssa for manifest patching * refactor: linting issue * test: add unit test * fix: integration tests * refactor: unwrapped error * fix: state flickering * chore: add linter exception * chore: remove linter exception * fix: null pointer ref in case of mandatory module * chore: Add helpful comment Co-authored-by: Christoph Schwägerl <[email protected]> * feat: add additional diff check in NeedToUpdate() * test: diff check in unit test * refactor: lint * refactor: remove manifest diff check * fix: module template integration test * test: add unit test * Revert "test: add unit test" This reverts commit a5a9102. * Revert "fix: module template integration test" This reverts commit 9ed7e26. * fix integration test * chore: retrigger * refactor: gofunmpt * docs: Apply suggestions from code review Co-authored-by: Małgorzata Świeca <[email protected]> --------- Co-authored-by: Christoph Schwägerl <[email protected]> Co-authored-by: Christoph Schwägerl <[email protected]> Co-authored-by: Małgorzata Świeca <[email protected]>
kyma-project · Jul 8, 2024 · 136476d · 136476d
1 parent dfd484b
commit 136476d
Show file tree

Hide file tree

Showing 6 changed files with 206 additions and 43 deletions.
diff --git a/...s/component-integration-installed/operator_v1beta2_moduletemplate_kcp-module_updated.yaml b/...s/component-integration-installed/operator_v1beta2_moduletemplate_kcp-module_updated.yaml
@@ -0,0 +1,56 @@
+apiVersion: operator.kyma-project.io/v1beta2
+kind: ModuleTemplate
+metadata:
+  name: moduletemplate-template-operator
+  namespace: default
+  labels:
+    "operator.kyma-project.io/module-name": "template-operator"
+spec:
+  channel: regular
+  data:
+    apiVersion: operator.kyma-project.io/v1alpha1
+    kind: Sample
+    metadata:
+      name: sample-yaml
+    spec:
+      initKey: valueUpdated
+      resourceFilePath: "./module-data/yaml"
+  descriptor:
+    component:
+      componentReferences: []
+      name: kyma-project.io/template-operator
+      provider: internal
+      repositoryContexts:
+        - baseUrl: registry.docker.io/kyma-project/sap-kyma-jellyfish-dev/template-operator
+          componentNameMapping: urlPath
+          type: ociRegistry
+      resources:
+        - access:
+            digest: sha256:db86408caca4c94250d8291aa79655b84146f9cc45e0da49f05a52b3722d74a0
+            type: localOciBlob
+          name: config
+          relation: local
+          type: yaml
+          version: v1.7.2
+        - access:
+            digest: sha256:1735cfa45bf07b63427c8e11717278f8847e352a66af7633611db902386d18ed
+            type: localOciBlob
+          name: raw-manifest
+          relation: local
+          type: yaml
+          version: v1.7.2
+      sources:
+        - access:
+            commit: 4e4b9d47cb655ca23e5c706462485ff7605e8d71
+            repoUrl: github.com/kyma-project/template-operator
+            type: gitHub
+          labels:
+            - name: git.kyma-project.io/ref
+              value: refs/heads/main
+              version: v1
+          name: module-sources
+          type: git
+          version: v1.7.2
+      version: v1.7.2
+    meta:
+      schemaVersion: v2
diff --git a/docs/technical-reference/api/manifest-cr.md b/docs/technical-reference/api/manifest-cr.md
@@ -2,6 +2,17 @@
 
 The [Manifest custom resource (CR)](../../../api/v1beta2/manifest_types.go) is our internal representation of what results from the resolution of a ModuleTemplate CR in the context of a single cluster represented by a Kyma CR. Thus, a lot of configuration elements are similar or entirely equivalent to the data layer found in a ModuleTemplate CR.
 
+## Patching
+
+The [Runner](../../../pkg/module/sync/runner.go) is responsible for creating and updating Manifest CRs using Server Side Apply (SSA). An update is only performed when one of the following conditions is met:
+
+1. The Manifest CR version differs from the Kyma CR's module status version.
+2. The Manifest CR channel differs from the Kyma CR's module status channel.
+3. The Manifest CR state differs from the Kyma CR's module status state.
+
+>[!NOTE] 
+>The module status is not present in the Kyma CR for mandatory modules, hence their Manifest CR is updated using SSA in every reconcile loop.
+
 ## Configuration
 
 ### **.spec.remote**

diff --git a/pkg/module/sync/runner.go b/pkg/module/sync/runner.go
@@ -64,7 +64,7 @@ func (r *Runner) ReconcileManifests(ctx context.Context, kyma *v1beta2.Kyma,
 				results <- nil
 				return
 			}
-			if err := r.updateManifests(ctx, kyma, module); err != nil {
+			if err := r.updateManifest(ctx, kyma, module); err != nil {
 				results <- fmt.Errorf("could not update module %s: %w", module.GetName(), err)
 				return
 			}
@@ -95,7 +95,7 @@ func (r *Runner) getModule(ctx context.Context, module client.Object) error {
 	return nil
 }
 
-func (r *Runner) updateManifests(ctx context.Context, kyma *v1beta2.Kyma,
+func (r *Runner) updateManifest(ctx context.Context, kyma *v1beta2.Kyma,
 	module *common.Module,
 ) error {
 	if err := r.setupModule(module, kyma); err != nil {
@@ -110,17 +110,34 @@ func (r *Runner) updateManifests(ctx context.Context, kyma *v1beta2.Kyma,
 		return commonerrs.ErrTypeAssert
 	}
 
+	moduleStatus := kyma.GetModuleStatusMap()[module.ModuleName]
 	if err := r.doUpdateWithStrategy(ctx, kyma.Labels[shared.ManagedBy], module.Enabled,
-		manifestObj); err != nil {
+		manifestObj, moduleStatus); err != nil {
 		return err
 	}
 	module.Manifest = manifestObj
 	return nil
 }
 
 func (r *Runner) doUpdateWithStrategy(ctx context.Context, owner string, isEnabledModule bool,
-	manifestObj *v1beta2.Manifest,
+	manifestObj *v1beta2.Manifest, kymaModuleStatus *v1beta2.ModuleStatus,
 ) error {
+	manifestInCluster := &v1beta2.Manifest{}
+	if err := r.Get(ctx, client.ObjectKey{
+		Namespace: manifestObj.GetNamespace(),
+		Name:      manifestObj.GetName(),
+	}, manifestInCluster); err != nil {
+		if !util.IsNotFound(err) {
+			return fmt.Errorf("error get manifest %s: %w", client.ObjectKeyFromObject(manifestObj), err)
+		}
+		manifestInCluster = nil
+	}
+
+	if !NeedToUpdate(manifestInCluster, manifestObj, kymaModuleStatus) {
+		// Point to the current state from the cluster for the outside sync of the manifest
+		*manifestObj = *manifestInCluster
+		return nil
+	}
 	if isEnabledModule {
 		return r.patchManifest(ctx, owner, manifestObj)
 	}
@@ -144,23 +161,27 @@ func (r *Runner) patchManifest(ctx context.Context, owner string, manifestObj *v
 
 func (r *Runner) updateAvailableManifestSpec(ctx context.Context, manifestObj *v1beta2.Manifest) error {
 	manifestInCluster := &v1beta2.Manifest{}
-
-	if err := r.Get(ctx, client.ObjectKey{Namespace: manifestObj.GetNamespace(), Name: manifestObj.GetName()},
-		manifestInCluster); err != nil {
+	if err := r.Get(ctx, client.ObjectKey{
+		Namespace: manifestObj.GetNamespace(),
+		Name:      manifestObj.GetName(),
+	}, manifestInCluster); err != nil {
 		return fmt.Errorf("error get manifest %s: %w", client.ObjectKeyFromObject(manifestObj), err)
 	}
-	if !needToUpdate(manifestInCluster, manifestObj) {
-		return nil
-	}
 	manifestInCluster.Spec = manifestObj.Spec
 	if err := r.Update(ctx, manifestInCluster); err != nil {
 		return fmt.Errorf("error update manifest %s: %w", client.ObjectKeyFromObject(manifestObj), err)
 	}
 	return nil
 }
 
-func needToUpdate(manifestInCluster, manifestObj *v1beta2.Manifest) bool {
-	return manifestInCluster.Spec.Version != manifestObj.Spec.Version
+func NeedToUpdate(manifestInCluster, manifestObj *v1beta2.Manifest, moduleStatus *v1beta2.ModuleStatus) bool {
+	if manifestInCluster == nil || moduleStatus == nil { // moduleStatus is nil in case of mandatory module
+		return true
+	}
+
+	return manifestObj.Spec.Version != moduleStatus.Version ||
+		manifestObj.Labels[shared.ChannelLabel] != moduleStatus.Channel ||
+		moduleStatus.State != manifestInCluster.Status.State
 }
 
 func (r *Runner) deleteManifest(ctx context.Context, module *common.Module) error {

diff --git a/pkg/module/sync/runner_test.go b/pkg/module/sync/runner_test.go
@@ -9,9 +9,11 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/kyma-project/lifecycle-manager/api/shared"
 	"github.com/kyma-project/lifecycle-manager/api/v1beta2"
 	"github.com/kyma-project/lifecycle-manager/pkg/module/sync"
 	"github.com/kyma-project/lifecycle-manager/pkg/testutils"
@@ -186,3 +188,83 @@ func configureModuleInKyma(
 		kyma.Status.Modules = append(kyma.Status.Modules, module)
 	}
 }
+
+func TestNeedToUpdate(t *testing.T) {
+	type args struct {
+		manifestInCluster *v1beta2.Manifest
+		manifestObj       *v1beta2.Manifest
+		moduleStatus      *v1beta2.ModuleStatus
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			"When manifest in cluster is nil, expect need to update",
+			args{nil, &v1beta2.Manifest{}, &v1beta2.ModuleStatus{}},
+			true,
+		},
+		{
+			"When new module version available, expect need to update",
+			args{
+				&v1beta2.Manifest{},
+				&v1beta2.Manifest{
+					ObjectMeta: apimetav1.ObjectMeta{
+						Labels: map[string]string{shared.ChannelLabel: "regular"},
+					},
+					Spec: v1beta2.ManifestSpec{Version: "0.2"},
+				}, &v1beta2.ModuleStatus{Version: "0.1", Channel: "regular"},
+			},
+			true,
+		},
+		{
+			"When channel switch, expect need to update",
+			args{
+				&v1beta2.Manifest{},
+				&v1beta2.Manifest{
+					ObjectMeta: apimetav1.ObjectMeta{
+						Labels: map[string]string{shared.ChannelLabel: "fast"},
+					},
+					Spec: v1beta2.ManifestSpec{Version: "0.1"},
+				}, &v1beta2.ModuleStatus{Version: "0.1", Channel: "regular"},
+			},
+			true,
+		},
+		{
+			"When cluster Manifest in divergent state, expect need to update",
+			args{
+				&v1beta2.Manifest{Status: shared.Status{
+					State: "Warning",
+				}},
+				&v1beta2.Manifest{},
+				&v1beta2.ModuleStatus{State: "Ready"},
+			},
+			true,
+		},
+		{
+			"When no update required, expect no update",
+			args{
+				&v1beta2.Manifest{
+					Status: shared.Status{
+						State: "Ready",
+					},
+					Spec: v1beta2.ManifestSpec{Version: "0.1"},
+				},
+				&v1beta2.Manifest{
+					ObjectMeta: apimetav1.ObjectMeta{
+						Labels: map[string]string{shared.ChannelLabel: "regular"},
+					},
+					Spec: v1beta2.ManifestSpec{Version: "0.1"},
+				},
+				&v1beta2.ModuleStatus{State: "Ready", Version: "0.1", Channel: "regular"},
+			},
+			false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.want, sync.NeedToUpdate(tt.args.manifestInCluster, tt.args.manifestObj, tt.args.moduleStatus), "needToUpdate(%v, %v, %v)", tt.args.manifestInCluster, tt.args.manifestObj, tt.args.moduleStatus)
+		})
+	}
+}
diff --git a/pkg/testutils/builder/moduletemplate.go b/pkg/testutils/builder/moduletemplate.go
@@ -143,6 +143,11 @@ func ComponentDescriptorFactoryFromSchema(schemaVersion compdesc.SchemaVersion)
 	return moduleTemplate.Spec.Descriptor
 }
 
+func ReadComponentDescriptorFromFile(template string, moduleTemplate *v1beta2.ModuleTemplate) {
+	// needs to be encapsulated in an outside call to make the runtime.Caller(1) find the proper path
+	readComponentDescriptorFromYaml(template, moduleTemplate)
+}
+
 func readComponentDescriptorFromYaml(template string, moduleTemplate *v1beta2.ModuleTemplate) {
 	_, filename, _, ok := runtime.Caller(1)
 	if !ok {

diff --git a/tests/integration/controller/kyma/manifest_test.go b/tests/integration/controller/kyma/manifest_test.go
@@ -16,6 +16,7 @@ import (
 	"github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/genericocireg/componentmapping"
 	"github.com/open-component-model/ocm/pkg/runtime"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -25,6 +26,7 @@ import (
 	"github.com/kyma-project/lifecycle-manager/internal/pkg/flags"
 
 	. "github.com/kyma-project/lifecycle-manager/pkg/testutils"
+	"github.com/kyma-project/lifecycle-manager/pkg/testutils/builder"
 )
 
 const (
@@ -91,41 +93,26 @@ var _ = Describe("Update Manifest CR", Ordered, func() {
 			WithArguments(kyma.GetName(), kyma.GetNamespace(), kcpClient, shared.StateReady).
 			Should(Succeed())
 
-		By("Update Module Template spec.data.spec field")
-		valueUpdated := "valueUpdated"
-		Eventually(updateKCPModuleTemplateSpecData(kyma.Name, valueUpdated), Timeout, Interval).Should(Succeed())
+		By("Update Module Template spec")
+		var moduleTemplateFromFile v1beta2.ModuleTemplate
+		builder.ReadComponentDescriptorFromFile("operator_v1beta2_moduletemplate_kcp-module_updated.yaml", &moduleTemplateFromFile)
 
-		By("CR updated with new value in spec.resource.spec")
-		Eventually(expectManifestSpecDataEquals(kyma.Name, valueUpdated), Timeout, Interval).Should(Succeed())
-
-		By("Update Module Template spec.descriptor.component values")
-		{
-			newComponentDescriptorRepositoryURL := func(moduleTemplate *v1beta2.ModuleTemplate) error {
-				descriptor, err := descriptorProvider.GetDescriptor(moduleTemplate)
-				if err != nil {
-					return err
-				}
-
-				repositoryContext := descriptor.GetEffectiveRepositoryContext().Object
-				_, ok := repositoryContext["baseUrl"].(string)
-				if !ok {
-					Fail("Can't find \"baseUrl\" property in ModuleTemplate spec")
-				}
-				repositoryContext["baseUrl"] = updateRepositoryURL
+		moduleTemplateInCluster := &v1beta2.ModuleTemplate{}
+		err := kcpClient.Get(ctx, client.ObjectKey{
+			Name:      createModuleTemplateName(module),
+			Namespace: kyma.GetNamespace(),
+		}, moduleTemplateInCluster)
+		Expect(err).ToNot(HaveOccurred())
 
-				newDescriptorRaw, err := compdesc.Encode(descriptor.ComponentDescriptor, compdesc.DefaultJSONCodec)
-				Expect(err).ToNot(HaveOccurred())
-				moduleTemplate.Spec.Descriptor.Raw = newDescriptorRaw
+		moduleTemplateInCluster.Spec = moduleTemplateFromFile.Spec
 
-				return nil
-			}
+		Eventually(kcpClient.Update, Timeout, Interval).
+			WithContext(ctx).
+			WithArguments(moduleTemplateInCluster).
+			Should(Succeed())
 
-			updateKCPModuleTemplateWith := updateKCPModuleTemplate(module, kyma.Spec.Channel)
-			update := func() error {
-				return updateKCPModuleTemplateWith(newComponentDescriptorRepositoryURL)
-			}
-			Eventually(update, Timeout, Interval).Should(Succeed())
-		}
+		By("CR updated with new value in spec.resource.spec")
+		Eventually(expectManifestSpecDataEquals(kyma.Name, "valueUpdated"), Timeout, Interval).Should(Succeed())
 
 		By("Manifest is updated with new value in spec.install.source")
 		{
@@ -209,6 +196,7 @@ var _ = Describe("Manifest.Spec is reset after manual update", Ordered, func() {
 
 		manifestImageSpec := extractInstallImageSpec(manifest.Spec.Install)
 		manifestImageSpec.Repo = updateRepositoryURL
+		manifest.Spec.Version = "v1.7.0" // required to allow for SSA of manifest
 
 		// is there a simpler way to update manifest.Spec.Install?
 		updatedBytes, err := json.Marshal(manifestImageSpec)