STOR-1353: Cleanup GCP Filestore instances on destroy #7251
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tsmetana: This pull request references STOR-1353 which is a valid jira issue. In response to this:
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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
tsmetana
force-pushed
the
wip-filestore-destroy-rebased
branch
from
0279f6e
to
2291fe1
Compare
|
/retest |
openshift-merge-robot
added
the
needs-rebase
|
@tsmetana: This pull request references STOR-1353 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. In response to this:
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. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
@tsmetana are you still interested in this PR? |
tsmetana
force-pushed
the
wip-filestore-destroy-rebased
branch
from
2291fe1
to
5019bcf
Compare
openshift-merge-robot
removed
the
needs-rebase
Sorry for the slow response: I failed to test the PR (I seem to be unable to install custom GCP cluster), but yes, we still would like to have this included. It's not super-urgent, but would be good to have to keep all the platforms behaving consistently. |
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
tsmetana
force-pushed
the
wip-filestore-destroy-rebased
branch
from
5019bcf
to
65fe111
Compare
|
/retest |
|
I'm not retesting the ci/prow/okd-e2e-aws-ovn-upgrade job: looks like this has never passed. |
tsmetana
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
I seem to be unable to install GCP with my patched installer. However I think the patch should be safe enough not to break things. |
tsmetana
force-pushed
the
wip-filestore-destroy-rebased
branch
2 times, most recently
from
2539c86
to
2fbc97f
Compare
pkg/destroy/gcp/filestore.go
Outdated
| _, err := instSvc.Delete(item.name).Context(ctx).Do() | ||
| if err != nil && isForbidden(err) { | ||
| o.deletePendingItems(item.typeName, []cloudResource{item}) | ||
| return fmt.Errorf("Insufficient permissions to delete filestore %s: giving up", item.name) |
There was a problem hiding this comment.
the linter is complaining about the caps. If we're not failing on missing perms, should this be an error, or should this be a logrus Warning? (in which case, with logrus warning it should stay capitalized).
There was a problem hiding this comment.
I'm unsure myself about this part. The uninstallation can't fail as such, so we only need some way to inform the user the resource deletion failed. Perhaps logging a warning and returning an error would work.
There was a problem hiding this comment.
as the error doesn't stop the destroyer, this is fine. just drop the caps to appease the linter
| return fmt.Errorf("Insufficient permissions to delete filestore %s: giving up", item.name) | |
| return fmt.Errorf("insufficient permissions to delete filestore %s: giving up", item.name) |
There was one other failure in go-lint that needed to be resolved
There was a problem hiding this comment.
I have added a log message (so the user is warned when the deletion fails because of missing permissions) and changed the character case for the error.
The other linter failure was caused by the new isForbidden function that now takes a different error type so copying the isNoOp was apparently incorrect.
There was a problem hiding this comment.
Just to explain the lint error: at some point we changed the linter we use but we limited the reporting to a certain commit and all following. That's why when you used existing code as a basis, the linter complained.
tsmetana
force-pushed
the
wip-filestore-destroy-rebased
branch
from
47c36f6
to
8eac404
Compare
|
/retest |
|
/lgtm |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/override ci/prow/okd-images |
|
@r4f4: Overrode contexts on behalf of r4f4: ci/prow/okd-images In response to this:
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-sigs/prow repository. |
|
@tsmetana: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
|
/hold Revision 8eac404 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
Thanks for the reviews and the help with this PR. After some discussion with the Storage team I need to keep this on hold: Filestore would behave differently from everything else, which is rather suboptimal, and we'd like to have it also properly documented and tested but there's no time for it in the upcoming release. :( |
|
/hold cancel Let's try again in the next relase. |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-merge-bot
bot
merged commit 3a7f60a
into
openshift:master
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-installer-altinfra-container-v4.17.0-202406031613.p0.g3a7f60a.assembly.stream.el9 for distgit ose-installer-altinfra. |
|
FYI: It looks like this PR broke a few CI jobs. These jobs are currently failing on Are we okay with reverting this PR to unblock failing CI jobs? |
Yeah. I think it's the best solution. I will take a look a the failed jobs and see if I can fix the PR to make them happy. |
@tsmetana thanks, are you also going to prepare a revert PR? |
Make use of openshift/gcp-filestore-csi-driver#17 and openshift/gcp-filestore-csi-driver-operator#38 to cleanup also GCP Filestore instances created dynamically by the CSI Filestore driver.