You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Kevin Klues <kl...@gmail.com> on 2016/12/02 06:54:56 UTC
Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the
io switchboard.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar and Jie Yu.
Bugs: MESOS-6467
https://issues.apache.org/jira/browse/MESOS-6467
Repository: mesos
Description
-------
Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
Diffs
-----
src/slave/containerizer/mesos/io/switchboard.cpp 594c37e531c5b26599989a126aede56954d460fa
Diff: https://reviews.apache.org/r/54297/diff/
Testing
-------
This is not tesedt yet. I plan to write a test and update this patch soon. Just wanted to get a preliminary patch out there for review.
Thanks,
Kevin Klues
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/#review157775
-----------------------------------------------------------
Let me commit the attach output review/`serv()` to use the streaming decoder first and then keep reviewing this. (We can't commit this anyways without tests)
- Anand Mazumdar
On Dec. 2, 2016, 6:54 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 6:54 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Jie Yu.
>
>
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 594c37e531c5b26599989a126aede56954d460fa
>
> Diff: https://reviews.apache.org/r/54297/diff/
>
>
> Testing
> -------
>
> This is not tesedt yet. I plan to write a test and update this patch soon. Just wanted to get a preliminary patch out there for review.
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/#review157891
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54274, 54296, 54297]
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 Dec. 3, 2016, 2:07 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2016, 2:07 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Jie Yu.
>
>
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 594c37e531c5b26599989a126aede56954d460fa
>
> Diff: https://reviews.apache.org/r/54297/diff/
>
>
> Testing
> -------
>
> This is not tesedt yet. I plan to write a test and update this patch soon. Just wanted to get a preliminary patch out there for review.
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
> On Dec. 3, 2016, 8:36 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 885
> > <https://reviews.apache.org/r/54297/diff/2/?file=1575106#file1575106line885>
> >
> > Can you add a comment here for posterity as to why these are not invariant checks i.e., the agent does not validate if the next message should be `PROCESS_IO`?
Why doesn't the server validate this (it could, and maybe should)? First message is `CONTAINER_ID` type, all others are `PROCESS_IO` type.
> On Dec. 3, 2016, 8:36 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 916
> > <https://reviews.apache.org/r/54297/diff/2/?file=1575106#file1575106line916>
> >
> > hmm, this message is a bit mis-leading given that you only start processing the control message from L953. This is just validating the control message itself.
> >
> > How about you introduce validation overloads. This would simplify your call sites a lot i.e., you won't have to manually set `response` for every error case. An example here:
> >
> > ```cpp
> > // Validate the call.
> > ...... error = validate(...);
> > if (error.isSome()) {
> > return BadRequest(error.get());
> > }
> >
> > // Validate the 'PROCESS_IO' message.
> > ....
> >
> > // Validate the control message.
> > ....
> >
> > // Process the control message.
> > ...
> >
> > // Validate the data message.
> > ...
> >
> > // Process the data message.
> >
> > ```
> >
> > What do you think?
Where would I put these? In `src/slave/validation.cpp` namespaced under `io_switchboard::call`, etc. Or maybe just local to my class?
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/#review157901
-----------------------------------------------------------
On Dec. 3, 2016, 2:07 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2016, 2:07 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Jie Yu.
>
>
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 594c37e531c5b26599989a126aede56954d460fa
>
> Diff: https://reviews.apache.org/r/54297/diff/
>
>
> Testing
> -------
>
> This is not tesedt yet. I plan to write a test and update this patch soon. Just wanted to get a preliminary patch out there for review.
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
> On Dec. 3, 2016, 8:36 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 916
> > <https://reviews.apache.org/r/54297/diff/2/?file=1575106#file1575106line916>
> >
> > hmm, this message is a bit mis-leading given that you only start processing the control message from L953. This is just validating the control message itself.
> >
> > How about you introduce validation overloads. This would simplify your call sites a lot i.e., you won't have to manually set `response` for every error case. An example here:
> >
> > ```cpp
> > // Validate the call.
> > ...... error = validate(...);
> > if (error.isSome()) {
> > return BadRequest(error.get());
> > }
> >
> > // Validate the 'PROCESS_IO' message.
> > ....
> >
> > // Validate the control message.
> > ....
> >
> > // Process the control message.
> > ...
> >
> > // Validate the data message.
> > ...
> >
> > // Process the data message.
> >
> > ```
> >
> > What do you think?
>
> Kevin Klues wrote:
> Where would I put these? In `src/slave/validation.cpp` namespaced under `io_switchboard::call`, etc. Or maybe just local to my class?
I've just updated the patch again with taking a stab at this. Let me know what you think.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/#review157901
-----------------------------------------------------------
On Dec. 4, 2016, 12:30 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2016, 12:30 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Jie Yu.
>
>
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
> src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
>
> Diff: https://reviews.apache.org/r/54297/diff/
>
>
> Testing
> -------
>
> make -j check
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Anand Mazumdar <an...@apache.org>.
> On Dec. 3, 2016, 8:36 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 916
> > <https://reviews.apache.org/r/54297/diff/2/?file=1575106#file1575106line916>
> >
> > hmm, this message is a bit mis-leading given that you only start processing the control message from L953. This is just validating the control message itself.
> >
> > How about you introduce validation overloads. This would simplify your call sites a lot i.e., you won't have to manually set `response` for every error case. An example here:
> >
> > ```cpp
> > // Validate the call.
> > ...... error = validate(...);
> > if (error.isSome()) {
> > return BadRequest(error.get());
> > }
> >
> > // Validate the 'PROCESS_IO' message.
> > ....
> >
> > // Validate the control message.
> > ....
> >
> > // Process the control message.
> > ...
> >
> > // Validate the data message.
> > ...
> >
> > // Process the data message.
> >
> > ```
> >
> > What do you think?
>
> Kevin Klues wrote:
> Where would I put these? In `src/slave/validation.cpp` namespaced under `io_switchboard::call`, etc. Or maybe just local to my class?
>
> Kevin Klues wrote:
> I've just updated the patch again with taking a stab at this. Let me know what you think.
Marking this as fixed, added comments on this.
> On Dec. 3, 2016, 8:36 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 885
> > <https://reviews.apache.org/r/54297/diff/2/?file=1575106#file1575106line885>
> >
> > Can you add a comment here for posterity as to why these are not invariant checks i.e., the agent does not validate if the next message should be `PROCESS_IO`?
>
> Kevin Klues wrote:
> Why doesn't the server validate this (it could, and maybe should)? First message is `CONTAINER_ID` type, all others are `PROCESS_IO` type.
This is a tech-debt that we should clean up eventually. See my latest comment around adding a `TODO` to move all the validation logic to the agent itself.
The `transform()` helper that the agent uses currently returns `Future<Nothing>` which in turn hides the actual error. Hence, all these validation errors would become `InternalServerError` for the client instead of `BadRequest`. Once we switch to using `process::loop()` (it was not there when the agent handler was being worked on), we would have similar logic to the one you have, to propogate the actual error back to the client.
We have to do so soon since `transform()` seems to be plagued by the unbounded future memory growth issue that BenH encountered.
I am dropping this issue in favor of the recent issue I raised to add a `TODO` to move the validation to the agent itself.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/#review157901
-----------------------------------------------------------
On Dec. 4, 2016, 12:54 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2016, 12:54 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Jie Yu.
>
>
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
> src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
>
> Diff: https://reviews.apache.org/r/54297/diff/
>
>
> Testing
> -------
>
> make -j check
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/#review157901
-----------------------------------------------------------
Modulo Jie's comments.
Did an initial pass. Looks pretty good! Mostly minor comments around style. The only major one being that we can simplify the input handler greatly by introducing validation overloads. See comsment later.
src/slave/containerizer/mesos/io/switchboard.cpp (line 17)
<https://reviews.apache.org/r/54297/#comment228544>
You might not need this after Jie's comment?
src/slave/containerizer/mesos/io/switchboard.cpp (line 863)
<https://reviews.apache.org/r/54297/#comment228524>
Nit: Quotes before 'ATTACH_CONTAINER_INPUT'
src/slave/containerizer/mesos/io/switchboard.cpp (lines 869 - 871)
<https://reviews.apache.org/r/54297/#comment228523>
Kill this since you assert on this a couple of lines later?
src/slave/containerizer/mesos/io/switchboard.cpp (line 880)
<https://reviews.apache.org/r/54297/#comment228525>
Can you add a comment here for posterity as to why these are not invariant checks i.e., the agent does not validate if the next message should be `PROCESS_IO`?
src/slave/containerizer/mesos/io/switchboard.cpp (line 884)
<https://reviews.apache.org/r/54297/#comment228526>
Nit: Can we be consistent with the agent validation strings here:
"Expecting 'agent_container_input.type' to be 'PROCESS_IO'"
Check `src/slave/validation.cpp`.
src/slave/containerizer/mesos/io/switchboard.cpp (line 890)
<https://reviews.apache.org/r/54297/#comment228527>
"Expecting 'attach_container_input.process_io' to be present"
src/slave/containerizer/mesos/io/switchboard.cpp (line 895)
<https://reviews.apache.org/r/54297/#comment228528>
const reference
src/slave/containerizer/mesos/io/switchboard.cpp (line 900)
<https://reviews.apache.org/r/54297/#comment228529>
"Expecting 'attach_container_input.process_io.type' to be present"
src/slave/containerizer/mesos/io/switchboard.cpp (lines 904 - 905)
<https://reviews.apache.org/r/54297/#comment228530>
Can you just check for `UNKNOWN` here?
Also, the error string can be
"'attach_container_input.process_io.type' is unknown"
src/slave/containerizer/mesos/io/switchboard.cpp (line 911)
<https://reviews.apache.org/r/54297/#comment228538>
hmm, this message is a bit mis-leading given that you only start processing the control message from L953. This is just validating the control message itself.
How about you introduce validation overloads. This would simplify your call sites a lot i.e., you won't have to manually set `response` for every error case. An example here:
```cpp
// Validate the call.
...... error = validate(...);
if (error.isSome()) {
return BadRequest(error.get());
}
// Validate the 'PROCESS_IO' message.
....
// Validate the control message.
....
// Process the control message.
...
// Validate the data message.
...
// Process the data message.
```
What do you think?
src/slave/containerizer/mesos/io/switchboard.cpp (lines 915 - 916)
<https://reviews.apache.org/r/54297/#comment228539>
Can you make the error string consistent here based on previous comments?
src/slave/containerizer/mesos/io/switchboard.cpp (lines 921 - 922)
<https://reviews.apache.org/r/54297/#comment228540>
Can you make the error string consistent here based on previous comments?
src/slave/containerizer/mesos/io/switchboard.cpp (line 929)
<https://reviews.apache.org/r/54297/#comment228541>
Nit: Missing quotes before 'TTY_INFO'
src/slave/containerizer/mesos/io/switchboard.cpp (line 945)
<https://reviews.apache.org/r/54297/#comment228535>
Const reference?
src/slave/containerizer/mesos/io/switchboard.cpp (line 989)
<https://reviews.apache.org/r/54297/#comment228542>
s/our/the
s/appropriately//
src/slave/containerizer/mesos/io/switchboard.cpp (line 992)
<https://reviews.apache.org/r/54297/#comment228543>
s/simply//
src/slave/containerizer/mesos/io/switchboard.cpp (line 997)
<https://reviews.apache.org/r/54297/#comment228531>
Can you use explicit capture here?
Also s/Future<Nothing>/Future<Nothing>&
src/slave/containerizer/mesos/io/switchboard.cpp (lines 1003 - 1010)
<https://reviews.apache.org/r/54297/#comment228532>
You should be able to combine these:
```cpp
failure = Failure("Failed writing to stdin:"
" " + (future.isFailed() ? future.failure() : "discarded");
```
src/slave/containerizer/mesos/io/switchboard.cpp (line 1021)
<https://reviews.apache.org/r/54297/#comment228533>
Can you use explicit capture here?
src/slave/containerizer/mesos/io/switchboard.cpp (line 1022)
<https://reviews.apache.org/r/54297/#comment228534>
s/connections/input connections
- Anand Mazumdar
On Dec. 3, 2016, 2:07 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2016, 2:07 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Jie Yu.
>
>
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 594c37e531c5b26599989a126aede56954d460fa
>
> Diff: https://reviews.apache.org/r/54297/diff/
>
>
> Testing
> -------
>
> This is not tesedt yet. I plan to write a test and update this patch soon. Just wanted to get a preliminary patch out there for review.
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/#review157914
-----------------------------------------------------------
Just a few more minor things to clean up.
src/slave/containerizer/mesos/io/switchboard.cpp (lines 832 - 833)
<https://reviews.apache.org/r/54297/#comment228558>
hmm, the `PROCESS_IO` validation can be done by the agent itself i.e., it seems to be independent of the business logic of the handler.
Can you add a `TODO` in the function declaration to move this logic to the agent in `src/slave/validation.hpp` if you don't want to do this now?
Also, can you change the method signature to be:
```cpp
Option<Error> IOSwitchboardServerProcess::validate(
const agent::Call::AttachContainerInput& call)
```
src/slave/containerizer/mesos/io/switchboard.cpp (lines 835 - 840)
<https://reviews.apache.org/r/54297/#comment228557>
With the change in method signature, move these to the handler (call site) itself. (L964)
src/slave/containerizer/mesos/io/switchboard.cpp (lines 863 - 925)
<https://reviews.apache.org/r/54297/#comment228560>
There seem perfect to use the switch statement on `type()` instead of the `if else` statements?
src/slave/containerizer/mesos/io/switchboard.cpp (line 974)
<https://reviews.apache.org/r/54297/#comment228561>
Any reasons for not using switch statements here? If not, can you use them here?
src/slave/containerizer/mesos/io/switchboard.cpp (lines 1012 - 1014)
<https://reviews.apache.org/r/54297/#comment228563>
Nit: Should fit in one line, no?
src/tests/containerizer/io_switchboard_tests.cpp (line 301)
<https://reviews.apache.org/r/54297/#comment228562>
Nice Test!
src/tests/containerizer/io_switchboard_tests.cpp (line 303)
<https://reviews.apache.org/r/54297/#comment228556>
s/only//
- Anand Mazumdar
On Dec. 4, 2016, 12:54 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2016, 12:54 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Jie Yu.
>
>
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
> src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
>
> Diff: https://reviews.apache.org/r/54297/diff/
>
>
> Testing
> -------
>
> make -j check
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/
-----------------------------------------------------------
(Updated Dec. 5, 2016, 7:18 a.m.)
Review request for mesos, Anand Mazumdar and Jie Yu.
Changes
-------
Needed to rebase again because I accidentally included something in a follow-on commit that should have been in this one.
Bugs: MESOS-6467
https://issues.apache.org/jira/browse/MESOS-6467
Repository: mesos
Description
-------
Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.cpp d5211b98616e72a27ca6b472a5ee83505c227f22
src/tests/containerizer/io_switchboard_tests.cpp ebf9bc75b8a256847a507954615241cff1da4dc3
Diff: https://reviews.apache.org/r/54297/diff/
Testing
-------
make -j check
Thanks,
Kevin Klues
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/
-----------------------------------------------------------
(Updated Dec. 5, 2016, 7:09 a.m.)
Review request for mesos, Anand Mazumdar and Jie Yu.
Changes
-------
Rebased on master.
Bugs: MESOS-6467
https://issues.apache.org/jira/browse/MESOS-6467
Repository: mesos
Description
-------
Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.cpp d5211b98616e72a27ca6b472a5ee83505c227f22
src/tests/containerizer/io_switchboard_tests.cpp ebf9bc75b8a256847a507954615241cff1da4dc3
Diff: https://reviews.apache.org/r/54297/diff/
Testing
-------
make -j check
Thanks,
Kevin Klues
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/
-----------------------------------------------------------
(Updated Dec. 4, 2016, 7:17 p.m.)
Review request for mesos, Anand Mazumdar and Jie Yu.
Changes
-------
Updated to address newest comments.
Bugs: MESOS-6467
https://issues.apache.org/jira/browse/MESOS-6467
Repository: mesos
Description
-------
Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
Diff: https://reviews.apache.org/r/54297/diff/
Testing
-------
make -j check
Thanks,
Kevin Klues
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/#review157925
-----------------------------------------------------------
Fix it, then Ship it!
I would file a follow up issue to add tests for the validation code itself.
src/slave/containerizer/mesos/io/switchboard.cpp (lines 492 - 493)
<https://reviews.apache.org/r/54297/#comment228587>
How about:
```cpp
// TODO(klueska): Move this to `src/slave/validation.hpp` and make the agent validate all the calls before forwarding them to the switchboard.
````
src/slave/containerizer/mesos/io/switchboard.cpp (lines 847 - 849)
<https://reviews.apache.org/r/54297/#comment228582>
Kill this, you already have an explicit `CHECK` for this before you invoke this.
src/slave/containerizer/mesos/io/switchboard.cpp (lines 852 - 866)
<https://reviews.apache.org/r/54297/#comment228584>
For this handler, it should *only* validate that the second record is a `ProcessIO` record. It shouldn't validate the other types and just return an error if the record is of some other type.
Can you just do:
```cpp
case agent::Call::AttachContainerInput::UNKNOWN:
case agent::Call::AttachContainerInput::CONTAINER_ID: {
return Error("Expecting 'attach_container_input.type' to be 'PROCESS_IO'"
" instead of: '" + call.type() + "'");
```
src/slave/containerizer/mesos/io/switchboard.cpp (lines 934 - 936)
<https://reviews.apache.org/r/54297/#comment228581>
We rely on the compiler failing if a `condition` is missed in the case statements `-Wswitch`.
Kill this and just have `UNREACHABLE()` in the outer scope.
src/slave/containerizer/mesos/io/switchboard.cpp (lines 939 - 941)
<https://reviews.apache.org/r/54297/#comment228585>
Kill this and replace with unreachable outside.
src/slave/containerizer/mesos/io/switchboard.cpp (lines 984 - 985)
<https://reviews.apache.org/r/54297/#comment228576>
Kill this invariant check.
The agent does not validate if the second record type is `PROCESS_IO`. We need to validate it in the handler itself for now as per our earlier discussion.
src/slave/containerizer/mesos/io/switchboard.cpp (lines 1044 - 1046)
<https://reviews.apache.org/r/54297/#comment228580>
Kill this and just have a `UNREACHABLE()`.
- Anand Mazumdar
On Dec. 4, 2016, 6:55 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2016, 6:55 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Jie Yu.
>
>
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
> src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
>
> Diff: https://reviews.apache.org/r/54297/diff/
>
>
> Testing
> -------
>
> make -j check
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/
-----------------------------------------------------------
(Updated Dec. 4, 2016, 6:55 a.m.)
Review request for mesos, Anand Mazumdar and Jie Yu.
Changes
-------
Updated based on Anand's comments.
Bugs: MESOS-6467
https://issues.apache.org/jira/browse/MESOS-6467
Repository: mesos
Description
-------
Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
Diff: https://reviews.apache.org/r/54297/diff/
Testing
-------
make -j check
Thanks,
Kevin Klues
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/
-----------------------------------------------------------
(Updated Dec. 4, 2016, 12:54 a.m.)
Review request for mesos, Anand Mazumdar and Jie Yu.
Changes
-------
Updated to remove circular dependency in reviews.
Bugs: MESOS-6467
https://issues.apache.org/jira/browse/MESOS-6467
Repository: mesos
Description
-------
Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
Diff: https://reviews.apache.org/r/54297/diff/
Testing
-------
make -j check
Thanks,
Kevin Klues
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/
-----------------------------------------------------------
(Updated Dec. 4, 2016, 12:30 a.m.)
Review request for mesos, Anand Mazumdar and Jie Yu.
Bugs: MESOS-6467
https://issues.apache.org/jira/browse/MESOS-6467
Repository: mesos
Description
-------
Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
Diffs
-----
src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
Diff: https://reviews.apache.org/r/54297/diff/
Testing (updated)
-------
make -j check
Thanks,
Kevin Klues
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/
-----------------------------------------------------------
(Updated Dec. 4, 2016, 12:30 a.m.)
Review request for mesos, Anand Mazumdar and Jie Yu.
Changes
-------
Updated to address the rest of Anand's comments.
Bugs: MESOS-6467
https://issues.apache.org/jira/browse/MESOS-6467
Repository: mesos
Description
-------
Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
Diff: https://reviews.apache.org/r/54297/diff/
Testing
-------
This is not tesedt yet. I plan to write a test and update this patch soon. Just wanted to get a preliminary patch out there for review.
Thanks,
Kevin Klues
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/
-----------------------------------------------------------
(Updated Dec. 3, 2016, 11:45 p.m.)
Review request for mesos, Anand Mazumdar and Jie Yu.
Changes
-------
Addressed most of Anand's comments (still waiting on some feedback for a few).
Bugs: MESOS-6467
https://issues.apache.org/jira/browse/MESOS-6467
Repository: mesos
Description
-------
Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503
src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80
Diff: https://reviews.apache.org/r/54297/diff/
Testing
-------
This is not tesedt yet. I plan to write a test and update this patch soon. Just wanted to get a preliminary patch out there for review.
Thanks,
Kevin Klues
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/
-----------------------------------------------------------
(Updated Dec. 3, 2016, 2:07 a.m.)
Review request for mesos, Anand Mazumdar and Jie Yu.
Changes
-------
Updated to return proper response types for all failure cases.
Bugs: MESOS-6467
https://issues.apache.org/jira/browse/MESOS-6467
Repository: mesos
Description
-------
Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
Diffs (updated)
-----
src/slave/containerizer/mesos/io/switchboard.cpp 594c37e531c5b26599989a126aede56954d460fa
Diff: https://reviews.apache.org/r/54297/diff/
Testing
-------
This is not tesedt yet. I plan to write a test and update this patch soon. Just wanted to get a preliminary patch out there for review.
Thanks,
Kevin Klues
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/#review157789
-----------------------------------------------------------
src/slave/containerizer/mesos/io/switchboard.cpp (lines 947 - 954)
<https://reviews.apache.org/r/54297/#comment228393>
Let's put this into stout/posix/os.hpp and call it `os::setWindowSize(unsigned short row, unsigned short column)`
- Jie Yu
On Dec. 2, 2016, 6:54 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 6:54 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Jie Yu.
>
>
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 594c37e531c5b26599989a126aede56954d460fa
>
> Diff: https://reviews.apache.org/r/54297/diff/
>
>
> Testing
> -------
>
> This is not tesedt yet. I plan to write a test and update this patch soon. Just wanted to get a preliminary patch out there for review.
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to
the io switchboard.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54297/#review157762
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54274, 54296, 54297]
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 Dec. 2, 2016, 6:54 a.m., Kevin Klues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 6:54 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Jie Yu.
>
>
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/io/switchboard.cpp 594c37e531c5b26599989a126aede56954d460fa
>
> Diff: https://reviews.apache.org/r/54297/diff/
>
>
> Testing
> -------
>
> This is not tesedt yet. I plan to write a test and update this patch soon. Just wanted to get a preliminary patch out there for review.
>
>
> Thanks,
>
> Kevin Klues
>
>