Re: Review Request 57973: Windows: Add `JobObjectManager` actor.

2017-03-29 Thread Andrew Schwartzmeyer 


> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/include/process/windows/jobobject.hpp
> > Lines 33 (patched)
> > 
> >
> > Can you move this comment down to the `extern` below?

Whoops.


> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/include/process/windows/jobobject.hpp
> > Lines 48-49 (patched)
> > 
> >
> > Try this:
> > ```
> > process::reap(pid)
> >   .onAny(defer(self(), &Self::cleanup, lambda::_1, pid));
> > ```

I'll give it a shot... Michael and I had a fun time with this.


> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/include/process/windows/jobobject.hpp
> > Lines 53 (patched)
> > 
> >
> > This shouldn't be a virtual function, presumably.

True!


> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/include/process/windows/jobobject.hpp
> > Lines 61-65 (patched)
> > 
> >
> > Hm... we need to clean these up eventually...
> > 
> > What sort of statistics will be gathered?  And how long does the job 
> > object need to live once gathered?

So, this ties into the resource limit APIs etc. Presumably a task can crash 
(process ends) but the status still needs to be able to report I/O used etc. To 
answer your question of what sort of statistics, I would need to move foward 
with the TaskInfo mapping issue. Once gathered, the job object can be fully 
closed.


> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/include/process/windows/jobobject.hpp
> > Lines 91-92 (patched)
> > 
> >
> > Outdated comment referring to the launcher here.

Whoops.


> On March 29, 2017, 9:36 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 1211 (patched)
> > 
> >
> > Perhaps add `(Windows only)` here.

Ah, yes.


- Andrew


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


On March 27, 2017, 10:47 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57973/
> ---
> 
> (Updated March 27, 2017, 10:47 p.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Bugs: MESOS-6868 and MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6868
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds a Windows specific actor for managing job objects.
> 
> A subprocess launched with the `ParentHook::CREATE_JOB()` is created
> within the context of a named Windows job object. The `JobObjectManager`
> takes ownership of the handle to the job object.
> 
> It is necessary to tie the lifetime of the job object to the actor by
> ownership of the open handle so that the job object can be queried for
> usage information even after the processes that were running within the
> job object have ended. These semantics were not changed; previously the
> same was achieved by leaking the handle and tieing it to the lifetime of
> the actual Mesos agent process, and implicitly depending on the
> operating system to close the open handle at the death of the process.
> 
> We ensure the proper death of the job object process group by defering a
> call to `cleanup()` to the process reaper for the given PID. This
> function uses the Windows system call to terminate the job object via
> `os::kill_job()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/windows/jobobject.hpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> f6ee24e2db43d63d91222549efee85421bbf9bf3 
>   3rdparty/libprocess/src/subprocess.cpp 
> 6dfb939ec151f724d383870a806ca3fc8fb0d931 
> 
> 
> Diff: https://reviews.apache.org/r/57973/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57973: Windows: Add `JobObjectManager` actor.

2017-03-29 Thread Joseph Wu 

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




3rdparty/libprocess/include/process/windows/jobobject.hpp
Lines 33 (patched)


Can you move this comment down to the `extern` below?



3rdparty/libprocess/include/process/windows/jobobject.hpp
Lines 48-49 (patched)


Try this:
```
process::reap(pid)
  .onAny(defer(self(), &Self::cleanup, lambda::_1, pid));
```



3rdparty/libprocess/include/process/windows/jobobject.hpp
Lines 53 (patched)


This shouldn't be a virtual function, presumably.



3rdparty/libprocess/include/process/windows/jobobject.hpp
Lines 61-65 (patched)


Hm... we need to clean these up eventually...

What sort of statistics will be gathered?  And how long does the job object 
need to live once gathered?



3rdparty/libprocess/include/process/windows/jobobject.hpp
Lines 91-92 (patched)


Outdated comment referring to the launcher here.



3rdparty/libprocess/src/process.cpp
Lines 1211 (patched)


Perhaps add `(Windows only)` here.


- Joseph Wu


On March 27, 2017, 3:47 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57973/
> ---
> 
> (Updated March 27, 2017, 3:47 p.m.)
> 
> 
> Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Bugs: MESOS-6868 and MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6868
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds a Windows specific actor for managing job objects.
> 
> A subprocess launched with the `ParentHook::CREATE_JOB()` is created
> within the context of a named Windows job object. The `JobObjectManager`
> takes ownership of the handle to the job object.
> 
> It is necessary to tie the lifetime of the job object to the actor by
> ownership of the open handle so that the job object can be queried for
> usage information even after the processes that were running within the
> job object have ended. These semantics were not changed; previously the
> same was achieved by leaking the handle and tieing it to the lifetime of
> the actual Mesos agent process, and implicitly depending on the
> operating system to close the open handle at the death of the process.
> 
> We ensure the proper death of the job object process group by defering a
> call to `cleanup()` to the process reaper for the given PID. This
> function uses the Windows system call to terminate the job object via
> `os::kill_job()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/windows/jobobject.hpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> f6ee24e2db43d63d91222549efee85421bbf9bf3 
>   3rdparty/libprocess/src/subprocess.cpp 
> 6dfb939ec151f724d383870a806ca3fc8fb0d931 
> 
> 
> Diff: https://reviews.apache.org/r/57973/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57973: Windows: Add `JobObjectManager` actor.

2017-03-27 Thread Andrew Schwartzmeyer 

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

(Updated March 27, 2017, 10:47 p.m.)


Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.


Changes
---

Add bugs.


Bugs: MESOS-6868 and MESOS-6892
https://issues.apache.org/jira/browse/MESOS-6868
https://issues.apache.org/jira/browse/MESOS-6892


Repository: mesos


Description
---

This commit adds a Windows specific actor for managing job objects.

A subprocess launched with the `ParentHook::CREATE_JOB()` is created
within the context of a named Windows job object. The `JobObjectManager`
takes ownership of the handle to the job object.

It is necessary to tie the lifetime of the job object to the actor by
ownership of the open handle so that the job object can be queried for
usage information even after the processes that were running within the
job object have ended. These semantics were not changed; previously the
same was achieved by leaking the handle and tieing it to the lifetime of
the actual Mesos agent process, and implicitly depending on the
operating system to close the open handle at the death of the process.

We ensure the proper death of the job object process group by defering a
call to `cleanup()` to the process reaper for the given PID. This
function uses the Windows system call to terminate the job object via
`os::kill_job()`.


Diffs
-

  3rdparty/libprocess/include/process/windows/jobobject.hpp PRE-CREATION 
  3rdparty/libprocess/src/process.cpp f6ee24e2db43d63d91222549efee85421bbf9bf3 
  3rdparty/libprocess/src/subprocess.cpp 
6dfb939ec151f724d383870a806ca3fc8fb0d931 


Diff: https://reviews.apache.org/r/57973/diff/1/


Testing (updated)
---

Testing done at end of chain.


Thanks,

Andrew Schwartzmeyer

Review Request 57973: Windows: Add `JobObjectManager` actor.

2017-03-27 Thread Andrew Schwartzmeyer 

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

Review request for mesos, John Kordich, Joseph Wu, Li Li, and Michael Park.


Repository: mesos


Description
---

This commit adds a Windows specific actor for managing job objects.

A subprocess launched with the `ParentHook::CREATE_JOB()` is created
within the context of a named Windows job object. The `JobObjectManager`
takes ownership of the handle to the job object.

It is necessary to tie the lifetime of the job object to the actor by
ownership of the open handle so that the job object can be queried for
usage information even after the processes that were running within the
job object have ended. These semantics were not changed; previously the
same was achieved by leaking the handle and tieing it to the lifetime of
the actual Mesos agent process, and implicitly depending on the
operating system to close the open handle at the death of the process.

We ensure the proper death of the job object process group by defering a
call to `cleanup()` to the process reaper for the given PID. This
function uses the Windows system call to terminate the job object via
`os::kill_job()`.


Diffs
-

  3rdparty/libprocess/include/process/windows/jobobject.hpp PRE-CREATION 
  3rdparty/libprocess/src/process.cpp f6ee24e2db43d63d91222549efee85421bbf9bf3 
  3rdparty/libprocess/src/subprocess.cpp 
6dfb939ec151f724d383870a806ca3fc8fb0d931 


Diff: https://reviews.apache.org/r/57973/diff/1/


Testing
---


Thanks,

Andrew Schwartzmeyer