NetApp / trident

Storage orchestrator for containers
Apache License 2.0
732 stars 218 forks source link

Document TridentBackendConfig custom resource #861

Open lindhe opened 8 months ago

lindhe commented 8 months ago

Describe the solution you'd like

The TridentBackendConfig custom resource is undocumented when inspecting it with kubectl explain:

$ kubectl explain TridentBackendConfig
KIND:     TridentBackendConfig
VERSION:  trident.netapp.io/v1

DESCRIPTION:
     <empty>

Having documentation for the object and each field in the object is immensely helpful when working with the system. This will also contribute to the API being self-documenting, reducing the burden of keeping external docs up-to-date.

Additional context

Documentation for custom resources is defined by the CRD.

rohit-arora-dev commented 8 months ago

@lindhe I agree there should be some minimalistic description, however, with TridentBackendConfig (TBC) its difficult to describe each of the fields. If you look at the Trident documentation, you will notice that you can use TBC with all the different kinds of Trident backends (ONTAP * 5 (flavors), Solifdire, FSx, ANF, GCP-CVO, etc.), each of them have different Spec and Status fields. This is possible because TridentBackendConfig uses schemaless CRDs (something we usually avoid), however in this case using a schemeless CRD allows us to use the same CR with different backend setups. An alternate would have been to introduce a new CRD for every backend i.e. 10+ Backend CRDs which would have made things way more complicated and potentially rewrite some of the existing implementation that works with tridentctl to create backends.

lindhe commented 8 months ago

I had no idea there was such a thing as a schemaless CRD! That sounds both powerful and horrifying. 😄 From your description, it sounds like you have a reasonable use-case for it. Or at least you have actively chosen this design, which is way better than ending up there from organic feature growth, in my opinion.

Anyway, it sounds like this is not as straight-forward as I thought. Maybe we can at least add the minimal description as you say, but we probably have to leave it at that. 🤷‍♂️

lindhe commented 8 months ago

This really bugs me, It does not sit well with me that we are stuck with a bunch of vague fields. It sounds like it will create a lot of problems when configuring systems.

I am trying to brain-storm some solutions. Do you think any of the following solutions might be reasonable? I lean most towards the More fields proposal below.

Longer descriptions.

We could choose to over-specify the description for each field, something like this:

FIELDS:

   spec   <Object>
     Specification of the TridentBackendConfig.

     For **ONTAP** systems, this will define A.

     For **SolidFire** systems, this will define B.

     […]

   status <Object>
     Most recently observed status of the TridentBackendConfig.

     For **ONTAP** systems, this will show C.

     For **SolidFire** systems, this will define D.

     […]

Or will it be way too much information to be reasonable?

More fields

Another approach could be to have a single custom resource (TridentBackendConfig), but for that object to have many more fields. I am thinking something like this:

FIELDS:

  specONTAP   <Object>
     Specification of the TridentBackendConfig for **ONTAP** systems. This will define A.

  specSolidFire   <Object>
     Specification of the TridentBackendConfig for **SolidFire** systems. This will define B.

     […]

   statusONTAP <Object>
     Most recently observed status of the TridentBackendConfig for **ONTAP** systems. This will show C.

   statusSolidFire <Object>
     Most recently observed status of the TridentBackendConfig for **SolidFire** systems. This will show D.

     […]

Just thinking about the data types I think this is reasonable, since spec for an ONTAP system is different than spec for a SolidFire system.

rohit-arora-dev commented 8 months ago

I truly appreciate that you want to provide consistent behaviour with TridentBackendConfig CRD. If I understand your proposal, you want to include this information in the description. Each of Trident's backends supports many different field types and the list is constantly growing, so in my personal opinion and since it is not autogenerated, there is a good possibility that it will very soon become out of date. The most accurate up-to-date information you would get is from Trident docs.

The More Fields option appears to be a proposal to convert a schemaless CRD to schema-based, if I understand it right. This would require a good amount of new engineering effort as well as seamlessly transitioning existing users to a new schema which won't be straightforward.

lindhe commented 8 months ago

Each of Trident's backends supports many different field types and the list is constantly growing, so in my personal opinion and since it is not autogenerated, there is a good possibility that it will very soon become out of date. The most accurate up-to-date information you would get is from Trident docs.

In my experience, it is way harder to keep documentation up-to-date than code. But I won't push this point if your existing workflow works well.

The More Fields option appears to be a proposal to convert a schemaless CRD to schema-based, if I understand it right. This would require a good amount of new engineering effort as well as seamlessly transitioning existing users to a new schema which won't be straightforward.

Yes, that would be a major (breaking) change.