mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 37336: Added `execute()` method to process::Subprocess
Date Sat, 21 Nov 2015 01:00:54 GMT


> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 53-75
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line53>
> >
> >     What's the motivation of storing this? Should the caller already have those
information?
> 
> Marco Massenzio wrote:
>     not necessarily - please note that this is executed asynchronously, so the caller
may have several of them running concurrently and having the invocation returned together
with the invocation will greatly simplifly the caller's code.
>     
>     The whole point of this patch is to make the caller's API less "low-level" and simplify
the life of those using this facility.
>     
>     I have, for example, a module that executes commands upon an HTTP request - having
the `invocation` returned to me with the result, greatly simplifies my code and enables me
to return a Response without waiting for execution.
> 
> Jie Yu wrote:
>     From what I can tell from the code you pasted below, doesn't seem to simplify the
caller's code *a lot*.

Point well taken, but I'm confused here, Jie - on the one hand, you here seem to be unhappy
it does not simplify enough, then below you suggest it simplifies too much.

The point here is - BenH and I went over the current usages of `subprocess()` and saw a common
pattern, and agreed on a way that would enable us to (a) cover 99% of usage patterns and (b)
avoid the same boilerplate code to be repeated over and over again.

Three months on, and countless reviews after, this is what we arrived at: it would be good,
at some point in time, to accept that this is **one** way of doing things, among countless
others - and I'd like this code committed.

Unless we all agree that it's either buggy/flawed/wrong, in which case, I'm happy to discard
this patch and re-start from scratch.


> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 119-130
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line119>
> >
> >     Why not just use a single `int status` field here. The users can use WEXITSTATUS
... to derive those information themself. This is also consistent with other places in the
code place.
> 
> Marco Massenzio wrote:
>     `Subprocess` does exactly that (actually, it uses an `Option<int>`) - this
leaves the caller with the burned to do all the legwork: again and again and again - it's
all over the code base.
>     
>     The point of this patch is to encapsulate that work, having it done (hopefully, properly)
in **one** place and avoid to have the same code sprinkled all over the codebase (and you
can tell, most places, by copy & paste).
> 
> Jie Yu wrote:
>     What if there's no returnCode (due to signal)? Should you use Option<int> returnCode
here? Similarly, should you use Option<int> for signal as well. What will the client
code be look like? Do you still need to check if (returnCode.isSome()) ... if (signal.isSome())
...
>     
>     Also
>     1) what if I want to know if WCOREDUMP is true or not, do you need to add another
boolean?
>     2) what if the reaper cannot reap the subprocess (i.e., we cannot get the exit status)?

Please see the usage in the tests (this same patch).
`returnCode` is EXIT_FAILURE and `signal` is non-zero - in case one cares (which most folks
don't seems to, but whatever).

if we can't get the exit status (`Subprocess::status` is `None()`) we return a `Failure` on
the returned future and an error message (which is what ultimately is done practically everywhere
`subprocess()` is used - while I guess I could've dreamt up tens of different scenarios, usage
is pretty consistent in Mesos codebase and is what we're trying to encapsulate here).

Note that `Subprocess::status` is still available to callers.


> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 328
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line328>
> >
> >     I don't like the name 'execute'. When you create the Subprocess instance, the
subprocss is already launched and exec'ed. This is rather waiting for the subprocess to terminate.
> 
> Marco Massenzio wrote:
>     This method most definitely does **not** wait.
>     
>     This is how one can use it as a caller (code simplified):
>     ```
>       auto s = process::subprocess(commandInfo.command(), args);
>     
>       if (s.isError()) {
>         LOG(ERROR) << "Could not spawn subprocess: " << s.error();
>         return http::ServiceUnavailable(s.error());
>       }
>     
>       store(s.get().pid());  // <-- needed to reconcile with GETs
>     
>       Future<CommandResult> result_ = s->execute();
>       result_.then([](const Future<CommandResult> &future) {
>             if (future.isFailed()) {
>               // mark the command as failed
>               return Nothing();
>             }
>             auto result = future.get();
>             // update status of job - use pid(); something equivalent to:
>             LOG(INFO) << "Result of '" << result.invocation.command <<
"'was: "
>                       << result.stdout();
>             return Nothing();
>           }).after(Seconds(30), [s](const Future<Nothing> &future) {
>             // update status of job to timed out; use `invocation` and `stdout`.
>             s.get().cleanup();
>             return Nothing();
>           });
>     
>       http::Response response = http::OK("{\"result\": \"OK\", \"pid\": \"" +
>                                          stringify(s.get().pid()) + "\"}");
>       response.headers["Content-Type"] = "application/json";
>       return response;
>     ```
> 
> Jie Yu wrote:
>     From the code above, can you just caputure commandInfo.command() in the lambda and
print it?
>     
>     ```
>     string command = commandInfo.command();
>     
>     result_.then([command](...) {
>       ...
>       LOG(INFO) << command << "...";
>     });
>     ```
> 
> Jie Yu wrote:
>     ALso, `auto s = process::subprocess(commandInfo.command(), args);` this line fork
and exec the subprocess. So having another `s->execute()` sounds very confusing to me.

I don't disagree - what would you suggest instead?

(note about the example above: it's contrived - one can also imagine storing the Future in
a map keyed by the id, and retrieve the outcome upon receiving a GET requests on the pid status;
there may be other scenarios where just passing the commandInfo and/or the args or whatever
in the lambda may be less desirable  -- but, again, this is *one* way of doing things, not
necessarily unique, and admittedly maybe not even *the* best).


> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 336-343
> > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line336>
> >
> >     Why introduce this method? I think the caller should be responsible for killing
the subprocess if needed.
> >     
> >     Also, os::killtree is in general not reliable and shouldn't be used in library
code.
> 
> Marco Massenzio wrote:
>     again, this is all about hiding implementation details from the caller and make their
life easier.
>     thanks for heads-up about `killtree()` - what would you suggest we use instead?
>     
>     when you say "unreliable" what do you mean, exactly?
>     that it may fail to actually kill the process or that it risks blowing up the app
or segfaulting or whatever?
> 
> Jie Yu wrote:
>     I guess I don't understand in what scenario should you issue killtree?
>     
>     Why it's not reliable? What if a process reparents to init? What if a process calls
setsid to escape from a session?

well, I'd ratherr have something that works 99.9% of the cases in which it tries to clean
up after itself, than nothing - obviously, if the process tries to do something clever to
escape / avoid termination, I'm really not sure what else could one do about it?

Again - what is your alternative suggestion?

In the apidoc I was being rather explicit about the "no-guarantees" nature of this method:
```
the caller may want to try and clean up the process.
```
we give no refunds for failures to cleanup :)
(we can of course be even more explicit about the issues you mentioned).


- Marco


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37336/#review107421
-----------------------------------------------------------


On Nov. 10, 2015, 8:51 p.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37336/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2015, 8:51 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3035
>     https://issues.apache.org/jira/browse/MESOS-3035
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The original API for `process::Subprocess` still left a lot of legwork
> to do for the caller; we have now added an `execute()` method
> that returns a `Future<Subprocess::Result>`.
>  
> `Subprocess::Result`, also introduced with this patch, contains useful information
> about the command invocation (an `Invocation` struct); the exit code; `stdout`;
> and, optionally, `stderr` too.
>  
> Once the Future completes, if successful, the caller will be able to retrieve
> stdout/stderr; whether the command was successful; and whether it received a signal
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/subprocess.hpp f17816e813d5efce1d3bb1ff1e1111850eeda3ba

>   3rdparty/libprocess/src/subprocess.cpp efe0018d0414c4137fd833c153eb262232e712bc 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp ac600a551fb1a7782ff33cce204b7819497ef54a

> 
> Diff: https://reviews.apache.org/r/37336/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> (also tested functionality with an anonymous module that exposes an `/execute` endpoint
and runs arbitrary commands, asynchronously,
> on an Agent)
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message