TFJob not marked as success when master exits but not workers

[jlewi]

I ran the simple TFJob we use for smoke tests but modified it to have PS, Workers, and Master
https://github.com/kubeflow/tf-operator/blob/master/test/workflows/components/simple_tfjob.jsonnet

The master is marked as succeeded but the job is still marked as running. See below.

This is different behavior from v1alpha1 and seems like a regression.

@gaocegege @ScorpioCPH

apiVersion: kubeflow.org/v1alpha2
  kind: TFJob
  metadata:
    annotations:
      ksonnet.io/managed: '{"pristine":"H4sIAAAAAAAA/8yQzUosMRCF9/cxzjo9f8zi3jzAXQjK4IguRKQmXWlDdychVY7IkHeX+APqE8yuOMU5fOecQDnccpGQIizG5wP7Kb0sUhmWxzVN+Yk2MBhD7GFx8/8iHWAws1JPSrAnRJoZFhLmPHG3Wq1h3jXJ5PhbZKcs2oXoC6EaSGbX7OqvOU/B0T6zk6ZckiiXdpWPj8CuDQqLUtFdmoJ7hcUVH7nAQHnOEyk3w1eoS1EpRC4Ce39CmGloKIMri5CW6rsUu/GvdH0afEr9Uv2jUOO3vfu33Xr/2QEWylFSaQVQH2qtBrv9T7bNGbHdpTL+3m57Nny11j9vAAAA//8BAAD//2bf3XNxAgAA"}'
    clusterName: ""
    creationTimestamp: 2018-06-11T22:01:43Z
    generation: 0
    labels:
      app.kubernetes.io/deploy-manager: ksonnet
    name: simple-001
    namespace: kubeflow-test-infra
    resourceVersion: "864756"
    selfLink: /apis/kubeflow.org/v1alpha2/namespaces/kubeflow-test-infra/tfjobs/simple-001
    uid: 06ce7f5a-6dc3-11e8-996c-42010af000af
  spec:
    tfReplicaSpecs:
      Master:
        replicas: 1
        restartPolicy: Never
        template:
          metadata:
            creationTimestamp: null
          spec:
            containers:
            - image: gcr.io/tf-on-k8s-dogfood/tf_sample:dc944ff
              name: tensorflow
              ports:
              - containerPort: 2222
                name: tfjob-port
              resources: {}
      PS:
        replicas: 2
        restartPolicy: Never
        template:
          metadata:
            creationTimestamp: null
          spec:
            containers:
            - image: gcr.io/tf-on-k8s-dogfood/tf_sample:dc944ff
              name: tensorflow
              ports:
              - containerPort: 2222
                name: tfjob-port
              resources: {}
      Worker:
        replicas: 4
        restartPolicy: Never
        template:
          metadata:
            creationTimestamp: null
          spec:
            containers:
            - image: gcr.io/tf-on-k8s-dogfood/tf_sample:dc944ff
              name: tensorflow
              ports:
              - containerPort: 2222
                name: tfjob-port
              resources: {}
  status:
    conditions:
    - lastTransitionTime: 2018-06-11T22:01:43Z
      lastUpdateTime: 2018-06-11T22:01:43Z
      message: TFJob simple-001 is created.
      reason: TFJobCreated
      status: "True"
      type: Created
    - lastTransitionTime: 2018-06-11T22:01:48Z
      lastUpdateTime: 2018-06-11T22:01:48Z
      message: TFJob simple-001 is running.
      reason: TFJobRunning
      status: "True"
      type: Running
    startTime: 2018-06-11T22:37:16Z
    tfReplicaStatuses:
      Master:
        succeeded: 1
      PS:
        active: 2
      Worker:
        active: 4

[jlewi]

The problem appears to be here
https://github.com/kubeflow/tf-operator/blob/master/pkg/controller.v2/controller_status.go#L45

The code doesn't appear to be implementing the termination policy logic as we discussed in the v1alpha2 spec. In particular

If there is a chief the job should be marked as completed when the chief exits.
If there is no chief worker 0 should be considered the chief.

[jlewi]

Support for exit policies was discussed here in the original proposal
kubeflow/community#30 (comment)

[gaocegege]

We have no logic about master replicas now, and I think we do not just miss the support of exit policies. I think it could be added in 0.3. My opinion is to make ps/worker high availability, WDYT

[gaocegege]

And FYI, the master spec is using Chief instead of Master.

[gaocegege]

I implement the logic #637 and the code will generate the cluster spec like this:

{'chief': ['host0:2222'],
'ps': ['host1:2222', 'host2:2222'],
'worker': ['host3:2222', 'host4:2222', 'host5:2222']}

Thus we need to write an example for the case.

[jlewi]

I don't think we can wait for 0.3 to implement exit policy. Without exit policy most TF jobs won't worker and this would be a major regression so we effectively can't launch v1alpha2 in 0.2; which would be a major blow to our plans to be v1 by the end of the year.

Lots of TF jobs either have an explicit chief or use worker 0 as the chief.

Can you open up an issue on high availability? Its not clear to me what this means or why its important.

[jlewi]

/cc @ddysher @DjangoPeng

[gaocegege]

OK, SGTM. we have 3 days before the DL and I will work on it.

[gaocegege]

I think it is closed by #637

[DjangoPeng]

thanks! @gaocegege

[372046933]

@gaocegege Can you provide a sample training with Chief, Master and Worker

[gaocegege]

@372046933 I do not really train a mode, I just use busybox to test the functionality.