> On Oct. 13, 2016, 9:58 p.m., Jiang Yan Xu wrote: > > src/master/validation.cpp, lines 503-504 > > <https://reviews.apache.org/r/45963/diff/16/?file=1533957#file1533957line503> > > > > MESOS-6374 is where we want eventually but for now the same check is in > > `validateDiskInfo()` as well so we don't have to duplicate it here?
Check for `volume.disk().has_volume()` is done since the next line calls `volume.disk().volume().mode()`. > On Oct. 13, 2016, 9:58 p.m., Jiang Yan Xu wrote: > > src/master/validation.cpp, lines 505-506 > > <https://reviews.apache.org/r/45963/diff/16/?file=1533957#file1533957line505> > > > > This is called when you destroy a volume as well. So if the framework > > is has changed the volume to RO after creating it then it cannot destroy it > > (at least in the form it was created)... > > > > I think we should just validate this in the following method? > > > > ``` > > Option<Error> validate( > > const Offer::Operation::Create& create, > > const Resources& checkpointedResources, > > const Option<string>& principal) {} > > ``` Since the offer to frameworks would only contain `Mode::RW`, this check is valid for both `CREATE` and `DESTROY` operations. So, we should not allow this volume as valid if the `DESTROY` operation marks this as `Mode::RO`. > On Oct. 13, 2016, 9:58 p.m., Jiang Yan Xu wrote: > > src/tests/containerizer/linux_filesystem_isolator_tests.cpp, line 1455 > > <https://reviews.apache.org/r/45963/diff/16/?file=1533961#file1533961line1455> > > > > Nit: use `frameworkInfo.set_role(DEFAULT_TEST_ROLE);`? I will leave it as `role1` instead of `DEFAULT_TEST_ROLE` since the remaining tests in this file use `role1`. We can change to `DEFAULT_TEST_ROLE` in a follow up commit when all tests move to using `DEFAULT_TEST_ROLE`. - Anindya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45963/#review152538 ----------------------------------------------------------- On Oct. 13, 2016, 5:24 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45963/ > ----------------------------------------------------------- > > (Updated Oct. 13, 2016, 5:24 a.m.) > > > Review request for mesos, Greg Mann and Jiang Yan Xu. > > > Bugs: MESOS-4324 > https://issues.apache.org/jira/browse/MESOS-4324 > > > Repository: mesos > > > Description > ------- > > Allow the task to specify the persistent volume access to be read-only > or read-write. Note that the persistent volume is always created as > read-write. > If the task is the first consumer of the shared persistent volume, then > set the ownership of the persistent volume to match that of the task. > Otherwise, launch the task but if the task is unable to read/write the > persistent volume, it would fail at that point of time. > > > Diffs > ----- > > src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d > src/slave/containerizer/docker.cpp d71386089bf7677872bcb1bd36e07da9263dcf0d > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > 8f62162519f12a157c28ca5f2a76502e466c1481 > src/slave/containerizer/mesos/isolators/filesystem/posix.cpp > af427c6e5691f1770ab3ebef79502eb2c2176c4a > src/tests/containerizer/linux_filesystem_isolator_tests.cpp > eb191a32381f9d1ca84ec29adf352dde375c2f2d > src/tests/master_validation_tests.cpp > 99e350e0587e73e9ee25ef20dd369cd146bd446a > > Diff: https://reviews.apache.org/r/45963/diff/ > > > Testing > ------- > > Tests successful. > > > Thanks, > > Anindya Sinha > >
