Re: Review Request 54196: Added API handler for LAUNCH_NESTED_CONTAINER_SESSION.

[Vinod Kone]

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

(Updated Dec. 2, 2016, 5:54 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

anand's comments. NNFR.


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


Repository: mesos


Description
---

In addition to launching the nested container the API handler
ensures that the container is destroyed if the connection breaks.


Diffs (updated)
-

  src/slave/http.cpp ace3575fd3a330f788e678283bc05e806cf9b264 
  src/slave/slave.hpp cfec9dd5fa3550ba1192fc86b1ff0d73ee1671e0 
  src/slave/validation.cpp 4005cfcd8bcdb923a73de471100c3567344234cf 
  src/tests/api_tests.cpp ea6e037c18fd116eff473ee246faa504ec37b7da 

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


Testing
---

make check

Added a basic test for now that tests the failure case. Will be adding more 
tests in subsequent reviews.


Thanks,

Vinod Kone

Re: Review Request 54196: Added API handler for LAUNCH_NESTED_CONTAINER_SESSION.

[Anand Mazumdar]

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


Fix it, then Ship it!




LGTM minus a small query regarding logging a warning message later.


src/slave/http.cpp (line 2426)


2 line indent



src/slave/http.cpp (line 2455)


Nit: s/and log on failure//



src/slave/http.cpp (line 2456)


Can you explicity capture here?



src/slave/http.cpp (lines 2514 - 2516)


hmm, this message seems a bit mis-leading. What happens if the command that 
was run in the debug container entrypoint completes? We would still log this 
warning message?



src/tests/api_tests.cpp (line 3641)


Kill this.



src/tests/api_tests.cpp (line 3653)


Kill this.



src/tests/api_tests.cpp (line 3683)


s/callbacks/pending callbacks


- Anand Mazumdar


On Dec. 2, 2016, 4:42 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54196/
> ---
> 
> (Updated Dec. 2, 2016, 4:42 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6471
> https://issues.apache.org/jira/browse/MESOS-6471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In addition to launching the nested container the API handler
> ensures that the container is destroyed if the connection breaks.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp ace3575fd3a330f788e678283bc05e806cf9b264 
>   src/slave/slave.hpp cfec9dd5fa3550ba1192fc86b1ff0d73ee1671e0 
>   src/slave/validation.cpp 4005cfcd8bcdb923a73de471100c3567344234cf 
>   src/tests/api_tests.cpp ea6e037c18fd116eff473ee246faa504ec37b7da 
> 
> Diff: https://reviews.apache.org/r/54196/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Added a basic test for now that tests the failure case. Will be adding more 
> tests in subsequent reviews.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54196: Added API handler for LAUNCH_NESTED_CONTAINER_SESSION.

[Vinod Kone]

> On Dec. 1, 2016, 7:49 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, line 1982
> > 
> >
> > Why not be explicit here and set it to `ContainerClass::DEFAULT` like 
> > we do for the corresponding session one?

done.


> On Dec. 1, 2016, 7:49 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, line 2197
> > 
> >
> > Nit: s/goes way/is interrupted

i did s/goes way/breaks/


> On Dec. 1, 2016, 7:49 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, lines 2234-2243
> > 
> >
> > hmm, can we create a lambda destroy and just invoke it with a `message` 
> > argument rather then duplicating it here and else where?

passing the message looks a bit ugly because we need to construct it with "+" 
and stringify(). so i opted to log the warning and call the lambda instead. let 
me know if that looks ok.


> On Dec. 1, 2016, 7:49 p.m., Anand Mazumdar wrote:
> > src/slave/http.cpp, line 2229
> > 
> >
> > Can we resolve this TODO? This can happen often if the launched 
> > container as part of session terminates. This would result in an EOF. 
> > 
> > You might want to sync up with Kevin regarding other cases when this 
> > can happen.

as discussed offline, if the container terminates we would get a failure 
instead of EOF here? but anyway, leveraged trick mentioned by mpark to combine 
the handlers.


> On Dec. 1, 2016, 7:49 p.m., Anand Mazumdar wrote:
> > src/tests/api_tests.cpp, line 3623
> > 
> >
> > hmm, this test would break the moment Jie commits Kevin's changes to 
> > add support to the containerizer for launching debug containers?

I reealized I used "TestContainerizer" and not "MesosContainerizer" here. So 
nothing broke after Jie's changes. I would like to use TestContainerizer 
because it lets me test more functionality.


- Vinod


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


On Dec. 2, 2016, 4:42 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54196/
> ---
> 
> (Updated Dec. 2, 2016, 4:42 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6471
> https://issues.apache.org/jira/browse/MESOS-6471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In addition to launching the nested container the API handler
> ensures that the container is destroyed if the connection breaks.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp ace3575fd3a330f788e678283bc05e806cf9b264 
>   src/slave/slave.hpp cfec9dd5fa3550ba1192fc86b1ff0d73ee1671e0 
>   src/slave/validation.cpp 4005cfcd8bcdb923a73de471100c3567344234cf 
>   src/tests/api_tests.cpp ea6e037c18fd116eff473ee246faa504ec37b7da 
> 
> Diff: https://reviews.apache.org/r/54196/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Added a basic test for now that tests the failure case. Will be adding more 
> tests in subsequent reviews.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54196: Added API handler for LAUNCH_NESTED_CONTAINER_SESSION.

[Vinod Kone]

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

(Updated Dec. 2, 2016, 4:42 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

leveraged the std::function trick suggested by mpark to simplify `connect.onAny`


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


Repository: mesos


Description
---

In addition to launching the nested container the API handler
ensures that the container is destroyed if the connection breaks.


Diffs (updated)
-

  src/slave/http.cpp ace3575fd3a330f788e678283bc05e806cf9b264 
  src/slave/slave.hpp cfec9dd5fa3550ba1192fc86b1ff0d73ee1671e0 
  src/slave/validation.cpp 4005cfcd8bcdb923a73de471100c3567344234cf 
  src/tests/api_tests.cpp ea6e037c18fd116eff473ee246faa504ec37b7da 

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


Testing
---

make check

Added a basic test for now that tests the failure case. Will be adding more 
tests in subsequent reviews.


Thanks,

Vinod Kone

Re: Review Request 54196: Added API handler for LAUNCH_NESTED_CONTAINER_SESSION.

[Vinod Kone]

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

(Updated Dec. 2, 2016, 2:38 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

anand's comments.


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


Repository: mesos


Description
---

In addition to launching the nested container the API handler
ensures that the container is destroyed if the connection breaks.


Diffs (updated)
-

  src/slave/http.cpp ace3575fd3a330f788e678283bc05e806cf9b264 
  src/slave/slave.hpp cfec9dd5fa3550ba1192fc86b1ff0d73ee1671e0 
  src/slave/validation.cpp 4005cfcd8bcdb923a73de471100c3567344234cf 
  src/tests/api_tests.cpp ea6e037c18fd116eff473ee246faa504ec37b7da 

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


Testing
---

make check

Added a basic test for now that tests the failure case. Will be adding more 
tests in subsequent reviews.


Thanks,

Vinod Kone

Re: Review Request 54196: Added API handler for LAUNCH_NESTED_CONTAINER_SESSION.

[Vinod Kone]

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

(Updated Dec. 2, 2016, 1:02 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

changes to work with the rebase. this doesn't address anand's comments yet.


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


Repository: mesos


Description
---

In addition to launching the nested container the API handler
ensures that the container is destroyed if the connection breaks.


Diffs (updated)
-

  src/slave/http.cpp ace3575fd3a330f788e678283bc05e806cf9b264 
  src/slave/slave.hpp cfec9dd5fa3550ba1192fc86b1ff0d73ee1671e0 
  src/slave/validation.cpp 4005cfcd8bcdb923a73de471100c3567344234cf 
  src/tests/api_tests.cpp ea6e037c18fd116eff473ee246faa504ec37b7da 

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


Testing
---

make check

Added a basic test for now that tests the failure case. Will be adding more 
tests in subsequent reviews.


Thanks,

Vinod Kone

Re: Review Request 54196: Added API handler for LAUNCH_NESTED_CONTAINER_SESSION.

[Anand Mazumdar]

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



Mostly minor comments all around. The two major ones include concerns around 
not killing the child container in the EOF case and if we should add the test 
at all in its present form?


src/slave/http.cpp (line 1982)


Why not be explicit here and set it to `ContainerClass::DEFAULT` like we do 
for the corresponding session one?



src/slave/http.cpp (lines 2129 - 2130)


How about?
```cpp
// Helper that reads data from `writer` and writes to `reader`.
// Returns a failed future if there are any errors reading or writing. The 
future is satisfied when we get a EOF.
```



src/slave/http.cpp (line 2136)


4 space indent



src/slave/http.cpp (line 2178)


Nit: comma after `ok`



src/slave/http.cpp (line 2181)


Can you move `slave->self()` to the next line and the next line would fit 
with it.

```cpp
.then(defer(
slave->self(), [=]...))
```



src/slave/http.cpp (line 2187)


Why do we need this logging?



src/slave/http.cpp (line 2196)


s/client we/client, we



src/slave/http.cpp (line 2197)


Nit: s/goes way/is interrupted



src/slave/http.cpp (line 2213)


s/write it to/write to



src/slave/http.cpp (line 2214)


s/Response/response



src/slave/http.cpp (line 2229)


Can we resolve this TODO? This can happen often if the launched container 
as part of session terminates. This would result in an EOF. 

You might want to sync up with Kevin regarding other cases when this can 
happen.



src/slave/http.cpp (lines 2234 - 2243)


hmm, can we create a lambda destroy and just invoke it with a `message` 
argument rather then duplicating it here and else where?



src/slave/http.cpp (line 2249)


Kill the extraneous space at the end since you have one extra one on the 
next line



src/tests/api_tests.cpp (line 3623)


hmm, this test would break the moment Jie commits Kevin's changes to add 
support to the containerizer for launching debug containers?



src/tests/api_tests.cpp (line 3679)


Use this directly in L3685. You don't need the accept header here due to 
the response being `InternalServerError`.


- Anand Mazumdar


On Dec. 1, 2016, 6:46 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54196/
> ---
> 
> (Updated Dec. 1, 2016, 6:46 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6471
> https://issues.apache.org/jira/browse/MESOS-6471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In addition to launching the nested container the API handler
> ensures that the container is destroyed if the connection breaks.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 87189dd6e2e099cb74faabd3ad26aaca11e5cef2 
>   src/slave/slave.hpp 05865a09724f7a46e9c06b02e59779513fc532aa 
>   src/slave/validation.cpp 46e84361244394196ee5d431c1da34686d6c938a 
>   src/tests/api_tests.cpp b2a70f3174824d3cb241a098d96e4d63a390f8bc 
> 
> Diff: https://reviews.apache.org/r/54196/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Added a basic test for now that tests the failure case. Will be adding more 
> tests in subsequent reviews.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54196: Added API handler for LAUNCH_NESTED_CONTAINER_SESSION.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [54196, 54194, 54193, 54245, 54115, 54114, 54113, 53990, 
50737, 52938, 51065, 50736, 53802, 53975, 53817]

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

Error:
2016-12-01 08:33:43 URL:https://reviews.apache.org/r/54245/diff/raw/ 
[21448/21448] -> "54245.patch" [1]
error: patch failed: src/slave/http.cpp:405
error: src/slave/http.cpp: patch does not apply
error: patch failed: src/slave/slave.hpp:632
error: src/slave/slave.hpp: patch does not apply

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

- Mesos ReviewBot


On Dec. 1, 2016, 6:46 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54196/
> ---
> 
> (Updated Dec. 1, 2016, 6:46 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6471
> https://issues.apache.org/jira/browse/MESOS-6471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In addition to launching the nested container the API handler
> ensures that the container is destroyed if the connection breaks.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 87189dd6e2e099cb74faabd3ad26aaca11e5cef2 
>   src/slave/slave.hpp 05865a09724f7a46e9c06b02e59779513fc532aa 
>   src/slave/validation.cpp 46e84361244394196ee5d431c1da34686d6c938a 
>   src/tests/api_tests.cpp b2a70f3174824d3cb241a098d96e4d63a390f8bc 
> 
> Diff: https://reviews.apache.org/r/54196/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Added a basic test for now that tests the failure case. Will be adding more 
> tests in subsequent reviews.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54196: Added API handler for LAUNCH_NESTED_CONTAINER_SESSION.

[Vinod Kone]

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

(Updated Dec. 1, 2016, 6:46 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

updated for signature change.


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


Repository: mesos


Description
---

In addition to launching the nested container the API handler
ensures that the container is destroyed if the connection breaks.


Diffs (updated)
-

  src/slave/http.cpp 87189dd6e2e099cb74faabd3ad26aaca11e5cef2 
  src/slave/slave.hpp 05865a09724f7a46e9c06b02e59779513fc532aa 
  src/slave/validation.cpp 46e84361244394196ee5d431c1da34686d6c938a 
  src/tests/api_tests.cpp b2a70f3174824d3cb241a098d96e4d63a390f8bc 

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


Testing
---

make check

Added a basic test for now that tests the failure case. Will be adding more 
tests in subsequent reviews.


Thanks,

Vinod Kone

Re: Review Request 54196: Added API handler for LAUNCH_NESTED_CONTAINER_SESSION.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54193, 54194, 54196]

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 Nov. 30, 2016, 4 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54196/
> ---
> 
> (Updated Nov. 30, 2016, 4 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6471
> https://issues.apache.org/jira/browse/MESOS-6471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In addition to launching the nested container the API handler
> ensures that the container is destroyed if the connection breaks.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 87189dd6e2e099cb74faabd3ad26aaca11e5cef2 
>   src/slave/slave.hpp 05865a09724f7a46e9c06b02e59779513fc532aa 
>   src/slave/validation.cpp 46e84361244394196ee5d431c1da34686d6c938a 
>   src/tests/api_tests.cpp b2a70f3174824d3cb241a098d96e4d63a390f8bc 
> 
> Diff: https://reviews.apache.org/r/54196/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Added a basic test for now that tests the failure case. Will be adding more 
> tests in subsequent reviews.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>