ocs-client-operator implementation

[Madhu-1]

This replaces #3, #4, #5, and #6, as this includes all the planned changes.

[jarrpa]

Overall this is a great PR, thank you for accommodating my commit requests. 🙏 Apart from the more specific review comments, I have a couple of broad questions:

Regarding the StorageClient CRD:

• I remember at some point having a conversation (months ago) about naming this StorageClient vs OcsClient. The reasoning for the latter probably make more sense when it was ocsclient.odf.openshift.io, but I can see storageclient.ocs.openshift.io also making sense these days. Is this related to the referenced (though unexplained) conversation about the API group name?
• Why are we limiting to one per namespace? I can imagine the argument being something about Secret/data isolation, but could I get a concrete example of where this would be an actual problem with the current design?

Regarding the StorageClassClaim implementation, this seems largely similar to the work I did in the previous PRs. If this is the case, could you add a Co-authored-by: [email protected] line to the relevant commit messages? If you feel any of the other commits are significantly based on that same prior work, please do add the line there as well.

Beyond that:

• Your commits are signed using your personal gmail account. I would think you should be using your @redhat.com account, was this intentional?
• I like the implementation of the status-report job, but boy does it look weird to have a main.go under pkg. Would it be possible to do as was done for the Provider and create a service/status-reporter directory for that job?

[Madhu-1]

Overall this is a great PR, thank you for accommodating my commit requests. pray Apart from the more specific review comments, I have a couple of broad questions:

Regarding the StorageClient CRD:

• I remember at some point having a conversation (months ago) about naming this StorageClient vs OcsClient. The reasoning for the latter probably make more sense when it was ocsclient.odf.openshift.io, but I can see storageclient.ocs.openshift.io also making sense these days. Is this related to the referenced (though unexplained) conversation about the API group name?

It was discussed in one of the meetings with @nb-ohad and @oritwas as it's more opt and as the name explains it's just a storageclient we decided on storage client name. it's not documented anywhere.

• Why are we limiting to one per namespace? I can imagine the argument being something about Secret/data isolation, but could I get a concrete example of where this would be an actual problem with the current design?

To keep it simple for now, we will go with one storageClient per namespace but in future/follow-up PR we will block creating multiple storageclient per provider and allow multiple storageclient connecting to different providers in the same namespace.

Regarding the StorageClassClaim implementation, this seems largely similar to the work I did in the previous PRs. If this is the case, could you add a Co-authored-by: [email protected] line to the relevant commit messages? If you feel any of the other commits are significantly based on that same prior work, please do add the line there as well.

Sorry missed adding it. Done 👍

Beyond that:

• Your commits are signed using your personal gmail account. I would think you should be using your @redhat.com account, was this intentional?

yes, it's intentional I use my Gmail id for all the projects I work with. Never used redhat.com till now.

• I like the implementation of the status-report job, but boy does it look weird to have a main.go under pkg. Would it be possible to do as was done for the Provider and create a service/status-reporter directory for that job?

Done 👍

[jarrpa]

• Your commits are signed using your personal gmail account. I would think you should be using your @redhat.com account, was this intentional?

yes, it's intentional I use my Gmail id for all the projects I work with. Never used redhat.com till now.

I haven't seen this before. Best I knew if you're doing work for redhat you should be signing it as such. But if that's what you've been doing and no one else has said anything about, I'm not gonna care. 😆

[jarrpa]

Excellent work! Just one last typo to fix, I don't entirely want to hold up the entire PR for this, but if not now then who knows when. 😆 If you or someone else has permissions to hit the big green merge button once you update the PR I hereby consent to you doing so ASAP.

[Madhu-1]

@jarrpa Please take a look at it; below are the major changes after discussing with @nb-ohad

• Remove the OCP check and consider the cluster is always OCP for now. I will open an issue to track the gap to run with standalone kubernetes
• Move complete CSI logic to ClusterVersion CR.
• Move CSI RBAC from code to yaml files which help in security audit

[Madhu-1]

Test results from OCP cluster

[🎩︎]mrajanna@fedora ocs-client-operator $]oc get po -nocs-client-ns
NAME                                                     READY   STATUS    RESTARTS   AGE
csi-addons-controller-manager-7fc65dff49-7mpxs           2/2     Running   0          49m
csi-cephfsplugin-6wtgv                                   2/2     Running   0          2m39s
csi-cephfsplugin-bwn6g                                   2/2     Running   0          2m52s
csi-cephfsplugin-p9f97                                   2/2     Running   0          2m49s
csi-cephfsplugin-provisioner-7bbcd8d9bf-gqgj9            5/5     Running   0          23m
csi-cephfsplugin-provisioner-7bbcd8d9bf-gqrxx            5/5     Running   0          23m
csi-rbdplugin-5k5cp                                      3/3     Running   0          25m
csi-rbdplugin-6mv8w                                      3/3     Running   0          25m
csi-rbdplugin-7bzzl                                      3/3     Running   0          25m
csi-rbdplugin-provisioner-5f8fd786d-65rx5                5/5     Running   0          3m12s
csi-rbdplugin-provisioner-5f8fd786d-hppg9                5/5     Running   0          3m18s
ocs-client-operator-controller-manager-d8df78b86-ls2gs   2/2     Running   0          3m43s

[jarrpa]

Just a few small things left. Apart from the code questions, there's a commit description that should be amended:

 csi: install ceph-csi driver

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

This also includes clusterversion controller
which can trigger an upgrade of csi driver
if required when a OCP cluster is upgraded.

There is a if/else check in main.go which helps
to install/test csidriver changes by checking
this is a ocp cluster or kubernetes cluster.

The if/else mentioned has been removed.

[jarrpa]

There's not a 100% guarantee that this will always be the name of the Deployment resource... we can probably leave this as-is for the sake of expediency, but I would like to see something that gets the Deployment based on the metadata of the Pod.

[jarrpa]

PR looks good to merge, please create an issue for this comment when you're back online.

[jarrpa]

LGTM!