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

feat: volume snapshot class #183

Merged
merged 5 commits into from May 11, 2022

Conversation

sp98
Copy link
Contributor

@sp98 sp98 commented May 4, 2022

lvmo controller should create volume snapshot class for each deviceclass

Testing:

  • Creation of volumesnapshot class by the lvmo controller:
oc get volumesnapshotclass
NAME          DRIVER               DELETIONPOLICY   AGE
odf-lvm-vg1   topolvm.cybozu.com   Delete           5s
  • Volume snapshot class:
oc get volumesnapshotclass odf-lvm-vg1 -oyaml
apiVersion: snapshot.storage.k8s.io/v1
deletionPolicy: Delete
driver: topolvm.cybozu.com
kind: VolumeSnapshotClass
metadata:
  creationTimestamp: "2022-05-05T05:01:47Z"
  generation: 1
  name: odf-lvm-vg1
  resourceVersion: "19572561"
  uid: 26c2437c-26ec-4727-8a77-954cd1874c3a

  • Deleting the lvm cluster cleans up the volumesnapshot class.
    Signed-off-by: Santosh Pillai sapillai@redhat.com

@sp98 sp98 changed the title feat: volume snapshot class [WIP] feat: volume snapshot class May 4, 2022
@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 May 4, 2022
@sp98 sp98 force-pushed the volume-snapshot-class branch 4 times, most recently from 1673051 to 91d1d49 Compare May 5, 2022 05:17
@sp98 sp98 changed the title [WIP] feat: volume snapshot class feat: volume snapshot class May 5, 2022
@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 May 5, 2022
@sp98 sp98 requested review from nbalacha and leelavg May 5, 2022 05:20
Copy link
Contributor

@nbalacha nbalacha left a comment

Choose a reason for hiding this comment

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

  1. We should not be installing the VolumeSnapshotClass CRD. That will be part of the k8s cluster.
  2. Please add the rbacs to https://github.com/red-hat-storage/lvm-operator/blob/e611a42df60f183701fe8540b965da7d53dbe93a/controllers/lvmcluster_controller.go#L80

@sp98
Copy link
Contributor Author

sp98 commented May 5, 2022

  1. We should not be installing the VolumeSnapshotClass CRD. That will be part of the k8s cluster.
    The suite_test would need this CRD to work. Else the tests will faile.
1.6517312682804594e+09	INFO	lvmcluster-controller	topolvm storage class	{"Request.Name": "test-lvmcluster", "Request.Namespace": "openshift-storage", "operation": "created", "name": "odf-lvm-test"}
1.6517312683031378e+09	ERROR	lvmcluster-controller	topolvm volume snapshot class reconcile failure	{"Request.Name": "test-lvmcluster", "Request.Namespace": "openshift-storage", "name": "odf-lvm-test", "error": "no matches for kind \"VolumeSnapshotClass\" in version \"snapshot.storage.k8s.io/v1\""}

Any suggestions to fix this?

  1. Please add the rbacs to https://github.com/red-hat-storage/lvm-operator/blob/e611a42df60f183701fe8540b965da7d53dbe93a/controllers/lvmcluster_controller.go#L80

I've added the RBAC annotations above the ensureCreated method of topolvm_snapshotClass.go file. Why do we need to add this in lvmcluster_controller.go? (asking because its not very clear to me).

@sp98 sp98 requested a review from nbalacha May 5, 2022 06:19
@nbalacha
Copy link
Contributor

nbalacha commented May 5, 2022

  1. We should not be installing the VolumeSnapshotClass CRD. That will be part of the k8s cluster.
    The suite_test would need this CRD to work. Else the tests will faile.
1.6517312682804594e+09	INFO	lvmcluster-controller	topolvm storage class	{"Request.Name": "test-lvmcluster", "Request.Namespace": "openshift-storage", "operation": "created", "name": "odf-lvm-test"}
1.6517312683031378e+09	ERROR	lvmcluster-controller	topolvm volume snapshot class reconcile failure	{"Request.Name": "test-lvmcluster", "Request.Namespace": "openshift-storage", "name": "odf-lvm-test", "error": "no matches for kind \"VolumeSnapshotClass\" in version \"snapshot.storage.k8s.io/v1\""}

Any suggestions to fix this?

Let me think about this. Off the top of my head , set a variable to disable snapshots and set that to true in the make test.

  1. Please add the rbacs to https://github.com/red-hat-storage/lvm-operator/blob/e611a42df60f183701fe8540b965da7d53dbe93a/controllers/lvmcluster_controller.go#L80

I've added the RBAC annotations above the ensureCreated method of topolvm_snapshotClass.go file. Why do we need to add this in lvmcluster_controller.go? (asking because its not very clear to me).

Missed that. Having them in the topolvm_snapshotClass.go is better.

@@ -0,0 +1,133 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

is this required only for non-ocp deployments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to look at how to avoid creating this CRD in lvmo. TestAPI test fails if without the CRD currently. Need to work around that.
"topolvm volume snapshot class reconcile failure","Request.Name":"lvmcluster-sample","Request.Namespace":"lvm-operator-system","name":"odf-lvm-vg1","error":"volumesnapshotclasses.snapshot.storage.k8s.io is forbidden: User \"system:serviceaccount:lvm-operator-system:controller-manager\" cannot create resource \"volumesnapshotclasses\" in API group \"snapshot.storage.k8s.io\" at the cluster scope","stacktrace":"github.com/red-hat-storage/lvm-operator/controllers.(*LVMClusterReconciler).reconcile\n\t/workspace/controllers/lvmcluster_controller.go:180\ngithub.com/red-hat-storage/lvm-operator/controllers.(*LVMClusterReconciler).Reconcile\n\t/workspace/controllers

One suggestion from Nithya was to have a volumesnapshotEnabled flag which is turned off in case of running the tests.

controllers/topolvm_snapshotClass.go Outdated Show resolved Hide resolved
controllers/topolvm_snapshotClass.go Outdated Show resolved Hide resolved
@sp98 sp98 changed the title feat: volume snapshot class [WIP]feat: volume snapshot class May 6, 2022
@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 May 6, 2022
@sp98 sp98 force-pushed the volume-snapshot-class branch 3 times, most recently from e4fbb1e to 04ea0c1 Compare May 10, 2022 05:40
@sp98 sp98 changed the title [WIP]feat: volume snapshot class feat: volume snapshot class May 10, 2022
@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 May 10, 2022
@sp98 sp98 requested a review from leelavg May 10, 2022 06:20
sp98 added 5 commits May 10, 2022 11:52
- creates volume snapshot class when lvm cluster is created
- deletes volume snaphot class when lvm cluster is deleted
- adds a new CRD for volume snapshot class
- Updates tests to add volume snapshot class to scheme

Signed-off-by: Santosh Pillai <sapillai@redhat.com>
Signed-off-by: Santosh Pillai <sapillai@redhat.com>
Signed-off-by: Santosh Pillai <sapillai@redhat.com>
Signed-off-by: Santosh Pillai <sapillai@redhat.com>
Signed-off-by: Santosh Pillai <sapillai@redhat.com>
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 10, 2022

@sp98: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/lvm-operator-bundle-e2e-aws b15a466 link false /test lvm-operator-bundle-e2e-aws

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leelavg, nbalacha, sp98

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 /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 May 11, 2022
@openshift-merge-robot openshift-merge-robot merged commit e97bdeb into openshift:main May 11, 2022
@nbalacha
Copy link
Contributor

/cherry-pick release-4.11

@openshift-cherrypick-robot

@nbalacha: #183 failed to apply on top of branch "release-4.11":

Applying: feat: add volumesnapshotClass resource unit
Applying: feat: add rbac rules for volumesnapshotClass
Applying: feat: go mod changes for volumesnapshotclass
Applying: feat: run make bundle
Applying: test: update e2e to validate snapshot class
Using index info to reconstruct a base tree...
M	e2e/validation.go
Falling back to patching base and 3-way merge...
Auto-merging e2e/validation.go
CONFLICT (content): Merge conflict in e2e/validation.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 test: update e2e to validate snapshot class
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.11

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants