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

[c-pius]

Description

Changes proposed in this pull request:

• Updates the control flow in state check to return explicit errors on unexpected conditions, i.e., no manager found, fallthrough
• Updates the control flow in reconciler to exit early with state Deleting when manifest has a deletion timestamp

The fallthrough case is not testable unfortunately. We may be able to get rid of it in the proposed further refactoring in the future.

Related issue(s)

• follow-up to fix: Fix faulty manifest state determination #1807

[ruanxin]

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

[ruanxin]

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.

[c-pius]

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?

[ruanxin]

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.

[c-pius]

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.

[ruanxin]

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"

[c-pius]

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
}

[ruanxin]

pls check the comment