Re: Review Request 37311: Implemented a 'read-only' Appc image store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/ --- (Updated Aug. 13, 2015, 2:39 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen. Changes --- Comments. Bugs: MESOS-3194 https://issues.apache.org/jira/browse/MESOS-3194 Repository: mesos Description --- Implemented a 'read-only' Appc image store. Diffs (updated) - src/Makefile.am 942003149b071a322933e2c085d9122903f69713 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e src/tests/containerizer/appc_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37311/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 37311: Implemented a 'read-only' Appc image store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/ --- (Updated Aug. 13, 2015, 3:11 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen. Changes --- Comments. NNFR. Bugs: MESOS-3194 https://issues.apache.org/jira/browse/MESOS-3194 Repository: mesos Description --- Implemented a 'read-only' Appc image store. Diffs (updated) - src/Makefile.am 942003149b071a322933e2c085d9122903f69713 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e src/tests/containerizer/appc_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37311/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 37311: Implemented a 'read-only' Appc image store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/#review95340 --- Ship it! src/slave/containerizer/provisioners/appc/store.cpp (lines 74 - 75) https://reviews.apache.org/r/37311/#comment150251 Please wrap the comments so that it's less jagged (e.g., wrapping at 70 char) src/slave/containerizer/provisioners/appc/store.cpp (lines 78 - 79) https://reviews.apache.org/r/37311/#comment150252 Ditto here. src/slave/containerizer/provisioners/appc/store.cpp (lines 119 - 120) https://reviews.apache.org/r/37311/#comment150250 Please make the comment here less jagged (e.g., wrapping at 70 char). src/slave/containerizer/provisioners/appc/store.cpp (line 121) https://reviews.apache.org/r/37311/#comment150254 s/createEntry/createImage/? - Jie Yu On Aug. 13, 2015, 9:39 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/ --- (Updated Aug. 13, 2015, 9:39 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen. Bugs: MESOS-3194 https://issues.apache.org/jira/browse/MESOS-3194 Repository: mesos Description --- Implemented a 'read-only' Appc image store. Diffs - src/Makefile.am 942003149b071a322933e2c085d9122903f69713 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e src/tests/containerizer/appc_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37311/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 37311: Implemented a 'read-only' Appc image store.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37311/#review95005
---
src/slave/containerizer/provisioners/appc/store.hpp (line 39)
https://reviews.apache.org/r/37311/#comment149753
Put '{' in a new line. Please fix all occurances in this file.
src/slave/containerizer/provisioners/appc/store.hpp (lines 39 - 55)
https://reviews.apache.org/r/37311/#comment149762
Can you Move this struct to be a nested struct in Store? (i.e.,
Store::Image)
src/slave/containerizer/provisioners/appc/store.hpp (line 46)
https://reviews.apache.org/r/37311/#comment149755
Can those be 'const'?
src/slave/containerizer/provisioners/appc/store.hpp (line 60)
https://reviews.apache.org/r/37311/#comment149756
One extra line please.
src/slave/containerizer/provisioners/appc/store.hpp (line 61)
https://reviews.apache.org/r/37311/#comment149760
Could you please add some comments about this class? It's not obvious what
this is for.
src/slave/containerizer/provisioners/appc/store.cpp (line 50)
https://reviews.apache.org/r/37311/#comment149769
No need to have a static 'create' function here since this class is in the
cpp file. You can just make the constructor public.
src/slave/containerizer/provisioners/appc/store.cpp (line 60)
https://reviews.apache.org/r/37311/#comment149770
Maybe s/store/root/ and add some comments about that?
src/slave/containerizer/provisioners/appc/store.cpp (line 108)
https://reviews.apache.org/r/37311/#comment149779
Please add a comment explaining that mkdir will return Nothing if images
dir already exist.
Do you want to do a os::exists check first to check if flags.appc_store_dir
exists?
src/slave/containerizer/provisioners/appc/store.cpp (lines 129 - 134)
https://reviews.apache.org/r/37311/#comment149793
No need to call 'realpath' here every time. We just need to make sure
'root' is a canonicalized realpath.
src/slave/containerizer/provisioners/appc/store.cpp (line 141)
https://reviews.apache.org/r/37311/#comment149794
s/id/imageId/
src/slave/containerizer/provisioners/appc/store.cpp (lines 141 - 144)
https://reviews.apache.org/r/37311/#comment149795
Hum, let's merge this function into 'recover' for now since it's the only
caller.
IN that way, you can get rid of this code because imageId is available
already.
src/slave/containerizer/provisioners/appc/store.cpp (line 171)
https://reviews.apache.org/r/37311/#comment149781
`s/images_/result/`
src/slave/containerizer/provisioners/appc/store.cpp (line 183)
https://reviews.apache.org/r/37311/#comment149789
s/entries/imageIds/
src/slave/containerizer/provisioners/appc/store.cpp (line 185)
https://reviews.apache.org/r/37311/#comment149782
storage entry is a little confusing to me. How about:
```
return Failure(
Failed to list all images under ' +
paths::getImagesDir(root) + ': +
entries.error());
```
src/slave/containerizer/provisioners/appc/store.cpp (line 188)
https://reviews.apache.org/r/37311/#comment149790
s/entry/imageId
src/tests/containerizer/appc_provisioner_tests.cpp (line 156)
https://reviews.apache.org/r/37311/#comment149799
s/id/imageId
src/tests/containerizer/appc_provisioner_tests.cpp (line 160)
https://reviews.apache.org/r/37311/#comment149798
s/image/imagePath/
src/tests/containerizer/appc_provisioner_tests.cpp (line 169)
https://reviews.apache.org/r/37311/#comment149797
`s/images_/_images`
- Jie Yu
On Aug. 10, 2015, 7:19 p.m., Jiang Yan Xu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37311/
---
(Updated Aug. 10, 2015, 7:19 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Timothy Chen.
Bugs: MESOS-3194
https://issues.apache.org/jira/browse/MESOS-3194
Repository: mesos
Description
---
Implemented a 'read-only' Appc image store.
Diffs
-
src/Makefile.am 942003149b071a322933e2c085d9122903f69713
src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION
src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION
src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46
src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e
src/tests/containerizer/appc_provisioner_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/37311/diff/
Testing
---
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 37311: Implemented a 'read-only' Appc image store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/ --- (Updated Aug. 10, 2015, 12:19 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Timothy Chen. Changes --- Makefile.am fix. Bugs: MESOS-3194 https://issues.apache.org/jira/browse/MESOS-3194 Repository: mesos Description --- Implemented a 'read-only' Appc image store. Diffs (updated) - src/Makefile.am 942003149b071a322933e2c085d9122903f69713 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e src/tests/containerizer/appc_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37311/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 37311: Implemented a 'read-only' Appc image store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/#review94814 --- src/slave/containerizer/provisioners/appc/store.cpp (line 63) https://reviews.apache.org/r/37311/#comment149447 This is a unordered list of layers for this image then? Who is keeping the order of the dependencies? src/slave/containerizer/provisioners/appc/store.cpp (line 191) https://reviews.apache.org/r/37311/#comment149446 This has the exact same message from line 197, perhaps we can have different messages so it's easier to know if it's just a directory check problem or actual file content problem src/tests/containerizer/appc_provisioner_tests.cpp (line 119) https://reviews.apache.org/r/37311/#comment149445 Btw looking at this and the read-only store, is there documentation around how users can potentially use this like a user guide? - Timothy Chen On Aug. 10, 2015, 7:19 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/ --- (Updated Aug. 10, 2015, 7:19 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Timothy Chen. Bugs: MESOS-3194 https://issues.apache.org/jira/browse/MESOS-3194 Repository: mesos Description --- Implemented a 'read-only' Appc image store. Diffs - src/Makefile.am 942003149b071a322933e2c085d9122903f69713 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e src/tests/containerizer/appc_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37311/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 37311: Implemented a 'read-only' Appc image store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/#review94810 --- Patch looks great! Reviews applied: [37307, 37308, 37309, 37310, 37311] All tests passed. - Mesos ReviewBot On Aug. 10, 2015, 7:19 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/ --- (Updated Aug. 10, 2015, 7:19 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Timothy Chen. Bugs: MESOS-3194 https://issues.apache.org/jira/browse/MESOS-3194 Repository: mesos Description --- Implemented a 'read-only' Appc image store. Diffs - src/Makefile.am 942003149b071a322933e2c085d9122903f69713 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e src/tests/containerizer/appc_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37311/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 37311: Implemented a 'read-only' Appc image store.
On Aug. 10, 2015, 3:20 p.m., Timothy Chen wrote: src/tests/containerizer/appc_provisioner_tests.cpp, line 119 https://reviews.apache.org/r/37311/diff/2/?file=1036569#file1036569line119 Btw looking at this and the read-only store, is there documentation around how users can potentially use this like a user guide? User guide for the image store/provisioner or just the read-only store? I don't think a 'read-only' store is going to be useful as a released feature (even though we have concerns for fetching into the store here right now). The 'put' feature will be added before a release. On Aug. 10, 2015, 3:20 p.m., Timothy Chen wrote: src/slave/containerizer/provisioners/appc/store.cpp, line 63 https://reviews.apache.org/r/37311/diff/2/?file=1036566#file1036566line63 This is a unordered list of layers for this image then? Who is keeping the order of the dependencies? No, this is purely a way of organizing images in the store: images of the same name are put under the same key: because we know the name is always going to be specified. This way is the image match process during provisioning can work on a shorter list narrowed down from all images currently in the store. The order of the dependencies are kept track of in the AppcImageManifest, when provisioned by the backend dependencies are applied according to the order specified there. - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/#review94814 --- On Aug. 10, 2015, 12:19 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37311/ --- (Updated Aug. 10, 2015, 12:19 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Timothy Chen. Bugs: MESOS-3194 https://issues.apache.org/jira/browse/MESOS-3194 Repository: mesos Description --- Implemented a 'read-only' Appc image store. Diffs - src/Makefile.am 942003149b071a322933e2c085d9122903f69713 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e src/tests/containerizer/appc_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37311/diff/ Testing --- make check. Thanks, Jiang Yan Xu
