Repository navigation
Add deletion policy field and decision logic #32
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| package common | ||
|
|
||
| // +kubebuilder:validation:Enum=Retain;Delete | ||
| type DeletionPolicy string | ||
|
|
||
| const ( | ||
| // DeletionPolicyRetain leaves the Temporal Cloud resource in place and | ||
| // only removes the custom resource | ||
| DeletionPolicyRetain DeletionPolicy = "Retain" | ||
|
|
||
| // DeletionPolicyDelete deletes the Temporal Cloud resource, and removes | ||
| // the custom resource after Temporal Cloud finishes deleting it. | ||
| DeletionPolicyDelete DeletionPolicy = "Delete" | ||
| ) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| package namespace | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "time" | ||
|
|
||
| "github.com/temporalio/cloud-operator/api/cloud/common" | ||
| "github.com/temporalio/cloud-operator/pkg/deletion" | ||
| ) | ||
|
|
||
| // Reasons for the decisions Decide makes. | ||
| const ( | ||
| ReasonDeleted = "Deleted" | ||
| ReasonIdentityUnverified = "IdentityUnverified" | ||
| ReasonNotCreatedByOperator = "NotCreatedByOperator" | ||
| ReasonDeleteProtected = "DeleteProtected" | ||
| ReasonNexusEndpointsTarget = "NexusEndpointsTarget" | ||
| ReasonDeleting = "Deleting" | ||
| ReasonWaiting = "Waiting" | ||
| ReasonSuspended = "Suspended" | ||
| ReasonUnknownState = "UnknownState" | ||
| ) | ||
|
|
||
| // Observed is the live state of a Temporal Cloud Namespace | ||
| type Observed struct { | ||
| // Found is false when Temporal Cloud returns NotFound. | ||
| Found bool | ||
| // State is the Namespace's state | ||
| State common.ResourceState | ||
| ResourceVersion string | ||
| CreatedTime *time.Time | ||
| // DeleteProtection is the live spec.lifecycle.enableDeleteProtection. | ||
| DeleteProtection bool | ||
| // NexusEndpoints is the number of Nexus endpoints targeting the | ||
| // Namespace. | ||
| NexusEndpoints int | ||
| } | ||
|
|
||
| // Decide decides what to do with a Namespace whose deletion policy is Delete, | ||
| // after deletion.Preflight returned false. It follows Temporal Cloud's delete | ||
| // rules: only an Active Namespace that isn't delete-protected, suspended or | ||
| // targeted by Nexus endpoints can be deleted. | ||
| // | ||
| // Namespace IDs are reused once a Namespace is gone, so Decide only deletes a | ||
| // Namespace whose creation time matches the one the operator recorded. | ||
| func Decide(rec deletion.Record, obs Observed) deletion.Decision { | ||
| switch { | ||
| case !obs.Found, obs.State == common.ResourceStateDeleted: | ||
| return decision(deletion.ActionRelease, ReasonDeleted, | ||
| "namespace %s is deleted in Temporal Cloud", rec.ID) | ||
| case rec.CreatedTime == nil: | ||
| return decision(deletion.ActionFail, ReasonIdentityUnverified, | ||
| "no creation time is recorded for namespace %s, so the operator can't confirm it created it; %s", rec.ID, deletion.RetainHint) | ||
| case !deletion.SameSecond(rec.CreatedTime, obs.CreatedTime): | ||
| return decision(deletion.ActionRelease, ReasonNotCreatedByOperator, | ||
| "namespace %s in Temporal Cloud is not the one the operator created; leaving it in Temporal Cloud", rec.ID) | ||
| } | ||
|
|
||
| switch obs.State { | ||
| case common.ResourceStateActive: | ||
| if obs.DeleteProtection { | ||
| return decision(deletion.ActionBlock, ReasonDeleteProtected, | ||
| "namespace %s has delete protection on; turn it off in Temporal Cloud, or %s", rec.ID, deletion.RetainHint) | ||
| } | ||
| if obs.NexusEndpoints > 0 { | ||
| return decision(deletion.ActionBlock, ReasonNexusEndpointsTarget, | ||
| "Nexus endpoints target namespace %s; delete them first, or %s", rec.ID, deletion.RetainHint) | ||
| } | ||
| return decision(deletion.ActionDelete, ReasonDeleting, | ||
| "deleting namespace %s in Temporal Cloud", rec.ID) | ||
| case common.ResourceStateDeleting, | ||
| common.ResourceStateActivating, | ||
| common.ResourceStateUpdating: | ||
| return decision(deletion.ActionWait, ReasonWaiting, | ||
| "namespace %s is %s in Temporal Cloud; waiting", rec.ID, obs.State) | ||
| case common.ResourceStateSuspended: | ||
| return decision(deletion.ActionBlock, ReasonSuspended, | ||
| "namespace %s is suspended, and Temporal Cloud doesn't delete suspended namespaces; contact Temporal support, or %s", rec.ID, deletion.RetainHint) | ||
| case common.ResourceStateDeleteFailed, | ||
| common.ResourceStateActivationFailed, | ||
| common.ResourceStateUpdateFailed, | ||
| // Temporal Cloud doesn't report Expired for namespaces today. | ||
| common.ResourceStateExpired: | ||
| return decision(deletion.ActionFail, "CloudState"+string(obs.State), | ||
| "namespace %s is %s in Temporal Cloud and can't be deleted; contact Temporal support, or %s", rec.ID, obs.State, deletion.RetainHint) | ||
| default: | ||
| return decision(deletion.ActionWait, ReasonUnknownState, | ||
| "namespace %s has state %q,waiting", rec.ID, obs.State) | ||
| } | ||
| } | ||
|
|
||
| func decision( | ||
| action deletion.Action, | ||
| reason string, | ||
| format string, | ||
| args ...any, | ||
| ) deletion.Decision { | ||
| return deletion.Decision{ | ||
| Action: action, | ||
| Reason: reason, | ||
| Message: fmt.Sprintf(format, args...), | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| // Package deletion provides shared logic for handling deleted custom resources. | ||
| // It makes no API calls; each resource uses live Temporal Cloud state to decide what to do. | ||
| package deletion | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "time" | ||
|
|
||
| "github.com/temporalio/cloud-operator/api/cloud/common" | ||
| ) | ||
|
|
||
| // Action is what to do next with a custom resource that is being deleted. | ||
| type Action int | ||
|
|
||
| const ( | ||
| // ActionRelease means the finalizer may be removed. Temporal Cloud is | ||
| // left as it is. | ||
| ActionRelease Action = iota + 1 | ||
| // ActionWait means Temporal Cloud is busy with the resource. | ||
| ActionWait | ||
| // ActionBlock means a person must unblock the delete. | ||
| ActionBlock | ||
| // ActionFail means the resource can't be deleted; stop until | ||
| // the custom resource changes. | ||
| ActionFail | ||
| // ActionDelete means send the delete request to Temporal Cloud. | ||
| ActionDelete | ||
| ) | ||
|
|
||
| // String returns the Action's name. | ||
| func (a Action) String() string { | ||
| switch a { | ||
| case ActionRelease: | ||
| return "Release" | ||
| case ActionWait: | ||
| return "Wait" | ||
| case ActionBlock: | ||
| return "Block" | ||
| case ActionFail: | ||
| return "Fail" | ||
| case ActionDelete: | ||
| return "Delete" | ||
| default: | ||
| return fmt.Sprintf("Action(%d)", int(a)) | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. where did you get the idea to do this? I'm just curious, because it's a pretty complicated interface you're proposing here that doesn't align with the way that resources are deleted in the Temporal Cloud API. For example, deleting a Namespace in the Temporal Cloud API simply returns an I was thinking that cloud-operator's controller-specific reconcilers would use a simpler interface for deleting a resource from Temporal Cloud API that just returns an For example, you might implement a // ResourceDelete deletes any managed external resources for the supplied
// [types.Resource]. Note that the Reconciler does NOT need to remove the
// finalizer on the associated [client.Object]. The Controller handles that.
// All the Reconciler needs to do is delete any owned resources or
// externally-managed resources associated with the supplied [types.Resource].
func (r *Reconciler) ResourceDelete(
ctx context.Context,
desired types.Resource,
) error {
var err error
log := ctrlreqlog.FromContext(ctx)
exit := log.Trace("resource.delete")
defer func() {
exit(err)
}()
tcc, err := r.CloudClientFor(ctx, desired)
if err != nil {
return fmt.Errorf("failed getting tcloud client: %w", err)
}
// We may have already called DeleteNamespace for this resource. If so, check
// that the delete operation has succeeded. If so, return a nil error. Otherwise,
// mark the resource as being in a state of pending deletion.
cr := desired.CR().(*tcloudv1alpha1.Namespace)
status := cr.Status
resState := status.State
switch (resState) {
case tcloudv1alpha1.ResourceStateDeleted:
// Nothing to do...
return nil
case tcloudv1alpha1.Deleting:
// Check the status of our ongoing deletion operation
opID := status.AsyncOperationID
if opID != "" {
req := tcloudservicev1.GetAsyncOperationRequest{Id: opID}
opRes, err := tcc.GetAsyncOperation(ctx, &req)
if err != nil {
return fmt.Errorf("failed getting async operation status: %w", err)
}
if opRes.GetState() == tcloudoperationv1.AsyncOperation_State_STATE_FULFILLED {
// Deletion succeeded, nothing to do.
return nil
} else {
// Deletion did not succeed. Add a Condition to the resource
// explaining the deletion has failed.
reason := fmt.Sprintf(...)
condition.SetStalled(desired, metav1.ConditionTrue, "ResourceDeleteFailed", reason)
return fmt.Errorf("failed deleting resource. async operation in state %s", resState.String())
}
default:
// Call the Temporal Cloud API to delete the Namespace.
req := tcloudservicev1.DeleteNamespaceRequest{...}
resp, err := tcc.DeleteNamespace(ctx, &req)
... handle errors...
}
}If the deletion failed due to a lifecycle deletion protection being set on the resource, then that error should be placed as a Condition on the resource and an error bubbled back up to prevent the Controller from removing the finalizer on the resource.
Collaborator
Author
There was a problem hiding this comment. I was actually going to bring this up myself during the crew k8s check in. I was having the same thoughts about whether we should just let Temporal Cloud enforce its own rules and keep the operator simple. But my design came from reading the delete handler in saas-control-plane and trying to mirror its validation so the conditions would be more specific in saying exactly why the delete is stuck instead of a generic failure. but I agree that duplicates what Cloud already enforces. |
||
|
|
||
| // Decision is what to do next, and why. | ||
| type Decision struct { | ||
| Action Action | ||
| // Reason is a condition reason. | ||
| Reason string | ||
| // Message explains the decision to the user. | ||
| Message string | ||
| } | ||
|
|
||
| // Record is what the operator recorded about the Temporal Cloud resource it | ||
| // created. | ||
| type Record struct { | ||
| // ID is the resource's Temporal Cloud ID. Only the create path sets it, so | ||
| // a non-empty ID means the operator created the resource. | ||
| ID string | ||
| // CreatedTime is the creation time Temporal Cloud reported for the | ||
| // resource. | ||
| CreatedTime *time.Time | ||
| } | ||
|
|
||
| // Reasons for the decisions Preflight makes. | ||
| const ( | ||
| ReasonInvalidDeletionPolicy = "InvalidDeletionPolicy" | ||
| ReasonRetained = "Retained" | ||
| ReasonNeverCreated = "NeverCreated" | ||
| ) | ||
|
|
||
| // RetainHint tells the user how to remove a custom resource whose Temporal | ||
| // Cloud resource can't be deleted. | ||
| const RetainHint = "set spec.deletionPolicy to Retain to remove this resource and leave it in Temporal Cloud" | ||
|
|
||
| // Resolve returns the effective deletion policy | ||
| func Resolve(policy, def common.DeletionPolicy) (common.DeletionPolicy, error) { | ||
| p := policy | ||
| if p == "" { | ||
| p = def | ||
| } | ||
| switch p { | ||
| case common.DeletionPolicyRetain, common.DeletionPolicyDelete: | ||
| return p, nil | ||
| default: | ||
| return "", fmt.Errorf("unknown deletion policy %q", p) | ||
| } | ||
| } | ||
|
|
||
| // Preflight decides from the custom resource alone, without reading Temporal | ||
| // Cloud. It returns true when the returned Decision is final. When it returns | ||
| // false, the caller reads the resource from Temporal Cloud and decides with | ||
| // the resource's own rules. | ||
| func Preflight( | ||
| policy common.DeletionPolicy, | ||
| def common.DeletionPolicy, | ||
| rec Record, | ||
| ) (Decision, bool) { | ||
| resolved, err := Resolve(policy, def) | ||
| if err != nil { | ||
| return Decision{ | ||
| Action: ActionFail, | ||
| Reason: ReasonInvalidDeletionPolicy, | ||
| Message: fmt.Sprintf("%v; set spec.deletionPolicy to Retain or Delete", err), | ||
| }, true | ||
| } | ||
| if resolved == common.DeletionPolicyRetain { | ||
| return Decision{ | ||
| Action: ActionRelease, | ||
| Reason: ReasonRetained, | ||
| Message: "deletion policy is Retain; leaving the resource in Temporal Cloud", | ||
| }, true | ||
| } | ||
| if rec.ID == "" { | ||
| return Decision{ | ||
| Action: ActionRelease, | ||
| Reason: ReasonNeverCreated, | ||
| Message: "the operator never created this resource in Temporal Cloud; nothing to delete", | ||
| }, true | ||
| } | ||
| return Decision{}, false | ||
| } | ||
|
|
||
| // SameSecond reports whether recorded and observed are the same time to the | ||
| // second. Recorded times are stored as metav1.Time, which keeps whole seconds | ||
| // only. A nil time never matches. | ||
| func SameSecond(recorded, observed *time.Time) bool { | ||
| if recorded == nil || observed == nil { | ||
| return false | ||
| } | ||
| return recorded.Truncate(time.Second).Equal(observed.Truncate(time.Second)) | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deletion policies for Kubernetes resources are canonically handled as Annotations. See ACK's setup or nine.ch setup or kflare.
I recommend making these annotations
cloud.temporal.io/deletion-policy: deleteandcloud.temporal.io/deletion-policy: retain. Define the annotation keys and values as constants in the existing fileapi/cloud/common/const.goThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another advantage of using these annotations is now you can use the common
kube-temporal/pkg/kube/annotationpackage and thetypes.Resource.Annotations()andtypes.Resource.ReplaceAnnotations()methods.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice call out!