`ACK.ResourceSynced` condition gets removed in ACK runtime `v0.16.0`

[vijtrip2]

Describe the bug
ACK.ResourceSynced gets removed after successful creation of the resource in ACK runtime v0.16.0

Context

In the new release v0.16.0, ACK runtime had a change to reset(remove) old Status.Conditions at the start of reconciler loop, and set latest Status.Conditions again during reconciliation. The latest Status.Conditions are then patched into the K8s object status at end of reconciler loop.

If at the end of reconciler loop, there is no ACK.ResourceSynced condition present inside Status.Conditions, then ACK.ResourceSynced=Unknown condition gets added.

ACK.ResourceSynced condition is an important condition for customers to determine whether the resource is synced or still under reconciliation process. The above mentioned change was made as a precursor change for a new feature to determine ACK.ResourceSynced condition by introspecting Spec/Status field values, or invoking AWS APIs, and NOT just relying on whether any error occurred during reconciliation loop or not.

Problem

While the proposed feature for setting ACK.ResourceSynced condition by introspecting Spec/Status fields is still under development, there is a side-effect from the precursor change causing test failures in various controllers(with new runtime) that assert on resource condition.

The side-effect is that, Even if ACK.ResourceSynced condition was set correctly during Create operation, the following reconciler loops remove this condition at beginning of loop. If the Update operation uses hooks to set this condition again, there are no problems but following edge case is still not handled.

Edge Case:
When a resource is first created, a finalizer is added and metadata of K8s resource is patched, which triggers a second reconciliation loop. During the second reconciliation loop, conditions are removed at beginning and since resource neither gets created, nor gets updated in this second loop, the ACK.ResourceSynced condition is never set and default to ACK.ResourceSynced=Unknown which is problematic, when resource has been successfully created.

Proposed Solutions (Sorted in order of speed of implementation)

1. Revert the precursor change, and fall back to old behavior. Add this change back when AWSResourceManager.IsResourceSynced() method is implemented

2. Keep the precursor change but instead of adding ACK.ResourceSynced=Unknown when condition is missing at the end, Set True or False depending on whether there was any reconciler error or not.

3. Complete the implementation of determining the ACK.ResourceSynced condition from AWSResourceManager.IsResourceSynced() method and not rely on hook code from various Create/Update methods.

[jaypipes]

@vijtrip2 for solution (2), could we potentially change that to: "Set True or False depending on whether there was any reconciler error AND whether there was no Diff found in the Delta"? In other words, set ACK.ResourceSynced=True when there was no error in the ReadOne and the returned resource from ReadOne matches the Spec fields of the originally-supplied resource?

[acornett21]

I feel that clearing out the statuses completely on each reconcile leads to a bad UI/UX experience. IE a user sees Success in the Status for a Bucket for example, then the Status would become not present and the user would no longer see what they preciously say 'Ready/Failed/etc', which is confusing.

[vijtrip2]

I feel that clearing out the statuses completely on each reconcile leads to a bad UI/UX experience.

That is not correct :) Only Status.Conditions is cleared and calculated again during reconciler loop.

Status would become not present and

This is not correct either, Status is patched. The flow is not like
"remove -> add again" but it's more like
"in-memory remove -> in-memory add latest conditions -> patch on disk"

[acornett21]

I was referring to Status.Conditions, sorry to not state that clearly.

Doing the below I never see any value being written to Status.Conditions
kubectl get bucket example-8675309 -oyaml -w

yields the below

status:
  ackResourceMetadata:
    ownerAccountID: <omitted>
  location: http://example-8675309.s3.amazonaws.com/

Am I missing something?

edit: picture for clarity when I say Status will be empty ie the UI will be empty...the Status field below shows the conditions.

A working example

[vijtrip2]

Thanks @acornett21 , for detailed explanation.

This may be a specific bug for S3 controller, but the new patch we just made, will make sure at least 'ACK.ResourceSynced' condition will always be present. If "True" or "False" synced status cannot be determined, atleast "Unknown" status will be populated.

There is also a separate effort to correctly determine 'ACK.ResourceSynced' condition as "True"/"False" and making sure it is always present. After the mentioned PR is merged, we will audit all the controllers and make sure setting of 'ACK.ResourceSynced' condition is consistent.

[RedbackThomson]

After reading articles like the following: https://maelvls.dev/kubernetes-conditions/ , it seems that the way we handle conditions does not match the upstream Kubernetes standard. I have also looked at the fluxcd/pkg repository (https://github.com/fluxcd/pkg/tree/main/runtime/conditions) and see that they handle conditions in the same way as Pod.

Would it be possible to align our designs more so to those. If we cannot change our version of condition to metav1.Condition directly, then our changes may include (at least):

• Adding a LastTransitionTime property to each condition
• Adding an ObservedGeneration property to each condition
• Not removing any conditions - only updating or adding new conditions (keyed by Type)

As for ResourceSynced, this would solve our problem by having our initial condition (after the first reconciliation loop) be Unknown, then updating the condition as and when needed afterwards.