Install CSI driver #3

Madhu-1 · 2023-01-09T09:08:15Z

Install CSI Driver based on the kubernetes version where the ocs-client operator is running.

Signed-off-by: Madhu Rajanna [email protected]

Madhu-1 · 2023-01-09T09:08:33Z

Madhu-1 · 2023-01-09T09:22:43Z

Install ceph-CSI driver based on the kubernetes version where the ocs-client operator is running. Signed-off-by: Madhu Rajanna <[email protected]>

oritwas · 2023-01-10T11:50:25Z

@Madhu-1 1,287 files changed is that correct?

Madhu-1 · 2023-01-10T12:02:35Z

@oritwas yes that is correct, most of the changes in that are auto-generated files and imported vendor directory change

oritwas · 2023-01-11T10:35:04Z

csi/csi.go

+limitations under the License.
+*/
+
+package csi


csi is a bit too generic, how about csi utils or csi manager?

This will also change the folder name to csi-manager? if it's fine I can change it.

As a file that contains code to create and maintain the resources required for a functional CSI driver, it might be appropriate to rename it to csidriver.

Also, shouldn't things like this go under something like pkg/csi/? Or is that out of fashion these days?

oritwas · 2023-01-11T10:38:03Z

csi/encryptionconfigmap.go

+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+)
+
+func createEncryptionConfigMap(ctx context.Context, c client.Client, ownerDep *appsv1.Deployment, log klog.Logger) error {


Why is it a separate file? let's move it to csi.go
Same for the monconfigmap and rbac, I don't see the advantage of having them separate files

i kept different logical entities in a new file for maintenance purposes as might add more encryption-related functions and also keep the number of lines in a single file to a lesser number. please let me know if you still want me to move it.

oritwas · 2023-01-11T10:42:34Z

csi/csidriver.go

+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func createOrUpdateCSIDriver(ctx context.Context, client client.Client, log klog.Logger, csiDriver *v1k8scsi.CSIDriver) error {


Why is it a separate file? let's move it to csi.go

same here as well. i kept different logical entities in a new file for maintenance purposes as it might add more functions and also keep the number of lines in a single file to a lesser number. please let me know if you still want me to move it.

jarrpa

I just realized midway through reviewing this: is this largely the same code as was committed in my earlier working branch, https://github.com/jarrpa/ocs-client-operator/commits/jarrpa-dev-main ? If so that would explain why there's so much core controller functionality missing. I believe I already reviewed everything that was committed there, so we shouldn't commit this PR at all.

What I remember since August is that my first two PRs didn't get a review for a few weeks, but I see you submitted one on Sep 21st... which would have been around the time I was needing to work on setting up my medical leave. If the majority of this work was already done, I believe I just need to go back and address your latest review on #1, and we can go from there. Is that fair?

jarrpa · 2023-01-20T15:14:26Z

.github/workflows/conf/commitlintrc.json

@@ -70,6 +70,7 @@
                "action",
                "api",
                "bundle",
+		"csi",


Fix indentation

jarrpa · 2023-01-20T15:18:06Z

Makefile

@@ -64,6 +64,7 @@ e2e-test: ginkgo ## TODO: Run end to end functional tests.
 ##@ Build

 build: container-build ## Build manager binary
+	go build -o ./bin/manager main.go


Why are you doing a local build after the container build? Best I remember, the use of a local go build should not be part of the default build procedure, we should always be using the container build unless the developer has a specific reason they need to do the local build during development.

Beyond this, there's already a go-build make target below that uses ./hack/go.build.sh to run the actual go build command.

jarrpa · 2023-01-20T15:20:58Z

api/v1alpha1/groupversion_info.go

-// Package v1alpha1 contains API Schema definitions for the odf v1alpha1 API group
-//+kubebuilder:object:generate=true
-//+groupName=odf.openshift.io
+// Package v1alpha1 contains API Schema definitions for the ocs v1alpha1 API group


Any specific reason for changing the group name? This is technically part of ODF.

jarrpa · 2023-01-20T15:21:43Z

controllers/sliceutils.go

@@ -27,6 +27,7 @@ func contains(slice []string, s string) bool {
 }

 // Removes a given string from a slice and returns the new slice
+//


Remove empty comment line

jarrpa · 2023-01-20T15:23:20Z

controllers/suite_test.go

@@ -29,7 +29,7 @@ import (
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"

-	odfv1alpha1 "github.com/red-hat-storage/ocs-client-operator/api/v1alpha1"
+	"github.com/red-hat-storage/ocs-client-operator/api/v1alpha1"


Move changes like this that just change import names to a separate commit, this will make it clearer which changes are actually relevant to functional changes.

jarrpa · 2023-01-20T15:29:56Z

main.go

+		os.Exit(1)
+	}
+
+	err = csi.CreateOrUpdateCSI(ctx, client, setupLog)


Why are you doing this in main and not in its own controller? Shouldn't we have a controller that actually watches and reconciles the CSI sidecars rather than having to restart the operator to reconcile them?

jarrpa · 2023-01-20T15:30:49Z

csi/csi.go

+limitations under the License.
+*/
+
+package csi


As a file that contains code to create and maintain the resources required for a functional CSI driver, it might be appropriate to rename it to csidriver.

Also, shouldn't things like this go under something like pkg/csi/? Or is that out of fashion these days?

jarrpa · 2023-01-20T15:32:32Z

csi/csi.go

+// +kubebuilder:rbac:groups="apps",resources=deployments,verbs=get;create;update;delete
+// +kubebuilder:rbac:groups="apps",resources=deployments/finalizers,verbs=update
+// +kubebuilder:rbac:groups="apps",resources=daemonsets,verbs=get;create;update;delete
+// +kubebuilder:rbac:groups="apps",resources=daemonsets/finalizers,verbs=update
+// +kubebuilder:rbac:groups="storage.k8s.io",resources=csidrivers,verbs=get;create;update;delete
+// +kubebuilder:rbac:groups="",resources=configmaps,verbs=create;update;delete
+// +kubebuilder:rbac:groups=security.openshift.io,resources=securitycontextconstraints,verbs=get;watch;create;patch;update
+// +kubebuilder:rbac:groups="",resources=serviceaccounts,verbs=get;create;update;delete
+// +kubebuilder:rbac:groups="",resources=serviceaccounts/finalizers,verbs=update
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles,verbs=bind;escalate;get;create;update;delete
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles/finalizers,verbs=update
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterrolebindings,verbs=bind;escalate;get;create;update;delete
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterrolebindings/finalizers,verbs=update
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=bind;escalate;get;create;update;delete
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles/finalizers,verbs=update
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=bind;escalate;get;create;update;delete
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings/finalizers,verbs=update


This looks like a lot for an initial set of rbac, but that's just at a first glance. Have you tested that these all strictly required for the operation of this operator? Otherwise I can take a closer look at them in a later review pass.

jarrpa · 2023-01-20T15:45:35Z

csi/csi.go

+		return err
+	}
+
+	err = createOrUpdateRBAC(ctx, client, operatorDeployment, log)


For this and all the create* functions, would it make sense to rename them to ensure* since we should be ensuring they all exist and are up to date?

Madhu-1 · 2023-01-23T07:46:35Z

I just realized midway through reviewing this: is this largely the same code as was committed in my earlier working branch, https://github.com/jarrpa/ocs-client-operator/commits/jarrpa-dev-main ? If so that would explain why there's so much core controller functionality missing. I believe I already reviewed everything that was committed there, so we shouldn't commit this PR at all.

What I remember since August is that my first two PRs didn't get a review for a few weeks, but I see you submitted one on Sep 21st... which would have been around the time I was needing to work on setting up my medical leave. If the majority of this work was already done, I believe I just need to go back and address your latest review on #1, and we can go from there. Is that fair?

Before addressing the review comments on the PR lets get settled on this one.

Yes, @jarrpa you are correct as you know I took over this one after you went on leave.

For me, at least this PR is doing more than https://github.com/jarrpa/ocs-client-operator/commits/jarrpa-dev-main before

Have the configuration at build time for the sidecar images to install based on the OCP/Kubernetes version
Different SCC, ClusterRoles, ClusterRoleBinding, Role, RoleBinding, ServiceAccount for cephfs and rbd driver.
Keep the rbd and cephfs template objects in a separate file.
Added logic to construct the csi configmap.

I will let you and @nb-ohad decide on the review part and take it forward. I dont have any objection to taking your PR forward.

jarrpa · 2023-01-23T15:06:00Z

Before addressing the review comments on the PR lets get settled on this one.

Yes, @jarrpa you are correct as you know I took over this one after you went on leave.

For me, at least this PR is doing more than https://github.com/jarrpa/ocs-client-operator/commits/jarrpa-dev-main before
* Have the configuration at build time for the sidecar images to install based on the OCP/Kubernetes version

* Different SCC, ClusterRoles, ClusterRoleBinding, Role, RoleBinding, ServiceAccount for cephfs and rbd driver.

* Keep the rbd and cephfs template objects in a separate file.

* Added logic to construct the csi configmap.

Sure, but even so this is massively incomplete as far as an actual implementation of the service we want to create. If the goal is to just get the code committed somewhere and write the actual operator as a rebase on that, that seems somewhat backwards.

jarrpa · 2023-01-23T16:10:23Z

Sure, but even so this is massively incomplete as far as an actual implementation of the service we want to create. If the goal is to just get the code committed somewhere and write the actual operator as a rebase on that, that seems somewhat backwards.

...okay, so I only saw your subsequent PRs after dropping this last comment. That's still somewhat backwards, but at least explains why so much in missing. I don't think I can reasonably review this commit in isolation, and certainly can't properly test it myself, so I think we should focus on PR #6 and close the other ones.

Madhu-1 force-pushed the install-csi branch 5 times, most recently from 1353797 to e1a38d7 Compare January 9, 2023 09:18

Madhu-1 force-pushed the install-csi branch 2 times, most recently from e8aada6 to aba8969 Compare January 9, 2023 10:14

csi: install ceph-CSI driver

d153155

Install ceph-CSI driver based on the kubernetes version where the ocs-client operator is running. Signed-off-by: Madhu Rajanna <[email protected]>

Madhu-1 force-pushed the install-csi branch from aba8969 to d153155 Compare January 9, 2023 10:21

oritwas reviewed Jan 11, 2023

View reviewed changes

jarrpa requested changes Jan 20, 2023

View reviewed changes

Madhu-1 mentioned this pull request Jan 24, 2023

ocs-client-operator implementation #7

Merged

Madhu-1 closed this Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install CSI driver #3

Install CSI driver #3

Madhu-1 commented Jan 9, 2023

Madhu-1 commented Jan 9, 2023

Madhu-1 commented Jan 9, 2023

oritwas commented Jan 10, 2023

Madhu-1 commented Jan 10, 2023

oritwas Jan 11, 2023

Madhu-1 Jan 11, 2023

jarrpa Jan 20, 2023

oritwas Jan 11, 2023

Madhu-1 Jan 11, 2023

oritwas Jan 11, 2023

Madhu-1 Jan 11, 2023

jarrpa left a comment

jarrpa Jan 20, 2023

jarrpa Jan 20, 2023

jarrpa Jan 20, 2023

jarrpa Jan 20, 2023

jarrpa Jan 20, 2023

jarrpa Jan 20, 2023

jarrpa Jan 20, 2023

jarrpa Jan 20, 2023

jarrpa Jan 20, 2023

Madhu-1 commented Jan 23, 2023

jarrpa commented Jan 23, 2023

jarrpa commented Jan 23, 2023

Install CSI driver #3

Install CSI driver #3

Conversation

Madhu-1 commented Jan 9, 2023

Madhu-1 commented Jan 9, 2023

Madhu-1 commented Jan 9, 2023

oritwas commented Jan 10, 2023

Madhu-1 commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarrpa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Jan 23, 2023

jarrpa commented Jan 23, 2023

jarrpa commented Jan 23, 2023