Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-10 Thread Kapil Arya 

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


Ship it!




Ship It!

- Kapil Arya


On Feb. 9, 2016, 6:47 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43258/
> ---
> 
> (Updated Feb. 9, 2016, 6:47 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kapil Arya.
> 
> 
> Bugs: MESOS-4490
> https://issues.apache.org/jira/browse/MESOS-4490
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified agent to get container status from containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a3830ff460a6f6c5661fb8a0172fae303b245889 
>   src/slave/slave.cpp 9dda3a2c4dc4c355488d34dc8d0606330a756f2a 
>   src/tests/master_tests.cpp 0357b1c259472213181a65e5adbe6d5caa1698ad 
>   src/tests/reconciliation_tests.cpp 1cbc3230d003a84277b91da6470828ebf73ef897 
>   src/tests/slave_tests.cpp b2b1fd4be933512c3dffa8c1c579b59782a37d77 
>   src/tests/status_update_manager_tests.cpp 
> 7bedd499a241a61938069381e0d4fafa4b8f96db 
> 
> Diff: https://reviews.apache.org/r/43258/diff/
> 
> 
> Testing
> ---
> 
> make and make check
> 
> 
> Also ran sudo tests . All tests passed except for these root tests:
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor (this is failing on the 
> master as well, and seems flaky ?)
> CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen  (Since swap was 
> enabled on the system)
> CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS (Since swap was 
> enabled on the system)
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-09 Thread Avinash sridharan 


> On Feb. 9, 2016, 1:33 a.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 3080-3091
> > 
> >
> > This looks problematic to me now as we introduce 'status()' interface 
> > for isolators. What if there's a network isolator that wants to set the 
> > NetworkInfo in ContainerStatus using an IP that's different than the agent 
> > IP (e.g., think about the calico network isolator). In that case, we'll 
> > still set the ip in the NetworkInfo to be the agent IP?
> > 
> > My hunch is that we should combine containerizer->status() with this in 
> > the same function because logicially speaking, both of them are mutating 
> > TaskStatus with additional runtime information (ContainerStatus).
> > 
> > Maybe:
> > ```
> > Future getContainerStatus(const ContainerID& 
> > containerId)
> > {
> >   return containerizer->status()
> > .then([]() {
> >   // Set IP in NetworkInfo if not yet set.
> > });
> > }
> > ```

Modified so that we set the container IP as the IP address of the agent only if 
the NetworkInfo is not set by the isolators after `containerizer->status` has 
been invoked.


> On Feb. 9, 2016, 1:33 a.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 3113-3114
> > 
> >
> > I am not sure if we want to set NetworkInfo in this case. Setting 
> > NetworkInfo.ip to be the agent IP is definitely problematic to me because 
> > it's unclear to me if a network isolator (e.g., calico) is used for the 
> > containr. Blindly returning the agent IP is definitely not correct.

Since, `ContainerStatus` is being updated after `containerizer->status` is 
called we shouldn't face this problem. Though if `NetworkInfo` is set by the 
`HookModules` we might end up hitting this particular case.


- Avinash


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


On Feb. 9, 2016, 8:32 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43258/
> ---
> 
> (Updated Feb. 9, 2016, 8:32 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kapil Arya.
> 
> 
> Bugs: MESOS-4490
> https://issues.apache.org/jira/browse/MESOS-4490
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified agent to get container status from containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a3830ff460a6f6c5661fb8a0172fae303b245889 
>   src/slave/slave.cpp 9dda3a2c4dc4c355488d34dc8d0606330a756f2a 
>   src/tests/master_tests.cpp 0357b1c259472213181a65e5adbe6d5caa1698ad 
>   src/tests/reconciliation_tests.cpp 1cbc3230d003a84277b91da6470828ebf73ef897 
>   src/tests/slave_tests.cpp b2b1fd4be933512c3dffa8c1c579b59782a37d77 
>   src/tests/status_update_manager_tests.cpp 
> 7bedd499a241a61938069381e0d4fafa4b8f96db 
> 
> Diff: https://reviews.apache.org/r/43258/diff/
> 
> 
> Testing
> ---
> 
> make and make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-09 Thread Avinash sridharan 

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

(Updated Feb. 9, 2016, 8:32 p.m.)


Review request for mesos, Jie Yu and Kapil Arya.


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


Repository: mesos


Description
---

Modified agent to get container status from containerizer.


Diffs (updated)
-

  src/slave/slave.hpp a3830ff460a6f6c5661fb8a0172fae303b245889 
  src/slave/slave.cpp 9dda3a2c4dc4c355488d34dc8d0606330a756f2a 
  src/tests/master_tests.cpp 0357b1c259472213181a65e5adbe6d5caa1698ad 
  src/tests/reconciliation_tests.cpp 1cbc3230d003a84277b91da6470828ebf73ef897 
  src/tests/slave_tests.cpp b2b1fd4be933512c3dffa8c1c579b59782a37d77 
  src/tests/status_update_manager_tests.cpp 
7bedd499a241a61938069381e0d4fafa4b8f96db 

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


Testing
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-09 Thread Avinash sridharan 

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

(Updated Feb. 9, 2016, 10:08 p.m.)


Review request for mesos, Jie Yu and Kapil Arya.


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


Repository: mesos


Description
---

Modified agent to get container status from containerizer.


Diffs
-

  src/slave/slave.hpp a3830ff460a6f6c5661fb8a0172fae303b245889 
  src/slave/slave.cpp 9dda3a2c4dc4c355488d34dc8d0606330a756f2a 
  src/tests/master_tests.cpp 0357b1c259472213181a65e5adbe6d5caa1698ad 
  src/tests/reconciliation_tests.cpp 1cbc3230d003a84277b91da6470828ebf73ef897 
  src/tests/slave_tests.cpp b2b1fd4be933512c3dffa8c1c579b59782a37d77 
  src/tests/status_update_manager_tests.cpp 
7bedd499a241a61938069381e0d4fafa4b8f96db 

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


Testing (updated)
---

make and make check


Also ran sudo tests . All tests passed except for these root tests:
DockerContainerizerTest.ROOT_DOCKER_Launch_Executor (this is failing on the 
master as well, and seems flaky ?)
CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen  (Since swap was 
enabled on the system)
CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS (Since swap was enabled 
on the system)


Thanks,

Avinash sridharan

Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-09 Thread Jie Yu 

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


Fix it, then Ship it!





src/slave/slave.cpp (line 3098)


Please add a second NOTE here saying that we don't set the ContainerStatus 
(including the IP address in NetworkInfo) because the container no longer 
exists. We cannot use the slave IP because it's unclear what network isolation 
is used.



src/slave/slave.cpp (line 3153)


s/containerStatus/future/



src/slave/slave.cpp (line 3155)


No need for this. Please use `StatusUpdate update` as the parameter so that 
you can directly modify it



src/slave/slave.cpp (line 3157)


s/container/containerStatus/



src/slave/slave.cpp (lines 3170 - 3183)


I think we should only do this if 'future' is Ready. Otherwise, we cannot 
tell if we should fill IP or not because a network isolator might be used and 
it advertised a different IP.



src/slave/slave.cpp (line 3188)


Kill this line


- Jie Yu


On Feb. 9, 2016, 10:08 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43258/
> ---
> 
> (Updated Feb. 9, 2016, 10:08 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kapil Arya.
> 
> 
> Bugs: MESOS-4490
> https://issues.apache.org/jira/browse/MESOS-4490
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified agent to get container status from containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a3830ff460a6f6c5661fb8a0172fae303b245889 
>   src/slave/slave.cpp 9dda3a2c4dc4c355488d34dc8d0606330a756f2a 
>   src/tests/master_tests.cpp 0357b1c259472213181a65e5adbe6d5caa1698ad 
>   src/tests/reconciliation_tests.cpp 1cbc3230d003a84277b91da6470828ebf73ef897 
>   src/tests/slave_tests.cpp b2b1fd4be933512c3dffa8c1c579b59782a37d77 
>   src/tests/status_update_manager_tests.cpp 
> 7bedd499a241a61938069381e0d4fafa4b8f96db 
> 
> Diff: https://reviews.apache.org/r/43258/diff/
> 
> 
> Testing
> ---
> 
> make and make check
> 
> 
> Also ran sudo tests . All tests passed except for these root tests:
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor (this is failing on the 
> master as well, and seems flaky ?)
> CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen  (Since swap was 
> enabled on the system)
> CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS (Since swap was 
> enabled on the system)
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-09 Thread Avinash sridharan 

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

(Updated Feb. 9, 2016, 11:47 p.m.)


Review request for mesos, Jie Yu and Kapil Arya.


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


Repository: mesos


Description
---

Modified agent to get container status from containerizer.


Diffs (updated)
-

  src/slave/slave.hpp a3830ff460a6f6c5661fb8a0172fae303b245889 
  src/slave/slave.cpp 9dda3a2c4dc4c355488d34dc8d0606330a756f2a 
  src/tests/master_tests.cpp 0357b1c259472213181a65e5adbe6d5caa1698ad 
  src/tests/reconciliation_tests.cpp 1cbc3230d003a84277b91da6470828ebf73ef897 
  src/tests/slave_tests.cpp b2b1fd4be933512c3dffa8c1c579b59782a37d77 
  src/tests/status_update_manager_tests.cpp 
7bedd499a241a61938069381e0d4fafa4b8f96db 

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


Testing
---

make and make check


Also ran sudo tests . All tests passed except for these root tests:
DockerContainerizerTest.ROOT_DOCKER_Launch_Executor (this is failing on the 
master as well, and seems flaky ?)
CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen  (Since swap was 
enabled on the system)
CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS (Since swap was enabled 
on the system)


Thanks,

Avinash sridharan

Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-09 Thread Jie Yu 

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


Ship it!




Ship It!

- Jie Yu


On Feb. 9, 2016, 11:47 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43258/
> ---
> 
> (Updated Feb. 9, 2016, 11:47 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kapil Arya.
> 
> 
> Bugs: MESOS-4490
> https://issues.apache.org/jira/browse/MESOS-4490
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified agent to get container status from containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a3830ff460a6f6c5661fb8a0172fae303b245889 
>   src/slave/slave.cpp 9dda3a2c4dc4c355488d34dc8d0606330a756f2a 
>   src/tests/master_tests.cpp 0357b1c259472213181a65e5adbe6d5caa1698ad 
>   src/tests/reconciliation_tests.cpp 1cbc3230d003a84277b91da6470828ebf73ef897 
>   src/tests/slave_tests.cpp b2b1fd4be933512c3dffa8c1c579b59782a37d77 
>   src/tests/status_update_manager_tests.cpp 
> 7bedd499a241a61938069381e0d4fafa4b8f96db 
> 
> Diff: https://reviews.apache.org/r/43258/diff/
> 
> 
> Testing
> ---
> 
> make and make check
> 
> 
> Also ran sudo tests . All tests passed except for these root tests:
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor (this is failing on the 
> master as well, and seems flaky ?)
> CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen  (Since swap was 
> enabled on the system)
> CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS (Since swap was 
> enabled on the system)
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-09 Thread Avinash sridharan 


> On Feb. 9, 2016, 10:23 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 3186-3199
> > 
> >
> > I think we should only do this if 'future' is Ready. Otherwise, we 
> > cannot tell if we should fill IP or not because a network isolator might be 
> > used and it advertised a different IP.

Agreed.


- Avinash


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


On Feb. 9, 2016, 11:47 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43258/
> ---
> 
> (Updated Feb. 9, 2016, 11:47 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kapil Arya.
> 
> 
> Bugs: MESOS-4490
> https://issues.apache.org/jira/browse/MESOS-4490
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified agent to get container status from containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a3830ff460a6f6c5661fb8a0172fae303b245889 
>   src/slave/slave.cpp 9dda3a2c4dc4c355488d34dc8d0606330a756f2a 
>   src/tests/master_tests.cpp 0357b1c259472213181a65e5adbe6d5caa1698ad 
>   src/tests/reconciliation_tests.cpp 1cbc3230d003a84277b91da6470828ebf73ef897 
>   src/tests/slave_tests.cpp b2b1fd4be933512c3dffa8c1c579b59782a37d77 
>   src/tests/status_update_manager_tests.cpp 
> 7bedd499a241a61938069381e0d4fafa4b8f96db 
> 
> Diff: https://reviews.apache.org/r/43258/diff/
> 
> 
> Testing
> ---
> 
> make and make check
> 
> 
> Also ran sudo tests . All tests passed except for these root tests:
> DockerContainerizerTest.ROOT_DOCKER_Launch_Executor (this is failing on the 
> master as well, and seems flaky ?)
> CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen  (Since swap was 
> enabled on the system)
> CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS (Since swap was 
> enabled on the system)
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-08 Thread Avinash sridharan 

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

(Updated Feb. 9, 2016, 4:16 a.m.)


Review request for mesos, Jie Yu and Kapil Arya.


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


Repository: mesos


Description
---

Modified agent to get container status from containerizer.


Diffs (updated)
-

  src/slave/slave.hpp a3830ff460a6f6c5661fb8a0172fae303b245889 
  src/slave/slave.cpp 9dda3a2c4dc4c355488d34dc8d0606330a756f2a 

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


Testing
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-08 Thread Avinash sridharan 


> On Feb. 8, 2016, 8:04 p.m., Avinash sridharan wrote:
> > src/slave/slave.cpp, line 3178
> > 
> >
> > Should we be destroying the container on failure of the `Future`? This 
> > is what we are doing in `_statusupdate`

The `LOG(WARNING)` in `_statusUpdate` suggests that its an implicit assumption 
in `_statusupdate` that it will be called from `containerizer->update`. Since 
the current changes are maintaining that assumption, we can drop this defect.


- Avinash


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


On Feb. 9, 2016, 4:16 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43258/
> ---
> 
> (Updated Feb. 9, 2016, 4:16 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Kapil Arya.
> 
> 
> Bugs: MESOS-4490
> https://issues.apache.org/jira/browse/MESOS-4490
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified agent to get container status from containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a3830ff460a6f6c5661fb8a0172fae303b245889 
>   src/slave/slave.cpp 9dda3a2c4dc4c355488d34dc8d0606330a756f2a 
> 
> Diff: https://reviews.apache.org/r/43258/diff/
> 
> 
> Testing
> ---
> 
> make and make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-05 Thread Avinash sridharan 

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

(Updated Feb. 6, 2016, 1:26 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Modified agent to get container status from containerizer.


Diffs (updated)
-

  src/slave/slave.hpp 632640293f14fb97aea6d7dd2c462c1f604638fb 
  src/slave/slave.cpp 1f4c8368feb0ce19963577582ce745acfb21aa9f 

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


Testing
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 43258: Modified agent to get container status from containerizer.

2016-02-05 Thread Guangya Liu 

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




src/slave/slave.cpp (lines 3228 - 3234)


I think that the current format is right, why update?

If you want to update, it should be the following, but I think there is no 
need to update here.
_statusUpdate(
None(), <
update,
pid,
executor->id,
executor->containerId,
executor->checkpoint);


- Guangya Liu


On 二月 6, 2016, 1:26 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43258/
> ---
> 
> (Updated 二月 6, 2016, 1:26 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4490
> https://issues.apache.org/jira/browse/MESOS-4490
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified agent to get container status from containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 632640293f14fb97aea6d7dd2c462c1f604638fb 
>   src/slave/slave.cpp 1f4c8368feb0ce19963577582ce745acfb21aa9f 
> 
> Diff: https://reviews.apache.org/r/43258/diff/
> 
> 
> Testing
> ---
> 
> make and make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>