mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jiang Yan Xu" <...@jxu.me>
Subject Re: Review Request 37881: Implemented AppcProvisioner.
Date Mon, 31 Aug 2015 01:55:54 GMT


> On Aug. 30, 2015, 12:33 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioner.cpp, line 42
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059944#file1059944line42>
> >
> >     I think we should only allow creating the appc provisioner with linux, since
I believe most of the appc components requires it right?

With CopyBackend it's not, right?


> On Aug. 30, 2015, 12:33 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioner.hpp, line 48
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059943#file1059943line48>
> >
> >     Perhaps its worth mentioning if any of the provisioner is failed to create.

The comment is now:

```
// Create provisioners based on specified flags. An error is returned if
// any of the provisioners specified in --provisioner failed to be created.
```

Reverted the Provisioner::create() semantics. See my reply to Jie's comment above.


> On Aug. 30, 2015, 12:33 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, line 106
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059946#file1059946line106>
> >
> >     I can't remember, but if the directory exists does it still throw as well?
> >     
> >     We should assume the directory should be there after recovery?

If the directory already exists it succeeds. os::mkdir() ignores any EEXIST by default (which
is convenient!)


> On Aug. 30, 2015, 12:33 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, line 125
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059946#file1059946line125>
> >
> >     Why does this not return a Try? We always have a backend specified right?

You mean `Try<hashmap<string, Owned<Backend>>>`?

The thought was that we are always going to attempt to create all supported backends, and
some of them will fail on some systems e.g., OverlayBackend, but its OK. We do this in order
to protect against potential leaked mounts after --appc_backend flag change.

Therefore the "error" case is captured by the hashmap being empty and a Try is unnecessary.


> On Aug. 30, 2015, 12:33 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, line 276
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059946#file1059946line276>
> >
> >     Ideally we should still log the return value though.

Now doing 

```
        backends[backend]->destroy(rootfs)
          .onFailed([rootfs](const std::string& error) {
            LOG(WARNING) << "Failed to destroy orphan rootfs '" << rootfs
                         << "': "<< error;
          });
```

to log it as a warning.


- Jiang Yan


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


On Aug. 30, 2015, 6:49 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2015, 6:49 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
>     https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioner.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838 
>   src/slave/containerizer/provisioner.cpp efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/paths.hpp 41e3bf79da0854406c488855f953111e67353829

>   src/slave/containerizer/provisioners/appc/paths.cpp 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7

>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/appc_provisioner_tests.cpp 47b66b9c30cefe8f9a8e2c1c1341776c2d235020

> 
> Diff: https://reviews.apache.org/r/37881/diff/
> 
> 
> Testing
> -------
> 
> sudo make check.
> 
> More test cases coming.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message