Skip to content

STOR-1767: Delete resources if spec.ManagementState is equal to Removed for removable CSI drivers#233

Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift:masterfrom
mpatlasov:STOR-1767-disable-storage-on-vSphere
Jul 25, 2024
Merged

STOR-1767: Delete resources if spec.ManagementState is equal to Removed for removable CSI drivers#233
openshift-merge-bot[bot] merged 2 commits intoopenshift:masterfrom
mpatlasov:STOR-1767-disable-storage-on-vSphere

Conversation

@mpatlasov
Copy link
Contributor

@mpatlasov mpatlasov commented Jun 21, 2024

The first commit bumps library-go to a newer version with openshift/library-go#1741. The second commit updates operator driver_starter.go to delete static assets if ManagementState equal to Removed.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 21, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 21, 2024 •
edited by openshift-ci bot
Loading

@mpatlasov: This pull request references STOR-1767 which is a valid jira issue.

Details

In response to this:

The first commit bumps library-go to a newer version with openshift/library-go#1741. The second commit updates operator driver_starter.go to delete static assets if ManagementState equal to Removed.

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.

@openshift-ci openshift-ci bot requested review from RomanBednar and tsmetana June 21, 2024 01:41
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mpatlasov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2024
@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jun 21, 2024

/retest-required

@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jun 21, 2024 •
edited
Loading

We don't have go 1.22.* in CI yet :-(
https://issues.redhat.com/browse/ART-9927

@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jun 24, 2024

/retest-required

@mpatlasov mpatlasov mentioned this pull request Jun 24, 2024
Closed
@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jun 25, 2024

/retest-required

2 similar comments
@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jun 26, 2024

/retest-required

@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jun 27, 2024

/retest-required

@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 1, 2024

/hold

This PR will wait for a small change in CSO's csidriveroperators/vsphere/08_deployment.yaml asset:

+        - name: OPERATOR_NAME
+          value: vmware-vsphere-csi-driver-operator

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2024
RomanBednar
RomanBednar reviewed Jul 1, 2024
View reviewed changes
@mpatlasov mpatlasov force-pushed the STOR-1767-disable-storage-on-vSphere branch from 3b1f1f8 to daa0265 Compare July 1, 2024 20:01
mpatlasov added a commit to mpatlasov/cluster-storage-operator that referenced this pull request Jul 1, 2024
@mpatlasov
STOR-1767: add env var OPERATOR_NAME to vsphere csi operator deployment
c529a03 
openshift/vmware-vsphere-csi-driver-operator#233 will do `allowOperatorRemovedState=True` for vsphere csi operator. This will trigger finalazer check in library-go:

```
	if management.IsOperatorRemovable() {
		if err := v1helpers.EnsureFinalizer(ctx, c.operatorClient, c.name); err != nil {
```

`EnsureFinalizer()`, in turn, calculates finalizer name either as `os.Getenv("OPERATOR_NAME")` or `os.Args[0]`. The latter doesn't work for vsphere because leading slash in `/usr/bin/vmware-vsphere-csi-driver-operator` is not allowed:

```
E0701 19:43:32.342151       1 base_controller.go:268] VMwareVSphereDriverControllerServiceController reconciliation failed: ClusterCSIDriver.operator.openshift.io "csi.vsphere.vmware.com" is invalid: metadata.finalizers: Invalid value: "/usr/bin/vmware-vsphere-csi-driver-operator.operator.openshift.io/VMwareVSphereDriverControllerServiceController": a qualified name must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')
```
@mpatlasov mpatlasov mentioned this pull request Jul 1, 2024
Merged
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2024
@mpatlasov mpatlasov force-pushed the STOR-1767-disable-storage-on-vSphere branch from daa0265 to 814dcbb Compare July 3, 2024 20:00
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2024
@mpatlasov mpatlasov force-pushed the STOR-1767-disable-storage-on-vSphere branch from 814dcbb to 7753336 Compare July 3, 2024 20:05
@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 3, 2024

/test e2e-vsphere-csi

@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 8, 2024

/test e2e-vsphere

@RomanBednar
Copy link
Contributor

RomanBednar commented Jul 8, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2024
@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 8, 2024

/unhold
/label docs-approved
/label px-approved

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2024
@mpatlasov mpatlasov mentioned this pull request Jul 9, 2024
Merged
@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 9, 2024

/hold

Waiting for openshift/library-go#1760 to merge. This PR will need update (bumping library-go) after it merges.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2024
@mpatlasov mpatlasov force-pushed the STOR-1767-disable-storage-on-vSphere branch from 05af972 to 7d218c8 Compare July 15, 2024 20:19
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2024
mpatlasov added 2 commits July 15, 2024 14:26
@mpatlasov
Bump library-go to delete resources if spec.ManagementState is equal …
96bf2f6 
…to Removed

```
go get github.com/openshift/library-go
go mod tidy && go mod vendor
```
@mpatlasov
STOR-1767: Use ConditionalStaticResourcesController for static assets
4706bbb 
This let to specify Create/Delete functions based on `ManagementState` value. So, if `ManagementState` goes to "Removed", static resources are deleted.
@mpatlasov mpatlasov force-pushed the STOR-1767-disable-storage-on-vSphere branch from 7d218c8 to 4706bbb Compare July 15, 2024 21:29
@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 15, 2024

New changes are detected. LGTM label has been removed.

Two updates from the latest review:

@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 16, 2024

/retest-required

@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 16, 2024

/unhold

(openshift/library-go#1760 merged)

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2024
@RomanBednar
Copy link
Contributor

RomanBednar commented Jul 17, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2024
@duanwei33
Copy link
Contributor

duanwei33 commented Jul 17, 2024

pre-merge verified with open PR:

  1. Cluster admin sets vSphere’s ClusterCSIDriver spec.managementState: Removed
  • vSphere CSI driver operator un-installs the CSI driver and deletes the related resources
  • Storage CO should be healthy
  • All existing PVs, PVCs, StorageClasses and vSphere StoragePolicies stay as they are
    • vSphere PVs cannot be used in new pods
    • vSphere PVs stay mounted + attached forever to existing nodes for existing Pods.
    • These pods stay Terminating forever after deletion.
    • StorageClasses could be removed
  1. Cluster admin sets vSphere’s ClusterCSIDriver spec.managementState: from Removed back to Managed
  • vSphere CSI driver operator installs the CSI driver back.
  • The CSI driver works well
  1. Performed one upgrade from one cluster with managementState:Removed to another version with the PR, the managementState stays Removed and no CSI Driver is installed

@duanwei33
Copy link
Contributor

duanwei33 commented Jul 17, 2024

/test e2e-vsphere-zones

@duanwei33
Copy link
Contributor

duanwei33 commented Jul 19, 2024

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jul 19, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 19, 2024 •
edited by openshift-ci bot
Loading

@mpatlasov: This pull request references STOR-1767 which is a valid jira issue.

Details

In response to this:

The first commit bumps library-go to a newer version with openshift/library-go#1741. The second commit updates operator driver_starter.go to delete static assets if ManagementState equal to Removed.

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.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 22, 2024

/retest-required

Remaining retests: 0 against base HEAD 05fdfea and 2 for PR HEAD 4706bbb in total

@duanwei33
Copy link
Contributor

duanwei33 commented Jul 23, 2024

/retest-required

3 similar comments
@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 24, 2024

/retest-required

@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 24, 2024

/retest-required

@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 24, 2024

/retest-required

@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 24, 2024

OK, now it's clear why e2e-vsphere and e2e-vsphere-csi failed:

E0724 19:50:08.797495       1 controller.go:974] error syncing claim "4719b799-f7cb-4a72-bc85-c56f252adf28": failed to provision volume with StorageClass "thin-csi": error generating accessibility requirements: no topology key found on CSINode ci-op-ztk028vs-5748b-pvbrp-worker-0-9c8h5

It's useless to /retest-required until that revert merged

@gnufied
Copy link
Member

gnufied commented Jul 25, 2024

/retest

1 similar comment
@mpatlasov
Copy link
Contributor Author

mpatlasov commented Jul 25, 2024

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 25, 2024

@mpatlasov: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

@openshift-merge-bot openshift-merge-bot bot merged commit 1ca230d into openshift:master Jul 25, 2024
@openshift-bot
Copy link

openshift-bot commented Jul 25, 2024

[ART PR BUILD NOTIFIER]

Distgit: ose-vmware-vsphere-csi-driver-operator
This PR has been included in build ose-vmware-vsphere-csi-driver-operator-container-v4.18.0-202407251911.p0.g1ca230d.assembly.stream.el9.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tsmetana tsmetana Awaiting requested review from tsmetana

1 more reviewer

@RomanBednar RomanBednar RomanBednar left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@RomanBednar RomanBednar

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mpatlasov @openshift-ci-robot @RomanBednar @duanwei33 @gnufied @openshift-bot @openshift-merge-robot