Fix auto retries when out of memory.

[Gerhut]

Opened for CI.

[coveralls]

Coverage increased (+0.01%) to 52.639% when pulling 1d67e9b on qixcheng/rest-server/disable-quota-oom-retries into dd37f6c on master.

[hao1939]

Hi @Gerhut ,

Could you rebase on master and remove changes on Jenkinsfile?
You'd better test your fix on your own environment.

[hao1939]

Here too.

[Gerhut]

Sure, I'll revert the Jenkins test before being merged

[hao1939]

So docker container won't be removed automatically.
Where do you clean up stopped containers?

[Gerhut]

Line 62 in exit_handler

[yqwang-ms]

You can also refer k8s launcher:
Docker OOMKilled may be due to exceed the Container memory limit or the Container workload spike or OS memory pressure, so it may be Permanent or Transient, Internal or External, so it should be an unknown failure which will count against maxretrycount.
See: https://github.com/Microsoft/frameworkcontroller/blob/8adcef25f6b00f2c903cba9fb85e8c69d3a02d98/pkg/apis/frameworkcontroller/v1/constants.go#L181

[fanyangCS]

please treat it as higher priority.

[mzmssg]

@Gerhut
We can't guarantee exit_handle will be executed, all codes here are besteffort. So the container might be left on the host.

[Gerhut]

You're right, but there is an abnormal state when exit_handle does not be executed. The legacy containers removing should be rely on cleaner.

[zhiltsov-max]

@Gerhut, hi, I'm working out on a solution for the same issue. I think there some unaccounted circumstances can occur, which we should handle there. Let's see.

1. User task logs can be pretty big (50 MiB, for example), so I don't think printing the container logs in the exit handler is a good idea. They're also already should be printed in the stdout log file. Similarly, docker inspect - I don't think it is useful there.
2. We are running the container with --oom-score-adj, meaning "if we have little free memory, kill something in this container", as the value is inherited by child processes. Thus, in theory, in the case of OOM any container child process can be killed, including the bootstrap script and the init process. In practice, however, the user task or its children gets killed more often (probably, as they use more resources and get higher priority). This means we have not only to check exited container OOM status, but also kernel log using dmesg or something else.
3. BTW, dmesg has its own buffer, which not too big. It can overflow if, for example, the user task has spawned too many subprocesses - dmesg prints their memory usage, so for truly reliable output we probably should use something else, like system log. Anyway, we can expect this situation to be rare and stay with dmesg, hoping the killing log message is still there, as it probably has happened recently.
4. exit_handler is executed before the script exits, and it can exit in several circumstances - including the system shutdown (TERM signal) and (correct me if I'm wrong) after the stop button in the web UI has been pressed. We should provide some time for "graceful stop" of the container, propagating the request to the user command. I'd suggest using docker stop there. We'll also require the some signal handling and propagation in the docker bootstrap script (I have an example of the implementation). Otherwise, the user can lost task results.
5. As was said before, in some cases exit handler is not invoked. This is not a big deal if some kind of regular trash-reaping job is present. So, probably it's not a problem.
6. The user command can expect OOM conditions for a one of the processes - probably, we can leave it on its own in this case as the task will probably be specifically prepared for this. Example: there is some not important subtask (to say, intermediate model testing), which is expensive, but can be skipped and should not fail the task. The user command can expect the failure and handle it properly. So, probably it's not a problem.
7. I can't find debug_log function in the script, maybe it is a bug.

I suggest considering this variant:

function get_docker_container_id()
{
  docker inspect --format {{.Id}} $1
}

function check_docker_oom_killed()
{
  docker inspect --format {{.State.OOMKilled}} $1
}

function check_kernel_oom_killer()
{
  dmesg | grep -i "kill" | grep "/docker/$container_id"
}

function exit_handler()
{
  rc=$?
  printf "[DEBUG] EXIT signal received in yarn container, performing clean up action...\n"

  docker stop --time=${PAI_GRACEFUL_EXIT_TIMEOUT:-35} $docker_name >/dev/null

  # Here: container (i.e. init process) or the user task have been killed
  # 137, 143 - not handled TERM and KILL signals
  if [[ "$rc" = "137" || "$rc" = "143" || "$(check_docker_oom_killed $docker_name)" ]]; then
    if [ "$(check_docker_oom_killed $docker_name)" = "true" ] || \
         check_kernel_oom_killer $(get_docker_container_id $docker_name) >/dev/null; then \
      echo "[ERROR] One of the container processes has been killed by the OOM killer"
      rc=55 # Some undefined erroneous exit code
    fi
  fi

  docker rm -f $docker_name >/dev/null

  echo $rc > "/var/lib/hadoopdata/nm-local-dir/nmPrivate/$APP_ID/$CONTAINER_ID/$CONTAINER_ID.pid.exitcode"

  exit $rc
}

[fanyangCS]

@zhiltsov-max , great suggestion! We also think of some points you raised previously (e.g., oom-score).
But overall we plan to let k8s to solve all the hassles, instead of hacking it one-by-one (see #2195 for details).

We plan to do some prototyping after this sprint (March release). Hence this PR is likely a short-term solution.

Meanwhile, we would love to hear your comments on #2195.

And last but not least, we welcome your contribution.