-
Notifications
You must be signed in to change notification settings - Fork 23
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
STOR-1726: Delete static resources when ClusterCSIDriver is removed #215
STOR-1726: Delete static resources when ClusterCSIDriver is removed #215
Conversation
@dobsonj: This pull request references STOR-1726 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.16.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 openshift-eng/jira-lifecycle-plugin repository. |
@dobsonj: GitHub didn't allow me to request PR reviews from the following users: openshift/storage. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dobsonj 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 |
@dobsonj: This pull request references STOR-1726 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.16.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 openshift-eng/jira-lifecycle-plugin repository. |
5eef90f
to
7e24ea5
Compare
/test smb-operator-e2e |
pkg/operator/starter.go
Outdated
} | ||
// return the state from the operator if it's not managed | ||
if opSpec.ManagementState != opv1.Managed { | ||
return opSpec.ManagementState |
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.
This will allow users to set managementState: Removed
for CSI drivers that are not removable and the operator will remove.
That's an interesting feature that would allow users to disable e.g. vSphere CSI driver on clusters that don't want it installed. But I think it should be discussed first.
For now and for non-removable CSI drivers, Removed
should be handled as Managed
(?) and should throw some sane error. Returning err
would degrade the cluster. It may be too strong, but it's the easiest thing to do.
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.
Ack, I reworked this in a second commit to maintain the existing behavior.
NewStaticResourcesController sets the conditional resource functions to nil
by default, which is interpreted by WithConditionalResources as "always create, never delete". So it's perfectly valid to pass nil
functions into WithConditionalStaticResourcesController and it will behave exactly as WithStaticResourcesController
did.
So now removable operators set the conditional functions, and non-removable operators leave them nil
for the simple static resources controller they always used before.
For removable operators, I changed getOperatorSyncState
to be consistent with how we manage static resources in gcp-filestore. It will not sync the resources if there is an error or if the state is not Managed
, and it removes them only if DeletionTimestamp
is set.
@dobsonj: This pull request references STOR-1726 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.16.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 openshift-eng/jira-lifecycle-plugin repository. |
/retest |
/lgtm |
/label docs-approved |
@dobsonj: This pull request references STOR-1726 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.16.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 openshift-eng/jira-lifecycle-plugin repository. |
@dobsonj: The following test 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/test-infra repository. I understand the commands that are listed here. |
ef2c2ec
into
openshift:master
This PR allows the smb-csi-driver-operator to clean up its static resources when the ClusterCSIDriver is removed.
The approach is the same as openshift/secrets-store-csi-driver-operator#8 (see also: secrets-store enhancement). This should allow us to follow a standard approach when moving other OLM operators to csi-operator just by setting
Removable: true
in theOperatorConfig
.Operators which are not removable will not define conditional functions, and will behave the same as before with the static resources controller.
Simple manual test:
/cc @openshift/storage @jsafrane @mpatlasov