[jira] [Created] (MESOS-2811) API hard to use, extend

["Cody Maloney (JIRA)" <ji...@apache.org>]

Cody Maloney created MESOS-2811:
-----------------------------------

             Summary: <process/subprocess.hpp> API hard to use, extend
                 Key: MESOS-2811
                 URL: https://issues.apache.org/jira/browse/MESOS-2811
             Project: Mesos
          Issue Type: Improvement
          Components: slave
    Affects Versions: 0.22.1
            Reporter: Cody Maloney


https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/subprocess.hpp

There are many overloads of subprocess() construction, a lot of them are very similar.

It passes environment in as an {{Option<std::map<std::string, std::string>>}} which isn't what stout's os::environment() returns. ({{hashmap<std::string, std::string> environment()}}. Ideally those should match for easy passing environments around + manipulating

It isn't possible to tell it not to copy in the environment of running process (Useful to isolate slave environments from the running process). This becomes critical when configuring mesos via environment variables. Currently mesos explicitly unsets LIBPROCESS_IP when launching new processes because that one is known to upset when mesos launches another libprocess based thing.

ExecEnv is just weird, it isn't great / modern C++, and results in a lot of unnecessary / useless copies of things as current, doesn't follow modern C++ interface standards.

The code is hard to read / follow:
{code}

  // Close the copies. We need to make sure that we do not close the
  // file descriptor assigned to stdin/stdout/stderr in case the
  // parent has closed stdin/stdout/stderr when calling this
  // function (in that case, a dup'ed file descriptor may have the
  // same file descriptor number as stdin/stdout/stderr).
  if (stdinFd[0] != STDIN_FILENO &&
      stdinFd[0] != STDOUT_FILENO &&
      stdinFd[0] != STDERR_FILENO) {
    while (::close(stdinFd[0]) == -1 && errno == EINTR);
  }
  if (stdoutFd[1] != STDIN_FILENO &&
      stdoutFd[1] != STDOUT_FILENO &&
      stdoutFd[1] != STDERR_FILENO) {
    while (::close(stdoutFd[1]) == -1 && errno == EINTR);
  }
  if (stderrFd[1] != STDIN_FILENO &&
      stderrFd[1] != STDOUT_FILENO &&
      stderrFd[1] != STDERR_FILENO) {
    while (::close(stderrFd[1]) == -1 && errno == EINTR);
  }
{code}
Why do we switch between fd[0] vs [1]? Why are we hand-coding "While EINTR" loops over and over? Doesn't stout have an os::close?

https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/subprocess.cpp#L165 -- os::execvpe() can fail for perfectly good reasons, we should really log the name of the command / info that was trying to be run. There shouldn't be a backtrace printed (which abort does).

A lot of the subprocess overloads re-implement needlessly functionality which the underlying exec() C APIs provide, using those apis instead of re-implementing all the variations would be a much better model.

Mesos doesn't use / need most of the subprocess overloads that exist. A lot of the usage patterns probably could / should be removed.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)