If a TFJob spec is invalid mark the job as failed with an appropriate condition #815

jlewi · 2018-08-31T23:47:35Z

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.
Fix Surface invalid spec errors in a more user friendly way #755
Added an E2E test
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

This change is

jlewi · 2018-08-31T23:47:53Z

/assign @gaocegege
/assign @richardsliu

TravisBuddy · 2018-09-01T00:11:25Z

Travis tests have failed

Hey @jlewi,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

gometalinter --config=linter_config.json --vendor ./...

pkg/controller.v2/tensorflow/job.go:1::warning: file is not goimported (goimports)
pkg/util/k8sutil/client.go:1::warning: file is not goimported (goimports)

3rd Build

gometalinter --config=linter_config.json --vendor ./...

pkg/controller.v2/tensorflow/job.go:1::warning: file is not goimported (goimports)
pkg/util/k8sutil/client.go:1::warning: file is not goimported (goimports)

travis_time:end:041cafb8:start=1535759387961045059,finish=1535759533070855959,duration=145109810900

coveralls · 2018-09-01T00:12:09Z

Coverage decreased (-1.3%) to 56.932% when pulling 685e744 on jlewi:invalid_spec into 1bc8425 on kubeflow:master.

TravisBuddy · 2018-09-01T02:02:55Z

Travis tests have failed

Hey @jlewi,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

gometalinter --config=linter_config.json --vendor ./...

pkg/controller.v2/tensorflow/job.go:1::warning: file is not goimported (goimports)
pkg/util/k8sutil/client.go:1::warning: file is not goimported (goimports)

3rd Build

gometalinter --config=linter_config.json --vendor ./...

pkg/controller.v2/tensorflow/job.go:1::warning: file is not goimported (goimports)
pkg/util/k8sutil/client.go:1::warning: file is not goimported (goimports)

travis_time:end:0aed1668:start=1535766190904099749,finish=1535766308575260949,duration=117671161200

TravisBuddy · 2018-09-01T02:26:01Z

Travis tests have failed

Hey @jlewi,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

gometalinter --config=linter_config.json --vendor ./...

pkg/controller.v2/tensorflow/job.go:1::warning: file is not goimported (goimports)
pkg/util/k8sutil/client.go:1::warning: file is not goimported (goimports)

3rd Build

gometalinter --config=linter_config.json --vendor ./...

pkg/util/k8sutil/client.go:1::warning: file is not goimported (goimports)
pkg/controller.v2/tensorflow/job.go:1::warning: file is not goimported (goimports)

travis_time:end:051e40d8:start=1535767586443320354,finish=1535767715411724086,duration=128968403732

TravisBuddy · 2018-09-01T02:32:41Z

Travis tests have failed

Hey @jlewi,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

gometalinter --config=linter_config.json --vendor ./...

pkg/controller.v2/tensorflow/job.go:1::warning: file is not goimported (goimports)
pkg/util/k8sutil/client.go:1::warning: file is not goimported (goimports)

3rd Build

gometalinter --config=linter_config.json --vendor ./...

pkg/util/k8sutil/client.go:1::warning: file is not goimported (goimports)
pkg/controller.v2/tensorflow/job.go:1::warning: file is not goimported (goimports)

travis_time:end:12b57e08:start=1535768130181658937,finish=1535768240004950095,duration=109823291158

… 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 * Add an E2E test; test_runner.py was getting overly complicated so I created a new main file to run the test and just call methods in test_runner.py as needed.

jlewi · 2018-09-01T03:16:18Z

/test all

gaocegege · 2018-09-01T05:17:01Z

/cc @johnugeorge

I think you might be interested in the PR.

/lgtm
/approve

I think creating a client every time we meet an invalid spec works well now but I will try to find a better way to implement it.

k8s-ci-robot · 2018-09-01T05:17:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [gaocegege]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the size/L label Aug 31, 2018

k8s-ci-robot requested review from cheyang and yph152 August 31, 2018 23:47

k8s-ci-robot assigned gaocegege and richardsliu Aug 31, 2018

jlewi changed the title ~~If a TFJob spec is invalid mark the job as failed with an appropriate…~~ If a TFJob spec is invalid mark the job as failed with an appropriate condition Aug 31, 2018

jlewi force-pushed the invalid_spec branch from 9ad2085 to 685e744 Compare September 1, 2018 02:57

k8s-ci-robot added the lgtm label Sep 1, 2018

k8s-ci-robot requested a review from johnugeorge September 1, 2018 05:17

k8s-ci-robot added the approved label Sep 1, 2018

k8s-ci-robot merged commit 454c5c1 into kubeflow:master Sep 1, 2018

jlewi mentioned this pull request Sep 4, 2018

Document Requirements/Best Practices for Operators kubeflow/kubeflow#1471

Closed

johnugeorge mentioned this pull request Sep 12, 2018

Mark Job failed when spec is invalid kubeflow/pytorch-operator#71

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

If a TFJob spec is invalid mark the job as failed with an appropriate condition #815

If a TFJob spec is invalid mark the job as failed with an appropriate condition #815

jlewi commented Aug 31, 2018 •

edited

Loading

jlewi commented Aug 31, 2018

TravisBuddy commented Sep 1, 2018

coveralls commented Sep 1, 2018 •

edited

Loading

TravisBuddy commented Sep 1, 2018

TravisBuddy commented Sep 1, 2018

TravisBuddy commented Sep 1, 2018

jlewi commented Sep 1, 2018

gaocegege commented Sep 1, 2018

k8s-ci-robot commented Sep 1, 2018

If a TFJob spec is invalid mark the job as failed with an appropriate condition #815

If a TFJob spec is invalid mark the job as failed with an appropriate condition #815

Conversation

jlewi commented Aug 31, 2018 • edited Loading

jlewi commented Aug 31, 2018

TravisBuddy commented Sep 1, 2018

Travis tests have failed

1st Build

3rd Build

coveralls commented Sep 1, 2018 • edited Loading

TravisBuddy commented Sep 1, 2018

Travis tests have failed

2nd Build

3rd Build

TravisBuddy commented Sep 1, 2018

Travis tests have failed

2nd Build

3rd Build

TravisBuddy commented Sep 1, 2018

Travis tests have failed

2nd Build

3rd Build

jlewi commented Sep 1, 2018

gaocegege commented Sep 1, 2018

k8s-ci-robot commented Sep 1, 2018

jlewi commented Aug 31, 2018 •

edited

Loading

coveralls commented Sep 1, 2018 •

edited

Loading