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

fix: Set manifest to deleting state; Raise errors on unexpected manager conditions #1833

Merged
merged 5 commits into from
Sep 9, 2024
Merged

fix: Set manifest to deleting state; Raise errors on unexpected manager conditions #1833

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
30a4c5f
fix: Use explicit errors when determining manager state
c-pius Sep 5, 2024
0c1c049
Merge branch 'main' into fix/explicit-errors-in-manager-state
lindnerby Sep 5, 2024
fc0461e
return early on deleting state; remove obsolete branch
c-pius Sep 6, 2024
2db02ca
remove re-setting the state in setManifestState
c-pius Sep 6, 2024
6c28f82
Merge branch 'main' into fix/explicit-errors-in-manager-state
c-pius Sep 6, 2024
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
9 changes: 4 additions & 5 deletions internal/declarative/v2/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,10 @@ func (r *Reconciler) syncResources(ctx context.Context, clnt Client, manifest *v
}
}

if !manifest.GetDeletionTimestamp().IsZero() {
return r.setManifestState(manifest, shared.StateDeleting)
Copy link
Contributor

Choose a reason for hiding this comment

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

but inside this setManifestState, we did same thing, set state to deleting when manifest has deletiontimestamp

https://github.com/kyma-project/lifecycle-manager/blob/main/internal/declarative/v2/reconciler.go#L418-L420

Copy link
Contributor

Choose a reason for hiding this comment

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

but consider rename this setManifestState, it's really misleading, if it's a set method, I don't expect the input state will be modified in the body of the function.

Copy link
Contributor Author

@c-pius c-pius Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch, thanks! I removed modifying the state arg in setManifestState. We need to check the deletion timestamp outside because we can't reliably determine the manager state if there is a deletion timestamp. And this way it is now also consistent. We first determine the state to transition to, then we call setManfiestState with that state.

TBH, I am also a bit confused about the rest of the setManifestState method. If I understand it right, we set Condition Installation to True and set Last Operator Message to installation is ready and resources can be used whenever we transition between states other than Processing. So if we go from Reay to Warning, we will keep this condition and message. If we go from Ready to Error, we will keep this condition and message, If we go from Processing to Error, we will keep this condition and message.... Is this expected or do we need to straighten out a few things?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the rest code is a bit bluring, I think we can improve it, but logically should fine I think.

When the reconcilation enter into setManifestState, we don't have Error state case, this should already requeued in previous steps.

So the rest state are Ready, Processing, Warning, basically all of these state means installation should be finished.

if !meta.IsStatusConditionTrue(status.Conditions, installationCondition.Type) || status.State != state
this meaning:

  1. the actual manifest CR status in cluster (status), the Installation condition is false; basically this means the actual installation condition in cluster is false.
  2. or actual manifest CR state ( status.State) is different than the state to be updated (state); this means we need to update the state,

in both case, we should update the installation condition to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I have created a follow-up to investigate this more thoroughly: #1846

Right now I don't know if I understand the setting of those conditions correctly. Let's discuss that.

But until then, we could merge this one as proposed if okay from your side.

Copy link
Contributor

Choose a reason for hiding this comment

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

this logic prevents all follow steps, right? if Manifest CR is under deletion, it always requeue with set state to Deleting

I think you need to update if !manifest.GetDeletionTimestamp().IsZero() && manifest.status.state != "deleting"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I think it only requeus when we have a difference in state. First, we determine the "target" state which is "Deleting" if we have a deletion timestamp. With this we call the setManfiestState function:

if !manifest.GetDeletionTimestamp().IsZero() {
		return r.setManifestState(manifest, shared.StateDeleting)
	}

And in the setManifestState function, we only update the status and return the ErrInstallationConditionRequiresUpdate error if it actually differs (if newState != status.State). To my understanding, we use this error to determine if the requeue is required. If we return nil, we don't requeue. I don't particularly like this, but I also think we should keep it for this chagne and then focus on improving with the created ticket.

func (r *Reconciler) setManifestState(manifest *v1beta2.Manifest, newState shared.State) error {
	status := manifest.GetStatus()

	if newState == shared.StateProcessing {
		manifest.SetStatus(status.WithState(shared.StateProcessing).WithOperation(waitingForResourcesMsg))
		return ErrInstallationConditionRequiresUpdate
	}

	installationCondition := newInstallationCondition(manifest)
	if newState != status.State || !meta.IsStatusConditionTrue(status.Conditions, installationCondition.Type) {
		installationCondition.Status = apimetav1.ConditionTrue
		meta.SetStatusCondition(&status.Conditions, installationCondition)

		manifest.SetStatus(status.WithState(newState).WithOperation(installationCondition.Message))
		return ErrInstallationConditionRequiresUpdate
	}

	return nil
}

}

managerState, err := r.checkManagerState(ctx, clnt, target)
if err != nil {
manifest.SetStatus(status.WithState(shared.StateError).WithErr(err))
Expand Down Expand Up @@ -393,16 +397,11 @@ 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
}

Expand Down
14 changes: 12 additions & 2 deletions internal/manifest/statecheck/state_check.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package statecheck

import (
"context"
"errors"

apiappsv1 "k8s.io/api/apps/v1"
"k8s.io/cli-runtime/pkg/resource"
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
35 changes: 28 additions & 7 deletions internal/manifest/statecheck/state_check_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -31,7 +33,9 @@ func TestManagerStateCheck_GetState(t *testing.T) {
},
},
},
isDeployment: true,
isDeployment: true,
isStateFulSet: false,
expectedError: nil,
},
{
name: "Test StatefulSet State Checker",
Expand All @@ -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 {
Expand All @@ -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)
Expand Down
Loading