fix: Set manifest to deleting state; Raise errors on unexpected manag…

…er conditions (#1833)

* fix: Use explicit errors when determining manager state

* return early on deleting state; remove obsolete branch

* remove re-setting the state in setManifestState

---------

Co-authored-by: Benjamin Lindner <50365642+lindnerby@users.noreply.github.com>

internal/declarative/v2/reconciler.go

@@ -69,6 +69,8 @@ type Reconciler struct {
 	specResolver           SpecResolver
 }
 
+const waitingForResourcesMsg = "waiting for resources to become ready"
+
 type ConditionType string
 
 const (
@@ -366,6 +368,10 @@ func (r *Reconciler) syncResources(ctx context.Context, clnt Client, manifest *v
 		}
 	}
 
+	if !manifest.GetDeletionTimestamp().IsZero() {
+		return r.setManifestState(manifest, shared.StateDeleting)
+	}
+
 	managerState, err := r.checkManagerState(ctx, clnt, target)
 	if err != nil {
 		manifest.SetStatus(status.WithState(shared.StateError).WithErr(err))
@@ -399,39 +405,28 @@ func (r *Reconciler) checkManagerState(ctx context.Context, clnt Client, target
 	error,
 ) {
 	managerReadyCheck := r.CustomStateCheck
-
 	managerState, err := managerReadyCheck.GetState(ctx, clnt, target)
 	if err != nil {
 		return shared.StateError, err
 	}
 
-	if managerState == shared.StateProcessing {
-		return shared.StateProcessing, nil
-	}
-
 	return managerState, nil
 }
 
-func (r *Reconciler) setManifestState(manifest *v1beta2.Manifest, state shared.State) error {
+func (r *Reconciler) setManifestState(manifest *v1beta2.Manifest, newState 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))
+	if newState == shared.StateProcessing {
+		manifest.SetStatus(status.WithState(shared.StateProcessing).WithOperation(waitingForResourcesMsg))
 		return ErrInstallationConditionRequiresUpdate
 	}
 
-	if !manifest.GetDeletionTimestamp().IsZero() {
-		state = shared.StateDeleting
-	}
-
 	installationCondition := newInstallationCondition(manifest)
-	if !meta.IsStatusConditionTrue(status.Conditions,
-		installationCondition.Type) || status.State != state {
+	if newState != status.State || !meta.IsStatusConditionTrue(status.Conditions, installationCondition.Type) {
 		installationCondition.Status = apimetav1.ConditionTrue
 		meta.SetStatusCondition(&status.Conditions, installationCondition)
-		manifest.SetStatus(status.WithState(state).
-			WithOperation(installationCondition.Message))
+
+		manifest.SetStatus(status.WithState(newState).WithOperation(installationCondition.Message))
 		return ErrInstallationConditionRequiresUpdate
 	}
 

internal/manifest/statecheck/state_check.go

@@ -2,6 +2,7 @@ package statecheck
 
 import (
 	"context"
+	"errors"
 
 	apiappsv1 "k8s.io/api/apps/v1"
 	"k8s.io/cli-runtime/pkg/resource"
@@ -30,6 +31,11 @@ const (
 	StatefulSetKind ManagerKind = "StatefulSet"
 )
 
+var (
+	ErrNoManagerProvided = errors.New("failed to find manager in provided resources")
+	ErrNoStateDetermined = errors.New("failed to determine state for manager")
+)
+
 type Manager struct {
 	kind        ManagerKind
 	deployment  *apiappsv1.Deployment
@@ -45,13 +51,16 @@ func NewManagerStateCheck(statefulSetChecker StatefulSetStateChecker,
 	}
 }
 
+// Determines the state based on the manager. The manager may either be a Deployment or a StatefulSet and
+// must be included in the provided resources.
+// Will be refactored with https://github.com/kyma-project/lifecycle-manager/issues/1831
 func (m *ManagerStateCheck) GetState(ctx context.Context,
 	clnt client.Client,
 	resources []*resource.Info,
 ) (shared.State, error) {
 	mgr := findManager(clnt, resources)
 	if mgr == nil {
-		return shared.StateReady, nil
+		return shared.StateError, ErrNoManagerProvided
 	}
 
 	switch mgr.kind {
@@ -61,7 +70,8 @@ func (m *ManagerStateCheck) GetState(ctx context.Context,
 		return m.deploymentStateChecker.GetState(mgr.deployment)
 	}
 
-	return shared.StateReady, nil
+	// fall through that should not be reached
+	return shared.StateError, ErrNoStateDetermined
 }
 
 func findManager(clt client.Client, resources []*resource.Info) *Manager {

internal/manifest/statecheck/state_check_test.go

@@ -18,9 +18,11 @@ import (
 
 func TestManagerStateCheck_GetState(t *testing.T) {
 	tests := []struct {
-		name         string
-		resources    []*resource.Info
-		isDeployment bool
+		name          string
+		resources     []*resource.Info
+		isDeployment  bool
+		isStateFulSet bool
+		expectedError error
 	}{
 		{
 			name: "Test Deployment State Checker",
@@ -31,7 +33,9 @@ func TestManagerStateCheck_GetState(t *testing.T) {
 					},
 				},
 			},
-			isDeployment: true,
+			isDeployment:  true,
+			isStateFulSet: false,
+			expectedError: nil,
 		},
 		{
 			name: "Test StatefulSet State Checker",
@@ -42,7 +46,16 @@ func TestManagerStateCheck_GetState(t *testing.T) {
 					},
 				},
 			},
-			isDeployment: false,
+			isDeployment:  false,
+			isStateFulSet: true,
+			expectedError: nil,
+		},
+		{
+			name:          "Test no manager found",
+			resources:     []*resource.Info{},
+			isDeployment:  false,
+			isStateFulSet: false,
+			expectedError: statecheck.ErrNoManagerProvided,
 		},
 	}
 	for _, testCase := range tests {
@@ -55,12 +68,20 @@ func TestManagerStateCheck_GetState(t *testing.T) {
 			deploymentChecker := &DeploymentStateCheckerStub{}
 			m := statecheck.NewManagerStateCheck(statefulsetChecker, deploymentChecker)
 			got, err := m.GetState(context.Background(), clnt, testCase.resources)
-			require.NoError(t, err)
+
+			if testCase.expectedError == nil {
+				require.NoError(t, err)
+			} else {
+				require.Equal(t, testCase.expectedError, err)
+			}
+
 			if testCase.isDeployment {
 				require.True(t, deploymentChecker.called)
 				require.False(t, statefulsetChecker.called)
 				require.Equal(t, shared.StateProcessing, got)
-			} else {
+			}
+
+			if testCase.isStateFulSet {
 				require.True(t, statefulsetChecker.called)
 				require.False(t, deploymentChecker.called)
 				require.Equal(t, shared.StateReady, got)