forked from kubeflow/training-operator
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
If a TFJob spec is invalid mark the job as failed with an appropriate…
… condition. * If a TFJob spec is invalid (e.g. can't be marshaled to TFJob YAML) we want to update the TFJob status to indicate it failed. * We need to use the REST API to update the TFJob status because we won't be able to deserialize the json to TFJob. * Related to kubeflow#755 * I created invalid-tfjob.jsonnet which can be used in an E2E test but I haven't included the E2E test in this PR. * I tested it manually and got the following result apiVersion: kubeflow.org/v1alpha2 kind: TFJob metadata: clusterName: "" creationTimestamp: 2018-08-31T23:37:14Z generation: 1 labels: app.kubernetes.io/deploy-manager: ksonnet ksonnet.io/component: invalid-tfjob name: invalid-tfjob namespace: kubeflow resourceVersion: "1826961" selfLink: /apis/kubeflow.org/v1alpha2/namespaces/kubeflow/tfjobs/invalid-tfjob uid: ca7b4b02-ad76-11e8-be57-42010a8e0084 spec: notTheActualField: Ps: replicas: 2 restartPolicy: Never template: spec: containers: - image: busybox name: tensorflow Worker: replicas: 4 restartPolicy: Never template: spec: containers: - image: busybox name: tensorflow status: conditions: - lastTransitionTime: 2018-08-31T23:37:14Z lastUpdateTime: 2018-08-31T23:37:14Z message: 'Failed to marshal the object to TFJob; the spec is invalid: Failed to marshal the object to TFJob' reason: FailedInvalidTFJobSpec status: "True" type: Failed tfReplicaStatuses: null
- Loading branch information
Showing
4 changed files
with
176 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package k8sutil | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "net/http" | ||
|
|
||
| tflogger "github.com/kubeflow/tf-operator/pkg/logger" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||
| "k8s.io/apimachinery/pkg/runtime/serializer" | ||
| "k8s.io/client-go/rest" | ||
| "k8s.io/client-go/kubernetes/scheme" | ||
| metav1unstructured "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| ) | ||
|
|
||
| // CRDRestClient defines an interface for working with CRDs using the REST client. | ||
| // In most cases we want to use the auto-generated clientset for specific CRDs. | ||
| // The only exception is when the CRD spec is invalid and we can't parse the type into the corresponding | ||
| // go struct. | ||
| type CRDClient interface { | ||
| // Update a TfJob. | ||
| Update(obj *metav1unstructured.Unstructured) (error) | ||
| } | ||
|
|
||
| // CRDRestClient uses the Kubernetes rest interface to talk to the CRD. | ||
| type CRDRestClient struct { | ||
| restcli *rest.RESTClient | ||
| } | ||
|
|
||
| func NewCRDRestClient(version *schema.GroupVersion) (*CRDRestClient, error) { | ||
| config, err := GetClusterConfig() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| config.GroupVersion = version | ||
| config.APIPath = "/apis" | ||
| config.ContentType = runtime.ContentTypeJSON | ||
| config.NegotiatedSerializer = serializer.DirectCodecFactory{CodecFactory: scheme.Codecs} | ||
|
|
||
| restcli, err := rest.RESTClientFor(config) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| cli := &CRDRestClient{ | ||
| restcli: restcli, | ||
| } | ||
| return cli, nil | ||
| } | ||
|
|
||
| // HttpClient returns the http client used. | ||
| func (c *CRDRestClient) Client() *http.Client { | ||
| return c.restcli.Client | ||
| } | ||
|
|
||
| func (c *CRDRestClient) Update(obj *metav1unstructured.Unstructured, plural string) (error) { | ||
| logger := tflogger.LoggerForUnstructured(obj, obj.GetKind()) | ||
| // TODO(jlewi): Can we just call obj.GetKind() to get the kind? I think that will return the singular | ||
| // not plural will that work? | ||
| if plural == "" { | ||
| logger.Errorf("Could not issue update because plural not set.") | ||
| return fmt.Errorf("plural must be set") | ||
| } | ||
| r := c.restcli.Put().Resource(plural).Namespace(obj.GetNamespace()).Name(obj.GetName()).Body(obj) | ||
| _, err := r.DoRaw() | ||
| if err != nil { | ||
| logger.Errorf("Could not issue update using URL: %v; error; %v", r.URL().String(), err) | ||
| } | ||
| return err | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // This is a test job to ensure we correctly handle the case where the job spec is not | ||
| // a valid TFJob and therefore can't be unmarshled to a TFJob struct. | ||
| // In this case we want to check that the TFJob status is updated correctly to reflect this. | ||
| // | ||
| local env = std.extVar("__ksonnet/environments"); | ||
| local params = std.extVar("__ksonnet/params").components["invalid-tfjob"]; | ||
|
|
||
| local k = import "k.libsonnet"; | ||
|
|
||
|
|
||
| local name = params.name; | ||
| local namespace = env.namespace; | ||
|
|
||
| local podTemplate = { | ||
| spec: { | ||
| containers: [ | ||
| { | ||
| name: "tensorflow", | ||
| // image doesn't matter because we won't actually create the pods | ||
| image: "busybox", | ||
| }, | ||
| ], | ||
| }, | ||
| }; | ||
|
|
||
| local job = { | ||
| apiVersion: "kubeflow.org/v1alpha2", | ||
| kind: "TFJob", | ||
| metadata: { | ||
| name: name, | ||
| namespace: namespace, | ||
| }, | ||
| spec: { | ||
| // Provide invalid json | ||
| notTheActualField: { | ||
| Ps: { | ||
| replicas: 2, | ||
| restartPolicy: "Never", | ||
| template: podTemplate, | ||
| }, | ||
| Worker: { | ||
| replicas: 4, | ||
| restartPolicy: "Never", | ||
| template: podTemplate, | ||
| }, | ||
| }, | ||
| }, | ||
| }; // job. | ||
|
|
||
| std.prune(k.core.v1.list.new([job])) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters