Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Jie Yu]

> On March 16, 2016, 11:08 p.m., Jie Yu wrote:
> > This patch breaks all the ROOT DOCKER tests in our internal CI. I've 
> > reverted it for now. Can you do a sudo make check with docker?
> 
> Steve Niemitz wrote:
> ok, I see what the problem is here.  The issue is with the 
> mesos-docker-executor code path (when launching a task w/ a TaskInfo, 
> launchExecutorProcess).  Because the code path only waits for the 
> mesos-docker-executor process to launch, the inspect in update() fails since 
> the docker container hasn't launched yet.
> 
> I'm not sure what the best solution is here, I'll play around with some 
> options.
> 
> Steve Niemitz wrote:
> Also, interestingly enough, if you remove the check in update() the 
> checks if the resources have changed, even without my patch it breaks because 
> the container isn't there yet, so this is even now just kind of accidentally 
> working.
> 
> Jie Yu wrote:
> Yeah, looks like the whole 'update' is problematic for the command task 
> case. I'll think about that tomorrow to see if there is a clean solution. 
> `docker->inspect` supports a retry interval (optional), basically, it'll wait 
> until the container is up. But the problem with that is: if there is error in 
> the executor, it'll wait forever. Maybe we could setup a timeout for that.
> 
> Steve Niemitz wrote:
> I thought about the inspect retry option, but it seems like it'll 
> introduce a lot more complexity (like you said, a timeout, etc).
> 
> How do you feel about commiting this w/ just the executor code path, and 
> fixing up the command task case in a new review?

Yeah, that sounds good to me.


- Jie


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


On March 16, 2016, 8:10 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated March 16, 2016, 8:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
>   src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

> On March 16, 2016, 11:08 p.m., Jie Yu wrote:
> > This patch breaks all the ROOT DOCKER tests in our internal CI. I've 
> > reverted it for now. Can you do a sudo make check with docker?
> 
> Steve Niemitz wrote:
> ok, I see what the problem is here.  The issue is with the 
> mesos-docker-executor code path (when launching a task w/ a TaskInfo, 
> launchExecutorProcess).  Because the code path only waits for the 
> mesos-docker-executor process to launch, the inspect in update() fails since 
> the docker container hasn't launched yet.
> 
> I'm not sure what the best solution is here, I'll play around with some 
> options.
> 
> Steve Niemitz wrote:
> Also, interestingly enough, if you remove the check in update() the 
> checks if the resources have changed, even without my patch it breaks because 
> the container isn't there yet, so this is even now just kind of accidentally 
> working.
> 
> Jie Yu wrote:
> Yeah, looks like the whole 'update' is problematic for the command task 
> case. I'll think about that tomorrow to see if there is a clean solution. 
> `docker->inspect` supports a retry interval (optional), basically, it'll wait 
> until the container is up. But the problem with that is: if there is error in 
> the executor, it'll wait forever. Maybe we could setup a timeout for that.

I thought about the inspect retry option, but it seems like it'll introduce a 
lot more complexity (like you said, a timeout, etc).

How do you feel about commiting this w/ just the executor code path, and fixing 
up the command task case in a new review?


- Steve


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


On March 16, 2016, 8:10 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated March 16, 2016, 8:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
>   src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Jie Yu]

> On March 16, 2016, 11:08 p.m., Jie Yu wrote:
> > This patch breaks all the ROOT DOCKER tests in our internal CI. I've 
> > reverted it for now. Can you do a sudo make check with docker?
> 
> Steve Niemitz wrote:
> ok, I see what the problem is here.  The issue is with the 
> mesos-docker-executor code path (when launching a task w/ a TaskInfo, 
> launchExecutorProcess).  Because the code path only waits for the 
> mesos-docker-executor process to launch, the inspect in update() fails since 
> the docker container hasn't launched yet.
> 
> I'm not sure what the best solution is here, I'll play around with some 
> options.
> 
> Steve Niemitz wrote:
> Also, interestingly enough, if you remove the check in update() the 
> checks if the resources have changed, even without my patch it breaks because 
> the container isn't there yet, so this is even now just kind of accidentally 
> working.

Yeah, looks like the whole 'update' is problematic for the command task case. 
I'll think about that tomorrow to see if there is a clean solution. 
`docker->inspect` supports a retry interval (optional), basically, it'll wait 
until the container is up. But the problem with that is: if there is error in 
the executor, it'll wait forever. Maybe we could setup a timeout for that.


- Jie


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


On March 16, 2016, 8:10 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated March 16, 2016, 8:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
>   src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

> On March 16, 2016, 11:08 p.m., Jie Yu wrote:
> > This patch breaks all the ROOT DOCKER tests in our internal CI. I've 
> > reverted it for now. Can you do a sudo make check with docker?
> 
> Steve Niemitz wrote:
> ok, I see what the problem is here.  The issue is with the 
> mesos-docker-executor code path (when launching a task w/ a TaskInfo, 
> launchExecutorProcess).  Because the code path only waits for the 
> mesos-docker-executor process to launch, the inspect in update() fails since 
> the docker container hasn't launched yet.
> 
> I'm not sure what the best solution is here, I'll play around with some 
> options.

Also, interestingly enough, if you remove the check in update() the checks if 
the resources have changed, even without my patch it breaks because the 
container isn't there yet, so this is even now just kind of accidentally 
working.


- Steve


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


On March 16, 2016, 8:10 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated March 16, 2016, 8:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
>   src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Jie Yu]

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



This patch breaks all the ROOT DOCKER tests in our internal CI. I've reverted 
it for now. Can you do a sudo make check with docker?

- Jie Yu


On March 16, 2016, 8:10 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated March 16, 2016, 8:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
>   src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Jie Yu]

> On March 2, 2016, 2:55 p.m., Steve Niemitz wrote:
> > @jieyu ping?

Sorry about the delay. Just saw this. Will get to it today. Can you ping me 
using email directly next time. The volume of emails from rb is too large so I 
tend to ignore them.


- Jie


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


On Feb. 20, 2016, 6:47 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Feb. 20, 2016, 6:47 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

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

(Updated March 16, 2016, 7:15 p.m.)


Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.


Changes
---

Fixed a regression introduced in refactoring _update() to update()


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


Repository: mesos


Description
---

Fix for docker containerizer not configuring CFS quotas correctly.

It would be nice to refactor all this isolation code in a way that can be 
shared between all containerizers, as this is basically just copied from the 
CgroupsCpushareIsolator, but that's a much bigger undertaking.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
  src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 

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


Testing
---


Thanks,

Steve Niemitz

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/docker.cpp (lines 1069 - 1076)


The indentation here should be 2 (instead of 4) since you're at it (I was 
planning to fix it for you before commit).



src/slave/containerizer/docker.cpp (line 1075)


The indentation here should be 2 as well.



src/slave/containerizer/docker.cpp (line 1321)


Can you add a NOTE about what does 'force' mean in the header file?


- Jie Yu


On March 16, 2016, 7:15 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated March 16, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
>   src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Jie Yu]

> On March 16, 2016, 4:27 p.m., Jie Yu wrote:
> > Ship It!

Can you confirm that this patch works in your environment?


- Jie


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


On Feb. 20, 2016, 6:47 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Feb. 20, 2016, 6:47 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 20, 2016, 6:47 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Feb. 20, 2016, 6:47 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

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

(Updated March 16, 2016, 8:10 p.m.)


Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

Fix for docker containerizer not configuring CFS quotas correctly.

It would be nice to refactor all this isolation code in a way that can be 
shared between all containerizers, as this is basically just copied from the 
CgroupsCpushareIsolator, but that's a much bigger undertaking.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
  src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 

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


Testing
---


Thanks,

Steve Niemitz

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

> On March 16, 2016, 4:27 p.m., Jie Yu wrote:
> > Ship It!
> 
> Jie Yu wrote:
> Can you confirm that this patch works in your environment?

Actually, it looks like the change to use update() instead of _update 
introduced a bug (the cgroups no longer get updated because the resources don't 
change).  I'll submit a fix in a minute.


- Steve


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


On Feb. 20, 2016, 6:47 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Feb. 20, 2016, 6:47 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

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



@jieyu ping?

- Steve Niemitz


On Feb. 20, 2016, 6:47 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Feb. 20, 2016, 6:47 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

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

(Updated Feb. 20, 2016, 6:47 p.m.)


Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

Fix for docker containerizer not configuring CFS quotas correctly.

It would be nice to refactor all this isolation code in a way that can be 
shared between all containerizers, as this is basically just copied from the 
CgroupsCpushareIsolator, but that's a much bigger undertaking.


Diffs (updated)
-

  src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 

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


Testing
---


Thanks,

Steve Niemitz

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

> On Feb. 18, 2016, 11:15 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 811-815
> > 
> >
> > This is done after the container has been launched. We should 
> > definitely call out the fact that there's a short window after the 
> > container has been launched before the container's cfs quota is updated. 
> > During that window, it's not subject to cfs throttling.
> > 
> > Alternatively, we could set `--cpu-period` and `--cpu-quota` during 
> > launch so that we don't have to worry about that window.
> 
> Steve Niemitz wrote:
> Is there a minimum supported version of docker for mesos?  Those 
> parameters were added in docker 1.7 I believe.
> 
> Jie Yu wrote:
> Yeah, I think for now, add a TODO should be sufficient. Once we bump the 
> minimum docker version support, we can address that TODO.

sgtm


- Steve


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


On Dec. 15, 2015, 8:14 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Dec. 15, 2015, 8:14 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

> On Feb. 18, 2016, 11:15 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, line 785
> > 
> >
> > Do you need to call update for this case (command task but agent is not 
> > running in a docker container)?
> 
> Steve Niemitz wrote:
> The command executor is a totally different code path, so calling update 
> wouldn't update the right thing.  I'd like to handle that in a seperate 
> review since its more complicated.
> 
> Jie Yu wrote:
> Hum, why do you think it's a totally different code path? 'update' will 
> identify the correct container (the container launched by the command 
> executor) by calling docker inspect using container->name(). The way we 
> generate container name is a little implicit in the code. I think you can 
> just do an 'update' after launch the executor process similar to the custom 
> executor's case. Or am I missing something?

Yeah you're totally right!  Thanks for clarifying that.  I'll make the changes 
here too.  That's awesome this should work for both code paths now.


- Steve


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


On Dec. 15, 2015, 8:14 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Dec. 15, 2015, 8:14 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Jie Yu]

> On Feb. 18, 2016, 11:15 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 811-815
> > 
> >
> > This is done after the container has been launched. We should 
> > definitely call out the fact that there's a short window after the 
> > container has been launched before the container's cfs quota is updated. 
> > During that window, it's not subject to cfs throttling.
> > 
> > Alternatively, we could set `--cpu-period` and `--cpu-quota` during 
> > launch so that we don't have to worry about that window.
> 
> Steve Niemitz wrote:
> Is there a minimum supported version of docker for mesos?  Those 
> parameters were added in docker 1.7 I believe.

Yeah, I think for now, add a TODO should be sufficient. Once we bump the 
minimum docker version support, we can address that TODO.


> On Feb. 18, 2016, 11:15 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, line 785
> > 
> >
> > Do you need to call update for this case (command task but agent is not 
> > running in a docker container)?
> 
> Steve Niemitz wrote:
> The command executor is a totally different code path, so calling update 
> wouldn't update the right thing.  I'd like to handle that in a seperate 
> review since its more complicated.

Hum, why do you think it's a totally different code path? 'update' will 
identify the correct container (the container launched by the command executor) 
by calling docker inspect using container->name(). The way we generate 
container name is a little implicit in the code. I think you can just do an 
'update' after launch the executor process similar to the custom executor's 
case. Or am I missing something?


- Jie


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


On Dec. 15, 2015, 8:14 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Dec. 15, 2015, 8:14 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

> On Feb. 18, 2016, 11:15 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, line 785
> > 
> >
> > Do you need to call update for this case (command task but agent is not 
> > running in a docker container)?

The command executor is a totally different code path, so calling update 
wouldn't update the right thing.  I'd like to handle that in a seperate review 
since its more complicated.


> On Feb. 18, 2016, 11:15 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 811-815
> > 
> >
> > This is done after the container has been launched. We should 
> > definitely call out the fact that there's a short window after the 
> > container has been launched before the container's cfs quota is updated. 
> > During that window, it's not subject to cfs throttling.
> > 
> > Alternatively, we could set `--cpu-period` and `--cpu-quota` during 
> > launch so that we don't have to worry about that window.

Is there a minimum supported version of docker for mesos?  Those parameters 
were added in docker 1.7 I believe.


- Steve


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


On Dec. 15, 2015, 8:14 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Dec. 15, 2015, 8:14 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Jie Yu]

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




src/slave/containerizer/docker.cpp (line 785)


Do you need to call update for this case (command task but agent is not 
running in a docker container)?



src/slave/containerizer/docker.cpp (lines 811 - 815)


This is done after the container has been launched. We should definitely 
call out the fact that there's a short window after the container has been 
launched before the container's cfs quota is updated. During that window, it's 
not subject to cfs throttling.

Alternatively, we could set `--cpu-period` and `--cpu-quota` during launch 
so that we don't have to worry about that window.



src/slave/containerizer/docker.cpp (line 812)


Ditto on using 'update()' here.



src/slave/containerizer/docker.cpp (lines 1131 - 1133)


Nits on style:
```
write = cgroups::cpu::cfs_period_us(
cpuHierarchy.get(),
cpuCgroup.get(),
CPU_CFS_PERIOD);

  
```



src/slave/containerizer/docker.cpp (lines 1142 - 1144)


Ditto on style:

```
write = cgroups::cpu::cfs_quota_us(
cpuHierarchy.get(),
cpuCgroup.get(),
quota);
```


- Jie Yu


On Dec. 15, 2015, 8:14 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Dec. 15, 2015, 8:14 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Ian Downes]

> On Dec. 15, 2015, 11:36 a.m., Ian Downes wrote:
> > src/slave/containerizer/docker.cpp, line 838
> > 
> >
> > Docker supports specifying the CFS period and quota to run a container 
> > with these flags {{--cpu-period=0}} and {{--cpu-quota=0}}. The correct 
> > solution is to set these using Docker itself when it runs the container. 
> > Subsequent updates will be done by Mesos in the update() call, as below in 
> > this review.

Please add a TODO for this.


> On Dec. 15, 2015, 11:36 a.m., Ian Downes wrote:
> > src/slave/containerizer/docker.cpp, line 976
> > 
> >
> > To reiterate my earlier comment: 
> > 1) We don't support changing the cfs period so it only needs to be set 
> > once, additional writes are unnecessary but aren't a problem. The 
> > MesosContainerizer isolator just writes it on every update rather than 
> > determining if it has already written it before. 
> > 2) The cfs quota changes depending on the CPU resource value so it 
> > definitely does need to be re-written at every update. Again, this is a 
> > straight copy-and-paste from the MC cpu isolator.
> > 
> > This part of the code looks fine to me. @Tim, do you concur?

@tnachen?


- Ian


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


On Dec. 15, 2015, 12:14 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Dec. 15, 2015, 12:14 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Ian Downes]

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




src/slave/containerizer/docker.cpp (line 812)


Why not call update() here, rather than _update()? It will determine the 
correct pid, just launched in launchExecutorContainer().


- Ian Downes


On Dec. 15, 2015, 12:14 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated Dec. 15, 2015, 12:14 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Ian Downes]

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


Initial CFS parameters should be specified to Docker using the appropriate 
flags, not tacked onto the end of launch() where we don't yet know the cgroup. 
Subsequent updates done by Mesos in update().


src/slave/containerizer/docker.cpp (line 838)


Docker supports specifying the CFS period and quota to run a container with 
these flags {{--cpu-period=0}} and {{--cpu-quota=0}}. The correct solution is 
to set these using Docker itself when it runs the container. Subsequent updates 
will be done by Mesos in the update() call, as below in this review.



src/slave/containerizer/docker.cpp (line 839)


FYI, it would be {{.then([]() { return true; });}} but this piece of code 
isn't needed, see previous comment.



src/slave/containerizer/docker.cpp (line 976)


To reiterate my earlier comment: 
1) We don't support changing the cfs period so it only needs to be set 
once, additional writes are unnecessary but aren't a problem. The 
MesosContainerizer isolator just writes it on every update rather than 
determining if it has already written it before. 
2) The cfs quota changes depending on the CPU resource value so it 
definitely does need to be re-written at every update. Again, this is a 
straight copy-and-paste from the MC cpu isolator.

This part of the code looks fine to me. @Tim, do you concur?


- Ian Downes


On April 14, 2015, 1:32 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated April 14, 2015, 1:32 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 5f4b4ce49a9523e4743e5c79da4050e6f9e29ed7 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Steve Niemitz]

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

(Updated Dec. 15, 2015, 8:14 p.m.)


Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.


Changes
---

Rebased changes to ToT


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


Repository: mesos


Description
---

Fix for docker containerizer not configuring CFS quotas correctly.

It would be nice to refactor all this isolation code in a way that can be 
shared between all containerizers, as this is basically just copied from the 
CgroupsCpushareIsolator, but that's a much bigger undertaking.


Diffs (updated)
-

  src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 

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


Testing
---


Thanks,

Steve Niemitz