update conditions on error for delete operation #175
Conversation
templates/pkg/resource/sdk.go.tpl
Outdated
| @@ -212,7 +212,7 @@ func (rm *resourceManager) updateConditions ( | |||
| } | |||
| } | |||
|
|
|||
| if rm.terminalAWSError(err) || err == ackerr.SecretTypeNotSupported || err == ackerr.SecretNotFound { | |||
| if !r.IsBeingDeleted() && (rm.terminalAWSError(err) || err == ackerr.SecretTypeNotSupported || err == ackerr.SecretNotFound) { | |||
There was a problem hiding this comment.
I suppose this is fine. But, would it be safer to just let the Terminal Condition be set on the resource even if it's being deleted?
There was a problem hiding this comment.
I don't think it's safe to set Terminal Condition here because if deletion encounters an error, then wouldn't the controller stop reconciling without deleting the finalizer?
There was a problem hiding this comment.
left this as-is for now, until we have another strategy. If a customer hits this(should be a corner case), the worse case is controller retires with back off
There was a problem hiding this comment.
I don't think it's safe to set Terminal Condition here because if deletion encounters an error, then wouldn't the controller stop reconciling without deleting the finalizer?
Yes, and that is what the controller should do I think. If the delete operation failed on the backend AWS service API call, then the resource will not be deleted from the AWS service, so we should keep the resource around on the k8s side, but in show a terminal condition.
There was a problem hiding this comment.
Yes, and that is what the controller should do I think. If the delete operation failed on the backend AWS service API call, then the resource will not be deleted from the AWS service, so we should keep the resource around on the k8s side, but in show a terminal condition.
Does this mean the only way to delete that resource (after resolving any errors) is for users to manually remove the finalizer via kubectl edit ? That seems weird cx or am I missing some details
There was a problem hiding this comment.
Ok, I tried the following,
- simulated terminal condition true on delete after which the controller stopped reconciling
- made a change in spec, got a warning
Warning: Detected changes to resource xgboost-model-7fmglbwd7t189edvny which is currently being deleted.
model.sagemaker.services.k8s.aws/xgboost-model-7fmglbwd7t189edvny configured
but the controller reconciled and deleted the resource from k8s and aws successfully
Since terminal codes should only be related to those which require change in spec. I think its fine to remove this.
@jaypipes I have updated the PR, please take a look
There was a problem hiding this comment.
Yes, and that is what the controller should do I think. If the delete operation failed on the backend AWS service API call, then the resource will not be deleted from the AWS service, so we should keep the resource around on the k8s side, but in show a terminal condition.
Does this mean the only way to delete that resource (after resolving any errors) is for users to manually remove the finalizer via
kubectl edit? That seems weird cx or am I missing some details
The correct user experience for manually removing a resource that is in a stuck state: using kubectl delete --force
surajkota
force-pushed
the
delete_conditions
branch
from
9a9617d to
cffb2fa
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, jaypipes, surajkota 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 |
Description of changes: - bring in changes from aws-controllers-k8s/code-generator#175, aws-controllers-k8s/code-generator#176 - corresponding unit and integ test changes - fix some erroneous unit tests like model package group and endpoint config - fix invalid name in unit test - fix assertion in notebook instance test since PR build was failing and add it to missing canary cleanup Testing: unit test tested manually, rely on PR build, later we can add more unit tests for delete failed scenario By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Description of changes:
Testing:
Manually introduced an error from sdkDelete and verified it gets updated in conditions
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.