Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Amol Deshmukh]

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

(Updated Jan. 12, 2016, 1:15 a.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.


Changes
---

- Refactored previous change to allow specifying the shiro post filter as a 
constructor arg for tests.
- Updated existing tests with assertions for the new shiro post filter behavior.


Bugs: AURORA-1576
https://issues.apache.org/jira/browse/AURORA-1576


Repository: aurora


Description
---

Allow for plugging in cli-configurable filters that are invoked post shiro 
filters.


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 4b6a872737e5934394e9511af7b6bd96c6034e72 
  src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
19c8a1fe06a24022da11f74d7c96b2942587 
  
src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java 
ac92117e14c105bec74e49d8e80eb56008237abf 

Diff: https://reviews.apache.org/r/42046/diff/


Testing
---

Unit tests:
```
$ ./gradlew clean test
...
BUILD SUCCESSFUL

Total time: 3 mins 24.616 secs
```

End to end tests:
```
$ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
...

*** OK (All tests passed) ***

aurora-scheduler-kerberos stop/waiting
aurora-scheduler start/running, process 15362
+ RETCODE=0
+ restore_netrc
+ mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
+ true
Connection to 127.0.0.1 closed.

```


Thanks,

Amol Deshmukh

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Aurora ReviewBot]

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

Ship it!


Master (f064dc1) 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 Jan. 12, 2016, 1:15 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 12, 2016, 1:15 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Zhitao Li]

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



src/main/java/org/apache/aurora/scheduler/OfferAllocation.java (line 188)


Hmm, I think I'd like to keep this for two reasons:

1. Even though only one callsite uses value other than `false`, the `false` 
value still dictates a `Predicate` to use, so we'll have more code duplicate if 
we move this out;
2. Right now aurora only supports `cpus` as revocable resource. I actually 
wanted to discuss this separately as current prediction in our company 
indicates that we might be more memory bounded. In such a case, we could be 
using memory as revocable resources, too.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 120)


I don't plan on any further usage of the interface other than here in this 
series of patches.

The only possible customization that could happen is the reverse ordering 
of resource preference (`'*'` vs `reserved role`) but I'd like to withhold it 
until someone request such feature.

I think I'll keep the interface definition, change the `AcceptedOfferImpl` 
to package private, and use a factory method anyway. We can revisit this later. 
How does this sound?


- Zhitao Li


On Jan. 11, 2016, 8:22 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 11, 2016, 8:22 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new interface OfferAllocation (with implementation), which 
> allcoates resources to resources field in TaskInfo and ExecutorInfo from an 
> offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present. This can be  customized with a differnet 
> comparator from PREFER_RESERVED in the future when needed.
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure;
> 2. Old code used to construct resources has not been cleaned up yet;
> 3. OfferAllocationImpl's constructor allows to throw, which is a bit awkward.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/OfferAllocation.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> Only testing with new and existing unit test. I'll work on more integration 
> test once we decide that this is the proper direction.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 41785: Replace scheduler log scaffolding with logback

[Bill Farner]

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

(Updated Jan. 11, 2016, 8:55 p.m.)


Review request for Aurora, John Sirois and Zameer Manji.


Changes
---

rebase


Repository: aurora


Description
---

Here's an example of the log format:
```
I1229 22:16:54.568 [pool-10-thread-1, FakeMaster:139] All offers consumed, 
suppressing offer cycle.
```
As opposed to what's on master:
```
I1230 06:26:14.987 THREAD143 
org.apache.aurora.scheduler.app.local.FakeMaster.lambda$start$0: All offers 
consumed, suppressing offer cycle.
```

I could more closely match the existing format, but i think the change is an 
improvement.


Diffs (updated)
-

  NEWS ecb26d2af2c624042e7819acde004278520b3cae 
  build.gradle 8e7ea3790d6a5d378cbd90021a9717d50874fdb9 
  commons/src/main/java/org/apache/aurora/common/logging/Glog.java 
5bae399cd9360a0093c67c608cf68b013a709194 
  commons/src/main/java/org/apache/aurora/common/logging/LogFormatter.java 
0cb621da2b2f286d9eda9eb18ecac087208d7b6b 
  commons/src/main/java/org/apache/aurora/common/logging/RootLogConfig.java 
26dd0aa5682faf31ba4cc086d32bf61557ef1fde 
  commons/src/main/java/org/apache/aurora/common/logging/log4j/GlogLayout.java 
1a90ded72c81a7e3fdc9dab515a2622c55563e6a 
  
commons/src/main/java/org/apache/aurora/common/net/http/handlers/LogConfig.java 
d3c6dddf86ac3dc0b36b851e16c8ada9a896b8f0 
  
commons/src/main/resources/org/apache/aurora/common/net/http/handlers/logconfig.st
  
  commons/src/test/java/org/apache/aurora/common/logging/LogFormatterTest.java 
9f041915c7fd4513a6255b05b3d0096779b5f1b3 
  commons/src/test/java/org/apache/aurora/common/logging/RootLogConfigTest.java 
9d55a1aaf555d6e25ad97622612fad61271d0e25 
  config/legacy_untested_classes.txt f183518122b7815e64d55b58b5a6de56ec7d0ef4 
  examples/vagrant/upstart/aurora-scheduler-kerberos.conf 
4f43892723db4744db205ea7dd107e9e9ce9d5db 
  examples/vagrant/upstart/aurora-scheduler.conf 
4033184451f36cb5f0233ea96e3dceaae6741275 
  src/main/java/org/apache/aurora/scheduler/app/Log4jConfigurator.java 
348ff1345a3d5ff4212a2cb211e973ad9e2ca2e8 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
de018dd3cd82b7a0a1cb285f8f3172dae529817f 
  src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
6c5654ba33783f81b8da7b6500bf4fc8f101e7e3 
  src/main/resources/logback.xml PRE-CREATION 

Diff: https://reviews.apache.org/r/41785/diff/


Testing
---

end-to-end tests are green


Thanks,

Bill Farner

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Zhitao Li]

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

(Updated Jan. 12, 2016, 2:29 a.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

- Review comments
- Adding 'mesos_role' to command line argument.


Bugs: AURORA-1109
https://issues.apache.org/jira/browse/AURORA-1109


Repository: aurora


Description
---

This review is a prototype for introducing multiple role support in Aurora.
This creates a new interface OfferAllocation (with implementation), which 
allcoates resources to resources field in TaskInfo and ExecutorInfo from an 
offer.

Current implementation prefers reserved resources over shared resources ('*' 
role) if both are present. This can be  customized with a differnet comparator 
from PREFER_RESERVED in the future when needed.

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure;
2. Old code used to construct resources has not been cleaned up yet;
3. OfferAllocationImpl's constructor allows to throw, which is a bit awkward.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
7c3d681c216b78eeecebbe950186e5a79c6fe982 
  src/main/java/org/apache/aurora/scheduler/Resources.java 
db422a959ee7b982c2a44323de41ad75d1a40754 
  
src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
 2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
8fdadda67478bb3110aa442b7d78493cf9c3edb4 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
7e8e456e288986eb0ce92a123b294e1e25d8ed18 
  src/test/java/org/apache/aurora/scheduler/AcceptedOfferImplTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
e4ae943303823ac4bfbe999ed22f5999484462d8 
  
src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
 33149ab415292eff04f38b61f2b1d1eac79f347a 
  src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
  src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
3cbe9acd75def14ae2e0986914ba621fb164b3e4 

Diff: https://reviews.apache.org/r/42126/diff/


Testing (updated)
---

1. Unit tested with old and new tests;
2. vagrant integration tests: I manually separate out the vagrant box's cpu and 
memory between 'aurora-test' role and '*' and verified that jobs can still be 
launched (I can post the vagrant change in another follow upon request).


Thanks,

Zhitao Li

Re: Review Request 42177: Vagrant change to reserve part of the dev cluster's resources to 'aurora-role'

[Aurora ReviewBot]

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

Ship it!


Master (f064dc1) 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 Jan. 12, 2016, 2:34 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42177/
> ---
> 
> (Updated Jan. 12, 2016, 2:34 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is how I tested changes in https://reviews.apache.org/r/42176/.
> 
> 
> Diffs
> -
> 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4033184451f36cb5f0233ea96e3dceaae6741275 
>   examples/vagrant/upstart/mesos-master.conf 
> 9d7491c08e14e3951b2fd2f74291a2009883c379 
>   examples/vagrant/upstart/mesos-slave.conf 
> 1ef059b17d16d4f1594a19ff6422ea653a413359 
> 
> Diff: https://reviews.apache.org/r/42177/diff/
> 
> 
> Testing
> ---
> 
> Integration test script.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Aurora ReviewBot]

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

Ship it!


Master (f064dc1) 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 Jan. 12, 2016, 2:29 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 12, 2016, 2:29 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new interface OfferAllocation (with implementation), which 
> allcoates resources to resources field in TaskInfo and ExecutorInfo from an 
> offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present. This can be  customized with a differnet 
> comparator from PREFER_RESERVED in the future when needed.
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure;
> 2. Old code used to construct resources has not been cleaned up yet;
> 3. OfferAllocationImpl's constructor allows to throw, which is a bit awkward.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferImplTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 41785: Replace scheduler log scaffolding with logback

[Aurora ReviewBot]

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


Master (6a5309a) is green with this patch.
  ./build-support/jenkins/build.sh

However, it appears that it might lack test coverage.

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

- Aurora ReviewBot


On Jan. 12, 2016, 4:55 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41785/
> ---
> 
> (Updated Jan. 12, 2016, 4:55 a.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Here's an example of the log format:
> ```
> I1229 22:16:54.568 [pool-10-thread-1, FakeMaster:139] All offers consumed, 
> suppressing offer cycle.
> ```
> As opposed to what's on master:
> ```
> I1230 06:26:14.987 THREAD143 
> org.apache.aurora.scheduler.app.local.FakeMaster.lambda$start$0: All offers 
> consumed, suppressing offer cycle.
> ```
> 
> I could more closely match the existing format, but i think the change is an 
> improvement.
> 
> 
> Diffs
> -
> 
>   NEWS ecb26d2af2c624042e7819acde004278520b3cae 
>   build.gradle 8e7ea3790d6a5d378cbd90021a9717d50874fdb9 
>   commons/src/main/java/org/apache/aurora/common/logging/Glog.java 
> 5bae399cd9360a0093c67c608cf68b013a709194 
>   commons/src/main/java/org/apache/aurora/common/logging/LogFormatter.java 
> 0cb621da2b2f286d9eda9eb18ecac087208d7b6b 
>   commons/src/main/java/org/apache/aurora/common/logging/RootLogConfig.java 
> 26dd0aa5682faf31ba4cc086d32bf61557ef1fde 
>   
> commons/src/main/java/org/apache/aurora/common/logging/log4j/GlogLayout.java 
> 1a90ded72c81a7e3fdc9dab515a2622c55563e6a 
>   
> commons/src/main/java/org/apache/aurora/common/net/http/handlers/LogConfig.java
>  d3c6dddf86ac3dc0b36b851e16c8ada9a896b8f0 
>   
> commons/src/main/resources/org/apache/aurora/common/net/http/handlers/logconfig.st
>   
>   
> commons/src/test/java/org/apache/aurora/common/logging/LogFormatterTest.java 
> 9f041915c7fd4513a6255b05b3d0096779b5f1b3 
>   
> commons/src/test/java/org/apache/aurora/common/logging/RootLogConfigTest.java 
> 9d55a1aaf555d6e25ad97622612fad61271d0e25 
>   config/legacy_untested_classes.txt f183518122b7815e64d55b58b5a6de56ec7d0ef4 
>   examples/vagrant/upstart/aurora-scheduler-kerberos.conf 
> 4f43892723db4744db205ea7dd107e9e9ce9d5db 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4033184451f36cb5f0233ea96e3dceaae6741275 
>   src/main/java/org/apache/aurora/scheduler/app/Log4jConfigurator.java 
> 348ff1345a3d5ff4212a2cb211e973ad9e2ca2e8 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> de018dd3cd82b7a0a1cb285f8f3172dae529817f 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 6c5654ba33783f81b8da7b6500bf4fc8f101e7e3 
>   src/main/resources/logback.xml PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41785/diff/
> 
> 
> Testing
> ---
> 
> end-to-end tests are green
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Review Request 42177: Vagrant change to reserve part of the dev cluster's resources to 'aurora-role'

[Zhitao Li]

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

Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Bugs: AURORA-1109
https://issues.apache.org/jira/browse/AURORA-1109


Repository: aurora


Description
---

This is how I tested changes in https://reviews.apache.org/r/42176/.


Diffs
-

  examples/vagrant/upstart/aurora-scheduler.conf 
4033184451f36cb5f0233ea96e3dceaae6741275 
  examples/vagrant/upstart/mesos-master.conf 
9d7491c08e14e3951b2fd2f74291a2009883c379 
  examples/vagrant/upstart/mesos-slave.conf 
1ef059b17d16d4f1594a19ff6422ea653a413359 

Diff: https://reviews.apache.org/r/42177/diff/


Testing
---

Integration test script.


Thanks,

Zhitao Li

Re: Review Request 34568: run tasks as user that match aurora-role

[Bill Farner]

> On Jan. 9, 2016, 4:43 p.m., Benjamin Staffin wrote:
> > Any chance of getting this merged?  It might be a surprise to some users, 
> > since right now there's no requirement for matching role accounts to exist 
> > inside the docker images, but with the right documentation about how to 
> > retain the "always root in the container" behavior it ought to be ok.

Unless Bhuvan is willing to revisit, you might have to take over the patch 
(which i would welcome).  I would like to see a few changes to accept this:
- enabled with a flag to the executor at least for the first release
- e2e test case verifying the expected behavior


- Bill


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


On Nov. 2, 2015, 10:26 a.m., Bhuvan Arumugam wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34568/
> ---
> 
> (Updated Nov. 2, 2015, 10:26 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Zameer Manji.
> 
> 
> Bugs: AURORA-1237
> https://issues.apache.org/jira/browse/AURORA-1237
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The tasks inside container should run as user that match aurora-role, if
> --nosetuid option is not passed for executor.
> 
> Testing Done:
> ./pants test.pytest --no-fast --options='--verbose' src/test/python::
> 
> Bugs closed: AURORA-1237
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 8ce9168 
> 
> Diff: https://reviews.apache.org/r/34568/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bhuvan Arumugam
> 
>

Re: Review Request 42077: Introduces -default_docker_parameters scheduler flag.

[Bill Farner]

> On Jan. 8, 2016, 10:50 a.m., Stephan Erb wrote:
> > docs/deploying-aurora-scheduler.md, line 187
> > 
> >
> > Make it more explicit that those will only be used if a job has no 
> > custom paramters.
> 
> George Sirois wrote:
> Yep, can do, although perhaps we should make sure everyone is ok with 
> that behavior (as opposed to implementing a merge).

I think a merge would be nice for cross-cutting parameters.  As for who trumps 
whom, i would lean towards the operator (since security controls are in play).  
Any thoughts on that approach?


- Bill


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


On Jan. 8, 2016, 10:28 a.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42077/
> ---
> 
> (Updated Jan. 8, 2016, 10:28 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1575
> https://issues.apache.org/jira/browse/AURORA-1575
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This flag allows cluster administrators to set arbitrary
> Docker parameters which will apply to all jobs.
> 
> Also cleans up some of the existing unit tests around task config.
> 
> 
> Diffs
> -
> 
>   README.md f3b23247cf3f7e550c1714b4fb8227a2baab3b41 
>   
> commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java
>  PRE-CREATION 
>   docs/deploying-aurora-scheduler.md 8a1e68e5d54e9b8b66139bfc731563668584fa77 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 4eee8e31a4ccd25ba4a5bcb60b67c79979c3b9b0 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> d7f3c60d383cf10afb0c0fcf4fe29972b183458c 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  05e8b714043dea89039ce9a1fc4b32c65ab15fe4 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  f3b62cc957186bc9673060830572bc1cc073ac49 
> 
> Diff: https://reviews.apache.org/r/42077/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Bill Farner]

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


Nice job!  Most comments here are nits to align code style/conventions.  I've 
mostly left `OfferAllocation.java` alone in this pass, but you should consider 
applying the spirit of these comments there (specifically immutability, testing 
public interface).

Also, the name `OfferAllocation` doesn't sound quite right.  How about 
`AcceptedOffer`?  I don't have a strong opinion here, names are hard and highly 
subjective :-)


src/main/java/org/apache/aurora/scheduler/OfferAllocation.java (line 188)


Consider handling `revocable` with a filter on the result of this function 
rather than an argument since only one caller passes something other than 
`false`.



src/main/java/org/apache/aurora/scheduler/ResourceSlot.java (line 100)


+1 here and below, consider doing that in this patch or an immediate 
follow-up if you'd like to minimize the patch delta.  Your call.



src/main/java/org/apache/aurora/scheduler/Resources.java (line 157)


Reminder to remove this



src/main/java/org/apache/aurora/scheduler/Resources.java (line 169)


Always prefer immutable when it makes sense.  For this function, it's a 
matter of changing to something like:

```
ImmutableList.Builder ranges = ImmutableList.builder();
...

ranges.addAll(Iterables.concat(r.getRanges().getRangeList().iterator()));
...
return ranges.build();
``



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 105)


Nit: our style is to only use the `final` keyword when necessary, or for 
class fields (for reference immutability).

s/final //



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 106)


This block of code doesn't do much explicit validation of fields within the 
protobuf, so IMHO you can safely remove this line.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 120)


IMHO this line removes the value of the interface indirection.  Are there 
follow-ups to this patch where you intend other bodies of code will only rely 
on the interface?



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 128)


Rather than having the constructor throw, consider creating a factory 
function that creates an `OfferAllocation`.  Additionally, please declare 
`throws InsufficientResourcesException` on the method signature.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 131)


ditto re: final



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 201)


Is `clearResources()` necessary here?  I think it makes sense to not 
intervene here if an operator with a custom config wants to specify resources, 
even if it's at their peril.



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 49)


I suggest you use the non-builder types in test cases.  Mutability can make 
these types of tests become brittle over time.  With a helper function, you 
should be able to produce an equally-concise test case that uses all immutable 
`Resource` objects.

```
private static Resource makeResouce(String role) {
  ...
}
```



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 69)


I generally prefer to avoid accessing private functions for the purposes of 
tests (unless it's prohibitive).  Seems like we can infer correct sort/filter 
behavior from the contents of `OfferAllocation` instances.  Does it make sense 
to only use the public API in that case?



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 82)


`ImmutableList.of()` can stand in for `Lists.newArrayList()` here and below.



src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 85)


The index-based access is probably not going to age well, especially given 
that the `addAll` calls above can technically add mulitple elements.  While it 
will be more verbose, consider using variables for each of values 

Re: Review Request 42077: Introduces -default_docker_parameters scheduler flag.

[Bill Farner]

> On Jan. 8, 2016, 10:50 a.m., Stephan Erb wrote:
> > docs/deploying-aurora-scheduler.md, line 187
> > 
> >
> > Make it more explicit that those will only be used if a job has no 
> > custom paramters.
> 
> George Sirois wrote:
> Yep, can do, although perhaps we should make sure everyone is ok with 
> that behavior (as opposed to implementing a merge).
> 
> Bill Farner wrote:
> I think a merge would be nice for cross-cutting parameters.  As for who 
> trumps whom, i would lean towards the operator (since security controls are 
> in play).  Any thoughts on that approach?
> 
> George Sirois wrote:
> A merge makes sense to me, although it still doesn't necessarily mitigate 
> all security issues. IMO running with allow_docker_parameters=true is a bit 
> of a caveat emptor situation, so maybe not worth trying to overengineer 
> around that.
> 
> Presumably the merge would be by *key*, where the value(s) from the 
> scheduler have precedence over job config? For example (assuming 
> allow_docker_parameters=true):
> 
> ```
> Scheduler: default_docker_parameters="foo=bar,foo=baz"
> Job Config: "foo=zap"
> 
> Final "foo=bar,foo=baz", NOT "foo=bar,foo=baz,foo=zap"
> ```

Stephan, any thoughts?  I tend to agree with George's comment above - allowing 
job submitters to specify arbitrary docker parameters should only really be 
used in a cluster that is secured externally to Aurora.  Perhaps the simplest 
approach is the best - don't try to mix parameters from these two sources.


- Bill


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


On Jan. 8, 2016, 10:28 a.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42077/
> ---
> 
> (Updated Jan. 8, 2016, 10:28 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1575
> https://issues.apache.org/jira/browse/AURORA-1575
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This flag allows cluster administrators to set arbitrary
> Docker parameters which will apply to all jobs.
> 
> Also cleans up some of the existing unit tests around task config.
> 
> 
> Diffs
> -
> 
>   README.md f3b23247cf3f7e550c1714b4fb8227a2baab3b41 
>   
> commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java
>  PRE-CREATION 
>   docs/deploying-aurora-scheduler.md 8a1e68e5d54e9b8b66139bfc731563668584fa77 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 4eee8e31a4ccd25ba4a5bcb60b67c79979c3b9b0 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> d7f3c60d383cf10afb0c0fcf4fe29972b183458c 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  05e8b714043dea89039ce9a1fc4b32c65ab15fe4 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  f3b62cc957186bc9673060830572bc1cc073ac49 
> 
> Diff: https://reviews.apache.org/r/42077/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Joshua Cohen]

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



src/main/java/org/apache/aurora/scheduler/OfferAllocation.java (lines 56 - 60)


One more style nit: our style for contination indents is to indent four 
spaces rather than align method args. Additionally a continuation-indented line 
should be immediately followed by a blank line. Which is to say, this (and 
other instances of aligned method args) should be formatted as:

public OfferAllocationImpl(
Offer offer,
ResourceSlot taskSlot,
...
TierInfo tierInfo) {

  requireNonNull(offer);
  ...


- Joshua Cohen


On Jan. 11, 2016, 5:41 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 11, 2016, 5:41 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new interface OfferAllocation (with implementation), which 
> allcoates resources to resources field in TaskInfo and ExecutorInfo from an 
> offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present. This can be  customized with a differnet 
> comparator from PREFER_RESERVED in the future when needed.
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure;
> 2. Old code used to construct resources has not been cleaned up yet;
> 3. OfferAllocationImpl's constructor allows to throw, which is a bit awkward.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/OfferAllocation.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> Only testing with new and existing unit test. I'll work on more integration 
> test once we decide that this is the proper direction.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42077: Introduces -default_docker_parameters scheduler flag.

[George Sirois]

> On Jan. 8, 2016, 6:50 p.m., Stephan Erb wrote:
> > docs/deploying-aurora-scheduler.md, line 187
> > 
> >
> > Make it more explicit that those will only be used if a job has no 
> > custom paramters.
> 
> George Sirois wrote:
> Yep, can do, although perhaps we should make sure everyone is ok with 
> that behavior (as opposed to implementing a merge).
> 
> Bill Farner wrote:
> I think a merge would be nice for cross-cutting parameters.  As for who 
> trumps whom, i would lean towards the operator (since security controls are 
> in play).  Any thoughts on that approach?

A merge makes sense to me, although it still doesn't necessarily mitigate all 
security issues. IMO running with allow_docker_parameters=true is a bit of a 
caveat emptor situation, so maybe not worth trying to overengineer around that.

Presumably the merge would be by *key*, where the value(s) from the scheduler 
have precedence over job config? For example (assuming 
allow_docker_parameters=true):

```
Scheduler: default_docker_parameters="foo=bar,foo=baz"
Job Config: "foo=zap"

Final "foo=bar,foo=baz", NOT "foo=bar,foo=baz,foo=zap"
```


- George


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


On Jan. 8, 2016, 6:28 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42077/
> ---
> 
> (Updated Jan. 8, 2016, 6:28 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1575
> https://issues.apache.org/jira/browse/AURORA-1575
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This flag allows cluster administrators to set arbitrary
> Docker parameters which will apply to all jobs.
> 
> Also cleans up some of the existing unit tests around task config.
> 
> 
> Diffs
> -
> 
>   README.md f3b23247cf3f7e550c1714b4fb8227a2baab3b41 
>   
> commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java
>  PRE-CREATION 
>   docs/deploying-aurora-scheduler.md 8a1e68e5d54e9b8b66139bfc731563668584fa77 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 4eee8e31a4ccd25ba4a5bcb60b67c79979c3b9b0 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> d7f3c60d383cf10afb0c0fcf4fe29972b183458c 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  05e8b714043dea89039ce9a1fc4b32c65ab15fe4 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  f3b62cc957186bc9673060830572bc1cc073ac49 
> 
> Diff: https://reviews.apache.org/r/42077/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 42145: Use tags instead of branches for release candidates.

[Bill Farner]

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



build-support/release/release-candidate (line 29)


I renamed this variable as i found it confusing to have `rc_tag_version` 
and `current_version_tag` variables.



build-support/release/release-candidate 


Looking through the commands, i didn't find that the use of `$base_dir` was 
necessary.  I think the resulting code is simpler without it.


- Bill Farner


On Jan. 11, 2016, 7:22 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42145/
> ---
> 
> (Updated Jan. 11, 2016, 7:22 a.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Follow-up from https://reviews.apache.org/r/42117 for release candidate script
> 
> 
> Diffs
> -
> 
>   build-support/release/release-candidate 
> 114bb6d284647ad9745679fa3d13684ffd45ac31 
> 
> Diff: https://reviews.apache.org/r/42145/diff/
> 
> 
> Testing
> ---
> 
> Ran `./build-support/release/release-candidate` in non-publish mode, verified 
> files and e-mail boilerplate generated.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42145: Use tags instead of branches for release candidates.

[Aurora ReviewBot]

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

Ship it!


Master (e4c9c73) 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 Jan. 11, 2016, 3:22 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42145/
> ---
> 
> (Updated Jan. 11, 2016, 3:22 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Follow-up from https://reviews.apache.org/r/42117 for release candidate script
> 
> 
> Diffs
> -
> 
>   build-support/release/release-candidate 
> 114bb6d284647ad9745679fa3d13684ffd45ac31 
> 
> Diff: https://reviews.apache.org/r/42145/diff/
> 
> 
> Testing
> ---
> 
> Ran `./build-support/release/release-candidate` in non-publish mode, verified 
> files and e-mail boilerplate generated.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42145: Use tags instead of branches for release candidates.

[Zameer Manji]

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

Ship it!


Ship It!

- Zameer Manji


On Jan. 11, 2016, 7:22 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42145/
> ---
> 
> (Updated Jan. 11, 2016, 7:22 a.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Follow-up from https://reviews.apache.org/r/42117 for release candidate script
> 
> 
> Diffs
> -
> 
>   build-support/release/release-candidate 
> 114bb6d284647ad9745679fa3d13684ffd45ac31 
> 
> Diff: https://reviews.apache.org/r/42145/diff/
> 
> 
> Testing
> ---
> 
> Ran `./build-support/release/release-candidate` in non-publish mode, verified 
> files and e-mail boilerplate generated.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42077: Introduces -default_docker_parameters scheduler flag.

[Stephan Erb]

> On Jan. 8, 2016, 7:50 p.m., Stephan Erb wrote:
> > docs/deploying-aurora-scheduler.md, line 187
> > 
> >
> > Make it more explicit that those will only be used if a job has no 
> > custom paramters.
> 
> George Sirois wrote:
> Yep, can do, although perhaps we should make sure everyone is ok with 
> that behavior (as opposed to implementing a merge).
> 
> Bill Farner wrote:
> I think a merge would be nice for cross-cutting parameters.  As for who 
> trumps whom, i would lean towards the operator (since security controls are 
> in play).  Any thoughts on that approach?
> 
> George Sirois wrote:
> A merge makes sense to me, although it still doesn't necessarily mitigate 
> all security issues. IMO running with allow_docker_parameters=true is a bit 
> of a caveat emptor situation, so maybe not worth trying to overengineer 
> around that.
> 
> Presumably the merge would be by *key*, where the value(s) from the 
> scheduler have precedence over job config? For example (assuming 
> allow_docker_parameters=true):
> 
> ```
> Scheduler: default_docker_parameters="foo=bar,foo=baz"
> Job Config: "foo=zap"
> 
> Final "foo=bar,foo=baz", NOT "foo=bar,foo=baz,foo=zap"
> ```
> 
> Bill Farner wrote:
> Stephan, any thoughts?  I tend to agree with George's comment above - 
> allowing job submitters to specify arbitrary docker parameters should only 
> really be used in a cluster that is secured externally to Aurora.  Perhaps 
> the simplest approach is the best - don't try to mix parameters from these 
> two sources.

Yeah, keeping it the way it is right now (no merge) seems sufficient. My 
comment above only aimed at making that current behaviour a little bit more 
obvious if one is just looking at the docs.


- Stephan


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


On Jan. 8, 2016, 7:28 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42077/
> ---
> 
> (Updated Jan. 8, 2016, 7:28 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1575
> https://issues.apache.org/jira/browse/AURORA-1575
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This flag allows cluster administrators to set arbitrary
> Docker parameters which will apply to all jobs.
> 
> Also cleans up some of the existing unit tests around task config.
> 
> 
> Diffs
> -
> 
>   README.md f3b23247cf3f7e550c1714b4fb8227a2baab3b41 
>   
> commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java
>  PRE-CREATION 
>   docs/deploying-aurora-scheduler.md 8a1e68e5d54e9b8b66139bfc731563668584fa77 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 4eee8e31a4ccd25ba4a5bcb60b67c79979c3b9b0 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> d7f3c60d383cf10afb0c0fcf4fe29972b183458c 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  05e8b714043dea89039ce9a1fc4b32c65ab15fe4 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  f3b62cc957186bc9673060830572bc1cc073ac49 
> 
> Diff: https://reviews.apache.org/r/42077/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Amol Deshmukh]

> On Jan. 10, 2016, 5:21 a.m., Bill Farner wrote:
> > -1, please add test coverage to mitigate likelihood of breaking this change.
> 
> Joshua Cohen wrote:
> +1 to the -1 ;). Please add tests!

Please let me know if you have suggestions/guidance for adding tests for this 
change. Given that this was a change to add generic hook to the module, I 
presumed existing tests would suffice (to prove no regression was introduced).


- Amol


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


On Jan. 9, 2016, 12:55 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 9, 2016, 12:55 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42077: Introduces -default_docker_parameters scheduler flag.

[Aurora ReviewBot]

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

Ship it!


Master (f064dc1) 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 Jan. 11, 2016, 7:32 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42077/
> ---
> 
> (Updated Jan. 11, 2016, 7:32 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1575
> https://issues.apache.org/jira/browse/AURORA-1575
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This flag allows cluster administrators to set arbitrary
> Docker parameters which will apply to all jobs.
> 
> Also cleans up some of the existing unit tests around task config.
> 
> 
> Diffs
> -
> 
>   README.md f3b23247cf3f7e550c1714b4fb8227a2baab3b41 
>   
> commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java
>  PRE-CREATION 
>   docs/deploying-aurora-scheduler.md 8a1e68e5d54e9b8b66139bfc731563668584fa77 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 4eee8e31a4ccd25ba4a5bcb60b67c79979c3b9b0 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> d7f3c60d383cf10afb0c0fcf4fe29972b183458c 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  05e8b714043dea89039ce9a1fc4b32c65ab15fe4 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  f3b62cc957186bc9673060830572bc1cc073ac49 
> 
> Diff: https://reviews.apache.org/r/42077/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 42077: Introduces -default_docker_parameters scheduler flag.

[George Sirois]

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

(Updated Jan. 11, 2016, 7:32 p.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

Made documentation more explicit + a bit more test cleanup. Should be good for 
review now.


Bugs: AURORA-1575
https://issues.apache.org/jira/browse/AURORA-1575


Repository: aurora


Description
---

This flag allows cluster administrators to set arbitrary
Docker parameters which will apply to all jobs.

Also cleans up some of the existing unit tests around task config.


Diffs (updated)
-

  README.md f3b23247cf3f7e550c1714b4fb8227a2baab3b41 
  
commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java 
PRE-CREATION 
  docs/deploying-aurora-scheduler.md 8a1e68e5d54e9b8b66139bfc731563668584fa77 
  src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
4eee8e31a4ccd25ba4a5bcb60b67c79979c3b9b0 
  src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
d7f3c60d383cf10afb0c0fcf4fe29972b183458c 
  
src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
 05e8b714043dea89039ce9a1fc4b32c65ab15fe4 
  
src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
 f3b62cc957186bc9673060830572bc1cc073ac49 

Diff: https://reviews.apache.org/r/42077/diff/


Testing
---

./build-support/jenkins/build.sh


Thanks,

George Sirois