ceph / ceph-csi-operator

Kubernetes operator for managing the CephCSI plugins
Apache License 2.0
13 stars 13 forks source link

Redundant `CephCSI` prefix in all the CRDs #10

Closed leelavg closed 1 month ago

leelavg commented 3 months ago

Naming of APIs in design doc so far is

  1. CephCSIOperatorConfig (v1a1)
  2. CephCSIDriver (v1a1)
  3. CephCSICephCluster (v1a1)
  4. CephCSIConfig (v1a1)
  5. CephCSIConfigMapping (v1)

The group we chose is csi.ceph.io, first point, do we need to have CephCSI prefix in all the CRDs? It'll be repeated in Kind/Resource and also in Group. Second, most of these APIs are mentioning a config and we are deriving desired state out of it, as such should we again have Config in the name as an abstraction?

After a rename the APIs will read as (in GR form)

  1. operators.csi.ceph.io
  2. drivers.csi.ceph.io
  3. cephclusters.csi.ceph.io (maybe clusters.csi.ceph.io)
  4. - (maybe we need to remove abstraction)
  5. mappings.csi.ceph.io (maybe elaborate on what is the mapping)

The only con (technically) that I can think of is during kubectl ops, we might be forced to type upto non-overlapping GR (unless we could use shortName) if there is a conflicting Kind/Resource. Since everything is a config, could we make group as config.csi.ceph.io?

nb-ohad commented 3 months ago

Regarding the kubectl ops, typing kubectl get cephcsidrivers is just two chars less than typing kubectl drivers.csi.ceph. I will argue the difference is meaningless.

I also would like to direct your attention to this answer on reddit on the same subject: https://www.reddit.com/r/kubernetes/comments/vszgrw/crd_naming_convention_how_to_avoid_collisions/

Madhu-1 commented 3 months ago

@travisn @blaineexe any thoughts on this one as we are talking about naming conventions in a couple of places already?

BlaineEXE commented 3 months ago

Personally, I understand the idea, but I think this will end up being a usability hassle for users. Consider that most users don't use fully qualified resource kinds when using kubectl. They just use kubectl get cephcluster or kubectl get cephobjectstore.

Also consider that "driver" and "operators" are broad topics that might conflict with resource names that are core to the k8s project today or might be added in the future.

"cephclusters" will conflict with the plural form of Rook's "cephcluster", which is going to contribute to user confusion, especially when these resources are deployed in the same ns as the Rook.

We have already seen users get confused when there are multiple different API providers of the StorageCluster type. I think that these issues are guaranteed to happen if this decision is made. Travis also shared an anecdote with us in Rook huddle today about users becoming confused in the early days of Rook when CephCluster was called "Cluster." There are 2 cases of prior proof to show that users will be confused by this choice.

From the outside, it seems to me like this is an attempt to save some characters on screen or on the commandline without consideration for how users will be negatively impacted. This might have made sense in the 80s when TTYs were limited to 80 characters wide, but in the modern era, unnecessarily vague, short naming is unnecessary and is considered an anti-pattern.

I am adamantly, 100% against this idea on the grounds of its experientially-proven poor user experience.


[Update]

I understand that name collisions in k8s cannot be guaranteed to not happen. That is why the api version can be given. But this just seems like a decision to be deliberately obtuse and cause users headache when we could easily give a name that has a low possibility of collision without users having to change their normal workflows.

Like, if StorageCluster has a collision, that is a bummer, but at least StorageCluster is a reasonably clear name even without the API component.

Just "Driver" is unnecessarily vague and will confuse users, I guarantee it.

travisn commented 3 months ago

This needs more discussion, I'd suggest it be reopened...

I do see an argument for not putting a prefix on all the types with the assumption that we don't normally expect a naming collision with the short name. While we can encourage in the docs to use a more qualified name, neither do we want a common scenario either for a collision. It is extremely confusing if you run oc get driver and you don't see your expected instead of the driver because another CRD with the same short name is installed, and it is hiding your instances. If everyone becomes accustomed to using a more qualified name, they won't hit this issue. That's fine if we declare it is an education issue.

CephCluster has been a very useful unique name because there are so many other products that could be called Cluster. A related example is EtcdCluster, where they also chose to use their prefix.

If the prefix is removed, and updating CRD names based on suggestions in #8, the CRD names might be:

operators.csi.ceph.io
drivers.csi.ceph.io
cephclusters.csi.ceph.io 
cephresources.csi.ceph.io 
cephresourcemappings.csi.ceph.io

While operators and drivers still seems way too generic IMO, the main problem is CephClusters. We cannot choose a name that conflicts with one of our own CRDs, this will just be too common of an issue. We would have to find a different name for that CRD if we remove the prefix. I can't think of another short name that is descriptive enough, other than adding the prefix back.

Ultimately the naming is up to us. There is nothing on K8s naming conventions that addresses the naming conflicts. @nb-ohad Do you see any other discussions on this thread besides the one reddit link? I didn't find others, so it doesn't seem like a standard one way or the other.

nixpanic commented 3 months ago

My preference would be to not provide any optional short-names for these CRDs. Users are not expected to interact with them much, so it isn't nice to clutter their kubectl search/auto-completion with these names either.

That also makes it much simpler to have clusters.csi.ceph.io and similar, they would be unique and very desciptive in any case.

The main drawback would be the extra typing for a few administrators and developers while testing. I think that the priority should be on the users that deploy applications, not the other two groups.

Madhu-1 commented 1 month ago

This is done in #62 closing it