mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 52292: Updated nested container agent API to allow custom ContainerIDs.
Date Tue, 27 Sep 2016 02:38:12 GMT


> On Sept. 27, 2016, 1:20 a.m., Anand Mazumdar wrote:
> > src/slave/validation.cpp, line 53
> > <https://reviews.apache.org/r/52292/diff/1/?file=1510620#file1510620line53>
> >
> >     Use `os::PATH_SEPARATOR` for checking for directories instead?

Although I'll use both POSIX and WINDOWS unconditionally so that both are always prevented.


> On Sept. 27, 2016, 1:20 a.m., Anand Mazumdar wrote:
> > src/slave/validation.cpp, line 208
> > <https://reviews.apache.org/r/52292/diff/1/?file=1510620#file1510620line208>
> >
> >     Should we also ensure that only one level of parent is set for the wait call
similar to the `LAUNCH` case? 
> >     
> >     Ditto for the `KILL` case.
> >     
> >     Provided we only want to support one level of nesting.

Well the thinking is that we only support launching 2 level nested containers *for now*.

I suppose the confusion here was that I placed this within the validation, rather than in
the agent's launch implementation by returning a 501. I'll move this to the latter location.


> On Sept. 27, 2016, 1:20 a.m., Anand Mazumdar wrote:
> > src/tests/slave_validation_tests.cpp, line 173
> > <https://reviews.apache.org/r/52292/diff/1/?file=1510622#file1510622line173>
> >
> >     Not yours: As per my earlier comment, each of these should have been scoped
into their own block.

Well, we sometimes follow this pattern but we definitely do not consistently do this. In this
case it seems straightforward enough that it didn't seem worth it to me.


> On Sept. 27, 2016, 1:20 a.m., Anand Mazumdar wrote:
> > src/tests/slave_validation_tests.cpp, line 44
> > <https://reviews.apache.org/r/52292/diff/1/?file=1510622#file1510622line44>
> >
> >     Can these sub tests be scoped into their own logic? Ditto for all the other
similar tests in this file. It's pretty easy for someone to modify something in these tests
that can thereby fail all the other assertions in this test.
> >     
> >     ```cpp
> >     {
> >       // No empty IDs.
> >     }
> >     
> >     {
> >       // No slashes.
> >     }
> >     ```

Well, I tried this out and the code takes a big readability hit because the test is much longer:

```
  // No empty IDs.
  containerId.set_value("");
  error = validation::container::validateContainerId(containerId);
  EXPECT_SOME(error);
```

```
  // No empty IDs.
  {
    ContainerID containerId;
    containerId.set_value("");
    
    Option<Error> error = 
      validation::container::validateContainerId(containerId);
      
    EXPECT_SOME(error);
  }
```

With all of them scoped it seems like it takes more effort to read the test. Since it's very
simple I'll omit scoping for now. If someone modifies something that breaks other assertions
it seems to me they could easily fix it?


> On Sept. 27, 2016, 1:20 a.m., Anand Mazumdar wrote:
> > src/slave/validation.cpp, line 158
> > <https://reviews.apache.org/r/52292/diff/1/?file=1510620#file1510620line158>
> >
> >     Why did you remove this alias variable `launchNestedContainer`?

The code looked cleaner without it, it also made the error messages match the check more closely
(since we print out the field names). Just seemed more readable when I played with having
aliases vs no aliases.


> On Sept. 27, 2016, 1:20 a.m., Anand Mazumdar wrote:
> > src/slave/validation.cpp, line 184
> > <https://reviews.apache.org/r/52292/diff/1/?file=1510620#file1510620line184>
> >
> >     hmm, I thought we won't support more than one level of nesting only on the default
executor. But, custom executors should be free to use arbitrary level of nesting?

We currently do not, although we will in the future. I moved this check into the agent's handler
to return 501.


- Benjamin


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


On Sept. 27, 2016, 12:16 a.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52292/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6241
>     https://issues.apache.org/jira/browse/MESOS-6241
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, we required that ContainerID.value is a UUID. The
> intention here was that each container has the same format, and
> we could remove the need for ContainerID.parent since all
> ContainerIDs would be unique globally.
> 
> With this change, we intend to keep ContainerID.parent and allow
> users to specify custom IDs (e.g. <executor uuid>.redis.backup).
> This makes for more readable IDs, and we maintain globally unique
> ContainerIDs since the executor container has a UUID generated by
> the agent.
> 
> With this change, we now validate that the user supplied IDs are
> setting the parent component of the ID correctly, and that the
> value does not contain problematic characters.
> 
> 
> Diffs
> -----
> 
>   include/mesos/agent/agent.proto 3397c4177813252b777bbe328ff92c9428366b70 
>   include/mesos/v1/agent/agent.proto 93c817c0be92f68dea28a9272ac034386ade02af 
>   src/slave/validation.hpp 94286f08ee124753dbac394d9c358fca725be594 
>   src/slave/validation.cpp 62b273302845dfa2bc1c07d753dfcce122674809 
>   src/tests/api_tests.cpp e857b17cfe5f05d59859263c025564d33700a26c 
>   src/tests/slave_validation_tests.cpp ee59aee6d68b518d3263de47d3fe4cfbeb1ab477 
> 
> Diff: https://reviews.apache.org/r/52292/diff/
> 
> 
> Testing
> -------
> 
> Updated existing tests and added a ContainerID validation test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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