You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2015/06/23 21:18:17 UTC
Review Request 35799: Support mounting relative paths with docker.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-2374
https://issues.apache.org/jira/browse/MESOS-2374
Repository: mesos
Description
-------
Support mounting relative paths with docker.
Diffs
-----
src/docker/docker.cpp 4e1300300392b404b6dcf62631492112fa4939ba
src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
Diff: https://reviews.apache.org/r/35799/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 35799: Support mounting relative paths with docker.
Posted by Timothy Chen <tn...@apache.org>.
> On June 23, 2015, 8:10 p.m., Till Toenshoff wrote:
> > src/tests/docker_tests.cpp, line 60
> > <https://reviews.apache.org/r/35799/diff/1/?file=990751#file990751line60>
> >
> > Out of curiosity, why 30 seconds?
Good question, it's the same timeout I've used in docker containerizer tests for just a arbitrary time. 30 seconds to me sounds like a long enough time to wait for a container to shutdown even on a very slow system.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/#review89042
-----------------------------------------------------------
On June 23, 2015, 7:18 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35799/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 7:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2374
> https://issues.apache.org/jira/browse/MESOS-2374
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support mounting relative paths with docker.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 4e1300300392b404b6dcf62631492112fa4939ba
> src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
>
> Diff: https://reviews.apache.org/r/35799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 35799: Support mounting relative paths with docker.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/#review89042
-----------------------------------------------------------
src/docker/docker.cpp (lines 381 - 382)
<https://reviews.apache.org/r/35799/#comment141647>
Even though it reads fine, I think our style suggests a different wrapping:
```
volumeConfig =
path::join(sandboxDirectory, volume.host_path()) + ":" + volumeConfig;
```
This should just fit the 80 chars limit if I am not mistaken.
src/tests/docker_tests.cpp (line 48)
<https://reviews.apache.org/r/35799/#comment141648>
Let's move the brace down to the next line:
```
class DockerTest : public MesosTest
{
[...]
```
src/tests/docker_tests.cpp (line 53)
<https://reviews.apache.org/r/35799/#comment141649>
Insert blank line here.
src/tests/docker_tests.cpp (line 60)
<https://reviews.apache.org/r/35799/#comment141650>
Out of curiosity, why 30 seconds?
src/tests/docker_tests.cpp (line 334)
<https://reviews.apache.org/r/35799/#comment141651>
We are trying to get all tests documented a little, hence it would be great if you added a comment line or two on top, describing what the test validates.
src/tests/docker_tests.cpp (line 340)
<https://reviews.apache.org/r/35799/#comment141653>
Insert blank line.
src/tests/docker_tests.cpp (line 360)
<https://reviews.apache.org/r/35799/#comment141654>
I would either get rid of this `containerName` as you only use it once, or I would make it const if you plan to reuse it in later updates.
- Till Toenshoff
On June 23, 2015, 7:18 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35799/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 7:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2374
> https://issues.apache.org/jira/browse/MESOS-2374
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support mounting relative paths with docker.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 4e1300300392b404b6dcf62631492112fa4939ba
> src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
>
> Diff: https://reviews.apache.org/r/35799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 35799: Support mounting relative paths with docker.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/#review89045
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35799]
All tests passed.
- Mesos ReviewBot
On June 23, 2015, 7:18 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35799/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 7:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2374
> https://issues.apache.org/jira/browse/MESOS-2374
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support mounting relative paths with docker.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 4e1300300392b404b6dcf62631492112fa4939ba
> src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
>
> Diff: https://reviews.apache.org/r/35799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 35799: Support mounting relative paths with docker.
Posted by Timothy Chen <tn...@apache.org>.
> On June 24, 2015, 10:13 a.m., Bernd Mathiske wrote:
> > src/tests/docker_tests.cpp, line 374
> > <https://reviews.apache.org/r/35799/diff/1/?file=990751#file990751line374>
> >
> > Use Flags::docker_sandbox_directory (or a constant that the flag also uses for its default value) instead. Not only will this fix the naked constant problem, it will also explain what this parameter does and make readers understand the test code much more quickly.
> >
> > I know this is copy-paste-induced, but the original is really, really bad. Please either fix now, or leave a TODO and a tech debt ticket.
We actually put anything here, so I just put the default settings. I can just use /tmp/sandbox if that makes it more obvious?
> On June 24, 2015, 10:13 a.m., Bernd Mathiske wrote:
> > src/tests/docker_tests.cpp, line 340
> > <https://reviews.apache.org/r/35799/diff/1/?file=990751#file990751line340>
> >
> > So this tests is for mounting a volume with a relative path. How about testing the opposite, using an absolute path?
Sure
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/#review89147
-----------------------------------------------------------
On June 23, 2015, 7:18 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35799/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 7:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2374
> https://issues.apache.org/jira/browse/MESOS-2374
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support mounting relative paths with docker.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 4e1300300392b404b6dcf62631492112fa4939ba
> src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
>
> Diff: https://reviews.apache.org/r/35799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 35799: Support mounting relative paths with docker.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On June 24, 2015, 3:13 a.m., Bernd Mathiske wrote:
> > src/tests/docker_tests.cpp, line 374
> > <https://reviews.apache.org/r/35799/diff/1/?file=990751#file990751line374>
> >
> > Use Flags::docker_sandbox_directory (or a constant that the flag also uses for its default value) instead. Not only will this fix the naked constant problem, it will also explain what this parameter does and make readers understand the test code much more quickly.
> >
> > I know this is copy-paste-induced, but the original is really, really bad. Please either fix now, or leave a TODO and a tech debt ticket.
>
> Timothy Chen wrote:
> We actually put anything here, so I just put the default settings. I can just use /tmp/sandbox if that makes it more obvious?
What you have put there now ("directory.get()") does not provide the user with an idea what is going on without looking up the definition of the parameters of Docker::run() (nor would "/tmp/sandbox"). This may seem like a moot point, since it is indeed arbitrary what to put there. But conveying that information to the reader is actually the real point in question. So this fix is worse than the original IMHO.
The previous string constant actually gave more of a hint and now I understand now why you named it "/mnt/mesos/sandbox". This kinda stands for "/arbitrary/mount/point/in/docker/for/the/Mesos/sandbox". Sorry for opening the issue in the first place! Please revert to "/mnt/mesos/sandbox". (Maybe add a helpful source code comment? :-)
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/#review89147
-----------------------------------------------------------
On June 26, 2015, 4:26 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35799/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 4:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2374
> https://issues.apache.org/jira/browse/MESOS-2374
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support mounting relative paths with docker.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 62aac2773d8b74e19619cf593064165892b03ba6
> src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
>
> Diff: https://reviews.apache.org/r/35799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 35799: Support mounting relative paths with docker.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/#review89147
-----------------------------------------------------------
src/tests/docker_tests.cpp (line 333)
<https://reviews.apache.org/r/35799/#comment141746>
So this tests is for mounting a volume with a relative path. How about testing the opposite, using an absolute path?
src/tests/docker_tests.cpp (line 367)
<https://reviews.apache.org/r/35799/#comment141747>
Use Flags::docker_sandbox_directory (or a constant that the flag also uses for its default value) instead. Not only will this fix the naked constant problem, it will also explain what this parameter does and make readers understand the test code much more quickly.
I know this is copy-paste-induced, but the original is really, really bad. Please either fix now, or leave a TODO and a tech debt ticket.
- Bernd Mathiske
On June 23, 2015, 12:18 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35799/
> -----------------------------------------------------------
>
> (Updated June 23, 2015, 12:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2374
> https://issues.apache.org/jira/browse/MESOS-2374
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support mounting relative paths with docker.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 4e1300300392b404b6dcf62631492112fa4939ba
> src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
>
> Diff: https://reviews.apache.org/r/35799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 35799: Support mounting relative paths with docker.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/#review89597
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35799]
All tests passed.
- Mesos ReviewBot
On June 26, 2015, 11:26 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35799/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 11:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2374
> https://issues.apache.org/jira/browse/MESOS-2374
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support mounting relative paths with docker.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 62aac2773d8b74e19619cf593064165892b03ba6
> src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
>
> Diff: https://reviews.apache.org/r/35799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 35799: Support mounting relative paths with docker.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/#review90006
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35799]
All tests passed.
- Mesos ReviewBot
On July 1, 2015, 12:14 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35799/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 12:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2374
> https://issues.apache.org/jira/browse/MESOS-2374
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support mounting relative paths with docker.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 62aac2773d8b74e19619cf593064165892b03ba6
> src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
>
> Diff: https://reviews.apache.org/r/35799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 35799: Support mounting relative paths with docker.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/#review90040
-----------------------------------------------------------
Ship it!
Looks good - just a few minor const. suggestions :)
src/tests/docker_tests.cpp (line 75)
<https://reviews.apache.org/r/35799/#comment142977>
Why not const?
src/tests/docker_tests.cpp (line 250)
<https://reviews.apache.org/r/35799/#comment142978>
Const?
src/tests/docker_tests.cpp (line 365)
<https://reviews.apache.org/r/35799/#comment142980>
Const?
src/tests/docker_tests.cpp (line 391)
<https://reviews.apache.org/r/35799/#comment142979>
Const?
- Till Toenshoff
On July 1, 2015, 12:14 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35799/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 12:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2374
> https://issues.apache.org/jira/browse/MESOS-2374
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support mounting relative paths with docker.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 62aac2773d8b74e19619cf593064165892b03ba6
> src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
>
> Diff: https://reviews.apache.org/r/35799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 35799: Support mounting relative paths with docker.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/
-----------------------------------------------------------
(Updated July 1, 2015, 12:14 a.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-2374
https://issues.apache.org/jira/browse/MESOS-2374
Repository: mesos
Description
-------
Support mounting relative paths with docker.
Diffs (updated)
-----
src/docker/docker.cpp 62aac2773d8b74e19619cf593064165892b03ba6
src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
Diff: https://reviews.apache.org/r/35799/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 35799: Support mounting relative paths with docker.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35799/
-----------------------------------------------------------
(Updated June 26, 2015, 11:26 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-2374
https://issues.apache.org/jira/browse/MESOS-2374
Repository: mesos
Description
-------
Support mounting relative paths with docker.
Diffs (updated)
-----
src/docker/docker.cpp 62aac2773d8b74e19619cf593064165892b03ba6
src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c
Diff: https://reviews.apache.org/r/35799/diff/
Testing
-------
make check
Thanks,
Timothy Chen