Tf-operator: [feature] Seperate the CRD and controller

[gaocegege]

Now we create CRD when the controller is initialized, and the implementation may cause some issues: #173 . Then I think if we could write a crd.yml similar to https://github.com/caicloud/kubeflow-controller/blob/master/examples/crd/crd.yml, to create the CRD directly in Kubernetes.

If we does it, then the operator's logic is clear: dealing with the lifecycle of TFJob.

[caogj]

i think a sperate crd yaml file should be the common pattern among k8s related project, e.g. istio.

[gaocegege]

@caogj

I think there are two patterns for this kind of project: operator and controller. In the operator pattern, the crd is usually created when the operator is initialized, e.g. etcd-operator. And in the controller pattern, the crd is usually placed in a sperate yaml file, e.g. kube-metacontroller and istio.

Personally, I prefer the latter since CRD and the operator/controller are decoupled by design.

[jlewi]

I vote for separating the CRD and controller.

Aside from this being common, CRD's are cluster scoped and not namespace scoped. But controllers can be name space scoped. We could potentially have multiple controllers running.

[gaocegege]

@jlewi Could you explain more about why CRD is cluster scoped :thinking:

/cc @DjangoPeng @ScorpioCPH (We have a discussion about it internally, too)

[jlewi]

Its my understanding that CRD's are cluster level resources you create the CRD at the cluster level not in a particular namespace. I think that's just how it works. The actual resources can be namespaced or cluster level scoped but not the definition.

/cc @enisco

[DjangoPeng]

Agree with @jlewi .

TFJob CRD itself should be cluster scope rather than namespace scope, but the CRD object may belong to a specific namespace.

The CRD guide also gives some explanations as below:

When you create a new CustomResourceDefinition (CRD), the Kubernetes API Server reacts by creating a new RESTful resource path, either namespaced or cluster-scoped, as specified in the CRD’s scope field. As with existing built-in objects, deleting a namespace deletes all custom objects in that namespace. CustomResourceDefinitions themselves are non-namespaced and are available to all namespaces.

I'm not sure whether the default value of scope field is Cluster. If not, we have to explicitly set it.

@gaocegege In our early internal discussion, we lay the problem aside. Now, it's the time to continues discussing it, although I think it's obvious that cluster scope is better.

I remember that @ScorpioCPH also votes for cluster scope.

Ref: https://github.com/caicloud/kubeflow-clientset/issues/18

[DjangoPeng]

BTW: We may need to set a dedicated Finalizer for TFJob CRD to handle some misoperations.

Ref: https://kubernetes.io/docs/tasks/access-kubernetes-api/extend-api-custom-resource-definitions/#finalizers

[jlewi]

@DjangoPeng Why do you want the CRD to be cluster scoped? If we scope it to cluster then all TfJobs are cluster scope which I don't think we want. We want it to be namespace scoped so that TfJobs are created in a particular namespace.

[DjangoPeng]

@jlewi I realised that there is a misunderstanding of scope field in the CRD after testing. What I want is

CustomResourceDefinitions themselves are non-namespaced and are available to all namespaces.
But the CRD (e.g. TFJob) object may belong to a specific namespace.

Actually, if we set CRD as cluster scope, all corresponding CRD objects are cluster scope. I set the scope as Cluster in this CronTab CRD example. Then I created a CronTab object and list all CronTab resources and I attached the results as below which verifies the point.

$ kubectl get ct --all-namespaces
NAMESPACE   NAME                 AGE
            my-new-cron-object   17m

Assuming that we keep the scope as Namespaced in CronTab CRD. I set different namespace (default and test) for two CronTab objects and then I found the two objects belong to corresponding namespace, which is what we want.

$ kubectl get ct --all-namespaces
NAMESPACE   NAME                    AGE
default     my-new-cron-object-0   27s
test        my-new-cron-object-1   24s

Now, I think scope field in CRD should be Namespaced and it means that we want all corresponding CRD objects are namespace scope, in other word, they belong to particular namespace. At the same time, whatever the scope filed is, CRD itself is cluster level rather than belong to any particular namespace.

[jlewi]

Yes.
Agree scope should be namespace.

[ScorpioCPH]

@jlewi @DjangoPeng @gaocegege

So which namespace should we put our CRD object (TFJob in our case):

• I prefer kube-system:
  
  
  
  • TFJob a critical resource which is expected to be restricted to kube-system namespace.
  
  
• Or leave it in default namespace:
  
  
  
  • It is used for all objects with no specified namespace (but in namespace scope)
  
  
• AFAIK, both kube-system and default may not be deleted, so they are safe for our case :)

[jlewi]

Since the CRD is cluster scope namespace doesn't actually matter. So I think we should just leave it blank.

[gaocegege]

We come to an agreement 🎉