Terminal condition sets ResourceSynced=False #94
Conversation
| @@ -29,8 +29,9 @@ var ( | |||
| NotManagedReason = "This resource already exists but is not managed by ACK. " + | |||
| "To bring the resource under ACK management, you should explicitly adopt " + | |||
| "the resource by creating a services.k8s.aws/AdoptedResource" | |||
| NotSyncedMessage = "Resource not synced" | |||
| SyncedMessage = "Resource synced successfully" | |||
| UnknownSyncedMessage = "Unable to determine if desired resource state matches latest observed state" | |||
There was a problem hiding this comment.
Instead of desired resource state do we want to just plainly say spec?
There was a problem hiding this comment.
Instead of
desired resource statedo we want to just plainly sayspec?
The issue with saying "spec" is that the spec can contain both the desired and latest state :) We compare full AWSResource objects to determine desired versus latest because some fields representing the latest state are set in the spec...
pkg/runtime/reconciler.go
Outdated
| // desired state of a resource, the resource's desired state | ||
| // will never match the latest observed state. Thus, | ||
| // ACK.ResourceSynced must be False. | ||
| condStatus = corev1.ConditionFalse | ||
| condMessage = ackcondition.SyncedMessage |
There was a problem hiding this comment.
| condMessage = ackcondition.SyncedMessage | |
| condMessage = ackcondition.NotSyncedMessage |
This patch updates the `resourceReconciler.ensureConditions()` method to set the `ACK.ResourceSynced` Condition value to `False` when a Terminal error has been returned from the resource manager. Previously, the `ACK.ResourceSynced` Condition value was erroneously being set to `True` due to a misunderstanding of what `ACK.ResourceSynced` means. `ACK.ResourceSynced` is a Condition that informs the Kubernetes user *whether the desired state of a resource matches the latest observed state of the resource*. In the case of a Terminal error, the desired state of a resource will *never* match the latest observed state of the resource because there is something invalid about the desired resource state. Further, this patch changes the default value for `ACK.ResourceSynced` Condition to be `Unknown` instead of `False` for any non-Terminal reconciler error. The reasoning behind this change is that for non-Terminal errors, we simply do not know whether the desired resource state matches the latest observed state or not. Signed-off-by: Jay Pipes <jaypipes@gmail.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jaypipes, RedbackThomson, vijtrip2 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This patch updates the
resourceReconciler.ensureConditions()method toset the
ACK.ResourceSyncedCondition value toFalsewhen a Terminalerror has been returned from the resource manager. Previously, the
ACK.ResourceSyncedCondition value was erroneously being set toTruedue to a misunderstanding of what
ACK.ResourceSyncedmeans.ACK.ResourceSyncedis a Condition that informs the Kubernetes userwhether the desired state of a resource matches the latest observed
state of the resource. In the case of a Terminal error, the desired
state of a resource will never match the latest observed state of the
resource because there is something invalid about the desired resource
state.
Further, this patch changes the default value for
ACK.ResourceSyncedCondition to be
Unknowninstead ofFalsefor any non-Terminalreconciler error. The reasoning behind this change is that for
non-Terminal errors, we simply do not know whether the desired resource
state matches the latest observed state or not.
Signed-off-by: Jay Pipes jaypipes@gmail.com
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.