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 54449: Check quotas are enabled in the XFS disk isolator.
Date Sat, 10 Dec 2016 02:38:54 GMT


> On Dec. 9, 2016, 4:37 p.m., Jiang Yan Xu wrote:
> > Can we add a simple test for `isQuotaEnabled()` in ROOT_XFS_QuotaTest?
> 
> James Peach wrote:
>     I didn't add a new test because I reasoned that this is implicitly tested already,
like `pathIsXfs`.

Implicitly testing it is fine and I was trying to find where both of them are implicitly tested
but all the tests only have the "successful case": the work_dir is indeed on the XFS mountpoint
with quota enabled. There are no tests where it's not?


> On Dec. 9, 2016, 4:37 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.cpp, line 407
> > <https://reviews.apache.org/r/54449/diff/1/?file=1578143#file1578143line407>
> >
> >     Remove redundant space:
> >     
> >     ```
> >     struct fs_quota_statv statv = {FS_QSTATV_VERSION1, 0};
> >     ```
> >     
> >     What's the `0` here? Do we need it? If we do, comment on it?
> 
> James Peach wrote:
>     It's zero-initializing the remaining fields of the struct.

Is this a well-defined pattern?

I thought zero-initialization doesn't requires the number `0`?
http://en.cppreference.com/w/cpp/language/zero_initialization
http://en.cppreference.com/w/cpp/language/aggregate_initialization

e.g.,

`struct fs_quota_statv statv = {FS_QSTATV_VERSION1};` is the same as `struct fs_quota_statv
statv = {FS_QSTATV_VERSION1, 0};`

`fs_disk_quota_t quota = {0};` is the same as `fs_disk_quota_t quota = {};`

Does it look right?

I mean, in term of style, the less "magic numbers" the better?


> On Dec. 9, 2016, 4:37 p.m., Jiang Yan Xu wrote:
> > configure.ac, line 1768
> > <https://reviews.apache.org/r/54449/diff/1/?file=1578140#file1578140line1768>
> >
> >     Is this change for `Q_XGETQSTATV` and `FS_QSTATV_VERSION1`?
> >     
> >     At least explain this (and the change of variable names XFS_PROJ_QUOTA ->
FS_PROJ_QUOTA in the review summary?
> 
> James Peach wrote:
>     Updated summary. The short story is that `linux/dqblk_xfs.h` is a superset and the
two headers are mutually incompatible.

You updated the comment summary via git? It's not synced over to RB. Could you paste it here?
Yeah the short story is good enough.


- Jiang Yan


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


On Dec. 9, 2016, 5:14 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2016, 5:14 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6732
>     https://issues.apache.org/jira/browse/MESOS-6732
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
> 
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
> 
> 
> Diffs
> -----
> 
>   configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374

>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a

>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8

> 
> Diff: https://reviews.apache.org/r/54449/diff/
> 
> 
> Testing
> -------
> 
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
> 
> 
> Thanks,
> 
> James Peach
> 
>


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