Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CloudStorage: Only delete and remove finalizer if there exists an annotation. #574

Merged
merged 5 commits into from
Mar 9, 2022

Conversation

kaovilai
Copy link
Member

@kaovilai kaovilai commented Feb 17, 2022

New:

  • Annotation oadp.openshift.io/cloudstorage-delete: "true" is now required to trigger bucket deletion

Fix:

  • If bucket is deleted successfully at the cloud provider, a finalizer on the CR is removed so the CR goes deletion continues
  • Add checks such that finalizer is only added if deletionTimestamp is nil.

Example annotation

metadata:
  annotations:
    oadp.openshift.io/cloudstorage-delete: "true"

If user wants to remove the CloudStorage custom resource from the cluster without deleting bucket from cloud provider, they can do so by

  • Don't add oadp.openshift.io/cloudstorage-delete: "true" annotation
  • oc delete cloudstorage/<name>
  • remove finalizer
kubectl patch -n openshift-adp cloudstorage/<name> \
    --type json \
    --patch='[ { "op": "remove", "path": "/metadata/finalizers" } ]'

@openshift-ci openshift-ci bot requested review from dymurray and rayfordj February 17, 2022 21:24
@kaovilai kaovilai force-pushed the delete_remove_finalizer branch from ea16689 to 84c6b92 Compare February 17, 2022 21:34
@kaovilai kaovilai force-pushed the delete_remove_finalizer branch from 84c6b92 to d6959bf Compare February 17, 2022 21:36
@kaovilai kaovilai marked this pull request as draft February 21, 2022 18:33
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 21, 2022
@kaovilai
Copy link
Member Author

Annotation ("oadp.openshift.io/cloudstorage-delete": "true") to activate bucket deletion in cloud providers, finalizer will be removed for you.

To remove CR without deleting bucket in cloud storage, user just have to remove finalizer.

@kaovilai kaovilai marked this pull request as ready for review February 21, 2022 20:42
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 21, 2022
@openshift-ci openshift-ci bot requested a review from sseago February 21, 2022 20:43
@kaovilai kaovilai force-pushed the delete_remove_finalizer branch from 2f6432c to 01335c7 Compare February 21, 2022 20:44
@kaovilai kaovilai changed the title CloudStorage: remove finalizer when delete is successful CloudStorage: Only delete and remove finalizer if there exists an annotation. Feb 21, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2022

Codecov Report

Merging #574 (b402ffa) into master (5a5cf32) will decrease coverage by 0.30%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #574      +/-   ##
==========================================
- Coverage   37.60%   37.30%   -0.31%     
==========================================
  Files          14       14              
  Lines        2917     2941      +24     
==========================================
  Hits         1097     1097              
- Misses       1737     1761      +24     
  Partials       83       83              
Impacted Files Coverage Δ
controllers/bucket_controller.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a5cf32...b402ffa. Read the comment docs.

@kaovilai
Copy link
Member Author

kaovilai commented Mar 4, 2022

/test 4.7-operator-unit-test

@kaovilai kaovilai requested a review from shawn-hurley March 7, 2022 17:45
deleted, err := clnt.Delete()
if err != nil {
log.Error(err, "unable to delete bucket")
b.EventRecorder.Event(&bucket, corev1.EventTypeWarning, "unable to delete bucket", fmt.Sprintf("unable to delete bucket: %v", bucket.Spec.Name))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the event needs to look different

Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after updating the event names

kaovilai added a commit to kaovilai/oadp-operator that referenced this pull request Mar 8, 2022
@kaovilai kaovilai force-pushed the delete_remove_finalizer branch from 58c3844 to b402ffa Compare March 8, 2022 19:43
@kaovilai
Copy link
Member Author

kaovilai commented Mar 8, 2022

/retest

@openshift-ci
Copy link

openshift-ci bot commented Mar 9, 2022

@kaovilai: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dymurray dymurray merged commit fe23cb9 into openshift:master Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants