openebs / lvm-localpv

Dynamically provision Stateful Persistent Node-Local Volumes & Filesystems for Kubernetes that is integrated with a backend LVM2 data storage stack.
Apache License 2.0
235 stars 92 forks source link

snapshot-controller container in controller pod fails when CRDs are not installed #264

Closed emiran-orange closed 9 months ago

emiran-orange commented 9 months ago

What steps did you take and what happened: Install chart with crd.volumeSnapshot=False, wait for container snapshot-controller to crash with the following error:

E1012 16:05:19.347196       1 main.go:89] Failed to list v1 volumesnapshots with error=the server could not find the requested resource (get volumesnapshots.snapshot.storage.k8s.io)
E1012 16:05:19.347240       1 main.go:207] Exiting due to failure to ensure CRDs exist during startup: timed out waiting for the condition

What did you expect to happen: Ability not to add snapshot-controller to pod

The output of the following commands will help us better understand what's going on:

Anything else you would like to add: volumeSnapshot CRDs are only conditioned on crd.volumeSnapshot while there is alos crd.enabled which is not interpreted anywhere...

Environment:

emiran-orange commented 9 months ago

Would it make sense to add lvmController.snapshotController.enabled and lvmController.snapshotter.enabled to condition the addition both containers to controller pod ? If so, I can drop a PR about it

Abhinandan-Purkait commented 9 months ago

@emiran-orange Is this to disable the snapshot feature? I am not sure if we want that to be configurable. The crd.volumeSnapshot=False would be considered valid combination if the cluster already has the snapshot crds. It's not desirable to disable the crd installation if crds are not present in the cluster already.

Or are you saying even if the crds are present and we set crd.volumeSnapshot=False, the csi containers still fail?

emiran-orange commented 9 months ago

@Abhinandan-Purkait Thanks for the highlight.

Disabling the snapshot feature was what I had in mind but I understand your position. I'm working on a soft mutitenant cluster it seems that I can achieve the same by not defining any volumesnapshotclass

emiran-orange commented 9 months ago

@Abhinandan-Purkait I noticed that volumeSnapshot CRDs are only conditioned on crd.volumeSnapshot while there is also crd.enabled which is not interpreted anywhere...

Is it on purpose ?

Abhinandan-Purkait commented 9 months ago

Okay. crd.enabled is a helm macro, which helm understands, that's the reason you aren't seeing it in any template. That is there to disable installation of all crds. FYI we have some other crds that we use for LVM. Setting crd.enabled to false will not install any crds that are a part of this helm chart. Hope that answers your question.

emiran-orange commented 9 months ago

Thanks for your time and sorry for the disturbance of this (non)issue.