-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: volume snapshot class #183
Conversation
1673051
to
91d1d49
Compare
bundle/manifests/controller-manager-metrics-service_v1_service.yaml
Outdated
Show resolved
Hide resolved
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.
- We should not be installing the VolumeSnapshotClass CRD. That will be part of the k8s cluster.
- Please add the rbacs to https://github.com/red-hat-storage/lvm-operator/blob/e611a42df60f183701fe8540b965da7d53dbe93a/controllers/lvmcluster_controller.go#L80
Any suggestions to fix this? I've added the RBAC annotations above the |
e0ebda0
to
783aa9f
Compare
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.
Missed that. Having them in the |
@@ -0,0 +1,133 @@ | |||
--- |
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.
is this required only for non-ocp deployments?
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.
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.
e4fbb1e
to
04ea0c1
Compare
04ea0c1
to
8dceee7
Compare
- 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 <[email protected]>
Signed-off-by: Santosh Pillai <[email protected]>
Signed-off-by: Santosh Pillai <[email protected]>
Signed-off-by: Santosh Pillai <[email protected]>
Signed-off-by: Santosh Pillai <[email protected]>
8dceee7
to
b15a466
Compare
@sp98: 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. |
[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 |
/cherry-pick release-4.11 |
@nbalacha: #183 failed to apply on top of branch "release-4.11":
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. |
lvmo controller should create volume snapshot class for each deviceclass
Testing:
Signed-off-by: Santosh Pillai [email protected]