Tf-operator: TfJob operator stops working on invalid spec

[jlewi]

I submitted a job with an invalid spec (container args contained integrs and not strings). The job was created but it was never started and the status was never updated. Furthermore, I think this blocked the TFJob operator from processing any other jobs. Deleting the job fixed things.

The TFJob operator showed the following logs.

E0422 01:21:04.083809       1 reflector.go:205] github.com/kubeflow/tf-operator/pkg/client/informers/externalversions/factory.go:59: Failed to list *v1alpha1.TFJob: v1alpha1.TFJobList: Items: []v1alpha1.TFJob: v1alpha1.TFJob: Spec: v1alpha1.TFJobSpec: ReplicaSpecs: []*v1alpha1.TFReplicaSpec: v1alpha1.TFReplicaSpec: Template: v1.PodTemplateSpec: Spec: v1.PodSpec: Containers: []v1.Container: v1.Container: Env: []v1.EnvVar: v1.EnvVar: v1.EnvVar: Value: ReadString: expects " or n, parsing 1996 ...,"value":1... at {"apiVersion":"kubeflow.org/v1alpha1","items":[{"apiVersion":"kubeflow.org/v1alpha1","kind":"TF

I believe what's happening is that since the spec is invalid the result of List can't be successfully parsed into a Go struct. As a result, I think the TFJob operator is unable to work.

I think this is a problem in the underlying informer package; i.e. its not robust to invalid specs. We should check if this is a known issue and if there is an existing bug. (Ideally, it would just ignore invalid specs).

I think we could fix this a number of ways in TFJob controller

• If we use CRD's spec validation feature and provide a swagger spec I think we could prevent invalid specs from being accepted in the first place

• The operator could try to catch this error and then find and update the invalid spec

  • Deleting the job might be confusing for users but maybe we could leave the job there but delete most fields and set status to "invalid spec"?

Swagger is probably the best place to start.

We should try to get this fixed in 0.2

[jlewi]

See also kubeflow/tf-operator#437 - OpenAPI Validation for TFJob controller

[gaocegege]

It is hard to validate the config using the feature crd validation since we have podtemplatespec in the definition. The feature does not support $ref, podtemplatespec heavily uses $ref. If we want to check it, we need to get the OpenAPI schema and have a hack on it, which I think is not what we want.

[jlewi]

Can you explain more about $ref and how it is used? Would it be possible to just use OpenAPI validation to ensure that container args are strings not integers?

I guess another solution might be to use an admission controller to validate the spec.

@enisoc Any suggestions on how to handle this?

@jessesuen I think you faced a similar problem with the Argo CRD what did you do?

[enisoc]

If you just want to validate container args, and not everything in PodTemplateSpec, then it may be feasible to write an OpenAPI schema by hand for that.

If you want to validate the whole PodTemplateSpec, the best workaround I've heard of so far is this one (although I haven't tried it personally):

https://github.com/kubernetes/kubernetes/issues/54579#issuecomment-370372942

[gaocegege]

Thanks, I will take a look. We want to validate the whole podtemplatespec 😄

[jessesuen]

@jessesuen I think you faced a similar problem with the Argo CRD what did you do?

@jlewi coincidentally, I literally just "fixed" this in the workflow controller. But it's more to workaround the upstream kubernetes issue: https://github.com/kubernetes/kubernetes/issues/57705.

I followed the recommendation in this comment:
https://github.com/kubernetes/kubernetes/issues/57705#issuecomment-354552314

Instead of using the auto-generated workflow informer, I wrote a unstructured.Unstructured informer which watches Unstructured objects of the argoproj.io/v1alpha1 workflow API resource type. Then I convert unstructured object to workflow objects using runtime.DefaultUnstructuredConverter.FromUnstructured. If this fails to convert, I now can update the workflow with a proper error message indicating the spec was invalid.

The fix can be seen here:
https://github.com/argoproj/argo/commit/8d96ea7b1b1ba843eb19a0632bc503d816ab9ef3

[gaocegege]

Thanks for your reply @jessesuen

@jlewi I wrote a tool to generate the validation from the OpenAPI specification: https://github.com/gaocegege/crd-validation. Generated CRD for tfjob v1alpha2 is https://github.com/gaocegege/crd-validation/blob/master/generated/tfjob-crd-v1alpha2.yaml

While we meet an issue from Kubernetes side: https://github.com/kubernetes/kubernetes/issues/59485#issuecomment-380032718. Kubernetes does not support addtionalproperties while it is needed for map type. Unfortunately, TFRelicaSpec is a map.

The upstream said it will be implemented in 1.11. Maybe we should wait for it. After 1.11 I think we are able to use the tool above to generate validations for all crds in kubeflow community.

[gaocegege]

At this moment validating all types in the CRD is not practical, and the crd validation feature has some limitations, such as lack of addtional properties support. I think the workaround from jessesuen is a good way to solve the problem.

[jlewi]

@gaocegege I like the idea of following @jessesuen's work around and then implementing what validation we can with CRD validation.

[gaocegege]

Yeah, I am working on it. 😄