fix #648: delete the statefulset with precondition set to the correct… #651
Conversation
This file contains 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
ChunyiLyu
reviewed
Apr 7, 2021
| @@ -54,7 +58,7 @@ func (r *RabbitmqClusterReconciler) prepareForDeletion(ctx context.Context, rabb | |||
| // Delete StatefulSet immediately after changing pod labels to minimize risk of them respawning. | |||
| // There is a window where the StatefulSet could respawn Pods without the deletion label in this order. | |||
| // But we can't delete it before because the DownwardAPI doesn't update once a Pod enters Terminating. | |||
| if err := r.Client.Delete(ctx, sts); client.IgnoreNotFound(err) != nil { | |||
| if err := r.Client.Delete(ctx, sts, &client.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &uid}}); client.IgnoreNotFound(err) != nil { | |||
There was a problem hiding this comment.
May I suggest modifying the comment section above to explain why setting Preconditions for deletion is necessary?
controllers/utils.go
Outdated
Comment on lines
70
to
81
| func (r *RabbitmqClusterReconciler) statefulSetUID(ctx context.Context, rmq *rabbitmqv1beta1.RabbitmqCluster) (types.UID, error) { | ||
| uid := types.UID("") | ||
| if sts, err := r.statefulSet(ctx, rmq); err == nil { | ||
| if ref := metav1.GetControllerOf(sts); ref != nil { | ||
| if string(rmq.GetUID()) == string(ref.UID) { | ||
| return sts.UID, nil | ||
| } | ||
| } | ||
| } | ||
| return uid, fmt.Errorf("failed to get the uid of the statefulset owned by the current rabbitmqCluster") | ||
| } | ||
|
|
There was a problem hiding this comment.
Two suggestions: 1) I would add a line of comment to explain that the statefulSet uid is returned successfully only when the controller reference uid matches the rabbitmqCluster uid. 2) I think errors of failing to get the statefulSet, failing to get controller reference, and reference uid does not match rmq uid are all different failures. The function only returns one type of error at the moment, and it's unclear which step has failed. It's more verbose but the error msg is clearer:
Suggested change
| func (r *RabbitmqClusterReconciler) statefulSetUID(ctx context.Context, rmq *rabbitmqv1beta1.RabbitmqCluster) (types.UID, error) { | |
| uid := types.UID("") | |
| if sts, err := r.statefulSet(ctx, rmq); err == nil { | |
| if ref := metav1.GetControllerOf(sts); ref != nil { | |
| if string(rmq.GetUID()) == string(ref.UID) { | |
| return sts.UID, nil | |
| } | |
| } | |
| } | |
| return uid, fmt.Errorf("failed to get the uid of the statefulset owned by the current rabbitmqCluster") | |
| } | |
| func (r *RabbitmqClusterReconciler) statefulSetUID(ctx context.Context, rmq *rabbitmqv1beta1.RabbitmqCluster) (types.UID, error) { | |
| var err error | |
| var sts *appsv1.StatefulSet | |
| var ref *metav1.OwnerReference | |
| if sts, err = r.statefulSet(ctx, rmq); err != nil { | |
| return "", fmt.Errorf("failed to get statefulSet: %s", err.Error()) | |
| } | |
| if ref = metav1.GetControllerOf(sts); ref == nil { | |
| return "", errors.New("failed to get controller reference for statefulSet") | |
| } | |
| if string(rmq.GetUID()) != string(ref.UID) { | |
| return "", errors.New("statefulSet not owner by current RabbitmqCluster") | |
| } | |
| return sts.UID, nil | |
| } |
|
@ChunyiLyu Thank you for the comments! I will try to address them today or tmr. |
…error msg in statefulSetUID
ChunyiLyu
approved these changes
Apr 8, 2021
Zerpet
approved these changes
Apr 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
… UID
This closes #648
Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Summary Of Changes
Ensure that the sts to be deleted has the correct uid (besides the name and namespace). A sts uid is a correct one if the sts's controller ref matches the current rabbitmqCluster's uid.