You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2017/09/07 00:44:00 UTC
Review Request 62145: WIP: Implemented Standalone Container API.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62145/
-----------------------------------------------------------
Review request for mesos, Gilbert Song and Jie Yu.
Bugs: MESOS-7492
https://issues.apache.org/jira/browse/MESOS-7492
Repository: mesos
Description
-------
Although the Standalone and Nested Container APIs are very similar,
there is little shared code between them due to differences in their
inputs, AuthZ, and outputs.
Diffs
-----
src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b
src/slave/http.cpp 3ea7829df8c1c35d2fa3a44f19a60b7e261042ce
Diff: https://reviews.apache.org/r/62145/diff/1/
Testing
-------
TODO
Thanks,
Joseph Wu
Re: Review Request 62145: WIP: Implemented Standalone Container API.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62145/#review185931
-----------------------------------------------------------
FAIL: Failed to apply the dependent review: 61805.
Failed command: `python.exe .\support\apply-reviews.py -n -r 61805`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62145
Relevant logs:
- [apply-review-61805-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62145/logs/apply-review-61805-stdout.log):
```
error: patch failed: src/slave/paths.cpp:69
error: src/slave/paths.cpp: patch does not apply
```
- Mesos Reviewbot Windows
On Sept. 7, 2017, 12:43 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62145/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 12:43 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-7492
> https://issues.apache.org/jira/browse/MESOS-7492
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Although the Standalone and Nested Container APIs are very similar,
> there is little shared code between them due to differences in their
> inputs, AuthZ, and outputs.
>
>
> Diffs
> -----
>
> src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b
> src/slave/http.cpp 3ea7829df8c1c35d2fa3a44f19a60b7e261042ce
>
>
> Diff: https://reviews.apache.org/r/62145/diff/2/
>
>
> Testing
> -------
>
> TODO
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 62145: Implemented Standalone Container API.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Nov. 8, 2017, 10:28 a.m., Jie Yu wrote:
> > src/slave/http.cpp
> > Line 2359 (original), 2420-2423 (patched)
> > <https://reviews.apache.org/r/62145/diff/4/?file=1879171#file1879171line2420>
> >
> > I think we should only try to set 'user' if agent `switch_user` flag is set.
> >
> > ```
> > if (slave->flags.switch_user) {
> > if (commandInfo.has_user()) {
> > containerUser = commandInfo.user();
> > }
> >
> > // If 'user' is not specified for standalone container,
> > // it'll be the same as not switching user in containerizer.
> > // No need to call os::user() here.
> > }
> > ```
As discussed offline, since we're making the AuthZ into ANY or NONE, some of this can go away.
> On Nov. 8, 2017, 10:28 a.m., Jie Yu wrote:
> > src/slave/http.cpp
> > Lines 2510 (patched)
> > <https://reviews.apache.org/r/62145/diff/4/?file=1879171#file1879171line2512>
> >
> > maybe try to remove the sandbox created above and return a InternalServerError()?
This is mirroring the logic of sandbox creation for normal top-level containers:
https://github.com/apache/mesos/blob/a595bcbf7afd9783e15f3e32cd9e70fa979531df/src/slave/paths.cpp#L613-L617
We don't fail the launch if the sandbox chown fails.
However, since this API is more user facing, it makes sense to fail here.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62145/#review190413
-----------------------------------------------------------
On Nov. 2, 2017, 8:57 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62145/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2017, 8:57 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Standalone and Nested Container APIs are very similar.
> This commit combines the two API implementations by adding a
> translation function (i.e. `launchNestedContainer` and
> `launchContainer`) which unpacks the V1 protobuf into fields
> which can be passed into a common function (i.e. `_launchContainer`).
>
> The common functions authorize based on the type of container being
> launched and it is possible to use both Standalone and Nested
> Container APIs interchangably for nested containers.
>
> This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
> calls, as these methods require different return protobufs based on
> the original call.
>
>
> Diffs
> -----
>
> src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b
> src/slave/http.cpp f2e06aff95e0628624b6ed25de222fd3f3577a0b
>
>
> Diff: https://reviews.apache.org/r/62145/diff/4/
>
>
> Testing
> -------
>
> See later in chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 62145: Implemented Standalone Container API.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62145/#review190413
-----------------------------------------------------------
src/slave/http.cpp
Line 2359 (original), 2420-2423 (patched)
<https://reviews.apache.org/r/62145/#comment267741>
I think we should only try to set 'user' if agent `switch_user` flag is set.
```
if (slave->flags.switch_user) {
if (commandInfo.has_user()) {
containerUser = commandInfo.user();
}
// If 'user' is not specified for standalone container,
// it'll be the same as not switching user in containerizer.
// No need to call os::user() here.
}
```
src/slave/http.cpp
Lines 2436 (patched)
<https://reviews.apache.org/r/62145/#comment267846>
```
containerUser = executor->user;
if (slave->flags.switch_user && commandInfo.has_user()) {
containerUser = commandInfo.user();
}
```
src/slave/http.cpp
Lines 2461 (patched)
<https://reviews.apache.org/r/62145/#comment267742>
Instead of passing a default user, i'd calculate that in the caller (`_launchContainer`) so that the contaienr user calculation is done in one place.
src/slave/http.cpp
Lines 2510 (patched)
<https://reviews.apache.org/r/62145/#comment267743>
maybe try to remove the sandbox created above and return a InternalServerError()?
src/slave/http.cpp
Lines 2641 (patched)
<https://reviews.apache.org/r/62145/#comment267850>
I am not sure if this is going to work. Looks like the way we do authorization for wait_nested_contianer is very special. It basically check if the top level container id matches or not.
For standalone container, that means somehow we need to use the same JWT token based authentication and set `cid` claim properly? is that what we plan to do?
I am not super familiar with the code around that. You may want to check with Greg on this?
- Jie Yu
On Nov. 2, 2017, 3:57 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62145/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2017, 3:57 p.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Standalone and Nested Container APIs are very similar.
> This commit combines the two API implementations by adding a
> translation function (i.e. `launchNestedContainer` and
> `launchContainer`) which unpacks the V1 protobuf into fields
> which can be passed into a common function (i.e. `_launchContainer`).
>
> The common functions authorize based on the type of container being
> launched and it is possible to use both Standalone and Nested
> Container APIs interchangably for nested containers.
>
> This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
> calls, as these methods require different return protobufs based on
> the original call.
>
>
> Diffs
> -----
>
> src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b
> src/slave/http.cpp f2e06aff95e0628624b6ed25de222fd3f3577a0b
>
>
> Diff: https://reviews.apache.org/r/62145/diff/4/
>
>
> Testing
> -------
>
> See later in chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 62145: Implemented Standalone Container API.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62145/#review190983
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/http.cpp
Line 2421 (original), 2453 (patched)
<https://reviews.apache.org/r/62145/#comment268570>
`switch_user` is posix only flag. You need to wrap with `ifndef __WINDOWS__`?
src/slave/http.cpp
Line 2422 (original), 2454-2456 (patched)
<https://reviews.apache.org/r/62145/#comment268571>
For nested container, we by default use executor's user? Can you add that logic back?
- Jie Yu
On Nov. 14, 2017, 1:32 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62145/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2017, 1:32 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Standalone and Nested Container APIs are very similar.
> This commit combines the two API implementations by adding a
> translation function (i.e. `launchNestedContainer` and
> `launchContainer`) which unpacks the V1 protobuf into fields
> which can be passed into a common function (i.e. `_launchContainer`).
>
> The common functions authorize based on the type of container being
> launched and it is possible to use both Standalone and Nested
> Container APIs interchangably for nested containers.
>
> This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
> calls, as these methods require different return protobufs based on
> the original call.
>
>
> Diffs
> -----
>
> src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b
> src/slave/http.cpp 22cdac97e1a8a28ca149043ffa1d0646073d7fdb
>
>
> Diff: https://reviews.apache.org/r/62145/diff/5/
>
>
> Testing
> -------
>
> See later in chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 62145: Implemented Standalone Container API.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62145/
-----------------------------------------------------------
(Updated Nov. 13, 2017, 5:32 p.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Changes
-------
* Changed AuthZ to use the AuthorizationAcceptor pattern.
* Removed the `user` fields from AuthZ objects due to decision to reduce granularity of AuthZ.
* Combined `__launchContainer` into `_launchContainer`.
* Made failure to chown standalone sandbox into a 500 error.
Bugs: MESOS-7305
https://issues.apache.org/jira/browse/MESOS-7305
Repository: mesos
Description
-------
The Standalone and Nested Container APIs are very similar.
This commit combines the two API implementations by adding a
translation function (i.e. `launchNestedContainer` and
`launchContainer`) which unpacks the V1 protobuf into fields
which can be passed into a common function (i.e. `_launchContainer`).
The common functions authorize based on the type of container being
launched and it is possible to use both Standalone and Nested
Container APIs interchangably for nested containers.
This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
calls, as these methods require different return protobufs based on
the original call.
Diffs (updated)
-----
src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b
src/slave/http.cpp 22cdac97e1a8a28ca149043ffa1d0646073d7fdb
Diff: https://reviews.apache.org/r/62145/diff/5/
Changes: https://reviews.apache.org/r/62145/diff/4-5/
Testing
-------
See later in chain.
Thanks,
Joseph Wu
Re: Review Request 62145: Implemented Standalone Container API.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62145/
-----------------------------------------------------------
(Updated Nov. 2, 2017, 8:57 a.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Changes
-------
Updated implementation of `WaitContainer` to match `WaitNestedContainer`.
Bugs: MESOS-7305
https://issues.apache.org/jira/browse/MESOS-7305
Repository: mesos
Description
-------
The Standalone and Nested Container APIs are very similar.
This commit combines the two API implementations by adding a
translation function (i.e. `launchNestedContainer` and
`launchContainer`) which unpacks the V1 protobuf into fields
which can be passed into a common function (i.e. `_launchContainer`).
The common functions authorize based on the type of container being
launched and it is possible to use both Standalone and Nested
Container APIs interchangably for nested containers.
This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
calls, as these methods require different return protobufs based on
the original call.
Diffs (updated)
-----
src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b
src/slave/http.cpp f2e06aff95e0628624b6ed25de222fd3f3577a0b
Diff: https://reviews.apache.org/r/62145/diff/4/
Changes: https://reviews.apache.org/r/62145/diff/3-4/
Testing
-------
See later in chain.
Thanks,
Joseph Wu
Re: Review Request 62145: Implemented Standalone Container API.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62145/
-----------------------------------------------------------
(Updated Oct. 16, 2017, 5:02 p.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Changes
-------
Fairly major change (the diff of diffs is not informative). This approach has slightly less code duplication and makes it possible to use the Nested Container API and Standalone Container API interchangably for nested containers.
Summary (updated)
-----------------
Implemented Standalone Container API.
Bugs: MESOS-7492
https://issues.apache.org/jira/browse/MESOS-7492
Repository: mesos
Description (updated)
-------
The Standalone and Nested Container APIs are very similar.
This commit combines the two API implementations by adding a
translation function (i.e. `launchNestedContainer` and
`launchContainer`) which unpacks the V1 protobuf into fields
which can be passed into a common function (i.e. `_launchContainer`).
The common functions authorize based on the type of container being
launched and it is possible to use both Standalone and Nested
Container APIs interchangably for nested containers.
This approach is somewhat messy for for the `WAIT_(NESTED_)CONTAINER`
calls, as these methods require different return protobufs based on
the original call.
Diffs (updated)
-----
src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b
src/slave/http.cpp f2e06aff95e0628624b6ed25de222fd3f3577a0b
Diff: https://reviews.apache.org/r/62145/diff/3/
Changes: https://reviews.apache.org/r/62145/diff/2-3/
Testing
-------
TODO
Thanks,
Joseph Wu
Re: Review Request 62145: WIP: Implemented Standalone Container API.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62145/#review184812
-----------------------------------------------------------
ERROR: Failed to apply patch 61805
Reviews applied: [61805, 61806, 60888, 60889, 60890, 60891, 62142, 62143, 62144, 62145]
Logs available here: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62145/logs
- Mesos Reviewbot Windows
On Sept. 7, 2017, 6:13 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62145/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 6:13 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-7492
> https://issues.apache.org/jira/browse/MESOS-7492
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Although the Standalone and Nested Container APIs are very similar,
> there is little shared code between them due to differences in their
> inputs, AuthZ, and outputs.
>
>
> Diffs
> -----
>
> src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b
> src/slave/http.cpp 3ea7829df8c1c35d2fa3a44f19a60b7e261042ce
>
>
> Diff: https://reviews.apache.org/r/62145/diff/1/
>
>
> Testing
> -------
>
> TODO
>
>
> Thanks,
>
> Joseph Wu
>
>