feat: Separate business status from module status (#1628)

* Remove custom CR state check, only check for deployment state and map pod state to amnifest CR * Remove custom CR state check, only check for deployment state and map pod state to amnifest CR * Remove arg from test func * Fix integration test * Fix integration test * Refactor Status propagation * Fix Status decoupling Test * Fix Status decoupling Test * Set deleting state if manifest has a deletion timestamp * Adapt state decoupling test * Add new Testcase * Use wrong config module for test * Use wrong config module for test * Fix wrong config module for test * Fix go lint * Add docs * Adress review comments * Gofumpt * Adress second review comments * Introduce more unit tests * Remove duplicated check * Remove duplicated check * Apply suggestions from code review Co-authored-by: Grzegorz Karaluch <[email protected]> * Remove Stateinfo * Apply suggestions from code review Co-authored-by: Iwona Langer <[email protected]> --------- Co-authored-by: Grzegorz Karaluch <[email protected]> Co-authored-by: Iwona Langer <[email protected]>
kyma-project · Jul 2, 2024 · 130e98f · 130e98f
1 parent 9419e58
commit 130e98f
Show file tree

Hide file tree

Showing 13 changed files with 352 additions and 682 deletions.
diff --git a/.github/actions/deploy-template-operator/action.yaml b/.github/actions/deploy-template-operator/action.yaml
@@ -48,7 +48,7 @@ runs:
         kubectl apply -f tests/moduletemplates/moduletemplate_template_operator_v2_fast.yaml
     - name: Create Template Operator Module with final state and final deletion state as `Warning` and apply
       working-directory: template-operator
-      if: ${{ matrix.e2e-test == 'module-status-propagation'}}
+      if: ${{ matrix.e2e-test == 'module-status-decoupling'}}
       shell: bash
       run: |
         pushd config/default
@@ -66,6 +66,23 @@ runs:
         sed -i 's/localhost:5111/k3d-kcp-registry.localhost:5000/g' ./template.yaml
         kubectl get crds
         kubectl apply -f template.yaml
+    - name: Create Template Operator Module with non-working image and apply
+      working-directory: template-operator
+      if: ${{ matrix.e2e-test == 'module-status-decoupling'}}
+      shell: bash
+      run: |
+        pushd config/default
+        echo \
+        "- op: replace
+          path: /spec/template/spec/containers/0/image
+          value: non-working-path" >> image_patch.yaml
+        cat image_patch.yaml
+        kustomize edit add patch --path image_patch.yaml --kind Deployment
+        popd
+        kyma alpha create module --kubebuilder-project  --channel=regular --name kyma.project.io/module/template-operator-misconfigured --version 1.1.1 --path . --registry localhost:5111 --insecure --module-archive-version-overwrite
+        sed -i 's/localhost:5111/k3d-kcp-registry.localhost:5000/g' ./template.yaml
+        kubectl get crds
+        kubectl apply -f template.yaml
     - name: Create Template Operator Module without default CR and apply
       working-directory: template-operator
       if: ${{ matrix.e2e-test == 'module-without-default-cr' }}

diff --git a/.github/workflows/test-e2e.yaml b/.github/workflows/test-e2e.yaml
@@ -29,7 +29,7 @@ jobs:
           - watcher-enqueue
           - kyma-deprovision-with-foreground-propagation
           - kyma-deprovision-with-background-propagation
-          - module-status-propagation
+          - module-status-decoupling
           - kyma-metrics
           - module-without-default-cr
           - module-consistency

diff --git a/docs/technical-reference/api/manifest-cr.md b/docs/technical-reference/api/manifest-cr.md
@@ -87,7 +87,14 @@ The resource is the default data that should be initialized for the module and i
 
 ### **.status**
 
-The Manifest CR status is an unmodified version of the [declarative status](../../../internal/declarative/README.md#resource-tracking), so the tracking process of the library applies. There is no custom API for this.
+The Manifest CR status is set based on the following logic, managed by the manifest reconciler: 
+
+- If the module defined in the Manifest CR is successfully applied and the deployed module is up and running, the status of the Manifest CR is set to `Ready`.
+- While the manifest is being applied and the Deployment is still starting, the status of the Manifest CR is set to `Processing`.
+- If the Deployment cannot start (for example, due to an `ImagePullBackOff` error) or if the application of the manifest fails, the status of the Manifest CR is set to `Error`.
+- If the Manifest CR is marked for deletion, the status of the Manifest CR is set to `Deleting`.
+
+This status provides a reliable way to track the state of the Manifest CR and the associated module. It offers insights into the deployment process and any potential issues while being decoupled from the module's business logic.
 
 ### **.metadata.labels**
 

diff --git a/internal/controller/manifest/controller.go b/internal/controller/manifest/controller.go
@@ -26,7 +26,7 @@ func NewReconciler(mgr manager.Manager,
 		declarativev2.WithSpecResolver(
 			manifest.NewSpecResolver(kcp, extractor),
 		),
-		declarativev2.WithCustomReadyCheck(manifest.NewCustomResourceReadyCheck()),
+		declarativev2.WithCustomReadyCheck(manifest.NewDeploymentReadyCheck()),
 		declarativev2.WithRemoteTargetCluster(lookup.ConfigResolver),
 		manifest.WithClientCacheKey(),
 		declarativev2.WithPostRun{manifest.PostRunCreateCR},

diff --git a/internal/declarative/v2/ready_check.go b/internal/declarative/v2/ready_check.go
@@ -13,13 +13,8 @@ import (
 
 var ErrNotValidClientObject = errors.New("object in resource info is not a valid client object")
 
-type StateInfo struct {
-	shared.State
-	Info string
-}
-
 type ReadyCheck interface {
-	Run(ctx context.Context, clnt Client, obj Object, resources []*resource.Info) (StateInfo, error)
+	Run(ctx context.Context, clnt Client, resources []*resource.Info) (shared.State, error)
 }
 
 func NewExistsReadyCheck() *ExistsReadyCheck {
@@ -31,17 +26,16 @@ type ExistsReadyCheck struct{}
 func (c *ExistsReadyCheck) Run(
 	ctx context.Context,
 	clnt Client,
-	_ Object,
 	resources []*resource.Info,
-) (StateInfo, error) {
+) (shared.State, error) {
 	for i := range resources {
 		obj, ok := resources[i].Object.(client.Object)
 		if !ok {
-			return StateInfo{State: shared.StateError}, ErrNotValidClientObject
+			return shared.StateError, ErrNotValidClientObject
 		}
 		if err := clnt.Get(ctx, client.ObjectKeyFromObject(obj), obj); client.IgnoreNotFound(err) != nil {
-			return StateInfo{State: shared.StateError}, fmt.Errorf("failed to fetch object by key: %w", err)
+			return shared.StateError, fmt.Errorf("failed to fetch object by key: %w", err)
 		}
 	}
-	return StateInfo{State: shared.StateReady}, nil
+	return shared.StateReady, nil
 }
diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go
@@ -363,7 +363,12 @@ func (r *Reconciler) syncResources(ctx context.Context, clnt Client, obj Object,
 		}
 	}
 
-	return r.checkTargetReadiness(ctx, clnt, obj, target)
+	deploymentState, err := r.checkDeploymentState(ctx, clnt, target)
+	if err != nil {
+		obj.SetStatus(status.WithState(shared.StateError).WithErr(err))
+		return err
+	}
+	return r.setManifestState(obj, deploymentState)
 }
 
 func hasDiff(oldResources []shared.Resource, newResources []shared.Resource) bool {
@@ -387,44 +392,49 @@ func hasDiff(oldResources []shared.Resource, newResources []shared.Resource) boo
 	return false
 }
 
-func (r *Reconciler) checkTargetReadiness(
-	ctx context.Context, clnt Client, manifest Object, target []*resource.Info,
-) error {
-	status := manifest.GetStatus()
-
+func (r *Reconciler) checkDeploymentState(
+	ctx context.Context, clnt Client, target []*resource.Info,
+) (shared.State, error) {
 	resourceReadyCheck := r.CustomReadyCheck
 
-	crStateInfo, err := resourceReadyCheck.Run(ctx, clnt, manifest, target)
+	deploymentState, err := resourceReadyCheck.Run(ctx, clnt, target)
 	if err != nil {
-		manifest.SetStatus(status.WithState(shared.StateError).WithErr(err))
-		return err
+		return shared.StateError, err
+	}
+
+	if deploymentState == shared.StateProcessing {
+		return shared.StateProcessing, nil
 	}
 
-	if crStateInfo.State == shared.StateProcessing {
-		waitingMsg := "waiting for resources to become ready: " + crStateInfo.Info
+	return deploymentState, nil
+}
+
+func (r *Reconciler) setManifestState(manifest Object, state shared.State) error {
+	status := manifest.GetStatus()
+
+	if state == shared.StateProcessing {
+		waitingMsg := "waiting for resources to become ready"
 		manifest.SetStatus(status.WithState(shared.StateProcessing).WithOperation(waitingMsg))
 		return ErrInstallationConditionRequiresUpdate
 	}
 
+	if !manifest.GetDeletionTimestamp().IsZero() {
+		state = shared.StateDeleting
+	}
+
 	installationCondition := newInstallationCondition(manifest)
-	if !meta.IsStatusConditionTrue(status.Conditions, installationCondition.Type) || status.State != crStateInfo.State {
+	if !meta.IsStatusConditionTrue(status.Conditions,
+		installationCondition.Type) || status.State != state {
 		installationCondition.Status = apimetav1.ConditionTrue
 		meta.SetStatusCondition(&status.Conditions, installationCondition)
-		manifest.SetStatus(status.WithState(crStateInfo.State).
-			WithOperation(generateOperationMessage(installationCondition, crStateInfo)))
+		manifest.SetStatus(status.WithState(state).
+			WithOperation(installationCondition.Message))
 		return ErrInstallationConditionRequiresUpdate
 	}
 
 	return nil
 }
 
-func generateOperationMessage(installationCondition apimetav1.Condition, stateInfo StateInfo) string {
-	if stateInfo.Info != "" {
-		return stateInfo.Info
-	}
-	return installationCondition.Message
-}
-
 func (r *Reconciler) removeModuleCR(ctx context.Context, clnt Client, obj Object) error {
 	if !obj.GetDeletionTimestamp().IsZero() {
 		for _, preDelete := range r.PreDeletes {