You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by ASHUTOSH JAIN <ha...@gmail.com> on 2014/03/16 16:23:25 UTC
Review Request 19263: a check on resources (mem) provided by user
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/
-----------------------------------------------------------
Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
Repository: mesos-git
Description
-------
resources (mem) provided by user not being checked
link to issue: https://issues.apache.org/jira/browse/MESOS-969
Diffs
-----
src/slave/main.cpp 8c2b70c
Diff: https://reviews.apache.org/r/19263/diff/
Testing
-------
make check
Thanks,
ASHUTOSH JAIN
Re: Review Request 19263: a check on resources (mem) provided by user
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/#review37614
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19263]
All tests passed.
- Mesos ReviewBot
On March 17, 2014, 5:21 p.m., ASHUTOSH JAIN wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19263/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 5:21 p.m.)
>
>
> Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
>
>
> Bugs: MESOS-969
> https://issues.apache.org/jira/browse/MESOS-969
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> resources (mem) provided by user not being checked
>
> link to issue: https://issues.apache.org/jira/browse/MESOS-969
>
>
> Diffs
> -----
>
> src/slave/main.cpp 8c2b70c
>
> Diff: https://reviews.apache.org/r/19263/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> ASHUTOSH JAIN
>
>
Re: Review Request 19263: a check on resources (mem) provided by user
Posted by Adam B <ad...@mesosphere.io>.
> On March 18, 2014, 11:58 a.m., Ian Downes wrote:
> > src/slave/main.cpp, line 138
> > <https://reviews.apache.org/r/19263/diff/3/?file=522993#file522993line138>
> >
> > Why can't these additional checks be done in Containerizer::resources (in slave/containerizer/containerizer.cpp) and then you can return Error() according to these new checks?
>
> ASHUTOSH JAIN wrote:
> just wanted to confirm if Error() would exit the code stating wrong mem or just output a warning.
> If it exits, then we could move the check part to containerizer.cpp.
+1 on moving the check to Containerizer::resources(). That would definitely make the new checks easier to unit test.
Ashutosh: If you run into an error (eg. parsing, or exceeds main memory size) during Containerizer::resources(), it will return a Try<Resources> with a value of Error("error message"). Rather than calling Containerizer::resources() directly from slave/main.cpp and checking for isError() on the return value, you can probably just rely on Slave::initialize() which already calls Containerizer::resources() and Exit()s if _resources.isError().
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/#review37601
-----------------------------------------------------------
On March 17, 2014, 10:21 a.m., ASHUTOSH JAIN wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19263/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 10:21 a.m.)
>
>
> Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
>
>
> Bugs: MESOS-969
> https://issues.apache.org/jira/browse/MESOS-969
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> resources (mem) provided by user not being checked
>
> link to issue: https://issues.apache.org/jira/browse/MESOS-969
>
>
> Diffs
> -----
>
> src/slave/main.cpp 8c2b70c
>
> Diff: https://reviews.apache.org/r/19263/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> ASHUTOSH JAIN
>
>
Re: Review Request 19263: a check on resources (mem) provided by user
Posted by ASHUTOSH JAIN <ha...@gmail.com>.
> On March 18, 2014, 6:58 p.m., Ian Downes wrote:
> > src/slave/main.cpp, line 138
> > <https://reviews.apache.org/r/19263/diff/3/?file=522993#file522993line138>
> >
> > Why can't these additional checks be done in Containerizer::resources (in slave/containerizer/containerizer.cpp) and then you can return Error() according to these new checks?
just wanted to confirm if Error() would exit the code stating wrong mem or just output a warning.
If it exits, then we could move the check part to containerizer.cpp.
- ASHUTOSH
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/#review37601
-----------------------------------------------------------
On March 17, 2014, 5:21 p.m., ASHUTOSH JAIN wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19263/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 5:21 p.m.)
>
>
> Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
>
>
> Bugs: MESOS-969
> https://issues.apache.org/jira/browse/MESOS-969
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> resources (mem) provided by user not being checked
>
> link to issue: https://issues.apache.org/jira/browse/MESOS-969
>
>
> Diffs
> -----
>
> src/slave/main.cpp 8c2b70c
>
> Diff: https://reviews.apache.org/r/19263/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> ASHUTOSH JAIN
>
>
Re: Review Request 19263: a check on resources (mem) provided by user
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/#review37601
-----------------------------------------------------------
src/slave/main.cpp
<https://reviews.apache.org/r/19263/#comment69215>
Why can't these additional checks be done in Containerizer::resources (in slave/containerizer/containerizer.cpp) and then you can return Error() according to these new checks?
- Ian Downes
On March 17, 2014, 5:21 p.m., ASHUTOSH JAIN wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19263/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 5:21 p.m.)
>
>
> Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
>
>
> Bugs: MESOS-969
> https://issues.apache.org/jira/browse/MESOS-969
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> resources (mem) provided by user not being checked
>
> link to issue: https://issues.apache.org/jira/browse/MESOS-969
>
>
> Diffs
> -----
>
> src/slave/main.cpp 8c2b70c
>
> Diff: https://reviews.apache.org/r/19263/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> ASHUTOSH JAIN
>
>
Re: Review Request 19263: a check on resources (mem) provided by user
Posted by ASHUTOSH JAIN <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/
-----------------------------------------------------------
(Updated March 17, 2014, 5:21 p.m.)
Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
Changes
-------
new diff with some minor modification.
Agenda of
- moving the resource check to some other file and
- whether to exit or start slave with mem_.total minus some memory for buffer
is still pending.
Bugs: MESOS-969
https://issues.apache.org/jira/browse/MESOS-969
Repository: mesos-git
Description
-------
resources (mem) provided by user not being checked
link to issue: https://issues.apache.org/jira/browse/MESOS-969
Diffs (updated)
-----
src/slave/main.cpp 8c2b70c
Diff: https://reviews.apache.org/r/19263/diff/
Testing
-------
make check
Thanks,
ASHUTOSH JAIN
Re: Review Request 19263: a check on resources (mem) provided by user
Posted by ASHUTOSH JAIN <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/
-----------------------------------------------------------
(Updated March 17, 2014, 10:04 a.m.)
Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
Changes
-------
just minor improvements and round 1 of modification and discussion.
Bugs: MESOS-969
https://issues.apache.org/jira/browse/MESOS-969
Repository: mesos-git
Description
-------
resources (mem) provided by user not being checked
link to issue: https://issues.apache.org/jira/browse/MESOS-969
Diffs
-----
src/slave/main.cpp 8c2b70c
Diff: https://reviews.apache.org/r/19263/diff/
Testing
-------
make check
Thanks,
ASHUTOSH JAIN
Re: Review Request 19263: a check on resources (mem) provided by user
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/#review37336
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19263]
All tests passed.
- Mesos ReviewBot
On March 16, 2014, 3:23 p.m., ASHUTOSH JAIN wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19263/
> -----------------------------------------------------------
>
> (Updated March 16, 2014, 3:23 p.m.)
>
>
> Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> resources (mem) provided by user not being checked
>
> link to issue: https://issues.apache.org/jira/browse/MESOS-969
>
>
> Diffs
> -----
>
> src/slave/main.cpp 8c2b70c
>
> Diff: https://reviews.apache.org/r/19263/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> ASHUTOSH JAIN
>
>
Re: Review Request 19263: a check on resources (mem) provided by user
Posted by ASHUTOSH JAIN <ha...@gmail.com>.
> On March 17, 2014, 4:28 a.m., Adam B wrote:
> > Good start, Ashutosh. I think validating the sanity of the resources flag is vitally important, but I have my concerns about whether this is best done in slave/main.cpp, or in Containerizer or Slave itself. I think we should also discuss the desired behavior when an invalid resource flag value is provided. Should we exit without allowing the slave to start, or should we default to a sane value?
> > Just a few questions and comments:
> > - At the top of the review page, just below "Submitter: ASHUTOSH JAIN", please set "Branch: master" and "Bugs: MESOS-969".
> > - As for testing, did you manually test that your changes fixed the problem you originally ran into? Can you write a unit test to represent that?
1. Initially I did the check in containerizer.cpp and if flags were more than the main memory then I set mem to total memory of system. But, this lead to failure of DFR Allocation test and Resource Reservation tests so Bmahler suggested to do this in main.cpp .
2. I did a manual testing to verify the changes. I can try writing unit test with some help from your side. Just point me to how to proceed (preferably some example to how to write a test and where to write it)
> On March 17, 2014, 4:28 a.m., Adam B wrote:
> > src/slave/main.cpp, lines 138-140
> > <https://reviews.apache.org/r/19263/diff/2/?file=521464#file521464line138>
> >
> > The entire code block you added should be inside an if (flags.resources.isSome()) {}, since you shouldn't bother with anything if flags.resources isNone.
I will modify this .
> On March 17, 2014, 4:28 a.m., Adam B wrote:
> > src/slave/main.cpp, lines 143-144
> > <https://reviews.apache.org/r/19263/diff/2/?file=521464#file521464line143>
> >
> > This should be an EXIT(1) if you cannot continue to launch the slave without this; or you could just LOG(ERROR) (preferable over cerr), and recover gracefully (set a default, or ignore?).
I will modify this .
> On March 17, 2014, 4:28 a.m., Adam B wrote:
> > src/slave/main.cpp, lines 153-155
> > <https://reviews.apache.org/r/19263/diff/2/?file=521464#file521464line153>
> >
> > Bytes mem = resources.mem().get();
I will update this.
> On March 17, 2014, 4:28 a.m., Adam B wrote:
> > src/slave/main.cpp, lines 158-168
> > <https://reviews.apache.org/r/19263/diff/2/?file=521464#file521464line158>
> >
> > You're not actually "defaulting to DEFAULT_MEM" here, you're just warning then exiting. You need to modify the value of flags.resources so that the Containerizer and Slave created below will use your new values in 'flags.resources'.
I will update this.
- ASHUTOSH
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/#review37335
-----------------------------------------------------------
On March 16, 2014, 3:23 p.m., ASHUTOSH JAIN wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19263/
> -----------------------------------------------------------
>
> (Updated March 16, 2014, 3:23 p.m.)
>
>
> Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> resources (mem) provided by user not being checked
>
> link to issue: https://issues.apache.org/jira/browse/MESOS-969
>
>
> Diffs
> -----
>
> src/slave/main.cpp 8c2b70c
>
> Diff: https://reviews.apache.org/r/19263/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> ASHUTOSH JAIN
>
>
Re: Review Request 19263: a check on resources (mem) provided by user
Posted by Adam B <ad...@mesosphere.io>.
> On March 16, 2014, 9:28 p.m., Adam B wrote:
> > Good start, Ashutosh. I think validating the sanity of the resources flag is vitally important, but I have my concerns about whether this is best done in slave/main.cpp, or in Containerizer or Slave itself. I think we should also discuss the desired behavior when an invalid resource flag value is provided. Should we exit without allowing the slave to start, or should we default to a sane value?
> > Just a few questions and comments:
> > - At the top of the review page, just below "Submitter: ASHUTOSH JAIN", please set "Branch: master" and "Bugs: MESOS-969".
> > - As for testing, did you manually test that your changes fixed the problem you originally ran into? Can you write a unit test to represent that?
>
> ASHUTOSH JAIN wrote:
> 1. Initially I did the check in containerizer.cpp and if flags were more than the main memory then I set mem to total memory of system. But, this lead to failure of DFR Allocation test and Resource Reservation tests so Bmahler suggested to do this in main.cpp .
> 2. I did a manual testing to verify the changes. I can try writing unit test with some help from your side. Just point me to how to proceed (preferably some example to how to write a test and where to write it)
>
> Ben Mahler wrote:
> Adam, to your point of exiting vs defaulting: we do not want to modify resources that are specified explicitly in the flags. That is, if an operator asks the slave to allocate 8GB, and the machine has 6GB, it would be preferable to let them know immediately rather than silently offer only 6GB. Otherwise, this could prove very confusing for operators.
>
> However, we can use definitely use defaults if the resources are unspecified.
* Agree with BenM. Exit if flags resources exceeds actual resources; use defaults if flags doesn't specify resources.
- Testing: Why were your changes causing failures in DRFAllocation/ResourceReservation tests? Maybe we need to mock out os::memory() so that the tests use resources from a mocked system instead of whatever machine is running the test (which would have unpredictable available resources).
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/#review37335
-----------------------------------------------------------
On March 17, 2014, 10:21 a.m., ASHUTOSH JAIN wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19263/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 10:21 a.m.)
>
>
> Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
>
>
> Bugs: MESOS-969
> https://issues.apache.org/jira/browse/MESOS-969
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> resources (mem) provided by user not being checked
>
> link to issue: https://issues.apache.org/jira/browse/MESOS-969
>
>
> Diffs
> -----
>
> src/slave/main.cpp 8c2b70c
>
> Diff: https://reviews.apache.org/r/19263/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> ASHUTOSH JAIN
>
>
Re: Review Request 19263: a check on resources (mem) provided by user
Posted by Ben Mahler <be...@gmail.com>.
> On March 17, 2014, 4:28 a.m., Adam B wrote:
> > Good start, Ashutosh. I think validating the sanity of the resources flag is vitally important, but I have my concerns about whether this is best done in slave/main.cpp, or in Containerizer or Slave itself. I think we should also discuss the desired behavior when an invalid resource flag value is provided. Should we exit without allowing the slave to start, or should we default to a sane value?
> > Just a few questions and comments:
> > - At the top of the review page, just below "Submitter: ASHUTOSH JAIN", please set "Branch: master" and "Bugs: MESOS-969".
> > - As for testing, did you manually test that your changes fixed the problem you originally ran into? Can you write a unit test to represent that?
>
> ASHUTOSH JAIN wrote:
> 1. Initially I did the check in containerizer.cpp and if flags were more than the main memory then I set mem to total memory of system. But, this lead to failure of DFR Allocation test and Resource Reservation tests so Bmahler suggested to do this in main.cpp .
> 2. I did a manual testing to verify the changes. I can try writing unit test with some help from your side. Just point me to how to proceed (preferably some example to how to write a test and where to write it)
Adam, to your point of exiting vs defaulting: we do not want to modify resources that are specified explicitly in the flags. That is, if an operator asks the slave to allocate 8GB, and the machine has 6GB, it would be preferable to let them know immediately rather than silently offer only 6GB. Otherwise, this could prove very confusing for operators.
However, we can use definitely use defaults if the resources are unspecified.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/#review37335
-----------------------------------------------------------
On March 17, 2014, 5:21 p.m., ASHUTOSH JAIN wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19263/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 5:21 p.m.)
>
>
> Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
>
>
> Bugs: MESOS-969
> https://issues.apache.org/jira/browse/MESOS-969
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> resources (mem) provided by user not being checked
>
> link to issue: https://issues.apache.org/jira/browse/MESOS-969
>
>
> Diffs
> -----
>
> src/slave/main.cpp 8c2b70c
>
> Diff: https://reviews.apache.org/r/19263/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> ASHUTOSH JAIN
>
>
Re: Review Request 19263: a check on resources (mem) provided by user
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19263/#review37335
-----------------------------------------------------------
Good start, Ashutosh. I think validating the sanity of the resources flag is vitally important, but I have my concerns about whether this is best done in slave/main.cpp, or in Containerizer or Slave itself. I think we should also discuss the desired behavior when an invalid resource flag value is provided. Should we exit without allowing the slave to start, or should we default to a sane value?
Just a few questions and comments:
- At the top of the review page, just below "Submitter: ASHUTOSH JAIN", please set "Branch: master" and "Bugs: MESOS-969".
- As for testing, did you manually test that your changes fixed the problem you originally ran into? Can you write a unit test to represent that?
src/slave/main.cpp
<https://reviews.apache.org/r/19263/#comment68853>
The entire code block you added should be inside an if (flags.resources.isSome()) {}, since you shouldn't bother with anything if flags.resources isNone.
src/slave/main.cpp
<https://reviews.apache.org/r/19263/#comment68854>
This should be an EXIT(1) if you cannot continue to launch the slave without this; or you could just LOG(ERROR) (preferable over cerr), and recover gracefully (set a default, or ignore?).
src/slave/main.cpp
<https://reviews.apache.org/r/19263/#comment68855>
Bytes mem = resources.mem().get();
src/slave/main.cpp
<https://reviews.apache.org/r/19263/#comment68852>
You're not actually "defaulting to DEFAULT_MEM" here, you're just warning then exiting. You need to modify the value of flags.resources so that the Containerizer and Slave created below will use your new values in 'flags.resources'.
src/slave/main.cpp
<https://reviews.apache.org/r/19263/#comment68850>
Should we really exit here, or just default to mem_.total, perhaps minus some buffer, like what happens in Containerizer::resources()? Can we refactor/reuse that code?
- Adam B
On March 16, 2014, 8:23 a.m., ASHUTOSH JAIN wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19263/
> -----------------------------------------------------------
>
> (Updated March 16, 2014, 8:23 a.m.)
>
>
> Review request for mesos, Adam B, Adam Berry, and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> resources (mem) provided by user not being checked
>
> link to issue: https://issues.apache.org/jira/browse/MESOS-969
>
>
> Diffs
> -----
>
> src/slave/main.cpp 8c2b70c
>
> Diff: https://reviews.apache.org/r/19263/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> ASHUTOSH JAIN
>
>