Re: Review Request 41869: Removed Docker auth server flag.

2016-01-04 Thread Timothy Chen 


> On Jan. 4, 2016, 5:42 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp, line 
> > 117
> > 
> >
> > Can we make this ```mutable``` and keep all the methods ```const``` ?

This is already mutable? I can't make them const if I want to modify a class's 
field variable right?


- Timothy


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


On Jan. 4, 2016, 2:13 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41869/
> ---
> 
> (Updated Jan. 4, 2016, 2:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently Docker auth server is being configured via docker_auth_server flag, 
> but we should actually get the docker auth server information from docker 
> registry. This allows us to remove one flag and also support both public and 
> private docker registry.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> e73d4d785bccf1d50af9e91d2d20dafb444bec68 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> 4e305ade1f9f8b42de5c8db636c6d3f5d8e2444f 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> 049d46cad5cf94a3fb5d74cbfe649850311d35ad 
>   src/slave/flags.hpp 2b2679c1ae68d120756eaf81e5728d20791d6746 
>   src/slave/flags.cpp a60d3c8022aba93fbd17a46dfff601fb1b25bbee 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d51f342dabf386fb618ef72ce3e36a8bd8c82b5f 
> 
> Diff: https://reviews.apache.org/r/41869/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 41869: Removed Docker auth server flag.

2016-01-04 Thread Timothy Chen 


> On Jan. 4, 2016, 6:33 a.m., Jojy Varghese wrote:
> > Thanks Tim for taking care of this. I have been thinking that we should 
> > have TokenManager keep a map of  -> . This 
> > would help us to have any realm being handled by the registry_client + 
> > token manager. This way, all pull requests will request the TokenManager 
> > for a token with ```getToken(realm, service, scope)```. TokenManager will 
> > lookup the cache and return the token or create a new item in the cache 
> > (after getting the token from realm).
> > 
> > What do you think?

My current thinking is that we create a token manager per realm and allow the 
registry client to talk to multiple registries. This patch I'm only creating 
one token manager for now, and will change this as soon as we start supporting 
private registries.
And also I think for the token manager we also need to allow the tuple 
(service, scope, user) to be the key.


- Timothy


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


On Jan. 4, 2016, 2:13 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41869/
> ---
> 
> (Updated Jan. 4, 2016, 2:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently Docker auth server is being configured via docker_auth_server flag, 
> but we should actually get the docker auth server information from docker 
> registry. This allows us to remove one flag and also support both public and 
> private docker registry.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> e73d4d785bccf1d50af9e91d2d20dafb444bec68 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> 4e305ade1f9f8b42de5c8db636c6d3f5d8e2444f 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> 049d46cad5cf94a3fb5d74cbfe649850311d35ad 
>   src/slave/flags.hpp 2b2679c1ae68d120756eaf81e5728d20791d6746 
>   src/slave/flags.cpp a60d3c8022aba93fbd17a46dfff601fb1b25bbee 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d51f342dabf386fb618ef72ce3e36a8bd8c82b5f 
> 
> Diff: https://reviews.apache.org/r/41869/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 41869: Removed Docker auth server flag.

2016-01-04 Thread Timothy Chen 


> On Jan. 4, 2016, 5:42 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp, lines 
> > 293-294
> > 
> >
> > These 2 lines can be combined as: 
> > 
> > ``` 
> > Try realmUrl = http::URL::parse(attributes.at("realm"));
> > ```

I'm using the realm variable later.


> On Jan. 4, 2016, 5:42 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp, line 
> > 285
> > 
> >
> > Since realm is needed only for tokenmanager creation, this could be 
> > moved inside the block starting at L260.

technically but I think it's safe to check everytime since we're assuming it's 
always available!


- Timothy


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


On Jan. 4, 2016, 2:13 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41869/
> ---
> 
> (Updated Jan. 4, 2016, 2:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently Docker auth server is being configured via docker_auth_server flag, 
> but we should actually get the docker auth server information from docker 
> registry. This allows us to remove one flag and also support both public and 
> private docker registry.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> e73d4d785bccf1d50af9e91d2d20dafb444bec68 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> 4e305ade1f9f8b42de5c8db636c6d3f5d8e2444f 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> 049d46cad5cf94a3fb5d74cbfe649850311d35ad 
>   src/slave/flags.hpp 2b2679c1ae68d120756eaf81e5728d20791d6746 
>   src/slave/flags.cpp a60d3c8022aba93fbd17a46dfff601fb1b25bbee 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d51f342dabf386fb618ef72ce3e36a8bd8c82b5f 
> 
> Diff: https://reviews.apache.org/r/41869/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 41869: Removed Docker auth server flag.

2016-01-03 Thread Jojy Varghese 

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


Thanks Tim for taking care of this. I have been thinking that we should have 
TokenManager keep a map of  -> . This would help 
us to have any realm being handled by the registry_client + token manager. This 
way, all pull requests will request the TokenManager for a token with 
```getToken(realm, service, scope)```. TokenManager will lookup the cache and 
return the token or create a new item in the cache (after getting the token 
from realm).

What do you think?

- Jojy Varghese


On Jan. 4, 2016, 2:13 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41869/
> ---
> 
> (Updated Jan. 4, 2016, 2:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently Docker auth server is being configured via docker_auth_server flag, 
> but we should actually get the docker auth server information from docker 
> registry. This allows us to remove one flag and also support both public and 
> private docker registry.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> e73d4d785bccf1d50af9e91d2d20dafb444bec68 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> 4e305ade1f9f8b42de5c8db636c6d3f5d8e2444f 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> 049d46cad5cf94a3fb5d74cbfe649850311d35ad 
>   src/slave/flags.hpp 2b2679c1ae68d120756eaf81e5728d20791d6746 
>   src/slave/flags.cpp a60d3c8022aba93fbd17a46dfff601fb1b25bbee 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d51f342dabf386fb618ef72ce3e36a8bd8c82b5f 
> 
> Diff: https://reviews.apache.org/r/41869/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 41869: Removed Docker auth server flag.

2016-01-03 Thread Jojy Varghese 

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



src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp (line 113)


Can we make this ```mutable``` and keep all the methods ```const``` ?



src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp (line 254)


Since realm is needed only for tokenmanager creation, this could be moved 
inside the block starting at L260.



src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp (lines 262 
- 263)


These 2 lines can be combined as: 

``` 
Try realmUrl = http::URL::parse(attributes.at("realm"));
```


- Jojy Varghese


On Jan. 4, 2016, 2:13 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41869/
> ---
> 
> (Updated Jan. 4, 2016, 2:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently Docker auth server is being configured via docker_auth_server flag, 
> but we should actually get the docker auth server information from docker 
> registry. This allows us to remove one flag and also support both public and 
> private docker registry.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> e73d4d785bccf1d50af9e91d2d20dafb444bec68 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> 4e305ade1f9f8b42de5c8db636c6d3f5d8e2444f 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> 049d46cad5cf94a3fb5d74cbfe649850311d35ad 
>   src/slave/flags.hpp 2b2679c1ae68d120756eaf81e5728d20791d6746 
>   src/slave/flags.cpp a60d3c8022aba93fbd17a46dfff601fb1b25bbee 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d51f342dabf386fb618ef72ce3e36a8bd8c82b5f 
> 
> Diff: https://reviews.apache.org/r/41869/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 41869: Removed Docker auth server flag.

2016-01-03 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [41868, 41869]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 4, 2016, 2:13 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41869/
> ---
> 
> (Updated Jan. 4, 2016, 2:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently Docker auth server is being configured via docker_auth_server flag, 
> but we should actually get the docker auth server information from docker 
> registry. This allows us to remove one flag and also support both public and 
> private docker registry.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> e73d4d785bccf1d50af9e91d2d20dafb444bec68 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> 4e305ade1f9f8b42de5c8db636c6d3f5d8e2444f 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> 049d46cad5cf94a3fb5d74cbfe649850311d35ad 
>   src/slave/flags.hpp 2b2679c1ae68d120756eaf81e5728d20791d6746 
>   src/slave/flags.cpp a60d3c8022aba93fbd17a46dfff601fb1b25bbee 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d51f342dabf386fb618ef72ce3e36a8bd8c82b5f 
> 
> Diff: https://reviews.apache.org/r/41869/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>