Re: Review Request 66502: Update python virtualenv

[Renan DelValle]

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


Ship it!




Ship It!

- Renan DelValle


On April 9, 2018, 2:10 a.m., se choi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66502/
> ---
> 
> (Updated April 9, 2018, 2:10 a.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Update python virtualenv
> 
> 
> Diffs
> -
> 
>   pants 312dd2035a5ad2e65a1fb3f52d1c36693c2624f0 
> 
> 
> Diff: https://reviews.apache.org/r/66502/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> se choi
> 
>

Re: Review Request 66490: Implement Mesos docker/volume isolator support for aurora

[Justin Venus]

> On April 9, 2018, 10:33 p.m., Renan DelValle wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift
> > Lines 203 (patched)
> > 
> >
> > Can we get away with using a string here instead of a struct?

Sure I'll look at refactoring it to use a string.


> On April 9, 2018, 10:33 p.m., Renan DelValle wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift
> > Lines 1263 (patched)
> > 
> >
> > I think the better solution here would be to make Volume.hostPath 
> > optional and handle hostPath not being set at the Java server side. Maybe 
> > have hostPath mirror to VolumeSource.hostpath while it is being deprecated.
> > 
> > This can be done somewhere in 
> > `src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java`

Will it break anything to make Volume.hostPath optional?   It's my unstanding 
that this is implicitly required now.


> On April 9, 2018, 10:33 p.m., Renan DelValle wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
> > Lines 277 (patched)
> > 
> >
> > I'm not 100% sure what happens if VolumeType is not set (esp since it's 
> > optional) here but it may crash the scheduler. There should be a check 
> > added to 
> > `src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java`
> > 
> > That way even if someone uses a custom client to schedule a job on 
> > Aurora it'll be able to catch this before it's too late.
> > 
> > It would also be great to add tests to 
> > `src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java`
> >  using this new feature.

I'll make some more tests and fix what shakes out.


> On April 9, 2018, 10:33 p.m., Renan DelValle wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
> > Lines 284 (patched)
> > 
> >
> > Can we replace all the iterables in this patch with their Java 8 
> > counterpart (e.g. streams)?
> > 
> > The project is currently moving in the direction of deprecating the use 
> > of Guava features that are now replaceable by features in Java 8.

Will do.


- Justin


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


On April 7, 2018, 12:04 a.m., Justin Venus wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66490/
> ---
> 
> (Updated April 7, 2018, 12:04 a.m.)
> 
> 
> Review request for Aurora, Renan DelValle and Stephan Erb.
> 
> 
> Bugs: AURORA-1983
> https://issues.apache.org/jira/browse/AURORA-1983
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This change implements the docker/volume isolator as described at 
> http://mesos.apache.org/documentation/latest/isolators/docker-volume/.
>  
> * update config thrift tests to check for docker/volume isolator support
> * update mesos task factory to support docker/volume isolator
> * update documentation for docker/volume isolator support
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> ef754e32172e7490a47a13e7b526f243ffa3efeb 
>   docs/reference/configuration.md d4b869b938105ba301fc88d41019af2f1707f6f4 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> bcb2bbf882f43d813dd26c746d806e78bae6bcf3 
>   src/main/python/apache/aurora/config/schema/base.py 
> a629bcd1261e5959da0a8458a55545d4e2c2a7a5 
>   src/main/python/apache/aurora/config/thrift.py 
> 6d2dde6e964daa68bf6f0e5bbbffecc5bd8c0431 
>   src/test/python/apache/aurora/config/test_thrift.py 
> 8e1d0e177959af12b97bdd1cd47845b72bc12fe1 
> 
> 
> Diff: https://reviews.apache.org/r/66490/diff/4/
> 
> 
> Testing
> ---
> 
> Tests pass locally
> ```sh
> ./gradlew test
> ./pants test src/test/python::
> ```
> 
> I've deployed this code to a test cluster with rexray, dvdcli, and the agents 
> have `docker/volume` isolation enabled.  I am able to exercise mounting an 
> EBS volume as I would expect into a container.
> 
> 
> Thanks,
> 
> Justin Venus
> 
>

Re: Review Request 66490: Implement Mesos docker/volume isolator support for aurora

[Renan DelValle]

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



Overall looks good to me. Few fixes here and there. I'll look at the python 
code more thorugoughly next round.


api/src/main/thrift/org/apache/aurora/gen/api.thrift
Lines 203 (patched)


Can we get away with using a string here instead of a struct?



api/src/main/thrift/org/apache/aurora/gen/api.thrift
Lines 1263 (patched)


I think the better solution here would be to make Volume.hostPath optional 
and handle hostPath not being set at the Java server side. Maybe have hostPath 
mirror to VolumeSource.hostpath while it is being deprecated.

This can be done somewhere in 
`src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java`



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
Lines 277 (patched)


I'm not 100% sure what happens if VolumeType is not set (esp since it's 
optional) here but it may crash the scheduler. There should be a check added to 
`src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java`

That way even if someone uses a custom client to schedule a job on Aurora 
it'll be able to catch this before it's too late.

It would also be great to add tests to 
`src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java` 
using this new feature.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
Lines 284 (patched)


Can we replace all the iterables in this patch with their Java 8 
counterpart (e.g. streams)?

The project is currently moving in the direction of deprecating the use of 
Guava features that are now replaceable by features in Java 8.


- Renan DelValle


On April 6, 2018, 5:04 p.m., Justin Venus wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66490/
> ---
> 
> (Updated April 6, 2018, 5:04 p.m.)
> 
> 
> Review request for Aurora, Renan DelValle and Stephan Erb.
> 
> 
> Bugs: AURORA-1983
> https://issues.apache.org/jira/browse/AURORA-1983
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This change implements the docker/volume isolator as described at 
> http://mesos.apache.org/documentation/latest/isolators/docker-volume/.
>  
> * update config thrift tests to check for docker/volume isolator support
> * update mesos task factory to support docker/volume isolator
> * update documentation for docker/volume isolator support
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> ef754e32172e7490a47a13e7b526f243ffa3efeb 
>   docs/reference/configuration.md d4b869b938105ba301fc88d41019af2f1707f6f4 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> bcb2bbf882f43d813dd26c746d806e78bae6bcf3 
>   src/main/python/apache/aurora/config/schema/base.py 
> a629bcd1261e5959da0a8458a55545d4e2c2a7a5 
>   src/main/python/apache/aurora/config/thrift.py 
> 6d2dde6e964daa68bf6f0e5bbbffecc5bd8c0431 
>   src/test/python/apache/aurora/config/test_thrift.py 
> 8e1d0e177959af12b97bdd1cd47845b72bc12fe1 
> 
> 
> Diff: https://reviews.apache.org/r/66490/diff/4/
> 
> 
> Testing
> ---
> 
> Tests pass locally
> ```sh
> ./gradlew test
> ./pants test src/test/python::
> ```
> 
> I've deployed this code to a test cluster with rexray, dvdcli, and the agents 
> have `docker/volume` isolation enabled.  I am able to exercise mounting an 
> EBS volume as I would expect into a container.
> 
> 
> Thanks,
> 
> Justin Venus
> 
>

Re: Review Request 66502: Update python virtualenv

[Aurora ReviewBot]

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


Ship it!




Master (2d6108b) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On April 9, 2018, 9:10 a.m., se choi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66502/
> ---
> 
> (Updated April 9, 2018, 9:10 a.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Update python virtualenv
> 
> 
> Diffs
> -
> 
>   pants 312dd2035a5ad2e65a1fb3f52d1c36693c2624f0 
> 
> 
> Diff: https://reviews.apache.org/r/66502/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> se choi
> 
>

Review Request 66502: Update python virtualenv

[se choi]

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

Review request for Aurora.


Repository: aurora


Description
---

Update python virtualenv


Diffs
-

  pants 312dd2035a5ad2e65a1fb3f52d1c36693c2624f0 


Diff: https://reviews.apache.org/r/66502/diff/1/


Testing
---


Thanks,

se choi