Re: Review Request 52462: Ported ReviewBot script to Windows.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [52462, 52461]

Failed command: ./support/apply-review.sh -n -r 52462

Error:
Traceback (most recent call last):
  File "support/apply-reviews.py", line 331, in 
reviewboard()
  File "support/apply-reviews.py", line 310, in reviewboard
apply_review()
  File "support/apply-reviews.py", line 121, in apply_review
fetch_patch()
  File "support/apply-reviews.py", line 128, in fetch_patch
ctx = ssl.create_default_context()
AttributeError: 'module' object has no attribute 'create_default_context'
Error in atexit._run_exitfuncs:
Traceback (most recent call last):
  File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
func(*targs, **kargs)
  File "support/apply-reviews.py", line 119, in 
atexit.register(lambda: os.remove('%s.patch' % patch_id()))
OSError: [Errno 2] No such file or directory: '52462.patch'
Error in sys.exitfunc:
Traceback (most recent call last):
  File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
func(*targs, **kargs)
  File "support/apply-reviews.py", line 119, in 
atexit.register(lambda: os.remove('%s.patch' % patch_id()))
OSError: [Errno 2] No such file or directory: '52462.patch'

Full log: https://builds.apache.org/job/mesos-reviewbot/15476/console

- Mesos ReviewBot


On Oct. 1, 2016, 12:34 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52462/
> ---
> 
> (Updated Oct. 1, 2016, 12:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Daniel Pravat, Artem Harutyunyan, 
> Alex Clemmer, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes which script ReviewBot uses to apply reviews and 
> what script ReviewBot runs if on Windows.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py f14951b753ffbc1a939d1991d7bb901741efe388 
> 
> Diff: https://reviews.apache.org/r/52462/diff/
> 
> 
> Testing
> ---
> 
> On Windows:
> python support/verify_reviews.py a b 1
> // ^ Fake username and password, so as to not post any reviews.
> 
> Tested successful build and unsuccessful build (introduced a syntax error in 
> libprocess).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 52466: Simplified the destroy logic in MesosContainerizer.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Oct. 1, 2016, 1:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52466/
> ---
> 
> (Updated Oct. 1, 2016, 1:21 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The return value of '_destroy' is not necessary. This patch also
> addressed an issue where the destroy can be discarded if it gets a
> discard from the caller since 'await' currently will transition the
> returned future to DISCARDED (which should be fixed).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 9e834fd916ea86730e2406ce31440cf9ebc3ba9d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> fc53b4ebab89565205b3fdec53fc7c3d594e7db3 
> 
> Diff: https://reviews.apache.org/r/52466/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52308: Add variable user to handle switchUser passed from executor.

[Sivaram Kannan]

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

(Updated Oct. 1, 2016, 1:35 a.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Add variable user to handle switchUser passed from executor.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.hpp 
f906a167f8897385af5f54e1e77cb790121a 

Diff: https://reviews.apache.org/r/52308/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52310: Switch the uid of the binary if a user is passed from the lib_logrotate.

[Sivaram Kannan]

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

(Updated Oct. 1, 2016, 1:34 a.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Switch the uid of the binary if a user is passed from the lib_logrotate.


Diffs
-

  src/slave/container_loggers/logrotate.cpp 
431bc3cbb54e94359078e4dae0b32ad301393640 

Diff: https://reviews.apache.org/r/52310/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52311: Pass the user value from executor of switch_user flag is set.

[Sivaram Kannan]

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

(Updated Oct. 1, 2016, 1:34 a.m.)


Review request for mesos and Joseph Wu.


Summary (updated)
-

Pass the user value from executor of switch_user flag is set.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description (updated)
---

Pass the user value from executor of switch_user flag is set.


Diffs (updated)
-

  src/slave/slave.cpp 958a33b962d6385e37af9372d9f8edd1f70bf676 

Diff: https://reviews.apache.org/r/52311/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52310: Switch the uid of the binary if a user is passed from the lib_logrotate.

[Sivaram Kannan]

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

(Updated Oct. 1, 2016, 1:33 a.m.)


Review request for mesos and Joseph Wu.


Summary (updated)
-

Switch the uid of the binary if a user is passed from the lib_logrotate.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description (updated)
---

Switch the uid of the binary if a user is passed from the lib_logrotate.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.cpp 
431bc3cbb54e94359078e4dae0b32ad301393640 

Diff: https://reviews.apache.org/r/52310/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52309: Pass the user variable from library to binary.

[Sivaram Kannan]

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

(Updated Oct. 1, 2016, 1:33 a.m.)


Review request for mesos and Joseph Wu.


Summary (updated)
-

Pass the user variable from library to binary.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description (updated)
---

Pass the user variable from library to binary.


Diffs (updated)
-

  src/slave/container_loggers/lib_logrotate.cpp 
0ca2b3d7dbb57c11c0740aed3914a6b75329af99 

Diff: https://reviews.apache.org/r/52309/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Review Request 52466: Simplified the destroy logic in MesosContainerizer.

[Jie Yu]

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

Review request for mesos, Benjamin Mahler and Gilbert Song.


Repository: mesos


Description
---

The return value of '_destroy' is not necessary. This patch also
addressed an issue where the destroy can be discarded if it gets a
discard from the caller since 'await' currently will transition the
returned future to DISCARDED (which should be fixed).


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
9e834fd916ea86730e2406ce31440cf9ebc3ba9d 
  src/slave/containerizer/mesos/containerizer.cpp 
fc53b4ebab89565205b3fdec53fc7c3d594e7db3 

Diff: https://reviews.apache.org/r/52466/diff/


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 52465: Fixed the race in master update slave.

[Guangya Liu]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

The reason that we need `updateSlave` first and then rescind offer
is because of a race condition case: there may be a batch allocation
triggered between rescind offer and `updateSlave`. In this case, the
order will be rescind offer -> batch allocation -> update slave. This
order will cause some issues when the oversubscribed resources was
shrinked.

Suppose the oversubscribed resources was shrinked from 2 to 1, then
after rescind offer finished, the batch allocation will allocate the
old 2 oversubscribed resources again, then update slave will update
the total oversubscribed resources to 1. This will cause the agent
host have some time overcommitted due to the tasks can still use 2
oversubscribed resources but not 1 oversubscribed resources, once
the tasks using the 2 oversubscribed resources finished, everything
goes back.

If we update slave first then rescind offer, the order will be update
slave -> batch allocation -> rescind offer, this order will have no
problem when shrinking resources. Suppose the oversubscribed resources
was shrinked from 2 to 1, then update slave will update total
oversubscribed resources to 1 directly, then the batch allocation will
not allocate any oversubscribed resources since there are more
allocated than total oversubscribed resources, then rescind offer
will rescind all offers using oversubscribed resources. This will
not lead the agent host to be overcommitted.


Diffs
-

  src/master/master.cpp c83ee2f9fa05372748ff5056229fbe2bf06bfabb 

Diff: https://reviews.apache.org/r/52465/diff/


Testing
---

Test with https://reviews.apache.org/r/51621/

```
./bin/mesos-tests.sh  
--gtest_filter="OversubscriptionTest.RescindRevocableOffer" --gtest_repeat=100
```


Thanks,

Guangya Liu

Re: Review Request 52438: Reduce log level to verbose MESOS-6295.

[Joseph Wu]

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


Ship it!




Note: I'm going to tweak the commit message ("description" field) with more 
background before committing.

- Joseph Wu


On Sept. 30, 2016, 12:17 p.m., Kunal Thakar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52438/
> ---
> 
> (Updated Sept. 30, 2016, 12:17 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Zhitao Li.
> 
> 
> Bugs: MESOS-6295
> https://issues.apache.org/jira/browse/MESOS-6295
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reduce log level to VLOG to reduce logging noise. Discussion in 
> https://issues.apache.org/jira/browse/MESOS-6295
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 958a33b962d6385e37af9372d9f8edd1f70bf676 
> 
> Diff: https://reviews.apache.org/r/52438/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>

Review Request 52462: Ported ReviewBot script to Windows.

[Joseph Wu]

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

Review request for mesos, Benjamin Hindman, Daniel Pravat, Artem Harutyunyan, 
Alex Clemmer, and Vinod Kone.


Repository: mesos


Description
---

Changes which script ReviewBot uses to apply reviews and 
what script ReviewBot runs if on Windows.


Diffs
-

  support/verify_reviews.py f14951b753ffbc1a939d1991d7bb901741efe388 

Diff: https://reviews.apache.org/r/52462/diff/


Testing
---

On Windows:
python support/verify_reviews.py a b 1
// ^ Fake username and password, so as to not post any reviews.

Tested successful build and unsuccessful build (introduced a syntax error in 
libprocess).


Thanks,

Joseph Wu

Review Request 52461: Ported apply-reviews.py script to Windows.

[Joseph Wu]

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

Review request for mesos, Benjamin Hindman, Daniel Pravat, Artem Harutyunyan, 
Alex Clemmer, and Vinod Kone.


Repository: mesos


Description
---

This removes some of the shell-specific things this script was doing
in favor of native Python, which works on Windows.


Diffs
-

  support/apply-reviews.py 8dc1817943d7681b90d64c10bcd82acd7b7cef9d 

Diff: https://reviews.apache.org/r/52461/diff/


Testing
---

On Windows and OSX:
python support/apply-reviews.py -n -r <#>


Thanks,

Joseph Wu

Re: Review Request 52450: Simplied the buildPath logic.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Sept. 30, 2016, 5:03 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52450/
> ---
> 
> (Updated Sept. 30, 2016, 5:03 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simplied the buildPath logic.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/paths.cpp 
> c34bd3340ca6507595e59fa3f87108eb6297a7ca 
> 
> Diff: https://reviews.apache.org/r/52450/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52450: Simplied the buildPath logic.

[Jie Yu]

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

(Updated Oct. 1, 2016, 12:03 a.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

Addressed comments from Kevin


Repository: mesos


Description
---

Simplied the buildPath logic.


Diffs (updated)
-

  src/slave/containerizer/mesos/paths.cpp 
c34bd3340ca6507595e59fa3f87108eb6297a7ca 

Diff: https://reviews.apache.org/r/52450/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 52450: Simplied the buildPath logic.

[Kevin Klues]

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




src/slave/containerizer/mesos/paths.cpp (lines 50 - 52)


I would rather be explicit in the PREFIX case even though it's the same as 
the JOIN case. It's also a little confusing that above we have the order:
```
PREFIX
SUFFIX
JOIN
```

but here we have:
```
PREFIX
JOIN
SUFFIX
```

These should be in the same order (and mimic the order of the enum).


- Kevin Klues


On Sept. 30, 2016, 9:35 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52450/
> ---
> 
> (Updated Sept. 30, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simplied the buildPath logic.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/paths.cpp 
> c34bd3340ca6507595e59fa3f87108eb6297a7ca 
> 
> Diff: https://reviews.apache.org/r/52450/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52460: Ignored cleanup request for unknown containers in posix isolators.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Sept. 30, 2016, 4:18 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52460/
> ---
> 
> (Updated Sept. 30, 2016, 4:18 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To be consistent with other isolators. If the operator changes
> isolation flags, the cleanup can be called for a container that is
> unknown to the new isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/posix.hpp 
> 26c693a9a228d41f38854698496612b063baa68c 
> 
> Diff: https://reviews.apache.org/r/52460/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52459: Ignored error in posix launcher destroy if the container is unknown.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Sept. 30, 2016, 4:17 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52459/
> ---
> 
> (Updated Sept. 30, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is consistent to what we do in linux launcher. A container can be
> destroyed in launcher but not yet fully cleaned up in isolators. If
> agent restarts, we should still allow the cleanup of those containers.
> So launcher should ignore destroy request for unknown containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launcher.cpp 
> 5d1410da40128c365c581ced11eff954a1f5627e 
> 
> Diff: https://reviews.apache.org/r/52459/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Kevin Klues]

> On Sept. 30, 2016, 11:12 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/paths.cpp, lines 146-149
> > 
> >
> > termination could be None, right? Calling error will an assertion 
> > failure.

Yes, of course. Stupid mistake.


- Kevin


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


On Sept. 30, 2016, 11:08 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52446/
> ---
> 
> (Updated Sept. 30, 2016, 11:08 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6287
> https://issues.apache.org/jira/browse/MESOS-6287
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, when a nested container was being destroyed, it's runtime
> directory was being deleted (just the same as a top-level container).
> However, this meant that calling 'wait()' on a previously terminated
> nested container would return 'None()' since its status had already
> been reaped. The problem with this, however, is that this will cause
> an entire pod to be terminated since it thinks that the container it
> is calling wait on cannot be found.
> 
> To fix this, we leave the runtime directory of nested containers
> around until their top-level containers are destroyed. Additionally,
> we checkpiont the entire termination state of the nested container
> into its runtime directory, so that subsequent calls to 'wait()' can
> retrieve the full termination state for the lifetime of the top-level
> container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 522d2c37229b07b66a0824c3e246c32f8d803b10 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/nested_container_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52446/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Kevin Klues]

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



- Kevin Klues


On Sept. 30, 2016, 11:08 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52446/
> ---
> 
> (Updated Sept. 30, 2016, 11:08 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6287
> https://issues.apache.org/jira/browse/MESOS-6287
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, when a nested container was being destroyed, it's runtime
> directory was being deleted (just the same as a top-level container).
> However, this meant that calling 'wait()' on a previously terminated
> nested container would return 'None()' since its status had already
> been reaped. The problem with this, however, is that this will cause
> an entire pod to be terminated since it thinks that the container it
> is calling wait on cannot be found.
> 
> To fix this, we leave the runtime directory of nested containers
> around until their top-level containers are destroyed. Additionally,
> we checkpiont the entire termination state of the nested container
> into its runtime directory, so that subsequent calls to 'wait()' can
> retrieve the full termination state for the lifetime of the top-level
> container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 522d2c37229b07b66a0824c3e246c32f8d803b10 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/nested_container_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52446/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 52460: Ignored cleanup request for unknown containers in posix isolators.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

To be consistent with other isolators. If the operator changes
isolation flags, the cleanup can be called for a container that is
unknown to the new isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/posix.hpp 
26c693a9a228d41f38854698496612b063baa68c 

Diff: https://reviews.apache.org/r/52460/diff/


Testing
---

make check


Thanks,

Jie Yu

Review Request 52459: Ignored error in posix launcher destroy if the container is unknown.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Gilbert Song.


Repository: mesos


Description
---

This is consistent to what we do in linux launcher. A container can be
destroyed in launcher but not yet fully cleaned up in isolators. If
agent restarts, we should still allow the cleanup of those containers.
So launcher should ignore destroy request for unknown containers.


Diffs
-

  src/slave/containerizer/mesos/launcher.cpp 
5d1410da40128c365c581ced11eff954a1f5627e 

Diff: https://reviews.apache.org/r/52459/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Jie Yu]

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




src/slave/containerizer/mesos/paths.cpp (lines 146 - 149)


termination could be None, right? Calling error will an assertion failure.


- Jie Yu


On Sept. 30, 2016, 11:08 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52446/
> ---
> 
> (Updated Sept. 30, 2016, 11:08 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6287
> https://issues.apache.org/jira/browse/MESOS-6287
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, when a nested container was being destroyed, it's runtime
> directory was being deleted (just the same as a top-level container).
> However, this meant that calling 'wait()' on a previously terminated
> nested container would return 'None()' since its status had already
> been reaped. The problem with this, however, is that this will cause
> an entire pod to be terminated since it thinks that the container it
> is calling wait on cannot be found.
> 
> To fix this, we leave the runtime directory of nested containers
> around until their top-level containers are destroyed. Additionally,
> we checkpiont the entire termination state of the nested container
> into its runtime directory, so that subsequent calls to 'wait()' can
> retrieve the full termination state for the lifetime of the top-level
> container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 522d2c37229b07b66a0824c3e246c32f8d803b10 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/nested_container_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52446/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Jie Yu]

> On Sept. 30, 2016, 9:52 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 711-713
> > 
> >
> > Can you introduce a helper in paths.hpp|cpp:
> > 
> > ```
> > paths::getContainerTerminationPath(...);
> > ```
> 
> Kevin Klues wrote:
> Sure. None of the other files have this currently though (i.e. pid, 
> status).
> Maybe we should commit this patch without this helper and then do a 
> separate patch that adds this helper for all checkpointed files.
> 
> I will hold off on this one until you confirm.

OK, let's follow up with a patch :)


- Jie


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


On Sept. 30, 2016, 11:08 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52446/
> ---
> 
> (Updated Sept. 30, 2016, 11:08 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6287
> https://issues.apache.org/jira/browse/MESOS-6287
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, when a nested container was being destroyed, it's runtime
> directory was being deleted (just the same as a top-level container).
> However, this meant that calling 'wait()' on a previously terminated
> nested container would return 'None()' since its status had already
> been reaped. The problem with this, however, is that this will cause
> an entire pod to be terminated since it thinks that the container it
> is calling wait on cannot be found.
> 
> To fix this, we leave the runtime directory of nested containers
> around until their top-level containers are destroyed. Additionally,
> we checkpiont the entire termination state of the nested container
> into its runtime directory, so that subsequent calls to 'wait()' can
> retrieve the full termination state for the lifetime of the top-level
> container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 522d2c37229b07b66a0824c3e246c32f8d803b10 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/nested_container_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52446/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Kevin Klues]

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

(Updated Sept. 30, 2016, 11:08 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Updated to address Jie's comments.


Bugs: MESOS-6287
https://issues.apache.org/jira/browse/MESOS-6287


Repository: mesos


Description
---

Previously, when a nested container was being destroyed, it's runtime
directory was being deleted (just the same as a top-level container).
However, this meant that calling 'wait()' on a previously terminated
nested container would return 'None()' since its status had already
been reaped. The problem with this, however, is that this will cause
an entire pod to be terminated since it thinks that the container it
is calling wait on cannot be found.

To fix this, we leave the runtime directory of nested containers
around until their top-level containers are destroyed. Additionally,
we checkpiont the entire termination state of the nested container
into its runtime directory, so that subsequent calls to 'wait()' can
retrieve the full termination state for the lifetime of the top-level
container.


Diffs (updated)
-

  src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
  src/slave/containerizer/mesos/containerizer.cpp 
522d2c37229b07b66a0824c3e246c32f8d803b10 
  src/slave/containerizer/mesos/paths.hpp 
1051c219c55253d03199045b6d2f43377ae93e53 
  src/slave/containerizer/mesos/paths.cpp 
6c6b4dcc39fbc00485552caab88457918e622e08 
  src/tests/containerizer/nested_container_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/52446/diff/


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Kevin Klues]

> On Sept. 30, 2016, 9:52 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 711-713
> > 
> >
> > Can you introduce a helper in paths.hpp|cpp:
> > 
> > ```
> > paths::getContainerTerminationPath(...);
> > ```

Sure. None of the other files have this currently though (i.e. pid, status).
Maybe we should commit this patch without this helper and then do a separate 
patch that adds this helper for all checkpointed files.

I will hold off on this one until you confirm.


> On Sept. 30, 2016, 9:52 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1699-1705
> > 
> >
> > Let's do that only for nested container.

It should still work as is (since top-level containers will simply never have 
this file), but it's probably better to be explicit. I'll change it.


> On Sept. 30, 2016, 9:52 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 2197-2203
> > 
> >
> > NO need for this? I think checkpoint supports a Protobuf Message 
> > directly.

Cool. I didn't realize this.


> On Sept. 30, 2016, 9:52 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 2211
> > 
> >
> > Totally orthogonol issue: I am wondering if we should prevent people 
> > from creating nested container under a legacy container?

It would definitely be a bug if they did because no runtime directory will 
exist for legacy containers. It would be a simple check in launch to verify 
this.


> On Sept. 30, 2016, 9:52 p.m., Jie Yu wrote:
> > src/tests/containerizer/nested_container_tests.cpp, line 32
> > 
> >
> > Please avoid this. Use 'using' explicitly

Yeah, this must have been copied in. I will be more careful next time.


> On Sept. 30, 2016, 9:52 p.m., Jie Yu wrote:
> > src/tests/containerizer/nested_container_tests.cpp, line 59
> > 
> >
> > s/false/true/

Are we making this tru for all tests going forward? I see a mix of false/true 
in existing tests.


> On Sept. 30, 2016, 9:52 p.m., Jie Yu wrote:
> > src/tests/containerizer/nested_container_tests.cpp, lines 95-97
> > 
> >
> > No need for this.

Copy and paste leftovers...


> On Sept. 30, 2016, 9:52 p.m., Jie Yu wrote:
> > src/tests/containerizer/nested_container_tests.cpp, line 116
> > 
> >
> > This looks redundant? The fact the first 'wait' returns means that 
> > container has been destroyed.

Yeah, it is. I only wrote it to be explicit (it actually always ends up logging 
that the container is already destroyed). Maybe it's more confusing this way 
though.


- Kevin


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


On Sept. 30, 2016, 11:08 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52446/
> ---
> 
> (Updated Sept. 30, 2016, 11:08 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6287
> https://issues.apache.org/jira/browse/MESOS-6287
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, when a nested container was being destroyed, it's runtime
> directory was being deleted (just the same as a top-level container).
> However, this meant that calling 'wait()' on a previously terminated
> nested container would return 'None()' since its status had already
> been reaped. The problem with this, however, is that this will cause
> an entire pod to be terminated since it thinks that the container it
> is calling wait on cannot be found.
> 
> To fix this, we leave the runtime directory of nested containers
> around until their top-level containers are destroyed. Additionally,
> we checkpiont the entire termination state of the nested container
> into its runtime directory, so that subsequent calls to 'wait()' can
> retrieve the full termination state for the lifetime of the top-level
> container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 522d2c37229b07b66a0824c3e246c32f8d803b10 
>   

Re: Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/containerizer.cpp (lines 711 - 713)


Can you introduce a helper in paths.hpp|cpp:

```
paths::getContainerTerminationPath(...);
```



src/slave/containerizer/mesos/containerizer.cpp (lines 1699 - 1705)


Let's do that only for nested container.



src/slave/containerizer/mesos/containerizer.cpp (line 1708)


Failed to get container termination file: ...



src/slave/containerizer/mesos/containerizer.cpp (lines 2197 - 2203)


NO need for this? I think checkpoint supports a Protobuf Message directly.



src/slave/containerizer/mesos/containerizer.cpp (line 2211)


Totally orthogonol issue: I am wondering if we should prevent people from 
creating nested container under a legacy container?



src/slave/containerizer/mesos/paths.cpp (lines 142 - 154)


you can use protobuf::read



src/tests/containerizer/nested_container_tests.cpp (line 32)


Please avoid this. Use 'using' explicitly



src/tests/containerizer/nested_container_tests.cpp (line 48)


2 lines apart



src/tests/containerizer/nested_container_tests.cpp (line 59)


s/false/true/



src/tests/containerizer/nested_container_tests.cpp (lines 95 - 97)


No need for this.



src/tests/containerizer/nested_container_tests.cpp (line 116)


This looks redundant? The fact the first 'wait' returns means that 
container has been destroyed.


- Jie Yu


On Sept. 30, 2016, 9:27 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52446/
> ---
> 
> (Updated Sept. 30, 2016, 9:27 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6287
> https://issues.apache.org/jira/browse/MESOS-6287
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, when a nested container was being destroyed, it's runtime
> directory was being deleted (just the same as a top-level container).
> However, this meant that calling 'wait()' on a previously terminated
> nested container would return 'None()' since its status had already
> been reaped. The problem with this, however, is that this will cause
> an entire pod to be terminated since it thinks that the container it
> is calling wait on cannot be found.
> 
> To fix this, we leave the runtime directory of nested containers
> around until their top-level containers are destroyed. Additionally,
> we checkpiont the entire termination state of the nested container
> into its runtime directory, so that subsequent calls to 'wait()' can
> retrieve the full termination state for the lifetime of the top-level
> container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 522d2c37229b07b66a0824c3e246c32f8d803b10 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/nested_container_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52446/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 52450: Simplied the buildPath logic.

[Jie Yu]

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

Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

Simplied the buildPath logic.


Diffs
-

  src/slave/containerizer/mesos/paths.cpp 
c34bd3340ca6507595e59fa3f87108eb6297a7ca 

Diff: https://reviews.apache.org/r/52450/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Kevin Klues]

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

(Updated Sept. 30, 2016, 9:27 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Updated based on Vinod's comments.


Bugs: MESOS-6287
https://issues.apache.org/jira/browse/MESOS-6287


Repository: mesos


Description
---

Previously, when a nested container was being destroyed, it's runtime
directory was being deleted (just the same as a top-level container).
However, this meant that calling 'wait()' on a previously terminated
nested container would return 'None()' since its status had already
been reaped. The problem with this, however, is that this will cause
an entire pod to be terminated since it thinks that the container it
is calling wait on cannot be found.

To fix this, we leave the runtime directory of nested containers
around until their top-level containers are destroyed. Additionally,
we checkpiont the entire termination state of the nested container
into its runtime directory, so that subsequent calls to 'wait()' can
retrieve the full termination state for the lifetime of the top-level
container.


Diffs (updated)
-

  src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
  src/slave/containerizer/mesos/containerizer.cpp 
522d2c37229b07b66a0824c3e246c32f8d803b10 
  src/slave/containerizer/mesos/paths.hpp 
1051c219c55253d03199045b6d2f43377ae93e53 
  src/slave/containerizer/mesos/paths.cpp 
6c6b4dcc39fbc00485552caab88457918e622e08 
  src/tests/containerizer/nested_container_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/52446/diff/


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Kevin Klues]

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




src/tests/containerizer/nested_container_tests.cpp (line 117)


The `AWAIT_READY()` on the next line does that already by waiting on 
`container->termination.future()` (which is only set in `_destroy()`


- Kevin Klues


On Sept. 30, 2016, 8:57 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52446/
> ---
> 
> (Updated Sept. 30, 2016, 8:57 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6287
> https://issues.apache.org/jira/browse/MESOS-6287
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, when a nested container was being destroyed, it's runtime
> directory was being deleted (just the same as a top-level container).
> However, this meant that calling 'wait()' on a previously terminated
> nested container would return 'None()' since its status had already
> been reaped. The problem with this, however, is that this will cause
> an entire pod to be terminated since it thinks that the container it
> is calling wait on cannot be found.
> 
> To fix this, we leave the runtime directory of nested containers
> around until their top-level containers are destroyed. Additionally,
> we checkpiont the entire termination state of the nested container
> into its runtime directory, so that subsequent calls to 'wait()' can
> retrieve the full termination state for the lifetime of the top-level
> container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 522d2c37229b07b66a0824c3e246c32f8d803b10 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/nested_container_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52446/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52415: Extended buildPath to support more modes.

[Jie Yu]

> On Sept. 30, 2016, 5:20 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/mesos/paths.cpp, lines 46-64
> > 
> >
> > I think the code is fine, but suggestion on simplifying:
> > 
> > const string path = buildPath(containerId.parent(), separator, mode);
> > 
> > switch (mode) {
> >   case PREFIX:
> >   case JOIN: return path::join(path, seperator, containerId.value());
> >   case SUFFIX: return path::join(path, containerId.value(), separator);
> >   default: UNREACHABLE();
> > }
> 
> Jie Yu wrote:
> You need termination condition for the recursion.
> 
> Benjamin Hindman wrote:
> The termination condition is the previous code that you already have, 
> that's why I didn't highlight that part of the code.

Oh, ic. Didn't notice that. Let me follow up with a patch.


- Jie


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


On Sept. 30, 2016, 5:14 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52415/
> ---
> 
> (Updated Sept. 30, 2016, 5:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extended buildPath to support more modes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1bce077e4aa97425b9cbdf8576a5dd52851c044e 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/mesos_containerizer_paths_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52415/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52444: Fixed the MesosContainerizer destroy issue.

[Jie Yu]

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

(Updated Sept. 30, 2016, 9:14 p.m.)


Review request for mesos and Gilbert Song.


Bugs: MESOS-6300
https://issues.apache.org/jira/browse/MESOS-6300


Repository: mesos


Description
---

When doing recursive destroy, we should return the collected future of
nested container destroys. Intead, we should fail the corresponding
termination and return that termination if nested container destroys
failed.

In addition, we cannot remove 'Container' struct from the internal map
when the destroy of a nested container failed. This is to ensure that
the top level container do not proceed with destroy if any of its
nested container failed to destroy.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
c542902714ccf2a951bf73fef62902f553d7ea1d 
  src/slave/containerizer/mesos/containerizer.cpp 
522d2c37229b07b66a0824c3e246c32f8d803b10 

Diff: https://reviews.apache.org/r/52444/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 52415: Extended buildPath to support more modes.

[Benjamin Hindman]

> On Sept. 30, 2016, 5:20 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/mesos/paths.cpp, lines 46-64
> > 
> >
> > I think the code is fine, but suggestion on simplifying:
> > 
> > const string path = buildPath(containerId.parent(), separator, mode);
> > 
> > switch (mode) {
> >   case PREFIX:
> >   case JOIN: return path::join(path, seperator, containerId.value());
> >   case SUFFIX: return path::join(path, containerId.value(), separator);
> >   default: UNREACHABLE();
> > }
> 
> Jie Yu wrote:
> You need termination condition for the recursion.

The termination condition is the previous code that you already have, that's 
why I didn't highlight that part of the code.


- Benjamin


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


On Sept. 30, 2016, 5:14 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52415/
> ---
> 
> (Updated Sept. 30, 2016, 5:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extended buildPath to support more modes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1bce077e4aa97425b9cbdf8576a5dd52851c044e 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/mesos_containerizer_paths_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52415/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52448: Fixed a CNI test that might cause a failed container destroy.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On Sept. 30, 2016, 8:45 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52448/
> ---
> 
> (Updated Sept. 30, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a CNI test that might cause a failed container destroy.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 0d611c196870b6adabea52a48abcd344c8dad5d1 
> 
> Diff: https://reviews.apache.org/r/52448/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Vinod Kone]

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


Fix it, then Ship it!




LGTM.


src/slave/containerizer/mesos/containerizer.cpp (line 1716)


`destroy()`



src/tests/containerizer/nested_container_tests.cpp (line 117)


can you wait until `destroy()` is executed to make sure?



src/tests/containerizer/nested_container_tests.cpp (line 130)


ditto.


- Vinod Kone


On Sept. 30, 2016, 8:57 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52446/
> ---
> 
> (Updated Sept. 30, 2016, 8:57 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6287
> https://issues.apache.org/jira/browse/MESOS-6287
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, when a nested container was being destroyed, it's runtime
> directory was being deleted (just the same as a top-level container).
> However, this meant that calling 'wait()' on a previously terminated
> nested container would return 'None()' since its status had already
> been reaped. The problem with this, however, is that this will cause
> an entire pod to be terminated since it thinks that the container it
> is calling wait on cannot be found.
> 
> To fix this, we leave the runtime directory of nested containers
> around until their top-level containers are destroyed. Additionally,
> we checkpiont the entire termination state of the nested container
> into its runtime directory, so that subsequent calls to 'wait()' can
> retrieve the full termination state for the lifetime of the top-level
> container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 522d2c37229b07b66a0824c3e246c32f8d803b10 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/nested_container_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52446/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52444: Fixed the MesosContainerizer destroy issue.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Sept. 30, 2016, 1:32 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52444/
> ---
> 
> (Updated Sept. 30, 2016, 1:32 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When doing recursive destroy, we should return the collected future of
> nested container destroys. Intead, we should fail the corresponding
> termination and return that termination if nested container destroys
> failed.
> 
> In addition, we cannot remove 'Container' struct from the internal map
> when the destroy of a nested container failed. This is to ensure that
> the top level container do not proceed with destroy if any of its
> nested container failed to destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> c542902714ccf2a951bf73fef62902f553d7ea1d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 522d2c37229b07b66a0824c3e246c32f8d803b10 
> 
> Diff: https://reviews.apache.org/r/52444/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Kevin Klues]

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

(Updated Sept. 30, 2016, 8:57 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Added JIRA link


Bugs: MESOS-6287
https://issues.apache.org/jira/browse/MESOS-6287


Repository: mesos


Description
---

Previously, when a nested container was being destroyed, it's runtime
directory was being deleted (just the same as a top-level container).
However, this meant that calling 'wait()' on a previously terminated
nested container would return 'None()' since its status had already
been reaped. The problem with this, however, is that this will cause
an entire pod to be terminated since it thinks that the container it
is calling wait on cannot be found.

To fix this, we leave the runtime directory of nested containers
around until their top-level containers are destroyed. Additionally,
we checkpiont the entire termination state of the nested container
into its runtime directory, so that subsequent calls to 'wait()' can
retrieve the full termination state for the lifetime of the top-level
container.


Diffs
-

  src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
  src/slave/containerizer/mesos/containerizer.cpp 
522d2c37229b07b66a0824c3e246c32f8d803b10 
  src/slave/containerizer/mesos/paths.hpp 
1051c219c55253d03199045b6d2f43377ae93e53 
  src/slave/containerizer/mesos/paths.cpp 
6c6b4dcc39fbc00485552caab88457918e622e08 
  src/tests/containerizer/nested_container_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/52446/diff/


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Review Request 52446: Updated 'destroy()' to checkpoint termination state of nested container.

[Kevin Klues]

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

Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Repository: mesos


Description
---

Previously, when a nested container was being destroyed, it's runtime
directory was being deleted (just the same as a top-level container).
However, this meant that calling 'wait()' on a previously terminated
nested container would return 'None()' since its status had already
been reaped. The problem with this, however, is that this will cause
an entire pod to be terminated since it thinks that the container it
is calling wait on cannot be found.

To fix this, we leave the runtime directory of nested containers
around until their top-level containers are destroyed. Additionally,
we checkpiont the entire termination state of the nested container
into its runtime directory, so that subsequent calls to 'wait()' can
retrieve the full termination state for the lifetime of the top-level
container.


Diffs
-

  src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
  src/slave/containerizer/mesos/containerizer.cpp 
522d2c37229b07b66a0824c3e246c32f8d803b10 
  src/slave/containerizer/mesos/paths.hpp 
1051c219c55253d03199045b6d2f43377ae93e53 
  src/slave/containerizer/mesos/paths.cpp 
6c6b4dcc39fbc00485552caab88457918e622e08 
  src/tests/containerizer/nested_container_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/52446/diff/


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 52415: Extended buildPath to support more modes.

[Jie Yu]

> On Sept. 30, 2016, 3:36 p.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/paths.hpp, lines 57-60
> > 
> >
> > To keep similar semantics as before, maybe it makes sense to set 
> > defaults for separator and mode:
> > 
> > ```
> > std::string buildPath(
> > const ContainerID& containerId,
> > const std::string& separator = "",
> > const Mode& mode = Mode::JOIN);
> > ```

In fact, I like it being more explicit :)


> On Sept. 30, 2016, 3:36 p.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/paths.hpp, line 41
> > 
> >
> > Should we use an `enum class` here? Without it, the enum will not be 
> > typed properly under `Mode`, so you could write things like:
> > 
> > `containerizer::paths::JOIN`
> > 
> > (which you currently do, and is totally legal)
> > 
> > With `enum class` you are forced to write:
> > 
> > `containerizer::paths::Mode::JOIN`
> > 
> > Not sure which is better, but it would be good to be explicit about it.

This is consistent with the Mode we have in 'strings'. But I agree with you 
probably having Mode::JOIN is better.

I'll follow up with a patch.


- Jie


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


On Sept. 30, 2016, 5:14 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52415/
> ---
> 
> (Updated Sept. 30, 2016, 5:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extended buildPath to support more modes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1bce077e4aa97425b9cbdf8576a5dd52851c044e 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/mesos_containerizer_paths_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52415/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52415: Extended buildPath to support more modes.

[Jie Yu]

> On Sept. 30, 2016, 5:20 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/mesos/paths.cpp, lines 46-64
> > 
> >
> > I think the code is fine, but suggestion on simplifying:
> > 
> > const string path = buildPath(containerId.parent(), separator, mode);
> > 
> > switch (mode) {
> >   case PREFIX:
> >   case JOIN: return path::join(path, seperator, containerId.value());
> >   case SUFFIX: return path::join(path, containerId.value(), separator);
> >   default: UNREACHABLE();
> > }

You need termination condition for the recursion.


- Jie


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


On Sept. 30, 2016, 5:14 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52415/
> ---
> 
> (Updated Sept. 30, 2016, 5:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extended buildPath to support more modes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1bce077e4aa97425b9cbdf8576a5dd52851c044e 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/mesos_containerizer_paths_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52415/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52445: Used posix launcher for multiple slave tests.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Sept. 30, 2016, 1:33 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52445/
> ---
> 
> (Updated Sept. 30, 2016, 1:33 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Linux launcher does not work when there are multiple slaves running on
> a single host because it uses cgroups which is global.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_offers_tests.cpp 
> 6f93cb69876bc45420d1867dc6917727d4d457ff 
> 
> Diff: https://reviews.apache.org/r/52445/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 52448: Fixed a CNI test that might cause a failed container destroy.

[Jie Yu]

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

Review request for mesos, Avinash sridharan and Gilbert Song.


Repository: mesos


Description
---

Fixed a CNI test that might cause a failed container destroy.


Diffs
-

  src/tests/containerizer/cni_isolator_tests.cpp 
0d611c196870b6adabea52a48abcd344c8dad5d1 

Diff: https://reviews.apache.org/r/52448/diff/


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 52442: Added logging for MesosContainerizer internal state.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Sept. 30, 2016, 1:28 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52442/
> ---
> 
> (Updated Sept. 30, 2016, 1:28 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a steaming function for MesosContainerizer internal
> state. It'll be nice to know the state before a container transition
> into 'DESTROYING' state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> c542902714ccf2a951bf73fef62902f553d7ea1d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 522d2c37229b07b66a0824c3e246c32f8d803b10 
> 
> Diff: https://reviews.apache.org/r/52442/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 52445: Used posix launcher for multiple slave tests.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

Linux launcher does not work when there are multiple slaves running on
a single host because it uses cgroups which is global.


Diffs
-

  src/tests/resource_offers_tests.cpp 6f93cb69876bc45420d1867dc6917727d4d457ff 

Diff: https://reviews.apache.org/r/52445/diff/


Testing
---

make check


Thanks,

Jie Yu

Review Request 52444: Fixed the MesosContainerizer destroy issue.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

When doing recursive destroy, we should return the collected future of
nested container destroys. Intead, we should fail the corresponding
termination and return that termination if nested container destroys
failed.

In addition, we cannot remove 'Container' struct from the internal map
when the destroy of a nested container failed. This is to ensure that
the top level container do not proceed with destroy if any of its
nested container failed to destroy.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
c542902714ccf2a951bf73fef62902f553d7ea1d 
  src/slave/containerizer/mesos/containerizer.cpp 
522d2c37229b07b66a0824c3e246c32f8d803b10 

Diff: https://reviews.apache.org/r/52444/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 52440: Fixed master to properly handle pending tasks.

[Vinod Kone]

> On Sept. 30, 2016, 8:23 p.m., Anand Mazumdar wrote:
> > src/master/master.cpp, line 3689
> > 
> >
> > Can we log here before we remove the tasks?

as discussed offline, we don't log about removing pending tasks in anywhere, so 
just doing it here seems inconsistent.


- Vinod


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


On Sept. 30, 2016, 8:21 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52440/
> ---
> 
> (Updated Sept. 30, 2016, 8:21 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6299
> https://issues.apache.org/jira/browse/MESOS-6299
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pending tasks are always removed from frameworks `pending` map,
> irrespective of whether the task launch is successful or not.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 756ab546851952bc1de24b1f469d232237b1c01c 
>   src/tests/master_authorization_tests.cpp 
> 74422c7a185e50eef32a045e55abe5d9ba83bce3 
>   src/tests/master_validation_tests.cpp 
> 16c5773aa44016f923e00cb348ded6b8c46d4b4b 
> 
> Diff: https://reviews.apache.org/r/52440/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Also confirmed that all the updated tests fail without the fix.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 52442: Added logging for MesosContainerizer internal state.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

This patch adds a steaming function for MesosContainerizer internal
state. It'll be nice to know the state before a container transition
into 'DESTROYING' state.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
c542902714ccf2a951bf73fef62902f553d7ea1d 
  src/slave/containerizer/mesos/containerizer.cpp 
522d2c37229b07b66a0824c3e246c32f8d803b10 

Diff: https://reviews.apache.org/r/52442/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 52440: Fixed master to properly handle pending tasks.

[Anand Mazumdar]

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


Fix it, then Ship it!




LGTM.


src/master/master.cpp (line 3689)


Can we log here before we remove the tasks?


- Anand Mazumdar


On Sept. 30, 2016, 8:21 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52440/
> ---
> 
> (Updated Sept. 30, 2016, 8:21 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6299
> https://issues.apache.org/jira/browse/MESOS-6299
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pending tasks are always removed from frameworks `pending` map,
> irrespective of whether the task launch is successful or not.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 756ab546851952bc1de24b1f469d232237b1c01c 
>   src/tests/master_authorization_tests.cpp 
> 74422c7a185e50eef32a045e55abe5d9ba83bce3 
>   src/tests/master_validation_tests.cpp 
> 16c5773aa44016f923e00cb348ded6b8c46d4b4b 
> 
> Diff: https://reviews.apache.org/r/52440/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Also confirmed that all the updated tests fail without the fix.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 52440: Fixed master to properly handle pending tasks.

[Vinod Kone]

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

(Updated Sept. 30, 2016, 8:21 p.m.)


Review request for mesos, Anand Mazumdar and Benjamin Mahler.


Changes
---

Updated couple task group validation tests as well.


Bugs: MESOS-6299
https://issues.apache.org/jira/browse/MESOS-6299


Repository: mesos


Description
---

Pending tasks are always removed from frameworks `pending` map,
irrespective of whether the task launch is successful or not.


Diffs (updated)
-

  src/master/master.cpp 756ab546851952bc1de24b1f469d232237b1c01c 
  src/tests/master_authorization_tests.cpp 
74422c7a185e50eef32a045e55abe5d9ba83bce3 
  src/tests/master_validation_tests.cpp 
16c5773aa44016f923e00cb348ded6b8c46d4b4b 

Diff: https://reviews.apache.org/r/52440/diff/


Testing (updated)
---

make check

Also confirmed that all the updated tests fail without the fix.


Thanks,

Vinod Kone

Re: Review Request 52439: Add Zhitao Li to contributor.yaml.

[Xiaojian Huang]

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


Ship it!




lgtm

- Xiaojian Huang


On Sept. 30, 2016, 8:08 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52439/
> ---
> 
> (Updated Sept. 30, 2016, 8:08 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Zhitao Li to contributor.yaml.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 578f3fdb3d3c469a2fe1274dcf665850cd1b285d 
> 
> Diff: https://reviews.apache.org/r/52439/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52439: Add Zhitao Li to contributor.yaml.

[Zhitao Li]

> On Sept. 30, 2016, 7:30 p.m., Vinod Kone wrote:
> > Ship It!
> 
> Xiaojian Huang wrote:
> Please hold off a bit :)

Added affiliation and uber.com email.


- Zhitao


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


On Sept. 30, 2016, 8:08 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52439/
> ---
> 
> (Updated Sept. 30, 2016, 8:08 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Zhitao Li to contributor.yaml.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 578f3fdb3d3c469a2fe1274dcf665850cd1b285d 
> 
> Diff: https://reviews.apache.org/r/52439/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52439: Add Zhitao Li to contributor.yaml.

[Xiaojian Huang]

> On Sept. 30, 2016, 7:30 p.m., Vinod Kone wrote:
> > Ship It!

Please hold off a bit :)


- Xiaojian


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


On Sept. 30, 2016, 7:18 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52439/
> ---
> 
> (Updated Sept. 30, 2016, 7:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Zhitao Li to contributor.yaml.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 578f3fdb3d3c469a2fe1274dcf665850cd1b285d 
> 
> Diff: https://reviews.apache.org/r/52439/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Review Request 52440: Fixed master to properly handle pending tasks.

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar and Benjamin Mahler.


Bugs: MESOS-6299
https://issues.apache.org/jira/browse/MESOS-6299


Repository: mesos


Description
---

Pending tasks are always removed from frameworks `pending` map,
irrespective of whether the task launch is successful or not.


Diffs
-

  src/master/master.cpp 756ab546851952bc1de24b1f469d232237b1c01c 
  src/tests/master_authorization_tests.cpp 
74422c7a185e50eef32a045e55abe5d9ba83bce3 
  src/tests/master_validation_tests.cpp 
16c5773aa44016f923e00cb348ded6b8c46d4b4b 

Diff: https://reviews.apache.org/r/52440/diff/


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 52439: Add Zhitao Li to contributor.yaml.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 30, 2016, 7:18 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52439/
> ---
> 
> (Updated Sept. 30, 2016, 7:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Zhitao Li to contributor.yaml.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 578f3fdb3d3c469a2fe1274dcf665850cd1b285d 
> 
> Diff: https://reviews.apache.org/r/52439/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52438: Reduce log level to verbose MESOS-6295.

[Zhitao Li]

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


Ship it!




Ship It!

- Zhitao Li


On Sept. 30, 2016, 7:17 p.m., Kunal Thakar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52438/
> ---
> 
> (Updated Sept. 30, 2016, 7:17 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Zhitao Li.
> 
> 
> Bugs: MESOS-6295
> https://issues.apache.org/jira/browse/MESOS-6295
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reduce log level to VLOG to reduce logging noise. Discussion in 
> https://issues.apache.org/jira/browse/MESOS-6295
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 958a33b962d6385e37af9372d9f8edd1f70bf676 
> 
> Diff: https://reviews.apache.org/r/52438/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>

Review Request 52438: Reduce log level to verbose MESOS-6295.

[Kunal Thakar]

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

Review request for mesos, Joseph Wu and Zhitao Li.


Bugs: MESOS-6295
https://issues.apache.org/jira/browse/MESOS-6295


Repository: mesos


Description
---

Reduce log level to VLOG to reduce logging noise. Discussion in 
https://issues.apache.org/jira/browse/MESOS-6295


Diffs
-

  src/slave/slave.cpp 958a33b962d6385e37af9372d9f8edd1f70bf676 

Diff: https://reviews.apache.org/r/52438/diff/


Testing
---


Thanks,

Kunal Thakar

Re: Review Request 52432: Reenabled the 'HealthCheckTest.GracePeriod' test.

[haosdent huang]

> On Sept. 30, 2016, 5:23 p.m., haosdent huang wrote:
> > It is flaky in slow machine because advance Clock before statusUpdate. May 
> > refer to https://reviews.apache.org/r/47089/diff/3#index_header about how 
> > to fix it.

Never mind, I discard that stale patch.


- haosdent


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


On Sept. 30, 2016, 6:12 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52432/
> ---
> 
> (Updated Sept. 30, 2016, 6:12 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test was disabled in 2014 because it was considered flaky. I
> updated it so that it runs quicker and ran it 1000 times without a
> single failure.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52432/diff/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --verbose --gtest_filter="HealthCheckTest.GracePeriod" 
> --break-on-error --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52435: Fixed children list issue and log in containerizer destroy.

[Gilbert Song]

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

(Updated Sept. 30, 2016, 11:20 a.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, and Vinod Kone.


Changes
---

Added 'return' for default case.


Repository: mesos


Description
---

This patch fixed the following issues:
1. Current the children list does not erase the nested container
   when the nested container is destroyed. We need to maintain
   this information.
2. Removed redundant 'Destroying nested container' log.
3. Added log in containerizer destory for container state change.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
c542902714ccf2a951bf73fef62902f553d7ea1d 
  src/slave/containerizer/mesos/containerizer.cpp 
522d2c37229b07b66a0824c3e246c32f8d803b10 

Diff: https://reviews.apache.org/r/52435/diff/


Testing
---

make check


Thanks,

Gilbert Song

Review Request 52435: Fixed children list issue and log in containerizer destroy.

[Gilbert Song]

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

Review request for mesos, Artem Harutyunyan, Jie Yu, and Vinod Kone.


Repository: mesos


Description
---

This patch fixed the following issues:
1. Current the children list does not erase the nested container
   when the nested container is destroyed. We need to maintain
   this information.
2. Removed redundant 'Destroying nested container' log.
3. Added log in containerizer destory for container state change.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
c542902714ccf2a951bf73fef62902f553d7ea1d 
  src/slave/containerizer/mesos/containerizer.cpp 
522d2c37229b07b66a0824c3e246c32f8d803b10 

Diff: https://reviews.apache.org/r/52435/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 52432: Reenabled the 'HealthCheckTest.GracePeriod' test.

[Gastón Kleiman]

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

(Updated Sept. 30, 2016, 6:12 p.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


Changes
---

Added changes done by haosdent in https://reviews.apache.org/r/47089. So this 
RR is basically the same as 47089, I'm fine with dropping this one in favour of 
haosdent's commit.


Repository: mesos


Description
---

This test was disabled in 2014 because it was considered flaky. I
updated it so that it runs quicker and ran it 1000 times without a
single failure.


Diffs (updated)
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

Diff: https://reviews.apache.org/r/52432/diff/


Testing
---

bin/mesos-tests.sh --verbose --gtest_filter="HealthCheckTest.GracePeriod" 
--break-on-error --gtest_repeat=1000


Thanks,

Gastón Kleiman

Re: Review Request 52434: Improved handling of tmp file creation in health check test.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On Sept. 30, 2016, 5:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52434/
> ---
> 
> (Updated Sept. 30, 2016, 5:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The tmp file used in the HealthStatusChange test is now created inside
> the tmp directory created for the test suite. This ensures that it will
> be cleaned up on tear down.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52434/diff/
> 
> 
> Testing
> ---
> 
> make check GTEST_FILTER="HealthCheckTest.*" V=0
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52432: Reenabled the 'HealthCheckTest.GracePeriod' test.

[haosdent huang]

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



It is flaky in slow machine because advance Clock before statusUpdate. May 
refer to https://reviews.apache.org/r/47089/diff/3#index_header about how to 
fix it.

- haosdent huang


On Sept. 30, 2016, 5:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52432/
> ---
> 
> (Updated Sept. 30, 2016, 5:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test was disabled in 2014 because it was considered flaky. I
> updated it so that it runs quicker and ran it 1000 times without a
> single failure.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52432/diff/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --verbose --gtest_filter="HealthCheckTest.GracePeriod" 
> --break-on-error --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52415: Extended buildPath to support more modes.

[Benjamin Hindman]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/paths.cpp (lines 46 - 64)


I think the code is fine, but suggestion on simplifying:

const string path = buildPath(containerId.parent(), separator, mode);

switch (mode) {
  case PREFIX:
  case JOIN: return path::join(path, seperator, containerId.value());
  case SUFFIX: return path::join(path, containerId.value(), separator);
  default: UNREACHABLE();
}


- Benjamin Hindman


On Sept. 30, 2016, 5:14 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52415/
> ---
> 
> (Updated Sept. 30, 2016, 5:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extended buildPath to support more modes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1bce077e4aa97425b9cbdf8576a5dd52851c044e 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/mesos_containerizer_paths_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52415/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52431: Reduced boilerplate from health check tests.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On Sept. 30, 2016, 5:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52431/
> ---
> 
> (Updated Sept. 30, 2016, 5:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reduced boilerplate from health check tests.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52431/diff/
> 
> 
> Testing
> ---
> 
> make check GTEST_FILTER="HealthCheckTest.*" V=0
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Review Request 52432: Reenabled the 'HealthCheckTest.GracePeriod' test.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov and haosdent huang.


Repository: mesos


Description
---

This test was disabled in 2014 because it was considered flaky. I
updated it so that it runs quicker and ran it 1000 times without a
single failure.


Diffs
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

Diff: https://reviews.apache.org/r/52432/diff/


Testing
---

bin/mesos-tests.sh --verbose --gtest_filter="HealthCheckTest.GracePeriod" 
--break-on-error --gtest_repeat=1000


Thanks,

Gastón Kleiman

Review Request 52434: Improved handling of tmp file creation in health check test.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov and haosdent huang.


Repository: mesos


Description
---

The tmp file used in the HealthStatusChange test is now created inside
the tmp directory created for the test suite. This ensures that it will
be cleaned up on tear down.


Diffs
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

Diff: https://reviews.apache.org/r/52434/diff/


Testing
---

make check GTEST_FILTER="HealthCheckTest.*" V=0


Thanks,

Gastón Kleiman

Review Request 52431: Reduced boilerplate from health check tests.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov and haosdent huang.


Repository: mesos


Description
---

Reduced boilerplate from health check tests.


Diffs
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

Diff: https://reviews.apache.org/r/52431/diff/


Testing
---

make check GTEST_FILTER="HealthCheckTest.*" V=0


Thanks,

Gastón Kleiman

Review Request 52431: Reduced boilerplate from health check tests.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov and haosdent huang.


Repository: mesos


Description
---

Reduced boilerplate from health check tests.


Diffs
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

Diff: https://reviews.apache.org/r/52431/diff/


Testing
---

make check GTEST_FILTER="HealthCheckTest.*" V=0


Thanks,

Gastón Kleiman

Re: Review Request 52415: Extended buildPath to support more modes.

[Kevin Klues]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/linux_launcher.cpp (lines 584 - 587)


If you agree with the comments below, you can leave off the `JOIN` here.



src/slave/containerizer/mesos/paths.hpp (line 41)


Should we use an `enum class` here? Without it, the enum will not be typed 
properly under `Mode`, so you could write things like:

`containerizer::paths::JOIN`

(which you currently do, and is totally legal)

With `enum class` you are forced to write:

`containerizer::paths::Mode::JOIN`

Not sure which is better, but it would be good to be explicit about it.



src/slave/containerizer/mesos/paths.hpp (lines 57 - 60)


To keep similar semantics as before, maybe it makes sense to set defaults 
for separator and mode:

```
std::string buildPath(
const ContainerID& containerId,
const std::string& separator = "",
const Mode& mode = Mode::JOIN);
```


- Kevin Klues


On Sept. 30, 2016, 5:14 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52415/
> ---
> 
> (Updated Sept. 30, 2016, 5:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extended buildPath to support more modes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1bce077e4aa97425b9cbdf8576a5dd52851c044e 
>   src/slave/containerizer/mesos/paths.hpp 
> 1051c219c55253d03199045b6d2f43377ae93e53 
>   src/slave/containerizer/mesos/paths.cpp 
> 6c6b4dcc39fbc00485552caab88457918e622e08 
>   src/tests/containerizer/mesos_containerizer_paths_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52415/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52416: Made 'parse' method in linux launcher take a real cgroup path.

[Jie Yu]

> On Sept. 30, 2016, 3:19 p.m., Benjamin Hindman wrote:
> >
> 
> Benjamin Hindman wrote:
> FYI, if we factor out `parse` so others can use it as well we won't have 
> `flags` (so we can't remove `flags.cgroups_root` from the prefix), which is 
> why this function originally wasn't a `LinuxLauncher` member function and we 
> removed it before.

Yup, i know :) Yesterday, i was thinking about this and i think we might want 
to add a 'parse' to paths.hpp|cpp (next to 'buildPath', basically a reverse 
operation to buildPath).


- Jie


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


On Sept. 30, 2016, 5:15 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52416/
> ---
> 
> (Updated Sept. 30, 2016, 5:15 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we pass a 'stripped' cgroup path the parse function. This
> is a little confusing to the reader. This patch move the 'strip' logic
> to the parse function so that its parameter makes more sense.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1bce077e4aa97425b9cbdf8576a5dd52851c044e 
> 
> Diff: https://reviews.apache.org/r/52416/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52308: Add variable user to handle switchUser passed from executor.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 3:30 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Add variable user to handle switchUser passed from executor.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.hpp 
f906a167f8897385af5f54e1e77cb790121a 

Diff: https://reviews.apache.org/r/52308/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52310: Switch the uid to passed user before spawning the process.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 3:29 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Switch the uid to passed user before spawning the process.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.cpp 
431bc3cbb54e94359078e4dae0b32ad301393640 

Diff: https://reviews.apache.org/r/52310/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52309: Set the user to stderr-logger binary too.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 3:29 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Set the user to stderr-logger binary too.


Diffs (updated)
-

  src/slave/container_loggers/lib_logrotate.cpp 
0ca2b3d7dbb57c11c0740aed3914a6b75329af99 

Diff: https://reviews.apache.org/r/52309/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52311: Set the executorInfo user before the custom executor code flow.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 3:28 p.m.)


Review request for mesos and Joseph Wu.


Summary (updated)
-

Set the executorInfo user before the custom executor code flow.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description (updated)
---

Set the executorInfo user before the custom executor code flow.


Diffs (updated)
-

  src/slave/slave.cpp 958a33b962d6385e37af9372d9f8edd1f70bf676 

Diff: https://reviews.apache.org/r/52311/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52416: Made 'parse' method in linux launcher take a real cgroup path.

[Benjamin Hindman]

> On Sept. 30, 2016, 3:19 p.m., Benjamin Hindman wrote:
> >

FYI, if we factor out `parse` so others can use it as well we won't have 
`flags` (so we can't remove `flags.cgroups_root` from the prefix), which is why 
this function originally wasn't a `LinuxLauncher` member function and we 
removed it before.


- Benjamin


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


On Sept. 30, 2016, 5:15 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52416/
> ---
> 
> (Updated Sept. 30, 2016, 5:15 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we pass a 'stripped' cgroup path the parse function. This
> is a little confusing to the reader. This patch move the 'strip' logic
> to the parse function so that its parameter makes more sense.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1bce077e4aa97425b9cbdf8576a5dd52851c044e 
> 
> Diff: https://reviews.apache.org/r/52416/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52416: Made 'parse' method in linux launcher take a real cgroup path.

[Benjamin Hindman]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/linux_launcher.cpp (line 276)


Fix the comment now.


- Benjamin Hindman


On Sept. 30, 2016, 5:15 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52416/
> ---
> 
> (Updated Sept. 30, 2016, 5:15 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we pass a 'stripped' cgroup path the parse function. This
> is a little confusing to the reader. This patch move the 'strip' logic
> to the parse function so that its parameter makes more sense.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1bce077e4aa97425b9cbdf8576a5dd52851c044e 
> 
> Diff: https://reviews.apache.org/r/52416/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52310: Switch the uid to passed user before spawning the process.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 2:49 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Switch the uid to passed user before spawning the process.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.cpp 
431bc3cbb54e94359078e4dae0b32ad301393640 

Diff: https://reviews.apache.org/r/52310/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52308: Add variable user to handle switchUser passed from executor.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 2:50 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Add variable user to handle switchUser passed from executor.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.hpp 
f906a167f8897385af5f54e1e77cb790121a 

Diff: https://reviews.apache.org/r/52308/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52309: Set the user to stderr-logger binary too.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 2:50 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Set the user to stderr-logger binary too.


Diffs (updated)
-

  src/slave/container_loggers/lib_logrotate.cpp 
0ca2b3d7dbb57c11c0740aed3914a6b75329af99 

Diff: https://reviews.apache.org/r/52309/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52311: Fix removed empty lines from slave.cpp.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 2:48 p.m.)


Review request for mesos and Joseph Wu.


Summary (updated)
-

Fix removed empty lines from slave.cpp.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description (updated)
---

Fix removed empty lines from slave.cpp.


Diffs (updated)
-

  src/slave/slave.cpp 958a33b962d6385e37af9372d9f8edd1f70bf676 

Diff: https://reviews.apache.org/r/52311/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Review Request 52424: Fix removed empty lines from slave.cpp.

[Sivaram Kannan]

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

Review request for mesos.


Repository: mesos


Description
---

Fix removed empty lines from slave.cpp.


Diffs
-


Diff: https://reviews.apache.org/r/52424/diff/


Testing
---


Thanks,

Sivaram Kannan

Re: Review Request 52310: Switch the uid to passed user before spawning the process.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 2:38 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Switch the uid to passed user before spawning the process.


Diffs
-

  src/slave/container_loggers/logrotate.cpp 
431bc3cbb54e94359078e4dae0b32ad301393640 

Diff: https://reviews.apache.org/r/52310/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52311: Set the executorInfo user before the custom executor code flow.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 2:38 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Set the executorInfo user before the custom executor code flow.


Diffs
-

  src/slave/slave.cpp 958a33b962d6385e37af9372d9f8edd1f70bf676 

Diff: https://reviews.apache.org/r/52311/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52309: Set the user to stderr-logger binary too.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 2:38 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Set the user to stderr-logger binary too.


Diffs
-

  src/slave/container_loggers/lib_logrotate.cpp 
0ca2b3d7dbb57c11c0740aed3914a6b75329af99 

Diff: https://reviews.apache.org/r/52309/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52311: Set the executorInfo user before the custom executor code flow.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 2:17 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Set the executorInfo user before the custom executor code flow.


Diffs (updated)
-

  src/slave/slave.cpp 958a33b962d6385e37af9372d9f8edd1f70bf676 

Diff: https://reviews.apache.org/r/52311/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52309: Set the user to stderr-logger binary too.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 2:17 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description
---

Set the user to stderr-logger binary too.


Diffs (updated)
-

  src/slave/container_loggers/lib_logrotate.cpp 
0ca2b3d7dbb57c11c0740aed3914a6b75329af99 

Diff: https://reviews.apache.org/r/52309/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52308: Add variable user to handle switchUser passed from executor.

[Sivaram Kannan]

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

(Updated Sept. 30, 2016, 2:16 p.m.)


Review request for mesos and Joseph Wu.


Bugs: MESOS-5856
https://issues.apache.org/jira/browse/MESOS-5856


Repository: mesos


Description (updated)
---

Add variable user to handle switchUser passed from executor.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.hpp 
f906a167f8897385af5f54e1e77cb790121a 

Diff: https://reviews.apache.org/r/52308/diff/


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52412: Supported logger with nested containers in Mesos Containerizer.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52412]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 30, 2016, 5:19 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52412/
> ---
> 
> (Updated Sept. 30, 2016, 5:19 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-6290
> https://issues.apache.org/jira/browse/MESOS-6290
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, there are two issues in mesos containerizer using logger
> for nested contaienrs:
> 1. An empty executorinfo is passed to logger when launching a nested
>container, it would potentially break some logger modules if any
>module tries to access the required proto field (e.g., executorId).
> 2. The logger does not reocver the nested containers yet in
>MesosContainerizer::recover.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 522d2c37229b07b66a0824c3e246c32f8d803b10 
> 
> Diff: https://reviews.apache.org/r/52412/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52416: Made 'parse' method in linux launcher take a real cgroup path.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52415, 52416]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 30, 2016, 5:15 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52416/
> ---
> 
> (Updated Sept. 30, 2016, 5:15 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we pass a 'stripped' cgroup path the parse function. This
> is a little confusing to the reader. This patch move the 'strip' logic
> to the parse function so that its parameter makes more sense.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 1bce077e4aa97425b9cbdf8576a5dd52851c044e 
> 
> Diff: https://reviews.apache.org/r/52416/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51784: Supported merging the launch command from isolators.

[Benjamin Bannier]

> On Sept. 12, 2016, 10:16 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1109
> > 
> >
> > Could we just have `arguments` MergeFrom? Because singular field (e.g., 
> > `value`) may be overwritten by some user modules accidentally.
> 
> Benjamin Bannier wrote:
> +1
> 
> Taking this a step further, given own function to merge a list of 
> `CommandInfo` into a single `CommandInfo`, e.g., `[CommandInfo] -> 
> Try(CommandInfo)`, would give us a chance to sanitize (e.g., are all 
> non-ignored scalar fields compatible?), and explicitly drop ignored elements 
> in a single, exposed spot.

What is the status on this?

I still believe it is important to verify that isolators play nice with each 
other as writers of isolators have very little control over this. I have 
sketched this on top of this change over here, 
https://github.com/bbannier/mesos/tree/r/51784. Currently that code emits 
`VLOG` messages whenever `CommandInfo`s from `ContainerLaunchInfo`s not as 
compatible as we wish, but I think ultimately we'd want to fail hard for such 
scenarios.


- Benjamin


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


On Sept. 11, 2016, 2:17 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51784/
> ---
> 
> (Updated Sept. 11, 2016, 2:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gilbert Song.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we only allow one isolator to specify the launch command
> for the container. This is not ideal because multiple isolators might
> want to add some flags to the command executor. For instance, the
> 'docker/runtime' isolator wants to specify '--task_command' and
> '--working_directory', and 'linux/capabilities' isolator wants to
> specify '--capabilities'.
> 
> This patch changes the semantics so that launch command from isolators
> are merged. However, it is isolator's responsibility to make sure the
> merged command is a valid command.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 89b7e8db38916d69d9b2d4fe305d4397b0859a10 
> 
> Diff: https://reviews.apache.org/r/51784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 52357: Improved handling of health check failures within the grace period.

[Gastón Kleiman]

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

(Updated Sept. 30, 2016, 11:29 a.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


Changes
---

Followed the suggestions from AlexR and Haosdent, which led to way fewer 
boilerplate in the test.


Bugs: MESOS-6170
https://issues.apache.org/jira/browse/MESOS-6170


Repository: mesos


Description
---

The health check library would ignore all failures until after the end
of the grace period.

This behaviour was misleading. With the changes in this commit, once a
health check succeeds for the first time, the grace period rule for
failures is not be applied any more.


Diffs (updated)
-

  src/health-check/health_checker.cpp ea93132f2a5d4828c75005f102eddc4c3131599d 
  src/tests/health_check_tests.cpp e6b02f23dc1b1b84381ab0af0e3df5918b60ae40 

Diff: https://reviews.apache.org/r/52357/diff/


Testing
---

Manual tests using Marathon.
Added a new unit test and verified that it fails without the change, but 
succeeds with it.


Thanks,

Gastón Kleiman

Re: Review Request 51009: Collect throttle related cpu.stat for Docker Containerizer.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51009]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 29, 2016, 8:37 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51009/
> ---
> 
> (Updated Sept. 29, 2016, 8:37 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, Jie Yu, and Steve 
> Niemitz.
> 
> 
> Bugs: MESOS-6031
> https://issues.apache.org/jira/browse/MESOS-6031
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Collect throttle related cpu.stat for Docker Containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 1d27761fcb3f310cf954d45ed41f4c89ecbd5982 
> 
> Diff: https://reviews.apache.org/r/51009/diff/
> 
> 
> Testing
> ---
> 
> Run agent with cfs quota enabled, and observe that throttle related metrics 
> are in `/containers` and `/monitoring/statistics`
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52357: Improved handling of health check failures within the grace period.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52357]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 29, 2016, 3:30 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52357/
> ---
> 
> (Updated Sept. 29, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-6170
> https://issues.apache.org/jira/browse/MESOS-6170
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The health check library would ignore all failures until after the end
> of the grace period.
> 
> This behaviour was misleading. With the changes in this commit, once a
> health check succeeds for the first time, the grace period rule for
> failures is not be applied any more.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> ea93132f2a5d4828c75005f102eddc4c3131599d 
>   src/tests/health_check_tests.cpp e6b02f23dc1b1b84381ab0af0e3df5918b60ae40 
> 
> Diff: https://reviews.apache.org/r/52357/diff/
> 
> 
> Testing
> ---
> 
> Manual tests using Marathon.
> Added a new unit test and verified that it fails without the change, but 
> succeeds with it.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>