You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2017/11/06 23:18:33 UTC
Re: Review Request 63276: Windows: Added `Cpu` and `Mem` isolators.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63276/
-----------------------------------------------------------
(Updated Nov. 6, 2017, 3:18 p.m.)
Review request for mesos, Aaron Wood, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, Joseph Wu, Li Li, and Michael Park.
Changes
-------
Renamed `cpuset` to `cpu`.
Summary (updated)
-----------------
Windows: Added `Cpu` and `Mem` isolators.
Bugs: MESOS-5462 and MESOS-6690
https://issues.apache.org/jira/browse/MESOS-5462
https://issues.apache.org/jira/browse/MESOS-6690
Repository: mesos
Description (updated)
-------
Instead of deriving from the POSIX isolators, we now have two real
Windows isolators that can be used together or separately. The `Cpu`
isolator enables a hard-cap CPU limit, and the `Mem` isolator enables a
hard-cap memory limit on the job object for the container.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
src/slave/containerizer/mesos/isolators/windows.hpp b0621a5fc411b8812722f7fcf6580ed64dac5382
src/slave/flags.cpp 63aaac218fdc067742d39f1fc8497723784d9595
Diff: https://reviews.apache.org/r/63276/diff/2/
Changes: https://reviews.apache.org/r/63276/diff/1-2/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 63276: Windows: Added `Cpu` and `Mem` isolators.
Posted by Akash Gupta <ak...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63276/#review192412
-----------------------------------------------------------
Ship it!
Ship It!
- Akash Gupta
On Nov. 30, 2017, 11:34 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63276/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2017, 11:34 p.m.)
>
>
> Review request for mesos, Aaron Wood, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Bugs: MESOS-5462 and MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-5462
> https://issues.apache.org/jira/browse/MESOS-6690
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Instead of deriving from the POSIX isolators, we now have two real
> Windows isolators that can be used together or separately. The `Cpu`
> isolator enables a hard-cap CPU limit, and the `Mem` isolator enables a
> hard-cap memory limit on the job object for the container.
>
> These classes are separate derivations of `MesosIsolatorProcess`,
> because introducing a `WindowsIsolatorProcess` base class would be
> abstraction for the sole purpose of code deduplication.
>
> Note that these isolators support nesting, and so must support empty
> `cpu` or `mem` resources. When these are not provided, the corresponding
> code to set the limits is simply not called.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 76ef6cac925e36a31e4f6c347271955411284056
> src/Makefile.am 4a3b728dc2cbfaf4b10068562778a87a6b331238
> src/slave/containerizer/mesos/containerizer.cpp 7f3b86d87cf82429c2627d4a32eb0d5adbcc3f29
> src/slave/containerizer/mesos/isolators/windows.hpp b0621a5fc411b8812722f7fcf6580ed64dac5382
> src/slave/containerizer/mesos/isolators/windows/cpu.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/cpu.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/mem.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/mem.cpp PRE-CREATION
> src/slave/flags.cpp 7998d9bb66995ad0d1285fcb1042321afdf917a9
>
>
> Diff: https://reviews.apache.org/r/63276/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 63276: Windows: Added `Cpu` and `Mem` isolators.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Nov. 30, 2017, 5:13 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/windows/cpu.hpp
> > Lines 66-67 (patched)
> > <https://reviews.apache.org/r/63276/diff/4/?file=1905078#file1905078line66>
> >
> > I'd prefer we define an `Info` struct that has both pid and cpuLimit so that we know the lifecycle of these two fields are tied together. THis is the conversion we've been following in other isolators.
> > ```
> > struct Info'
> > {
> > Option<pid_t> pid;
> > Option<double> cpuLimit;
> > };
> >
> > hashmap<ContainerID, Info> infos;
> > ```
Yeah, this worked out much better! Thanks. Improvments posted here: https://reviews.apache.org/r/64272/
> On Nov. 30, 2017, 5:13 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/windows/cpu.cpp
> > Lines 75 (patched)
> > <https://reviews.apache.org/r/63276/diff/4/?file=1905079#file1905079line75>
> >
> > So someone can call `prepare` multiple times if they don't set cpu limits? This should be addressed if you use the Info suggestion above.
Good catch. This was remedied using the `Info` abstraction.
> On Nov. 30, 2017, 5:13 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/windows/cpu.cpp
> > Lines 79 (patched)
> > <https://reviews.apache.org/r/63276/diff/4/?file=1905079#file1905079line79>
> >
> > I'd use
> > ```
> > const Resources resources = containerConfig.resources();
> > ```
> >
> > Using `{...}` is fragile if we add more fields to `Resources` object.
I looked it up and couldn't figure out the case where this would become an error, because so long as there isn't a `std::initializer_list` constructor (there isn't, yet, and `T` here isn't an aggregate type as we define ctors for it), then:
> All constructors of T participate in overload resolution against the set of arguments that consists of the elements of the braced-init-list
But I fixed it anyway as there must be and edge case I don't understand.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63276/#review192414
-----------------------------------------------------------
On Nov. 30, 2017, 3:34 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63276/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2017, 3:34 p.m.)
>
>
> Review request for mesos, Aaron Wood, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Bugs: MESOS-5462 and MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-5462
> https://issues.apache.org/jira/browse/MESOS-6690
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Instead of deriving from the POSIX isolators, we now have two real
> Windows isolators that can be used together or separately. The `Cpu`
> isolator enables a hard-cap CPU limit, and the `Mem` isolator enables a
> hard-cap memory limit on the job object for the container.
>
> These classes are separate derivations of `MesosIsolatorProcess`,
> because introducing a `WindowsIsolatorProcess` base class would be
> abstraction for the sole purpose of code deduplication.
>
> Note that these isolators support nesting, and so must support empty
> `cpu` or `mem` resources. When these are not provided, the corresponding
> code to set the limits is simply not called.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 76ef6cac925e36a31e4f6c347271955411284056
> src/Makefile.am 4a3b728dc2cbfaf4b10068562778a87a6b331238
> src/slave/containerizer/mesos/containerizer.cpp 7f3b86d87cf82429c2627d4a32eb0d5adbcc3f29
> src/slave/containerizer/mesos/isolators/windows.hpp b0621a5fc411b8812722f7fcf6580ed64dac5382
> src/slave/containerizer/mesos/isolators/windows/cpu.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/cpu.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/mem.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/mem.cpp PRE-CREATION
> src/slave/flags.cpp 7998d9bb66995ad0d1285fcb1042321afdf917a9
>
>
> Diff: https://reviews.apache.org/r/63276/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 63276: Windows: Added `Cpu` and `Mem` isolators.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63276/#review192414
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/windows/cpu.hpp
Lines 66-67 (patched)
<https://reviews.apache.org/r/63276/#comment270549>
I'd prefer we define an `Info` struct that has both pid and cpuLimit so that we know the lifecycle of these two fields are tied together. THis is the conversion we've been following in other isolators.
```
struct Info'
{
Option<pid_t> pid;
Option<double> cpuLimit;
};
hashmap<ContainerID, Info> infos;
```
src/slave/containerizer/mesos/isolators/windows/cpu.cpp
Lines 53 (patched)
<https://reviews.apache.org/r/63276/#comment270531>
We typically do not use namespace prefix like `std::` or `mesos::slave::` in the cpp file. We use `using` clause on the top of the source file to make the code more concise.
Please fix all in this patch.
src/slave/containerizer/mesos/isolators/windows/cpu.cpp
Lines 75 (patched)
<https://reviews.apache.org/r/63276/#comment270558>
So someone can call `prepare` multiple times if they don't set cpu limits? This should be addressed if you use the Info suggestion above.
src/slave/containerizer/mesos/isolators/windows/cpu.cpp
Lines 79 (patched)
<https://reviews.apache.org/r/63276/#comment270535>
I'd use
```
const Resources resources = containerConfig.resources();
```
Using `{...}` is fragile if we add more fields to `Resources` object.
src/slave/containerizer/mesos/isolators/windows/mem.hpp
Lines 67-68 (patched)
<https://reviews.apache.org/r/63276/#comment270560>
Ditto.
- Jie Yu
On Nov. 30, 2017, 11:34 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63276/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2017, 11:34 p.m.)
>
>
> Review request for mesos, Aaron Wood, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Bugs: MESOS-5462 and MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-5462
> https://issues.apache.org/jira/browse/MESOS-6690
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Instead of deriving from the POSIX isolators, we now have two real
> Windows isolators that can be used together or separately. The `Cpu`
> isolator enables a hard-cap CPU limit, and the `Mem` isolator enables a
> hard-cap memory limit on the job object for the container.
>
> These classes are separate derivations of `MesosIsolatorProcess`,
> because introducing a `WindowsIsolatorProcess` base class would be
> abstraction for the sole purpose of code deduplication.
>
> Note that these isolators support nesting, and so must support empty
> `cpu` or `mem` resources. When these are not provided, the corresponding
> code to set the limits is simply not called.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 76ef6cac925e36a31e4f6c347271955411284056
> src/Makefile.am 4a3b728dc2cbfaf4b10068562778a87a6b331238
> src/slave/containerizer/mesos/containerizer.cpp 7f3b86d87cf82429c2627d4a32eb0d5adbcc3f29
> src/slave/containerizer/mesos/isolators/windows.hpp b0621a5fc411b8812722f7fcf6580ed64dac5382
> src/slave/containerizer/mesos/isolators/windows/cpu.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/cpu.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/mem.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/mem.cpp PRE-CREATION
> src/slave/flags.cpp 7998d9bb66995ad0d1285fcb1042321afdf917a9
>
>
> Diff: https://reviews.apache.org/r/63276/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 63276: Windows: Added `Cpu` and `Mem` isolators.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63276/
-----------------------------------------------------------
(Updated Nov. 30, 2017, 3:34 p.m.)
Review request for mesos, Aaron Wood, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, Joseph Wu, Li Li, and Michael Park.
Changes
-------
Rebased, fixed style, used correct resources protobuf, fixed to support unspecified resources (required for nested containers and things like the `ContainerDaemon` where a unit test specifies `None()` for resources).
Bugs: MESOS-5462 and MESOS-6690
https://issues.apache.org/jira/browse/MESOS-5462
https://issues.apache.org/jira/browse/MESOS-6690
Repository: mesos
Description (updated)
-------
Instead of deriving from the POSIX isolators, we now have two real
Windows isolators that can be used together or separately. The `Cpu`
isolator enables a hard-cap CPU limit, and the `Mem` isolator enables a
hard-cap memory limit on the job object for the container.
These classes are separate derivations of `MesosIsolatorProcess`,
because introducing a `WindowsIsolatorProcess` base class would be
abstraction for the sole purpose of code deduplication.
Note that these isolators support nesting, and so must support empty
`cpu` or `mem` resources. When these are not provided, the corresponding
code to set the limits is simply not called.
Diffs (updated)
-----
src/CMakeLists.txt 76ef6cac925e36a31e4f6c347271955411284056
src/Makefile.am 4a3b728dc2cbfaf4b10068562778a87a6b331238
src/slave/containerizer/mesos/containerizer.cpp 7f3b86d87cf82429c2627d4a32eb0d5adbcc3f29
src/slave/containerizer/mesos/isolators/windows.hpp b0621a5fc411b8812722f7fcf6580ed64dac5382
src/slave/containerizer/mesos/isolators/windows/cpu.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/windows/cpu.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/windows/mem.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/windows/mem.cpp PRE-CREATION
src/slave/flags.cpp 7998d9bb66995ad0d1285fcb1042321afdf917a9
Diff: https://reviews.apache.org/r/63276/diff/4/
Changes: https://reviews.apache.org/r/63276/diff/3-4/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 63276: Windows: Added `Cpu` and `Mem` isolators.
Posted by Akash Gupta <ak...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63276/#review190966
-----------------------------------------------------------
Ship it!
Ship It!
- Akash Gupta
On Nov. 14, 2017, 1:57 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63276/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2017, 1:57 a.m.)
>
>
> Review request for mesos, Aaron Wood, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Bugs: MESOS-5462 and MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-5462
> https://issues.apache.org/jira/browse/MESOS-6690
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Instead of deriving from the POSIX isolators, we now have two real
> Windows isolators that can be used together or separately. The `Cpu`
> isolator enables a hard-cap CPU limit, and the `Mem` isolator enables a
> hard-cap memory limit on the job object for the container.
>
> These classes are separate derivations of `MesosIsolatorProcess`,
> because introducing a `WindowsIsolatorProcess` base class would be
> abstraction for the sole purpose of code deduplication.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt b0e223121f24ac0dc604dc2c639158b817eec535
> src/Makefile.am 955f01a665019d7750980ff16f126ad63b524594
> src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
> src/slave/containerizer/mesos/isolators/windows.hpp b0621a5fc411b8812722f7fcf6580ed64dac5382
> src/slave/containerizer/mesos/isolators/windows/cpu.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/cpu.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/mem.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/windows/mem.cpp PRE-CREATION
> src/slave/flags.cpp 0eeecdc6023443a99c8e8fe29c2f7bf791c0a36e
>
>
> Diff: https://reviews.apache.org/r/63276/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 63276: Windows: Added `Cpu` and `Mem` isolators.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63276/
-----------------------------------------------------------
(Updated Nov. 13, 2017, 5:57 p.m.)
Review request for mesos, Aaron Wood, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, Joseph Wu, Li Li, and Michael Park.
Changes
-------
Refactored based on Jie's comments.
Bugs: MESOS-5462 and MESOS-6690
https://issues.apache.org/jira/browse/MESOS-5462
https://issues.apache.org/jira/browse/MESOS-6690
Repository: mesos
Description (updated)
-------
Instead of deriving from the POSIX isolators, we now have two real
Windows isolators that can be used together or separately. The `Cpu`
isolator enables a hard-cap CPU limit, and the `Mem` isolator enables a
hard-cap memory limit on the job object for the container.
These classes are separate derivations of `MesosIsolatorProcess`,
because introducing a `WindowsIsolatorProcess` base class would be
abstraction for the sole purpose of code deduplication.
Diffs (updated)
-----
src/CMakeLists.txt b0e223121f24ac0dc604dc2c639158b817eec535
src/Makefile.am 955f01a665019d7750980ff16f126ad63b524594
src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
src/slave/containerizer/mesos/isolators/windows.hpp b0621a5fc411b8812722f7fcf6580ed64dac5382
src/slave/containerizer/mesos/isolators/windows/cpu.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/windows/cpu.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/windows/mem.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/windows/mem.cpp PRE-CREATION
src/slave/flags.cpp 0eeecdc6023443a99c8e8fe29c2f7bf791c0a36e
Diff: https://reviews.apache.org/r/63276/diff/3/
Changes: https://reviews.apache.org/r/63276/diff/2-3/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 63276: Windows: Added `Cpu` and `Mem` isolators.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Nov. 8, 2017, 3:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/windows.hpp
> > Lines 47 (patched)
> > <https://reviews.apache.org/r/63276/diff/2/?file=1882401#file1882401line48>
> >
> > Can you move the implementation to a cpp file (instead of putting in the header)?
Gladly!
> On Nov. 8, 2017, 3:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/windows.hpp
> > Lines 74-82 (patched)
> > <https://reviews.apache.org/r/63276/diff/2/?file=1882401#file1882401line75>
> >
> > That's the default implementation in MesosIsolatorProcess. Simply remove this method?
Good catch.
> On Nov. 8, 2017, 3:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/windows.hpp
> > Lines 35-41 (original), 100-109 (patched)
> > <https://reviews.apache.org/r/63276/diff/2/?file=1882401#file1882401line101>
> >
> > Remove this given this is the default behavor in parent class?
Good catch.
> On Nov. 8, 2017, 3:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/windows.hpp
> > Line 60 (original), 153 (patched)
> > <https://reviews.apache.org/r/63276/diff/2/?file=1882401#file1882401line159>
> >
> > I think we should call `update` in `isolate` too. Otherwise, the process won't be subject to cpu/mem limitation until the next `update` is called.
> >
> > I you think inheritence is hard, maybe just get rid the base class. a little bit code duplication is not too bad and will make it much easier to read.
I'm not sure I totally get you. Joe had told me (that is, if I remember correctly), that both `isolate` and `update` will be called before the task is considered "started" the first time. I guess we should verify this, and if it isn't the case, then `isolate` should also call the same code as `update`.
As for:
> I you think inheritence is hard, maybe just get rid the base class. a little bit code duplication is not too bad and will make it much easier to read.
What design were you suggesting? Currently the `WindowsMemIsolatorProcess` and `WindowsCpuIsolatorProcess` classes inherit from `WindowsIsolatorProcess`, which inherits from `MesosIsolatorProcess`. Are you saying that removing the intermediate class, `WindowsIsolatorProcess`, would look cleaner?
> On Nov. 8, 2017, 3:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/windows.hpp
> > Line 64 (original), 210 (patched)
> > <https://reviews.apache.org/r/63276/diff/2/?file=1882401#file1882401line216>
> >
> > 2 lines apart.
Gotcha.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63276/#review190505
-----------------------------------------------------------
On Nov. 6, 2017, 3:18 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63276/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2017, 3:18 p.m.)
>
>
> Review request for mesos, Aaron Wood, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Bugs: MESOS-5462 and MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-5462
> https://issues.apache.org/jira/browse/MESOS-6690
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Instead of deriving from the POSIX isolators, we now have two real
> Windows isolators that can be used together or separately. The `Cpu`
> isolator enables a hard-cap CPU limit, and the `Mem` isolator enables a
> hard-cap memory limit on the job object for the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
> src/slave/containerizer/mesos/isolators/windows.hpp b0621a5fc411b8812722f7fcf6580ed64dac5382
> src/slave/flags.cpp 63aaac218fdc067742d39f1fc8497723784d9595
>
>
> Diff: https://reviews.apache.org/r/63276/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 63276: Windows: Added `Cpu` and `Mem` isolators.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63276/#review190505
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/windows.hpp
Lines 47 (patched)
<https://reviews.apache.org/r/63276/#comment267935>
Can you move the implementation to a cpp file (instead of putting in the header)?
src/slave/containerizer/mesos/isolators/windows.hpp
Lines 74-82 (patched)
<https://reviews.apache.org/r/63276/#comment267936>
That's the default implementation in MesosIsolatorProcess. Simply remove this method?
src/slave/containerizer/mesos/isolators/windows.hpp
Lines 35-41 (original), 100-109 (patched)
<https://reviews.apache.org/r/63276/#comment267937>
Remove this given this is the default behavor in parent class?
src/slave/containerizer/mesos/isolators/windows.hpp
Line 60 (original), 153 (patched)
<https://reviews.apache.org/r/63276/#comment267943>
I think we should call `update` in `isolate` too. Otherwise, the process won't be subject to cpu/mem limitation until the next `update` is called.
I you think inheritence is hard, maybe just get rid the base class. a little bit code duplication is not too bad and will make it much easier to read.
src/slave/containerizer/mesos/isolators/windows.hpp
Line 64 (original), 210 (patched)
<https://reviews.apache.org/r/63276/#comment267941>
2 lines apart.
- Jie Yu
On Nov. 6, 2017, 11:18 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63276/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2017, 11:18 p.m.)
>
>
> Review request for mesos, Aaron Wood, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Bugs: MESOS-5462 and MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-5462
> https://issues.apache.org/jira/browse/MESOS-6690
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Instead of deriving from the POSIX isolators, we now have two real
> Windows isolators that can be used together or separately. The `Cpu`
> isolator enables a hard-cap CPU limit, and the `Mem` isolator enables a
> hard-cap memory limit on the job object for the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
> src/slave/containerizer/mesos/isolators/windows.hpp b0621a5fc411b8812722f7fcf6580ed64dac5382
> src/slave/flags.cpp 63aaac218fdc067742d39f1fc8497723784d9595
>
>
> Diff: https://reviews.apache.org/r/63276/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>