Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated May 9, 2017, 2:31 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Rebased. NNTR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
3d724945812c0359ed175ce232f70886dc4401c8 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
c163b882fb2fc463537d6906c5a47b24a9a756c4 


Diff: https://reviews.apache.org/r/58262/diff/8/

Changes: https://reviews.apache.org/r/58262/diff/7-8/


Testing
---

See https://reviews.apache.org/r/58718/


Thanks,

Alexander Rukletsov

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated May 3, 2017, 4:45 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Moved recovery to the previous patch; addressed comments.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


Diff: https://reviews.apache.org/r/58262/diff/7/

Changes: https://reviews.apache.org/r/58262/diff/6-7/


Testing
---

See https://reviews.apache.org/r/58718/


Thanks,

Alexander Rukletsov

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Vinod Kone]

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


Fix it, then Ship it!




LGTM modulo Jie's comments.


src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 244 (patched)


s/, also/even/



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 301 (patched)


s/access/accesses/ ?


- Vinod Kone


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/6/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.hpp
Lines 367 (patched)


I think we should store `Option` here so that we don't 
end up adding more and more field in Info struct.


- Jie Yu


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/6/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

> On April 27, 2017, 11:15 p.m., Jie Yu wrote:
> >
> 
> Jie Yu wrote:
> See my comments in https://reviews.apache.org/r/58263/
> 
> We should probably just checkpoint `ContainerLaunchInfo`.

Yeah, this sounds like a good idea. Checkpoint the whole `ContainerLaunchInfo`, 
but restore only environment and working directory for now. I'll update the 
chain.


> On April 27, 2017, 11:15 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1468-1469 (patched)
> > 
> >
> > Should we return failure here? If we launch a debug container later 
> > under this container, will it silently miss some environment variables?

I think it is not fatal if it's just environment, however, this is 
questionable. In any way, since we now will be checkpointing the whole 
`ContainerLaunchInfo`, I think it makes sense to propagate the checkpointing 
failure if any. Dropping this since the code will be migrated.


- Alexander


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


On April 28, 2017, 4:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 28, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/6/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 28, 2017, 4:14 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


Diff: https://reviews.apache.org/r/58262/diff/6/

Changes: https://reviews.apache.org/r/58262/diff/5-6/


Testing
---

See https://reviews.apache.org/r/58718/


Thanks,

Alexander Rukletsov

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Jie Yu]

> On April 27, 2017, 11:15 p.m., Jie Yu wrote:
> >

See my comments in https://reviews.apache.org/r/58263/

We should probably just checkpoint `ContainerLaunchInfo`.


- Jie


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


On April 27, 2017, 10:11 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 27, 2017, 10:11 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/5/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.hpp
Lines 366 (patched)


I would make this `Option` and mention in the comment that 
this field won't be set for orphan containers.



src/slave/containerizer/mesos/containerizer.cpp
Lines 1386-1390 (original), 1417-1421 (patched)


For the sake of consistency, i'd still prefer include agent generated 
environment variables in the final result.

For debug container, the list is currently empty so it does not really 
matter. But just in case in the future that the agent wants to add some 
environment variables for debug container.

Please update the comments above as well.



src/slave/containerizer/mesos/containerizer.cpp
Lines 1468-1469 (patched)


Should we return failure here? If we launch a debug container later under 
this container, will it silently miss some environment variables?



src/slave/containerizer/mesos/containerizer.cpp
Lines 1468-1469 (patched)


Should we return failure here? If we launch a debug container later under 
this container, will it silently miss some environment variables?



src/slave/containerizer/mesos/paths.cpp
Lines 372 (patched)


I'd kill this line.


- Jie Yu


On April 27, 2017, 10:11 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 27, 2017, 10:11 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/5/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 27, 2017, 10:11 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/slave/containerizer/mesos/paths.hpp 
d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
  src/slave/containerizer/mesos/paths.cpp 
ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


Diff: https://reviews.apache.org/r/58262/diff/5/

Changes: https://reviews.apache.org/r/58262/diff/4-5/


Testing
---

See https://reviews.apache.org/r/58718/


Thanks,

Alexander Rukletsov

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Jie Yu]

> On April 26, 2017, 3:58 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1396 (patched)
> > 
> >
> > I'd suggest we list the priority for environments for debug container 
> > here in the comments as well.
> > ```
> > 1) user specified env
> > 2) returned by isolators
> > 3) passed from containerizer
> > 4) inheirited from the parent
> > ```
> 
> Vinod Kone wrote:
> Are either 2) or 3) specified by the operator? If yes, should operator be 
> able to override user's env settings?
> 
> Alexander Rukletsov wrote:
> I'm not sure we should include containerizer env for debug containers. It 
> will most probably only lead to duplicates, because this env will already be 
> inherited from the parent.

containerizer env for any nested container should be empty at the moment.


- Jie


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


On April 25, 2017, 9:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 25, 2017, 9:06 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 56db7eb1193c9812b62b9149c9c2b2dd9b66701c 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

> On April 26, 2017, 3:58 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1396 (patched)
> > 
> >
> > I'd suggest we list the priority for environments for debug container 
> > here in the comments as well.
> > ```
> > 1) user specified env
> > 2) returned by isolators
> > 3) passed from containerizer
> > 4) inheirited from the parent
> > ```
> 
> Vinod Kone wrote:
> Are either 2) or 3) specified by the operator? If yes, should operator be 
> able to override user's env settings?

I'm not sure we should include containerizer env for debug containers. It will 
most probably only lead to duplicates, because this env will already be 
inherited from the parent.


- Alexander


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


On April 25, 2017, 9:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 25, 2017, 9:06 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 56db7eb1193c9812b62b9149c9c2b2dd9b66701c 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Vinod Kone]

> On April 26, 2017, 3:58 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1396 (patched)
> > 
> >
> > I'd suggest we list the priority for environments for debug container 
> > here in the comments as well.
> > ```
> > 1) user specified env
> > 2) returned by isolators
> > 3) passed from containerizer
> > 4) inheirited from the parent
> > ```

Are either 2) or 3) specified by the operator? If yes, should operator be able 
to override user's env settings?


- Vinod


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


On April 25, 2017, 9:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 25, 2017, 9:06 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 56db7eb1193c9812b62b9149c9c2b2dd9b66701c 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Vinod Kone]

> On April 13, 2017, 12:45 a.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1386-1389 (original), 1401-1404 (patched)
> > 
> >
> > Previously debug containers had this set in the env as well? But not 
> > anymore?
> 
> Alexander Rukletsov wrote:
> Correct. `environment` represents "basic" environment, e.g. agent 
> environment. I'd argue that we should not set it for debug containers, but 
> rather inherit parent's basic environment as part of inheritance process. 
> Moreover, currently, debug container launch path does not set `environment` 
> at all: 
> https://github.com/apache/mesos/blob/aca6796b703a8925319087d33d7dc5e5539f50d3/src/slave/containerizer/mesos/containerizer.cpp#L1830

I see. Ok.


- Vinod


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


On April 25, 2017, 9:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 25, 2017, 9:06 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 56db7eb1193c9812b62b9149c9c2b2dd9b66701c 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.cpp
Lines 1396 (patched)


I'd suggest we list the priority for environments for debug container here 
in the comments as well.
```
1) user specified env
2) returned by isolators
3) passed from containerizer
4) inheirited from the parent
```



src/slave/containerizer/mesos/containerizer.cpp
Line 1380 (original), 1406 (patched)


Isolator generated environments? I think we should include it for debug 
container as well.


- Jie Yu


On April 25, 2017, 9:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 25, 2017, 9:06 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 56db7eb1193c9812b62b9149c9c2b2dd9b66701c 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 29007898ec04e922266068a8519731b13d351a82 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58718/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 25, 2017, 9:06 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
56db7eb1193c9812b62b9149c9c2b2dd9b66701c 
  src/slave/containerizer/mesos/paths.hpp 
d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
  src/slave/containerizer/mesos/paths.cpp 
ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


Diff: https://reviews.apache.org/r/58262/diff/4/


Testing (updated)
---

See https://reviews.apache.org/r/58718/


Thanks,

Alexander Rukletsov

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 25, 2017, 9:05 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
56db7eb1193c9812b62b9149c9c2b2dd9b66701c 
  src/slave/containerizer/mesos/paths.hpp 
d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
  src/slave/containerizer/mesos/paths.cpp 
ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


Diff: https://reviews.apache.org/r/58262/diff/4/

Changes: https://reviews.apache.org/r/58262/diff/3-4/


Testing
---

See https://reviews.apache.org/r/58263/


Thanks,

Alexander Rukletsov

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

> On April 13, 2017, 12:45 a.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1386-1389 (original), 1401-1404 (patched)
> > 
> >
> > Previously debug containers had this set in the env as well? But not 
> > anymore?

Correct. `environment` represents "basic" environment, e.g. agent environment. 
I'd argue that we should not set it for debug containers, but rather inherit 
parent's basic environment as part of inheritance process. Moreover, currently, 
debug container launch path does not set `environment` at all: 
https://github.com/apache/mesos/blob/aca6796b703a8925319087d33d7dc5e5539f50d3/src/slave/containerizer/mesos/containerizer.cpp#L1830


- Alexander


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


On April 7, 2017, 4:21 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 7, 2017, 4:21 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> bc611a5e085de10e9953b5f942d98f2b5747fce6 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58263/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Vinod Kone]

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




src/slave/containerizer/mesos/containerizer.cpp
Line 1377 (original), 1395 (patched)


indentation?



src/slave/containerizer/mesos/containerizer.cpp
Line 1380 (original), 1398 (patched)


s/MergeFrom/CopyFrom/ because `containerEnvironment` is empty at this point.



src/slave/containerizer/mesos/containerizer.cpp
Lines 1386-1389 (original), 1401-1404 (patched)


Previously debug containers had this set in the env as well? But not 
anymore?


- Vinod Kone


On April 7, 2017, 4:21 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 7, 2017, 4:21 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> bc611a5e085de10e9953b5f942d98f2b5747fce6 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58263/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

> On April 7, 2017, 2:30 p.m., Jie Yu wrote:
> > What if agent crashes and restart?

Jie, I've added checkpointing. We probably need test this functionality. Do you 
think we should write a separate test or you have an idea which test we maybe 
can augment?


- Alexander


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


On April 7, 2017, 4:21 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 7, 2017, 4:21 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> bc611a5e085de10e9953b5f942d98f2b5747fce6 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58263/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 7, 2017, 4:21 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Checkpointed container's launch envirnoment.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
bc611a5e085de10e9953b5f942d98f2b5747fce6 
  src/slave/containerizer/mesos/paths.hpp 
d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
  src/slave/containerizer/mesos/paths.cpp 
ed4bbd2491e71ad1e4a41e0575b514377d02da9b 


Diff: https://reviews.apache.org/r/58262/diff/3/

Changes: https://reviews.apache.org/r/58262/diff/2-3/


Testing
---

See https://reviews.apache.org/r/58263/


Thanks,

Alexander Rukletsov

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Jie Yu]

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



What if agent crashes and restart?

- Jie Yu


On April 7, 2017, 11:05 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 7, 2017, 11:05 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> bc611a5e085de10e9953b5f942d98f2b5747fce6 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/2/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58263/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 7, 2017, 11:05 a.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
bc611a5e085de10e9953b5f942d98f2b5747fce6 


Diff: https://reviews.apache.org/r/58262/diff/2/

Changes: https://reviews.apache.org/r/58262/diff/1-2/


Testing (updated)
---

See https://reviews.apache.org/r/58263/


Thanks,

Alexander Rukletsov

Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

DEBUG containers inherit their environment and working directory from
the parent container.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
bc611a5e085de10e9953b5f942d98f2b5747fce6 


Diff: https://reviews.apache.org/r/58262/diff/1/


Testing
---

make check on Mac OS and various Linux distros.


Thanks,

Alexander Rukletsov