Only identify specific exit codes as retryable error

[wenzhel101]

The criteria to decide permanent error vs retryable error is not correct. Basically, the current exit code range[128, 255] for retryable errors is too broad and it will misclassify some permanent errors as retryable. For example:

1. user code exits with negative code(e.g. sys.exit(-1)). The container exit code will be 255(exit_code % 256).
2. user code was killed by SIGSEGV, SIGABRT, etc. Those cases are mostly caused by the problem in mem allocation from user code.

The proposal is to only allow retry for specific error codes that are more likely to be caused by transient issues(e.g. VM was rescheduled or VM was deleted by mistake):
130 = (128+2) Container terminated by Control-C
137 = (128+9) Container received a SIGKILL
143 = (128+15) Container received a SIGTERM

The list can be extended if we see other retryable cases.

The safe approach is to classify all the non-zero exit code as permanent errors.

---

This change is 

[coveralls]

Coverage increased (+1.1%) to 49.643% when pulling 96d933e on 0olwzo0:master into ff2958a on kubeflow:master.

[wenzhel101]

/assign @jlewi

[jlewi]

If we treat all errors as permanent then how do we deal with retryable errors like the gRPC server going down?

Or the process being killed by SIGTERM because a node became unhealthy?

[wenzhel101]

It depends that how to define the retryable error. The gRPC server going down can be related to user code. Then it makes some sense to say it's still user error(though it's not permanent). And for the SIGTERM case, it's also hard to tell if the node was manually shutdown by user(user error) or by the service like GCE(retryable?).
I agree that we may not be able to handle it perfectly. It depends on how frequently the edge cases happen. IMO, If all of the edge cases are not 'likely' to happen, it's fine to treat them all as permanent errors.

[jlewi]

I can say from experience that retryable errors are an issue and treating all errors is permanent as a no go.

On Cloud VMs can die and this will cause workers to die. Treating this as a permanent error and failing the job is not the right thing to do.

Do you agree can we close this PR?

[wenzhel101]

I updated the PR to only allow to retry for some known cases from CloudML Engine. And I think the list can be extended if we see other data points.
What do you think, @jlewi?

[jose5918]

Doesn't it make more sense to treat all errors as retryable and classify certain ones we know are permanent. Basically the reverse of what is being proposed here. Kubernetes already has the CrashLoopBackOff for things that are failing and it's possible to put a limit on the time the operator lets a job stay in a failed state.

[jlewi]

The criteria to decide permanent error vs retryable error is not correct. Basically, the current exit code range[128, 255] for retryable errors is too broad and it will misclassify some permanent errors as retryable.

What's an example of an error code that would be missclassified using the current schema?

I don't think this list is sufficient
130 = (128+2) Container terminated by Control-C
137 = (128+9) Container received a SIGKILL
143 = (128+15) Container received a SIGTERM

TensorFlow is a distributed system. So when one process goes down; e.g. SIGTERM because VM goes down. This error can propogate to other TF workers e.g. because gRPC now encounters errors causing exceptions to be thrown.

These exceptions need to be caught and turned into exit codes that indicate the proper behavior e.g.
permanent or retryable.

So I think users need a schema that gives them the ability to indicate whether an exit is retryable or not.

The original schema was chosen for simplicity, symmetry and to give users the ability to define an error as retryable or permanent

1-128 - permanent
129-255 - retryable

This automatically classifies most unix triggered failures as retryable which I believe is the correct behavior. Does anyone have a counter example?

We treat OOMs as permanent but we detect those based on K8s/Docker signals not exit codes.

/cc @gaocegege @ScorpioCPH

[gaocegege]

I am not sure if it is the conventions from Google so I did not leave comments here.

Personally, I agree with @jlewi . And if we need to support different policies for different clouds. There are two options:

• Support multiple cloud environments adding cloud environment name #451
• Support more restart policies, e.g. one for CloudML

FYI There is a doc about sys.exit in python.

Most systems require it to be in the range 0–127, and produce undefined results otherwise. Some systems have a convention for assigning specific meanings to specific exit codes, but these are generally underdeveloped

[wenzhel101]

This automatically classifies most unix triggered failures as retryable which I believe is the correct
behavior. Does anyone have a counter example?

It's not true that all unix triggered failures are retryable. An example we have seen on CloudML Engine is SIGSEGV, which is related to user code and won't recover after retry. In this case, keeping the job running is a wrong behavior and the issue may not be noticed by users(TFJob is RUNNING).
We also saw some users do sys.exit(-1) to exit from their programs. They meant to stop executing the program but it will be retried using tf-operator.

To me, from the cloud service perspective, keeping the resources running after a misclassification of retryable error is not a good idea(but it may be not true for non-cloud environment). I agree with @gaocegege, supporting custom restart policy is a better option. But I still don't think [128, 255] is the right default range for exit code to retry.

[jlewi]

Thanks that's a good example.

Does anyone know off hand what exit code Python uses for an unhandled exception? I think the default behavior for that should be retyable.

If we want to be explicit about exist codes then I think we should do the following

• Define exit codes corresponding to user defined retryable and permanent errors

• Define behavior for exit codes corresponding to relevant unix signals

• Explicitly define all other exit codes as undefined and not make any guarantees.

[jlewi]

Any thoughts?

[wenzhel101]

Define exit codes corresponding to user defined retryable and permanent errors

Does it mean that users provide a predefined list of exit code to retry?

Define behavior for exit codes corresponding to relevant unix signals

The question is how to get the full list of the retryable signals. Do you think SIGTERM and SIGKILL are good enough as the start point?

Explicitly define all other exit codes as undefined and not make any guarantees.

What do you mean by 'not make any guarantees'? Is it to identify them as permanent error?

[gaocegege]

@ddysher I think so. We should have a plan to support customized restartpolicy in v1alpha2.

[jlewi]

What do you mean by 'not make any guarantees'? Is it to identify them as permanent error?
I mean the behavior is undefined; i.e. we don't specify whether an exit code will be treated as retryable or permanent. Its left to the implementation to decide.

I think in addition to SIGTERM and SIGKILL we should figure out what exit code Python by default uses for unhandled exceptions and map that to retryable errors.

We should also pick two exit codes one to correspond to user defined retryable and one to correspond to user defined exit codes.

@ddysher Yes I think the behavior should be the same for v1alpha2. We should define function IsRetryableExitCode so that we can use the same code in both implementations.

[wenzhel101]

We should also pick two exit codes one to correspond to user defined retryable and one to correspond to user defined exit codes.

How about SIGUSR1 & SIGUSR2?

[jlewi]

SGTM

[wenzhel101]

FYI, I found this link is helpful.
Here is my proposal:

• Permanent Error:

  • 1: general errors
  • 2: Misuse of shell builtins
  • 126: Command invoked cannot execute
  • 127: Command not found
  • 128: Invalid argument to exit
  • 139: terminated by SIGSEGV(Invalid memory reference)

• Retryable Error:

  • 130: SIGINT(interrupted from keyboard CTRL-C)
  • 137: terminated by SIGKILL
  • 143: terminated by SIGTERM
  • 138: corresponds to SIGUSR1. Reserved in tf-operator for user specified retryable errors.

• Others are undefined, no guarantee about the behavior(currently handle them as permanent error).

I think it's hard to define the behavior for every sys signal, so it's better to start with known ones. What do you guys think?

[jlewi]

This looks good to me.

@gaocegege @ScorpioCPH thoughts?

[gaocegege]

SGTM

[wenzhel101]

Updated the PR to address the discussion. PTAL!

[jlewi]

Can we make this a utility function IsRetryableExitCode? I'd like to be able to use the same function in the v1alpha1 and v1alpha2 controllers.

[wenzhel101]

Done, Move it to pkg/util/train/train_util.go

[gaocegege]

LGTM, and I agree with jlewi to add a function in utility package.

[gaocegege]

/ok-to-test

[wenzhel101]

Addressed the comment, PTAL!

[gaocegege]

@jlewi LGTY?

[jlewi]

Thank you so much this is a great change.
Apologies it took so long. We are trying to ramp up as a community and need to grow the pool of reviewers.

/lgtm
/approver

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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 [jlewi]

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