Re: Review Request 64267: Supported basic auto image gc in the agent.

[Zhitao Li]

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


Fix it, then Ship it!




Ship It!


src/slave/slave.cpp
Line 6170 (original), 6231 (patched)


I feel that an early return makes the code more readable.


- Zhitao Li


On Dec. 6, 2017, 7:13 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64267/
> ---
> 
> (Updated Dec. 6, 2017, 7:13 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported basic auto image gc in the agent.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 06afd522ac424faa427366219cb212ee18a81c33 
>   src/slave/slave.cpp 49270013537356c8fe9150d757b064bc3bbae3cb 
> 
> 
> Diff: https://reviews.apache.org/r/64267/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 64267: Supported basic auto image gc in the agent.

[Gilbert Song]

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

(Updated Dec. 6, 2017, 11:13 a.m.)


Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.


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


Repository: mesos


Description
---

Supported basic auto image gc in the agent.


Diffs (updated)
-

  src/slave/slave.hpp 06afd522ac424faa427366219cb212ee18a81c33 
  src/slave/slave.cpp 49270013537356c8fe9150d757b064bc3bbae3cb 


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

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 64267: Supported basic auto image gc in the agent.

[Gilbert Song]

> On Dec. 3, 2017, 2:33 p.m., Zhitao Li wrote:
> > src/slave/slave.cpp
> > Lines 6159 (patched)
> > 
> >
> > Not necessarily this diff, but I wonder what kind of high level 
> > separation we want to keep between `Slave` and `Containerizer` in the long 
> > run. For example, the `docker_store_dir` is more of a containerizer 
> > internal configuration rather than logically belonging to the `Slave` class 
> > here.
> > 
> > Maybe a more structured alternative is to either create the 
> > containerizer with an `ImageGcConfig`, or pass it down to the `pruneImages` 
> > API, and let `containerizer` to handle related logic?
> > 
> > This is just food for thought for now.

Agree. We should think about this question in Unified Artifect store design. 
Added a TODO.


- Gilbert


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


On Dec. 1, 2017, 5:10 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64267/
> ---
> 
> (Updated Dec. 1, 2017, 5:10 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported basic auto image gc in the agent.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 643d8559244f0842e82b293c1ef99cb26e111059 
>   src/slave/slave.cpp a9aa987ade67542b85aeb51356d00a4c14c6be9d 
> 
> 
> Diff: https://reviews.apache.org/r/64267/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 64267: Supported basic auto image gc in the agent.

[Gilbert Song]

> On Dec. 3, 2017, 2:19 p.m., Zhitao Li wrote:
> > src/slave/slave.cpp
> > Lines 6171-6173 (patched)
> > 
> >
> > I think adding an early return is more readable

can we return? for both cases (no matter trigger gc or not), we need to call 
`delay()` for the next gc cycle. let's follow the `_checkDiskUsage()`.


- Gilbert


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


On Dec. 1, 2017, 5:10 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64267/
> ---
> 
> (Updated Dec. 1, 2017, 5:10 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported basic auto image gc in the agent.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 643d8559244f0842e82b293c1ef99cb26e111059 
>   src/slave/slave.cpp a9aa987ade67542b85aeb51356d00a4c14c6be9d 
> 
> 
> Diff: https://reviews.apache.org/r/64267/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 64267: Supported basic auto image gc in the agent.

[Gilbert Song]

> On Dec. 2, 2017, 1:02 a.m., Qian Zhang wrote:
> > src/slave/slave.cpp
> > Lines 621 (patched)
> > 
> >
> > What if `image_providers` is specified by the operator but Mesos 
> > containerizer is not enabled? Do we still want to enable auto image gc in 
> > this case?

we should have the containerizer check. thanks!


> On Dec. 2, 2017, 1:02 a.m., Qian Zhang wrote:
> > src/slave/slave.cpp
> > Lines 6171-6173 (patched)
> > 
> >
> > This should be an `else if`, elsewise we will access `usage.get()` even 
> > it is not ready.

good catch!


- Gilbert


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


On Dec. 1, 2017, 5:10 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64267/
> ---
> 
> (Updated Dec. 1, 2017, 5:10 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported basic auto image gc in the agent.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 643d8559244f0842e82b293c1ef99cb26e111059 
>   src/slave/slave.cpp a9aa987ade67542b85aeb51356d00a4c14c6be9d 
> 
> 
> Diff: https://reviews.apache.org/r/64267/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 64267: Supported basic auto image gc in the agent.

[Zhitao Li]

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




src/slave/slave.cpp
Lines 6159 (patched)


Not necessarily this diff, but I wonder what kind of high level separation 
we want to keep between `Slave` and `Containerizer` in the long run. For 
example, the `docker_store_dir` is more of a containerizer internal 
configuration rather than logically belonging to the `Slave` class here.

Maybe a more structured alternative is to either create the containerizer 
with an `ImageGcConfig`, or pass it down to the `pruneImages` API, and let 
`containerizer` to handle related logic?

This is just food for thought for now.



src/slave/slave.cpp
Lines 6173 (patched)


Add a VLOG to to print out relevant argument values (disk_headroom and 
usage) so we know this is triggered?



src/slave/slave.cpp
Lines 6179 (patched)


Add a VLOG for else case?


- Zhitao Li


On Dec. 2, 2017, 1:10 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64267/
> ---
> 
> (Updated Dec. 2, 2017, 1:10 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported basic auto image gc in the agent.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 643d8559244f0842e82b293c1ef99cb26e111059 
>   src/slave/slave.cpp a9aa987ade67542b85aeb51356d00a4c14c6be9d 
> 
> 
> Diff: https://reviews.apache.org/r/64267/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 64267: Supported basic auto image gc in the agent.

[Zhitao Li]

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




src/slave/slave.cpp
Lines 6171-6173 (patched)


I think adding an early return is more readable


- Zhitao Li


On Dec. 2, 2017, 1:10 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64267/
> ---
> 
> (Updated Dec. 2, 2017, 1:10 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported basic auto image gc in the agent.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 643d8559244f0842e82b293c1ef99cb26e111059 
>   src/slave/slave.cpp a9aa987ade67542b85aeb51356d00a4c14c6be9d 
> 
> 
> Diff: https://reviews.apache.org/r/64267/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 64267: Supported basic auto image gc in the agent.

[Qian Zhang]

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




src/slave/slave.cpp
Lines 621 (patched)


What if `image_providers` is specified by the operator but Mesos 
containerizer is not enabled? Do we still want to enable auto image gc in this 
case?



src/slave/slave.cpp
Lines 6171-6173 (patched)


This should be an `else if`, elsewise we will access `usage.get()` even it 
is not ready.


- Qian Zhang


On Dec. 2, 2017, 9:10 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64267/
> ---
> 
> (Updated Dec. 2, 2017, 9:10 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported basic auto image gc in the agent.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 643d8559244f0842e82b293c1ef99cb26e111059 
>   src/slave/slave.cpp a9aa987ade67542b85aeb51356d00a4c14c6be9d 
> 
> 
> Diff: https://reviews.apache.org/r/64267/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>