Re: Review Request 47869: Adding support for GPU resource
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47869/#review135245 --- Ship it! Ship It! - Joshua Cohen On May 26, 2016, 8:57 p.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47869/ > --- > > (Updated May 26, 2016, 8:57 p.m.) > > > Review request for Aurora, Joshua Cohen and Stephan Erb. > > > Repository: aurora > > > Description > --- > > This patch adds support for the Mesos GPU resource. While we have to migrate > to Mesos 0.29.0 for this change to take effect, nothing prevents us from > supporting it end-to-end in Aurora now. > > I have also refactored the `configSummary.html` to display all resources > (including ports) in the same section. The table is populated dinamically > with optional resources (GPU, PORTS) hidden if no values are provided for > them. > > > Diffs > - > > RELEASE-NOTES.md 6e878c54d78356f5fc31d98c40b1d95ef30e48af > api/src/main/thrift/org/apache/aurora/gen/api.thrift > a99889c1f2d9e10825f87ea669532ad78641880f > docs/features/resource-isolation.md > d08b59fe030dd94bad4593c1aea905c2ed66e951 > docs/reference/configuration.md e77ee603f35744b6bc6d350927a636f1fd2cf552 > examples/vagrant/mesos_config/etc_mesos-slave/resources > 5bfe779fbb98822d0c58dd92e34765c5586946db > examples/vagrant/upstart/aurora-scheduler.conf > 3d9e706de564df5e24cb34265bebc0db1cad11a0 > src/main/java/org/apache/aurora/scheduler/app/AppModule.java > c81bd7938c86ed48202d8d464063b64fdd21114e > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > 221417f63cc812f0ccd6dbe76ff2734d289e7dfb > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > 4ef202c17c322ca9800d84da31d0bc6ee832d275 > src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java > 6a4f110ff461876ca14c24947f4813d5f2a0dae5 > src/main/python/apache/aurora/config/thrift.py > 81a505550314c9c41f00f7c5f5bd9e093b6199c6 > src/main/python/apache/thermos/config/schema_base.py > a6768e67189b0560afef844d6b269bed8ada5f2f > src/main/python/apache/thermos/config/schema_helpers.py > 46394bb8ffcd8b75a23d8d3ad2113f4fa1eacad2 > src/main/resources/scheduler/assets/configSummary.html > 36df616babf9a391fa3a6b5b4ff0e49ae412ea2d > src/main/resources/scheduler/assets/js/filters.js > 98f786ef50cb8b2e0a086853c639f2d180270e15 > > src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java > ddf8143709563effc87a4f5c14c188d826f6dbe8 > > src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java > a5dda25a4fbbafba6baa814d28bba96f51049125 > src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java > 9cce64193549e80f2493d9e58bd84f6fadad6cfd > src/test/python/apache/aurora/config/test_thrift.py > 8769db34f2bb8cc3a52ef5c3ef95b14ee808a57e > src/test/python/apache/thermos/config/test_schema.py > 0440ee525a58f7c1fd79babf0c8a1c8320cde80a > src/test/sh/org/apache/aurora/e2e/http/http_example.aurora > 219c40fb94561f0a390cac16e643bf4332c51aad > src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora > 08553e4f48f137e0455ad07287086311171c06bd > src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora > 8b3a50ba6de992560593987f3db254baa4d29a41 > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > abe0ca75c6a2c0ace15fce68ad0e5c9aa98193a4 > > Diff: https://reviews.apache.org/r/47869/diff/ > > > Testing > --- > > unit and e2e tests > > > File Attachments > > > config_summary.png > > https://reviews.apache.org/media/uploaded/files/2016/05/26/ea0c14e2-f968-4223-8ce0-af942346547b__config_summary.png > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 47853: Isolate the executor's filesystem from the task's.
> On May 26, 2016, 12:17 p.m., Stephan Erb wrote: > > src/main/python/apache/aurora/executor/common/sandbox.py, lines 208-230 > > <https://reviews.apache.org/r/47853/diff/1/?file=1393961#file1393961line208> > > > > Seeing this spelled out, I feel like we should not have such code in > > Thermos. It is rather low-level, hard to get compeltely right, and can > > impose security risks if done wrong. > > > > Mesos already has to maintain a version of the `chroot`/`pivot_root` > > mechanism. We should try to leverage it. Otherwise, this will be an up-hill > > battle that we cannot win in the long run, especially given the limited > > number of Aurora committers > > > > I see a couple options: > > > > a) We get the Mesos guys to export their `pivot_root` code so that all > > we have to do is call a single Mesos command, just like it could currently > > be done for the `mesos-fetcher` or the entire `mesos-containerizer`: > > > > ``` > > vagrant@aurora:~$ /usr/libexec/mesos/mesos-containerizer > > Usage: /usr/libexec/mesos/mesos-containerizer [OPTIONS] > > > > Available subcommands: > > help > > launch > > mount > > ``` > > > > b) We live with the fact that we'll always run Thermos within the > > container. This will require Python, but we could drop the dependency on > > other Mesos dependencies by migrating to the new executor HTTP API. > > > > > > c) We stick to the `MesosContainer.image` for the user container, but > > use a `Volume` `image` in order to mount a minimal container that is just > > packaging Thermos and its dependencies. We could then launch the executor > > from the mounted volume such as `/thermos/bin/thermox_executor.sh` (with > > appropriate P`YTHON_PATH` and `LD_LIBRARY_PATH`). The user could would > > still be running using the user-image. > > > > > > Approaches b) and c) keep the door open for us to run the executor as > > an un-priviledged user. This has security advantages and would als make it > > easier for users to leverage security features such as the new Mesos Linux > > Capabilities > > https://docs.google.com/document/d/1YiTift8TQla2vq3upQr7K-riQ_pQ-FKOCOsysQJROGc/edit?pref=2&pli=1 Just to capture offline discussion here: I've reached out to mesos-dev list and they're open to the idea of adding a subcommand to mesos-containerizer that would be responsible for isolating a task's filesystem. We hope to work on and contribute this patch next week during Mesoscon. Given that, I'm going to hold off on this review for the time being. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47853/#review134955 --- On May 25, 2016, 9:18 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47853/ > --- > > (Updated May 25, 2016, 9:18 p.m.) > > > Review request for Aurora, Maxim Khutornenko and Stephan Erb. > > > Repository: aurora > > > Description > --- > > This changes the approach to launching tasks with filesystem images in the > unified containerizer. Instead of adding an `Image` to the `MesosContainer`, > we instead add the task filesystem as a `Volume` with an associated image. > This image is mounted in the mesos directory under the `taskfs` path. The > executor, on start up does the following: > > 1. Creates user/group under the taskfs root. > 2. `pivot_root`s into the taskfs, while bind mounting the sandbox under that > root as well as mounting procfs. > 3. From there, task execution is essentially unchanged minus some slight > changes to the environment depending on whether we're running in a pivoted > root. > > > Diffs > - > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > a99889c1f2d9e10825f87ea669532ad78641880f > examples/vagrant/upstart/aurora-scheduler.conf > 3d9e706de564df5e24cb34265bebc0db1cad11a0 > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > 3b01801d929dd61ee989495bf38af8f03e9f5ad4 > src/main/python/apache/aurora/executor/common/sandbox.py > be1deba6219462c9fdaaf07a583851b85fe974bf > src/main/python/apache/aurora/executor/thermos_task_runner.py > 3896e3
Re: Review Request 48082: AURORA-1624 Make 'tier' required and remove support for 'production' flag in Job configuration - New thrift API for retrieving tier configuration
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48082/#review136092
---
Looks good to me in general. Mostly nits below...
src/main/java/org/apache/aurora/scheduler/TierManager.java (lines 46 - 48)
<https://reviews.apache.org/r/48082/#comment201085>
Add javadoc for both of these.
src/main/java/org/apache/aurora/scheduler/TierManager.java (lines 60 - 61)
<https://reviews.apache.org/r/48082/#comment201080>
Style nit, our continuation indent style is:
public TierConfig(
@JsonProperty("...") String defaultTier,
...) {
// first line of method body here, note blank line above.
src/main/java/org/apache/aurora/scheduler/TierManager.java (line 62)
<https://reviews.apache.org/r/48082/#comment201084>
nit: s/Default/Default tier
src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java
(lines 381 - 385)
<https://reviews.apache.org/r/48082/#comment201089>
`Tier` should be generated with a constructor that takes all of these args?
Can we just do...
TierInfo tierInfo = entry.getValue();
return new Tier(entry.getKey(), tierInfo.isPreemptible(),
tierInfo.isRevocable());
src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java
(lines 391 - 392)
<https://reviews.apache.org/r/48082/#comment201087>
Should fit on one line?
- Joshua Cohen
On May 31, 2016, 7:02 p.m., Mehrdad Nurolahzade wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48082/
> ---
>
> (Updated May 31, 2016, 7:02 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> ---
>
> AURORA-1624 Make 'tier' required and remove support for 'production' flag in
> Job configuration - New thrift API for retrieving tier configuration
>
>
> Diffs
> -
>
> api/src/main/thrift/org/apache/aurora/gen/api.thrift
> ed94f249d85ac0e438213924c777cf7c029a119a
> src/main/java/org/apache/aurora/scheduler/TierManager.java
> b96189be0ada1623665c2bff2550c1d72d7bc3dd
> src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java
> e431b58c933886f46c095240704d3eb0ceea2d80
> src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java
> 0d4f04403ec20c21b7cfacd706557cd191579f0a
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
> 4d032b96d55dd0b92fab338220e66628e38cbb11
> src/main/resources/org/apache/aurora/scheduler/tiers.json
> f724c5ad03a1315bc55dac35d98fdef45625e017
> src/test/java/org/apache/aurora/scheduler/TierManagerTest.java
> 95174bb7454a9e7c075ebaa6a4f84bf55fbc2652
> src/test/java/org/apache/aurora/scheduler/TierModuleTest.java
> 6b4e7a0d5e64d0632c66273ad516f737b2ef4a92
>
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
> 2122f744089adaee24328a634a2f786d1ef9720f
>
> Diff: https://reviews.apache.org/r/48082/diff/
>
>
> Testing
> ---
>
> Manual/Explorative: Invoked from CLI-side to see if the new API call works as
> intended
> Integration: ./build-support/jenkins/build.sh
> E2E: ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Mehrdad Nurolahzade
>
>
Re: Review Request 48216: Change the UI to refer to Mesos Agents instead of Mesos Slaves.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48216/#review136096 --- Ship it! Ship It! - Joshua Cohen On June 3, 2016, 6:30 p.m., Stephan Erb wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48216/ > --- > > (Updated June 3, 2016, 6:30 p.m.) > > > Review request for Aurora and Joshua Cohen. > > > Bugs: AURORA-1449 and AURORA-1450 > https://issues.apache.org/jira/browse/AURORA-1449 > https://issues.apache.org/jira/browse/AURORA-1450 > > > Repository: aurora > > > Description > --- > > Change the UI to refer to Mesos Agents instead of Mesos Slaves. > > This change also moves the /slaves endpoint to /agents and adds a > compatibility redirect. > > > Diffs > - > > RELEASE-NOTES.md 21a141f1acadfdaa94247b75de2f734b11868137 > config/legacy_untested_classes.txt 10e8b77a8a66372a6ec514c2dac2774c77eecd52 > src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java > 4faf53fac63b3657b9cb3ce805ae390972283a50 > src/main/java/org/apache/aurora/scheduler/http/Slaves.java > f63fb7be35e2d538d34f6c4a88fb191916d7ab6e > > src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java > 93bc672cdf4151f2dc5bd63fbf4a8ce708ed5f42 > src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java > 85ca0ea723095fedf522e90d55e46e9de156f602 > src/main/java/org/apache/aurora/scheduler/scheduling/SchedulingModule.java > 577edcbf362493d577e2f12c876f1dbb9387ad79 > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java > 5be39814901f92100176f7f6ea847d5042adb201 > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java > 1ddc11732fce22fac2b068e0846a29ee2646719b > src/main/python/apache/aurora/client/base.py > 901335b0934fbde9769e2e93c06b27de428dd302 > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py > b2cc78d8ed6782928eb85e56c51b503c85e1796e > src/main/resources/org/apache/aurora/scheduler/http/slaves.st > 009e5af5ebecde8875f2651f8fe33ef4bc79457a > src/main/resources/org/apache/aurora/scheduler/http/utilization.st > ae497a7c2b5c76a74e5b7ed07b98286a2542aa12 > src/main/resources/scheduler/assets/index.html > 5679b19b18d6dcb480682be72309f922636868b0 > src/main/resources/scheduler/assets/js/filters.js > ec35d81537b44b7bf76cf4cf134b96d44904fee1 > > Diff: https://reviews.apache.org/r/48216/diff/ > > > Testing > --- > > * ran ./build-support/jenkins/build.sh > * checked in the Vagrant box that /slaves and /agents are both working. > > > Thanks, > > Stephan Erb > >
Re: Review Request 48218: Document the logfile locations used in the vagrant box
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48218/#review136098 --- Ship it! docs/getting-started/vagrant.md (lines 153 - 154) <https://reviews.apache.org/r/48218/#comment201106> Might be worth mentioning the other log levels for mesos? Problems are often found in the WARN logs, right? - Joshua Cohen On June 3, 2016, 7:20 p.m., Stephan Erb wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48218/ > --- > > (Updated June 3, 2016, 7:20 p.m.) > > > Review request for Aurora and Joshua Cohen. > > > Repository: aurora > > > Description > --- > > Document the logfile locations used in the vagrant box > > > Diffs > - > > docs/getting-started/vagrant.md 44606009759a68cb142be4f2c7061ad8d0a39066 > > Diff: https://reviews.apache.org/r/48218/diff/ > > > Testing > --- > > > Thanks, > > Stephan Erb > >
Re: Review Request 48082: AURORA-1624 Make 'tier' required and remove support for 'production' flag in Job configuration - New thrift API for retrieving tier configuration
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48082/#review136354 --- Ship it! lgtm, just a couple of style nits. src/main/java/org/apache/aurora/scheduler/TierManager.java (line 51) <https://reviews.apache.org/r/48082/#comment201373> How do you feel about renaming to `getDefaultTierName`? src/main/java/org/apache/aurora/scheduler/TierManager.java (lines 75 - 76) <https://reviews.apache.org/r/48082/#comment201369> checkArgument( tiers.containsKey(defaultTier), "Default tier..."); src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java (line 382) <https://reviews.apache.org/r/48082/#comment201370> move `entry.getKey()` to the next line. src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java (lines 384 - 385) <https://reviews.apache.org/r/48082/#comment201371> extract `entry.getValue()` to a var. - Joshua Cohen On June 6, 2016, 7:13 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48082/ > --- > > (Updated June 6, 2016, 7:13 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > AURORA-1624 Make 'tier' required and remove support for 'production' flag in > Job configuration - New thrift API for retrieving tier configuration > > > Diffs > - > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > ed94f249d85ac0e438213924c777cf7c029a119a > src/main/java/org/apache/aurora/scheduler/TierManager.java > b96189be0ada1623665c2bff2550c1d72d7bc3dd > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > e431b58c933886f46c095240704d3eb0ceea2d80 > src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java > 0d4f04403ec20c21b7cfacd706557cd191579f0a > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > 4d032b96d55dd0b92fab338220e66628e38cbb11 > src/main/resources/org/apache/aurora/scheduler/tiers.json > f724c5ad03a1315bc55dac35d98fdef45625e017 > src/test/java/org/apache/aurora/scheduler/TierManagerTest.java > 95174bb7454a9e7c075ebaa6a4f84bf55fbc2652 > src/test/java/org/apache/aurora/scheduler/TierModuleTest.java > 6b4e7a0d5e64d0632c66273ad516f737b2ef4a92 > > src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java > 2122f744089adaee24328a634a2f786d1ef9720f > > Diff: https://reviews.apache.org/r/48082/diff/ > > > Testing > --- > > Manual/Explorative: Invoked from CLI-side to see if the new API call works as > intended > Integration: ./build-support/jenkins/build.sh > E2E: ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 47998: Converting resource counters to use new resource fields
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47998/#review136514
---
Ship it!
Feel free to ignore both comments below ;).
src/main/java/org/apache/aurora/scheduler/http/Utilization.java (lines 126 -
136)
<https://reviews.apache.org/r/47998/#comment201577>
Feel free to punt on this, but maybe extract a private method that takes
`ResourceType` since the logic is identical for all of these?
private log getResourceValue(ResourceType type) {
return getBag().valueOf(type).longValue();
}
public long getCpu() {
return getResourceValue(ResourceType.CPU);
}
...
src/main/java/org/apache/aurora/scheduler/stats/ResourceCounter.java (line 69)
<https://reviews.apache.org/r/47998/#comment201576>
s/global//
- Joshua Cohen
On June 6, 2016, 11:22 p.m., Maxim Khutornenko wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47998/
> ---
>
> (Updated June 6, 2016, 11:22 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> ---
>
> The side-effect of this refactoring is converting from GB to MB in RAM/Disk
> metrics. This is consistent with the rest of the system and does not require
> an additional complexity of some sort of a unit converter in `ResourceType`.
> All scheduler consumed resources stats will change as:
> _cpu -> _cpu_cores
> _ram_gb -> _ram_mb
> _disk_gb -> _disk_gb
>
>
> Diffs
> -
>
> RELEASE-NOTES.md 96bc3ca4bfd85c3e307f186ab24252c297ba336c
> src/main/java/org/apache/aurora/scheduler/http/Utilization.java
> 4a609e368a451f3f3b0b0fde9770aed213325f0d
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
> 612525c32654c60962a0319a66905c325a0d5a1c
> src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java
> bfe75839785da6f763cf72a7341c1dedec0b2745
> src/main/java/org/apache/aurora/scheduler/stats/ResourceCounter.java
> 5231e9f547004534c027289111544e707a64e6ec
> src/main/java/org/apache/aurora/scheduler/stats/TaskStatCalculator.java
> 2511a39fecadc775aee574515f52ac49ba137855
> src/main/resources/org/apache/aurora/scheduler/http/utilization.st
> acb8f46a3e830f85338b8cb44e7ad046afce5786
> src/test/java/org/apache/aurora/scheduler/resources/ResourceTypeTest.java
> 7ba5567910c46127dab546ad3ad3bb70f562ec2b
> src/test/java/org/apache/aurora/scheduler/stats/ResourceCounterTest.java
> fd4b434aef9d18c94eac51c6ea58512df1f07346
>
> Diff: https://reviews.apache.org/r/47998/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 48360: Updated documentation to include custom executor explaination
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48360/#review136552 --- Thanks for adding this, great to get it documented! Would you mind cleaning up the inline double spaces and the trailing whitespace that reviewboard is showing (the red in the middle/at the end of some lines). docs/operations/configuration.md (lines 147 - 148) <https://reviews.apache.org/r/48360/#comment201628> "the scheduler can be configured to utilize a custom executor by specifying the `-custom_executor_config` flag. docs/operations/configuration.md (lines 151 - 152) <https://reviews.apache.org/r/48360/#comment201629> "The configuration file must be valid JSON and contain, at minimum, the name, command and resources fields." docs/operations/configuration.md (lines 154 - 162) <https://reviews.apache.org/r/48360/#comment201633> Rather than explaining this in conversational English, it might be easier to understand if we just have a table that lists the allowed attributes for each property in the config file? Something like... ### command **Property** | **Description** --- | - **value** (required) | The command to execute. **arguments** (optional) | A list of arguments to pass to the command. **uris** (optional) | Resources to download into the task sandbox. ### volume_mounts ... What do you think? docs/operations/configuration.md (line 217) <https://reviews.apache.org/r/48360/#comment201625> s/nformation/information docs/operations/configuration.md (line 218) <https://reviews.apache.org/r/48360/#comment201626> move this up to the previous line? - Joshua Cohen On June 7, 2016, 9 p.m., Renan DelValle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48360/ > --- > > (Updated June 7, 2016, 9 p.m.) > > > Review request for Aurora, Joshua Cohen and Bill Farner. > > > Repository: aurora > > > Description > --- > > Adding documentation for using a custom executor configuration. > > > Diffs > - > > docs/operations/configuration.md 65cf64a713db87996b92bf8305d1fc565163f106 > > Diff: https://reviews.apache.org/r/48360/diff/ > > > Testing > --- > > > Thanks, > > Renan DelValle > >
Re: Review Request 48360: Updated documentation to include custom executor explaination
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48360/#review136647
---
This looks great. Just a few language tweaks and it's good to go!
docs/operations/configuration.md (line 176)
<https://reviews.apache.org/r/48360/#comment201735>
"to the resource"
docs/operations/configuration.md (line 178)
<https://reviews.apache.org/r/48360/#comment201736>
s/onto/into the
docs/operations/configuration.md (line 179)
<https://reviews.apache.org/r/48360/#comment201737>
s/resource/resources
docs/operations/configuration.md (lines 193 - 195)
<https://reviews.apache.org/r/48360/#comment201738>
Can you reorder these so it's: host_path, container_path, mode? That'll
make reading the descriptions in order easier since the reader won't have to
refer to later properties to understand the one they're reading.
Also, re: mode, provide example values (either `RW` or `RO`)?
docs/operations/configuration.md (line 194)
<https://reviews.apache.org/r/48360/#comment201739>
s/container/the container
Also change host_path to ```host_path```
docs/operations/configuration.md (line 195)
<https://reviews.apache.org/r/48360/#comment201740>
s/which//
s/container/the container
docs/operations/configuration.md (line 200)
<https://reviews.apache.org/r/48360/#comment201734>
Can you surround this block with ``` so it's rendered as code?
- Joshua Cohen
On June 7, 2016, 11:26 p.m., Renan DelValle wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48360/
> -------
>
> (Updated June 7, 2016, 11:26 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> ---
>
> Adding documentation for using a custom executor configuration.
>
>
> Diffs
> -
>
> docs/operations/configuration.md 65cf64a713db87996b92bf8305d1fc565163f106
>
> Diff: https://reviews.apache.org/r/48360/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Renan DelValle
>
>
Re: Review Request 48360: Updated documentation to include custom executor explaination
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48360/#review136665 --- Ship it! Ship It! - Joshua Cohen On June 8, 2016, 5:29 p.m., Renan DelValle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48360/ > --- > > (Updated June 8, 2016, 5:29 p.m.) > > > Review request for Aurora, Joshua Cohen and Bill Farner. > > > Repository: aurora > > > Description > --- > > Adding documentation for using a custom executor configuration. > > > Diffs > - > > docs/operations/configuration.md 65cf64a713db87996b92bf8305d1fc565163f106 > > Diff: https://reviews.apache.org/r/48360/diff/ > > > Testing > --- > > > Thanks, > > Renan DelValle > >
Review Request 48492: Don't require user when initializing a Docker sandbox.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48492/ --- Review request for Aurora, Maxim Khutornenko and Dmitriy Shirchenko. Bugs: AURORA-1709 https://issues.apache.org/jira/browse/AURORA-1709 Repository: aurora Description --- Don't require user when initializing a Docker sandbox. Diffs - src/main/python/apache/aurora/executor/common/sandbox.py be1deba6219462c9fdaaf07a583851b85fe974bf Diff: https://reviews.apache.org/r/48492/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests. Thanks, Joshua Cohen
Re: Review Request 48492: Don't require user when initializing a Docker sandbox.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48492/ --- (Updated June 9, 2016, 3:38 p.m.) Review request for Aurora, Maxim Khutornenko and Dmitriy Shirchenko. Changes --- Add tests. Bugs: AURORA-1709 https://issues.apache.org/jira/browse/AURORA-1709 Repository: aurora Description --- Don't require user when initializing a Docker sandbox. Diffs (updated) - src/main/python/apache/aurora/executor/common/sandbox.py be1deba6219462c9fdaaf07a583851b85fe974bf src/test/python/apache/aurora/executor/common/test_sandbox.py e47d9b8822deb36cb9cfa0554ef89d6cda80f3e9 Diff: https://reviews.apache.org/r/48492/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests. Thanks, Joshua Cohen
Re: Review Request 48492: Don't require user when initializing a Docker sandbox.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48492/ --- (Updated June 9, 2016, 3:55 p.m.) Review request for Aurora, Maxim Khutornenko and Dmitriy Shirchenko. Changes --- isort. Bugs: AURORA-1709 https://issues.apache.org/jira/browse/AURORA-1709 Repository: aurora Description --- Don't require user when initializing a Docker sandbox. Diffs (updated) - src/main/python/apache/aurora/executor/common/sandbox.py be1deba6219462c9fdaaf07a583851b85fe974bf src/test/python/apache/aurora/executor/common/test_sandbox.py e47d9b8822deb36cb9cfa0554ef89d6cda80f3e9 Diff: https://reviews.apache.org/r/48492/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests. Thanks, Joshua Cohen
Re: Review Request 48492: Don't require user when initializing a Docker sandbox.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48492/#review136824 --- @ReviewBot retry - Joshua Cohen On June 9, 2016, 3:55 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48492/ > --- > > (Updated June 9, 2016, 3:55 p.m.) > > > Review request for Aurora, Maxim Khutornenko and Dmitriy Shirchenko. > > > Bugs: AURORA-1709 > https://issues.apache.org/jira/browse/AURORA-1709 > > > Repository: aurora > > > Description > --- > > Don't require user when initializing a Docker sandbox. > > > Diffs > - > > src/main/python/apache/aurora/executor/common/sandbox.py > be1deba6219462c9fdaaf07a583851b85fe974bf > src/test/python/apache/aurora/executor/common/test_sandbox.py > e47d9b8822deb36cb9cfa0554ef89d6cda80f3e9 > > Diff: https://reviews.apache.org/r/48492/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > e2e tests. > > > Thanks, > > Joshua Cohen > >
Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49048/#review139407 --- Just some style stuff. src/main/python/apache/aurora/client/cli/context.py (lines 136 - 137) <https://reviews.apache.org/r/49048/#comment204630> continuation indent here should be... next( (t.name for t in ...), tier_configurations.defaultTierName) src/main/python/apache/aurora/client/cli/context.py (lines 141 - 143) <https://reviews.apache.org/r/49048/#comment204631> same here src/test/python/apache/aurora/client/cli/test_context.py (lines 90 - 95) <https://reviews.apache.org/r/49048/#comment204632> fix indent here as well. src/test/python/apache/aurora/client/cli/test_context.py (line 98) <https://reviews.apache.org/r/49048/#comment204634> The names of these tests are a little unclear, it took some time to parse what each one was testing. Do you think it would make sense to rename to make their asserts more explict. E.g. `test_get_config_production_and_tier_unset_is_preemptible` and `test_get_config_production_set_is_preferred`, etc.? - Joshua Cohen On June 23, 2016, 10:35 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49048/ > --- > > (Updated June 23, 2016, 10:35 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > AURORA-1710 Make 'tier' required and remove support for 'production' flag in > Job configuration - CLI changes > > > Diffs > - > > src/main/python/apache/aurora/client/api/__init__.py > 68baf8fdb90cd26100159401c46c9963c24332b3 > src/main/python/apache/aurora/client/cli/context.py > 9b1511801d031ff48b81c25688a55cb586b8ac66 > src/main/python/apache/aurora/client/config.py > 2fc12559016d406c347adb416a5166cca31c961e > src/main/python/apache/aurora/config/__init__.py > 65923be1cb8b88139b8eab0ac5b75428972d3cb1 > src/test/python/apache/aurora/client/cli/test_command_hooks.py > 2130f1fa71be02a004cdf8e476a270c81a7105d3 > src/test/python/apache/aurora/client/cli/test_context.py > 204ca092adad8bf43c5032a02f61bf303fb0b2fc > src/test/python/apache/aurora/client/cli/test_create.py > 8c27e2b340bb0a5fb5bcb44ef94d433e7f92c76c > src/test/python/apache/aurora/client/cli/test_cron.py > f3c522ed94a2d774865811ceb546bf9df083c14f > src/test/python/apache/aurora/client/cli/test_plugins.py > a545fece5e2b3e0017a61e1be9ac478372b1f34d > src/test/python/apache/aurora/client/cli/test_restart.py > 967d560e5c7eb0ed85b215fb11d9751b8666acb5 > src/test/python/apache/aurora/client/cli/util.py > 7b4558ec7f0fb0fd2902591bc6a90dc15051dd6e > src/test/python/apache/aurora/client/test_config.py > b1a3c1865819899ef19173be0f861783a2631d0a > > Diff: https://reviews.apache.org/r/49048/diff/ > > > Testing > --- > > ``` > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > *** OK (All tests passed) *** > > mesos-master start/running, process 26868 > + RETCODE=0 > + restore_netrc > + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc > + true > Connection to 127.0.0.1 closed. > > real 19m46.324s > user 0m1.496s > sys 0m0.774s > ``` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 49218: Add support for Mesos Fetcher
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49218/#review139426 --- I'm not super familiar with the Mesos resource fetcher, but I'm assuming Mesos does not apply any access control on the uris grabbed by the fetcher (based on the fact that we already use this to grab the thermos executor from whatever path is configured via the scheduler command line)? Am I missing something, or is this potentially a privilege escalation to just blindly allow user tasks to grab arbitrary URIs into their sandbox? Is there any way to control this? I think at the very least we should wire this functionality off by default via a command line flag, rejecting any tasks that request uris in this fashion if it's not explicitly enabled. src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java (line 150) <https://reviews.apache.org/r/49218/#comment204657> Fix this copy/paste. src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml (line 196) <https://reviews.apache.org/r/49218/#comment204658> This should be `u_id` not `m_id`. src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml (lines 408 - 411) <https://reviews.apache.org/r/49218/#comment204659> Indent 2 - Joshua Cohen On June 24, 2016, 11:01 p.m., Renan DelValle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49218/ > --- > > (Updated June 24, 2016, 11:01 p.m.) > > > Review request for Aurora. > > > Repository: aurora > > > Description > --- > > Adding a URIs field to TaskConfig inside the ThriftAPI so that users are able > to specify resources they wish to download into the sandbox per job. > > > Diffs > - > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > 3e6daf53dd563dd7a2d494cc95e9a0aba0b6 > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > 4089b79da8079243703eead884e80bcf736f8b29 > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > 3b01801d929dd61ee989495bf38af8f03e9f5ad4 > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java > c76164292cf62d2181374c09f8bf6d8d3358e982 > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java > 571201094c1e576e496495a01cb83f6c57decfa8 > > src/main/java/org/apache/aurora/scheduler/storage/db/migration/V007_CreateURIsTable.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java > a90cb00e240df25dce6d55728859768e22d741a6 > > src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml > 2c8af8b88e41b3b381cac831fd43b1057e4df0aa > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql > 5069bedc08bb7111d0e0f101c8a2c81495b97bc9 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > 58785bfa37ff214f26e9f94d836e6df40e411c3b > src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java > a883b0e33bfec1d14e6fe4ee8ed2200d93acaeec > > Diff: https://reviews.apache.org/r/49218/diff/ > > > Testing > --- > > ./gradlew build -Pq > ./build-support/jenkins/build.sh > bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > > Thanks, > > Renan DelValle > >
Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49048/#review139923 --- Ship it! - Joshua Cohen On June 28, 2016, 11:57 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49048/ > --- > > (Updated June 28, 2016, 11:57 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Bugs: AURORA-1710 > https://issues.apache.org/jira/browse/AURORA-1710 > > > Repository: aurora > > > Description > --- > > AURORA-1710 Make 'tier' required and remove support for 'production' flag in > Job configuration - CLI changes > > > Diffs > - > > RELEASE-NOTES.md af2061c7605c12a066778bd99ec1a3857bee6dec > src/main/python/apache/aurora/client/api/__init__.py > 68baf8fdb90cd26100159401c46c9963c24332b3 > src/main/python/apache/aurora/client/cli/context.py > 9b1511801d031ff48b81c25688a55cb586b8ac66 > src/main/python/apache/aurora/client/config.py > 2fc12559016d406c347adb416a5166cca31c961e > src/test/python/apache/aurora/client/cli/test_command_hooks.py > 2130f1fa71be02a004cdf8e476a270c81a7105d3 > src/test/python/apache/aurora/client/cli/test_context.py > 204ca092adad8bf43c5032a02f61bf303fb0b2fc > src/test/python/apache/aurora/client/cli/test_create.py > 8c27e2b340bb0a5fb5bcb44ef94d433e7f92c76c > src/test/python/apache/aurora/client/cli/test_cron.py > f3c522ed94a2d774865811ceb546bf9df083c14f > src/test/python/apache/aurora/client/cli/test_plugins.py > a545fece5e2b3e0017a61e1be9ac478372b1f34d > src/test/python/apache/aurora/client/cli/test_restart.py > 967d560e5c7eb0ed85b215fb11d9751b8666acb5 > src/test/python/apache/aurora/client/cli/util.py > 7b4558ec7f0fb0fd2902591bc6a90dc15051dd6e > src/test/python/apache/aurora/client/test_config.py > b1a3c1865819899ef19173be0f861783a2631d0a > > Diff: https://reviews.apache.org/r/49048/diff/ > > > Testing > --- > > ``` > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > *** OK (All tests passed) *** > > mesos-master start/running, process 26868 > + RETCODE=0 > + restore_netrc > + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc > + true > Connection to 127.0.0.1 closed. > > real 19m46.324s > user 0m1.496s > sys 0m0.774s > ``` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 49218: Add support for Mesos Fetcher
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49218/#review139926 --- Fix it, then Ship it! lgtm modulo the below. Please fix or drop issues as warranted and then I can commit this. src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 267) <https://reviews.apache.org/r/49218/#comment205262> Is there a mesos ticket tracking this? If so, can you add it here? src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (lines 270 - 271) <https://reviews.apache.org/r/49218/#comment205263> Duplicate calls to `setExecutable`? src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java (line 301) <https://reviews.apache.org/r/49218/#comment205265> It's hard to tell from looking at the review, do we have the inverse of this scenario covered as well (i.e. mesos fetcher is disabled and task config has no mesos fetcher uris set)? If not, would you mind adding coverage for that? - Joshua Cohen On June 28, 2016, 9:39 p.m., Renan DelValle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49218/ > --- > > (Updated June 28, 2016, 9:39 p.m.) > > > Review request for Aurora. > > > Repository: aurora > > > Description > --- > > Adding a URIs field to TaskConfig inside the ThriftAPI so that users are able > to specify resources they wish to download into the sandbox per job. > > > Diffs > - > > RELEASE-NOTES.md af2061c7605c12a066778bd99ec1a3857bee6dec > api/src/main/thrift/org/apache/aurora/gen/api.thrift > 3e6daf53dd563dd7a2d494cc95e9a0aba0b6 > docs/features/mesos-fetcher.md PRE-CREATION > src/main/java/org/apache/aurora/scheduler/app/AppModule.java > 6c7c75ac86458884bc767736caf47fb56fc8 > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > 4089b79da8079243703eead884e80bcf736f8b29 > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > fe18c0f9876a736ea34d4eab92219013cd1e > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > 3b01801d929dd61ee989495bf38af8f03e9f5ad4 > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java > c76164292cf62d2181374c09f8bf6d8d3358e982 > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java > 571201094c1e576e496495a01cb83f6c57decfa8 > > src/main/java/org/apache/aurora/scheduler/storage/db/migration/V007_CreateMesosFetcherURIsTable.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java > a90cb00e240df25dce6d55728859768e22d741a6 > > src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml > 2c8af8b88e41b3b381cac831fd43b1057e4df0aa > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql > 5069bedc08bb7111d0e0f101c8a2c81495b97bc9 > > src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java > 2dff80b5213e98c778d71955517e5f9227d7d0c1 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > 58785bfa37ff214f26e9f94d836e6df40e411c3b > > src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java > b1593f682f48ea66339bc2372de3e4f14e40be32 > src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java > a883b0e33bfec1d14e6fe4ee8ed2200d93acaeec > src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java > ed69996593f5556d80a9229063ef5c7d658e2cfb > > Diff: https://reviews.apache.org/r/49218/diff/ > > > Testing > --- > > ./gradlew build -Pq > bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > Tested with a custom client submitting TaskConfigs which included URIs when > the -enable_mesos_fetcher_for_jobs flag was on as well as when it was off. > > > Thanks, > > Renan DelValle > >
Review Request 49384: Upgrade to Mesos 0.28.2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49384/ --- Review request for Aurora and Maxim Khutornenko. Repository: aurora Description --- Upgrade to Mesos 0.28.2. Release notes: http://mesos.apache.org/blog/mesos-0-28-0-released/, http://mesos.apache.org/blog/mesos-0-28-2-released/ Upgrade instructions here: http://mesos.apache.org/documentation/latest/upgrades/. Maxim: do we need to do anything about http://mesos.apache.org/documentation/latest/upgrades/#0-28-x-resource-precision? Diffs - 3rdparty/python/BUILD 85db569b43a4a121ed6c6b14688e19dd0278e145 RELEASE-NOTES.md 5beda5dcb50ad1d6f50c3deb322587df58bc27d5 Vagrantfile e7e113def27f241fa48a550885eff8faba52375a build-support/packer/build.sh 2a01d77a08959652a1607f1ade2bb8fbe7555762 build.gradle 0d0d94a80613965282724dd82e61b2363d6a5361 src/main/java/org/apache/aurora/scheduler/base/Conversions.java f2ff7abb51101c603df1bd1b21c4920a348743e1 Diff: https://reviews.apache.org/r/49384/diff/ Testing --- e2e ./gradlew build -Pq Thanks, Joshua Cohen
Re: Review Request 49384: Upgrade to Mesos 0.28.2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49384/#review140006 --- @ReviewBot retry - Joshua Cohen On June 29, 2016, 4:33 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49384/ > --- > > (Updated June 29, 2016, 4:33 p.m.) > > > Review request for Aurora and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > Upgrade to Mesos 0.28.2. > > Release notes: http://mesos.apache.org/blog/mesos-0-28-0-released/, > http://mesos.apache.org/blog/mesos-0-28-2-released/ > > Upgrade instructions here: > http://mesos.apache.org/documentation/latest/upgrades/. Maxim: do we need to > do anything about > http://mesos.apache.org/documentation/latest/upgrades/#0-28-x-resource-precision? > > > Diffs > - > > 3rdparty/python/BUILD 85db569b43a4a121ed6c6b14688e19dd0278e145 > RELEASE-NOTES.md 5beda5dcb50ad1d6f50c3deb322587df58bc27d5 > Vagrantfile e7e113def27f241fa48a550885eff8faba52375a > build-support/packer/build.sh 2a01d77a08959652a1607f1ade2bb8fbe7555762 > build.gradle 0d0d94a80613965282724dd82e61b2363d6a5361 > src/main/java/org/apache/aurora/scheduler/base/Conversions.java > f2ff7abb51101c603df1bd1b21c4920a348743e1 > > Diff: https://reviews.apache.org/r/49384/diff/ > > > Testing > --- > > e2e > ./gradlew build -Pq > > > Thanks, > > Joshua Cohen > >
Re: Review Request 49384: Upgrade to Mesos 0.28.2.
> On June 29, 2016, 4:44 p.m., Maxim Khutornenko wrote: > > build.gradle, line 368 > > <https://reviews.apache.org/r/49384/diff/1/?file=1433125#file1433125line368> > > > > Should this be 0.28.3 instead? Looking at the changelog I don't see any > > relevant changes for us but in case it's easy to bump the version up it > > would be great to have the most recent release linked to 0.15.0. > > > > https://github.com/apache/mesos/blob/master/CHANGELOG#L571? As far as I can tell mesos 0.28.3 has not yet been released. Am I missing something? Latest release [here](http://mesos.apache.org/downloads/) is 0.28.2. Same [here](https://mesosphere.com/downloads/) for the Mesosphere rpms? More than happy to bump up to 0.28.3 when it does actually get released, but it doesn't seem to be available yet. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49384/#review140007 ------- On June 29, 2016, 4:33 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49384/ > --- > > (Updated June 29, 2016, 4:33 p.m.) > > > Review request for Aurora and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > Upgrade to Mesos 0.28.2. > > Release notes: http://mesos.apache.org/blog/mesos-0-28-0-released/, > http://mesos.apache.org/blog/mesos-0-28-2-released/ > > Upgrade instructions here: > http://mesos.apache.org/documentation/latest/upgrades/. Maxim: do we need to > do anything about > http://mesos.apache.org/documentation/latest/upgrades/#0-28-x-resource-precision? > > > Diffs > - > > 3rdparty/python/BUILD 85db569b43a4a121ed6c6b14688e19dd0278e145 > RELEASE-NOTES.md 5beda5dcb50ad1d6f50c3deb322587df58bc27d5 > Vagrantfile e7e113def27f241fa48a550885eff8faba52375a > build-support/packer/build.sh 2a01d77a08959652a1607f1ade2bb8fbe7555762 > build.gradle 0d0d94a80613965282724dd82e61b2363d6a5361 > src/main/java/org/apache/aurora/scheduler/base/Conversions.java > f2ff7abb51101c603df1bd1b21c4920a348743e1 > > Diff: https://reviews.apache.org/r/49384/diff/ > > > Testing > --- > > e2e > ./gradlew build -Pq > > > Thanks, > > Joshua Cohen > >
Re: Review Request 49384: Upgrade to Mesos 0.28.2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49384/#review140012 --- @ReviewBot retry - Joshua Cohen On June 29, 2016, 4:33 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49384/ > --- > > (Updated June 29, 2016, 4:33 p.m.) > > > Review request for Aurora and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > Upgrade to Mesos 0.28.2. > > Release notes: http://mesos.apache.org/blog/mesos-0-28-0-released/, > http://mesos.apache.org/blog/mesos-0-28-2-released/ > > Upgrade instructions here: > http://mesos.apache.org/documentation/latest/upgrades/. Maxim: do we need to > do anything about > http://mesos.apache.org/documentation/latest/upgrades/#0-28-x-resource-precision? > > > Diffs > - > > 3rdparty/python/BUILD 85db569b43a4a121ed6c6b14688e19dd0278e145 > RELEASE-NOTES.md 5beda5dcb50ad1d6f50c3deb322587df58bc27d5 > Vagrantfile e7e113def27f241fa48a550885eff8faba52375a > build-support/packer/build.sh 2a01d77a08959652a1607f1ade2bb8fbe7555762 > build.gradle 0d0d94a80613965282724dd82e61b2363d6a5361 > src/main/java/org/apache/aurora/scheduler/base/Conversions.java > f2ff7abb51101c603df1bd1b21c4920a348743e1 > > Diff: https://reviews.apache.org/r/49384/diff/ > > > Testing > --- > > e2e > ./gradlew build -Pq > > > Thanks, > > Joshua Cohen > >
Re: Review Request 49384: Upgrade to Mesos 0.28.2.
> On June 29, 2016, 5:33 p.m., Maxim Khutornenko wrote: > > @ReviewBot retry I just ran the jenkins script locally and it passed ok, so I'm going to merge this. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49384/#review140022 --- On June 29, 2016, 4:33 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49384/ > --- > > (Updated June 29, 2016, 4:33 p.m.) > > > Review request for Aurora and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > Upgrade to Mesos 0.28.2. > > Release notes: http://mesos.apache.org/blog/mesos-0-28-0-released/, > http://mesos.apache.org/blog/mesos-0-28-2-released/ > > Upgrade instructions here: > http://mesos.apache.org/documentation/latest/upgrades/. Maxim: do we need to > do anything about > http://mesos.apache.org/documentation/latest/upgrades/#0-28-x-resource-precision? > > > Diffs > - > > 3rdparty/python/BUILD 85db569b43a4a121ed6c6b14688e19dd0278e145 > RELEASE-NOTES.md 5beda5dcb50ad1d6f50c3deb322587df58bc27d5 > Vagrantfile e7e113def27f241fa48a550885eff8faba52375a > build-support/packer/build.sh 2a01d77a08959652a1607f1ade2bb8fbe7555762 > build.gradle 0d0d94a80613965282724dd82e61b2363d6a5361 > src/main/java/org/apache/aurora/scheduler/base/Conversions.java > f2ff7abb51101c603df1bd1b21c4920a348743e1 > > Diff: https://reviews.apache.org/r/49384/diff/ > > > Testing > --- > > e2e > ./gradlew build -Pq > > > Thanks, > > Joshua Cohen > >
Re: Review Request 49478: Fixing e2e tests failing due to mesos-slave state.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49478/#review140289 --- Ship it! Ship It! - Joshua Cohen On June 30, 2016, 11:29 p.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49478/ > --- > > (Updated June 30, 2016, 11:29 p.m.) > > > Review request for Aurora and Joshua Cohen. > > > Repository: aurora > > > Description > --- > > Removing slave.info that is written any time a vagrant box restarts. This > includes the base image building as well as when the vagrant box is created > before the provisioning scripts are applied. > > Here is the relevant change on mesos side that causes it: > https://github.com/mesosphere/mesos-deb-packaging/commit/e05426acdedfec905b3895c5350e36c6a6590e42 > > > Diffs > - > > build-support/packer/README.md b3570adbe8018952d8a9fcec9911269f5639136e > examples/vagrant/provision-dev-cluster.sh > b1f661ea90ac427621518f653cce1b7630d2a6ed > > Diff: https://reviews.apache.org/r/49478/diff/ > > > Testing > --- > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 49732: Update package scripts to 0.15.0.
> On July 7, 2016, 2:33 a.m., John Sirois wrote: > > I am slightly worried the tests only seem to work after this change ... > > IIUC Aurora 0.15.0 should work with mesos 0.27.2 (Aurora should work with > > the mesos its developed against +/- 1). > > John Sirois wrote: > Oh, now I understand I think. You used this patch to do the 0.15.0 > release, which is in error. The 0.15.0 release should have a >= 0.27.2 mesos > constraint. > > If that makes sense, un-shipit. > > Stephan Erb wrote: > That observation regarding specifying >=0.27 even though we have build > against 0.28 probably correct. > > FWIW, our previous packages have been inconsistent in that regard as well: > > * 0.12 is build against Mesos 0.25 but specifies >=0.21.1 for Debian and > ==0.25 for Centos > * 0.13 is build against Mesos 0.26 but specifies >=0.26 > * 0.14 is build against Mesos 0.27.2 but specifies >=0.27.2 > > Maxim Khutornenko wrote: > > Aurora should work with the mesos its developed against > > That may or may not be true. It all depends on what we change in Aurora > to bump up the Mesos version. In 0.15.0, we upgraded Mesos to 0.28.2 that > required us to treat the new Mesos task state (KILLING). This means 0.15.0 > cannot rely on 0.27.2 anymore and >= 0.28.2 seems the only logical constraint > there. Yeah, unfortunately there's no great way to upgrade to 0.28.2 without explicitly depending on the new `TaskState.KILLING` value. We either had to make the change I made (add explicit support for the state to `Conversions` or we would have had to update `ConversionsTest` to explicitly ignore that task state, either way it would require 0.28.2. I suppose we *could* ignore it in the test by checking for `"KILLING".equals(state.getName())`. That seems a bit hacky but it would allow us to upgrade to 0.28.2 while letting 0.15.0 still run against 0.27.x. Is that preferable, or are we ok with bending the rules in this instance? - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49732/#review141120 --- On July 7, 2016, 12:30 a.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49732/ > --- > > (Updated July 7, 2016, 12:30 a.m.) > > > Review request for Aurora and Stephan Erb. > > > Repository: aurora-packaging > > > Description > --- > > Update package scripts to 0.15.0. > > > Diffs > - > > README.md 3a7cf45034b7896c23588fed83176468ca627ebc > specs/debian/control e4d23ba6190ebbd3afbb930e305d76f5b61a5dac > specs/rpm/aurora.spec 7a368fd8128153a3167032727fe011a8a7457853 > test/deb/debian-jessie/README.md a98407b96c673eb6ca6269d646755926d51fd4ab > test/deb/debian-jessie/provision.sh > ed4364f69a63493d7a1fd6791ef743609c99b924 > test/deb/ubuntu-trusty/README.md c5579857443c73d8343a82210e34be98ad3a86da > test/deb/ubuntu-trusty/provision.sh > 5b4ec472a23d5d401a64a6a72743c392d048f949 > test/rpm/centos-7/README.md e5649015934e4714a054a5be7a83f9c333b70144 > test/rpm/centos-7/provision.sh 5aa88a5d86a990131cdbdae5d93aeb75a1dc7c90 > > Diff: https://reviews.apache.org/r/49732/diff/ > > > Testing > --- > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 49732: Update package scripts to 0.15.0.
> On July 7, 2016, 2:33 a.m., John Sirois wrote: > > I am slightly worried the tests only seem to work after this change ... > > IIUC Aurora 0.15.0 should work with mesos 0.27.2 (Aurora should work with > > the mesos its developed against +/- 1). > > John Sirois wrote: > Oh, now I understand I think. You used this patch to do the 0.15.0 > release, which is in error. The 0.15.0 release should have a >= 0.27.2 mesos > constraint. > > If that makes sense, un-shipit. > > Stephan Erb wrote: > That observation regarding specifying >=0.27 even though we have build > against 0.28 probably correct. > > FWIW, our previous packages have been inconsistent in that regard as well: > > * 0.12 is build against Mesos 0.25 but specifies >=0.21.1 for Debian and > ==0.25 for Centos > * 0.13 is build against Mesos 0.26 but specifies >=0.26 > * 0.14 is build against Mesos 0.27.2 but specifies >=0.27.2 > > Maxim Khutornenko wrote: > > Aurora should work with the mesos its developed against > > That may or may not be true. It all depends on what we change in Aurora > to bump up the Mesos version. In 0.15.0, we upgraded Mesos to 0.28.2 that > required us to treat the new Mesos task state (KILLING). This means 0.15.0 > cannot rely on 0.27.2 anymore and >= 0.28.2 seems the only logical constraint > there. > > Joshua Cohen wrote: > Yeah, unfortunately there's no great way to upgrade to 0.28.2 without > explicitly depending on the new `TaskState.KILLING` value. We either had to > make the change I made (add explicit support for the state to `Conversions` > or we would have had to update `ConversionsTest` to explicitly ignore that > task state, either way it would require 0.28.2. I suppose we *could* ignore > it in the test by checking for `"KILLING".equals(state.getName())`. That > seems a bit hacky but it would allow us to upgrade to 0.28.2 while letting > 0.15.0 still run against 0.27.x. > > Is that preferable, or are we ok with bending the rules in this instance? > > Maxim Khutornenko wrote: > I don't think we can shy away from the fact that Mesos upgrades may and > will require making backwards incompatible Aurora changes as far as Mesos > versions go. The whole purpose of supporting the ">=" syntax is to define the > cluster upgrade as "first, upgrade Mesos then - Aurora". Trying to support -1 > version in addition to +1 will only make our lifes harder and may even be > impossible in the future (e.g.: switching from `launchTasks` to > `acceptOffers`) > > John, given the above, how would like to proceed with this patch? Yeah, we're going to have a similar issue for the 1.0 upgrade in that if we operators opt in to GPU resources we're going to have to send a framework capability that was only recently introduced. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49732/#review141120 --- On July 7, 2016, 12:30 a.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49732/ > --- > > (Updated July 7, 2016, 12:30 a.m.) > > > Review request for Aurora and Stephan Erb. > > > Repository: aurora-packaging > > > Description > --- > > Update package scripts to 0.15.0. > > > Diffs > - > > README.md 3a7cf45034b7896c23588fed83176468ca627ebc > specs/debian/control e4d23ba6190ebbd3afbb930e305d76f5b61a5dac > specs/rpm/aurora.spec 7a368fd8128153a3167032727fe011a8a7457853 > test/deb/debian-jessie/README.md a98407b96c673eb6ca6269d646755926d51fd4ab > test/deb/debian-jessie/provision.sh > ed4364f69a63493d7a1fd6791ef743609c99b924 > test/deb/ubuntu-trusty/README.md c5579857443c73d8343a82210e34be98ad3a86da > test/deb/ubuntu-trusty/provision.sh > 5b4ec472a23d5d401a64a6a72743c392d048f949 > test/rpm/centos-7/README.md e5649015934e4714a054a5be7a83f9c333b70144 > test/rpm/centos-7/provision.sh 5aa88a5d86a990131cdbdae5d93aeb75a1dc7c90 > > Diff: https://reviews.apache.org/r/49732/diff/ > > > Testing > --- > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes
> On June 29, 2016, 3:39 a.m., Joshua Cohen wrote: > > > > Mehrdad Nurolahzade wrote: > Should this be merged with upstream now that 0.15 is released? Yes, we can land this now. Would you mind rebasing? Then I can commit it. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49048/#review139923 --- On June 28, 2016, 11:57 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49048/ > --- > > (Updated June 28, 2016, 11:57 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Bugs: AURORA-1710 > https://issues.apache.org/jira/browse/AURORA-1710 > > > Repository: aurora > > > Description > --- > > AURORA-1710 Make 'tier' required and remove support for 'production' flag in > Job configuration - CLI changes > > > Diffs > - > > RELEASE-NOTES.md af2061c7605c12a066778bd99ec1a3857bee6dec > src/main/python/apache/aurora/client/api/__init__.py > 68baf8fdb90cd26100159401c46c9963c24332b3 > src/main/python/apache/aurora/client/cli/context.py > 9b1511801d031ff48b81c25688a55cb586b8ac66 > src/main/python/apache/aurora/client/config.py > 2fc12559016d406c347adb416a5166cca31c961e > src/test/python/apache/aurora/client/cli/test_command_hooks.py > 2130f1fa71be02a004cdf8e476a270c81a7105d3 > src/test/python/apache/aurora/client/cli/test_context.py > 204ca092adad8bf43c5032a02f61bf303fb0b2fc > src/test/python/apache/aurora/client/cli/test_create.py > 8c27e2b340bb0a5fb5bcb44ef94d433e7f92c76c > src/test/python/apache/aurora/client/cli/test_cron.py > f3c522ed94a2d774865811ceb546bf9df083c14f > src/test/python/apache/aurora/client/cli/test_plugins.py > a545fece5e2b3e0017a61e1be9ac478372b1f34d > src/test/python/apache/aurora/client/cli/test_restart.py > 967d560e5c7eb0ed85b215fb11d9751b8666acb5 > src/test/python/apache/aurora/client/cli/util.py > 7b4558ec7f0fb0fd2902591bc6a90dc15051dd6e > src/test/python/apache/aurora/client/test_config.py > b1a3c1865819899ef19173be0f861783a2631d0a > > Diff: https://reviews.apache.org/r/49048/diff/ > > > Testing > --- > > ``` > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > *** OK (All tests passed) *** > > mesos-master start/running, process 26868 > + RETCODE=0 > + restore_netrc > + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc > + true > Connection to 127.0.0.1 closed. > > real 19m46.324s > user 0m1.496s > sys 0m0.774s > ``` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 49512: [FEEDBACK] Add thermos option to monitor whole docker storage disk usage
This is why I haven't commented on the review as well. I'm close to updating my patch to take advantage of some upstream changes to Mesos that allows us to rely on their logic for isolating a filesystem ( https://reviews.apache.org/r/49569/), hopefully will have something out by EOW. That said, it will depend on Mesos 1.0.0 so realistically won't be immediately usable. On Tue, Jul 12, 2016 at 1:08 PM, Stephan Erb wrote: > > > > On July 4, 2016, 9:55 a.m., Stephan Erb wrote: > > > Have you considered querying Mesos for the disk usage of the task? > That would be in line with our desire to also leave the isolation up to > Mesos https://issues.apache.org/jira/browse/AURORA-1033 > > > > Martin Hrabovcin wrote: > > I wasn't aware of this ticket and I looked at mesos implementation. > I think this would be still useful for aurora/docker users as mesos > currently does only XFS or du (similar to aurora) isolation. Docker > containerizer has no support for storage quota. > > Have you had a chance to look at the universal containerizer in Mesos? > Would it work for your current usecase? Joshua is currently working on > support for the universal containerizer in Mesos. Once this has landed, the > existing `du` isolation would also work for Docker containers. > > I am not against your change in general (it looks good!). I am just trying > to figure out if we would actually be needed. > > > - Stephan > > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49512/#review140619 > --- > > > On July 1, 2016, 4:51 p.m., Martin Hrabovcin wrote: > > > > --- > > This is an automatically generated e-mail. To reply, visit: > > https://reviews.apache.org/r/49512/ > > --- > > > > (Updated July 1, 2016, 4:51 p.m.) > > > > > > Review request for Aurora. > > > > > > Repository: aurora > > > > > > Description > > --- > > > > I'd like to get feedback on this patch. > > > > Thermos currently monitors only sandbox directory but when running with > Docker containerizer process can write data to whole container filesystem. > This patch gives option to monitor whole docker container filesystem. > > > > Behavior is unchanged if new flag isn't provided. > > > > > > Diffs > > - > > > > examples/jobs/docker/hello_docker.aurora > 47adb3932323120e62649f43353719b76c48a963 > > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py > 203fc47d74840889a1192dc867fef5584b704685 > > src/main/python/apache/aurora/executor/common/resource_manager.py > b7dc40d8973ec2e5998ab4f6ff988051a70bb1ab > > src/main/python/apache/thermos/monitoring/disk.py > 52c5d74fd70b5942ea3ef5101ba3f27bfc98fc21 > > src/main/python/apache/thermos/monitoring/resource.py > 53d0ff1a71c27f053c59acca556c35d1e5ac91f0 > > > > src/test/python/apache/aurora/executor/common/test_resource_manager_integration.py > fe74bd1d3ecd89fca1b5b2251202cbbc0f24 > > src/test/python/apache/thermos/monitoring/test_disk.py > e57467c61d94d67e6cf6b766c062588235b4b235 > > > > Diff: https://reviews.apache.org/r/49512/diff/ > > > > > > Testing > > --- > > > > e2e testing > > unit tests > > manual testing in Vagrant dev box and on custom dev cluster with option > enabled. > > > > > > Thanks, > > > > Martin Hrabovcin > > > > > >
Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50052/#review142326 --- How do you feel about adding a unit test for `getOffers` that verifies all this new logic? src/main/java/org/apache/aurora/scheduler/http/Offers.java (line 89) <https://reviews.apache.org/r/50052/#comment207906> Is there any reason that all of these `*_TO_BEAN` functions return an Object rather than `Map` (since that's what the root `TO_BEAN` function returns) - Joshua Cohen On July 14, 2016, 11:08 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50052/ > --- > > (Updated July 14, 2016, 11:08 p.m.) > > > Review request for Aurora, Joshua Cohen and Stephan Erb. > > > Bugs: AURORA-1736 > https://issues.apache.org/jira/browse/AURORA-1736 > > > Repository: aurora > > > Description > --- > > AURORA-1736 Display reservations and persistent volumes in /offers debug http > endpoint > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/http/Offers.java > 80f082410896a50d86c7886736caf79581f5051c > > Diff: https://reviews.apache.org/r/50052/diff/ > > > Testing > --- > > Manual, Jenkins, and end_to_end > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50052/#review142990 --- Ship it! I'm not super bothered by the double labels, so I'm ok with shipping this. If others feel strongly that they're problematic then we can investigate a custom jackson serializer to clean them up. - Joshua Cohen On July 20, 2016, 4:26 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50052/ > --- > > (Updated July 20, 2016, 4:26 p.m.) > > > Review request for Aurora, Joshua Cohen and Stephan Erb. > > > Bugs: AURORA-1736 > https://issues.apache.org/jira/browse/AURORA-1736 > > > Repository: aurora > > > Description > --- > > AURORA-1736 Display reservations and persistent volumes in /offers debug http > endpoint > > > Diffs > - > > RELEASE-NOTES.md 29d224d5596eb060356f1343cf347db79b1ad687 > config/legacy_untested_classes.txt 1ea2183ab20cc5c6bca147bcea4e5c708d576b62 > src/main/java/org/apache/aurora/scheduler/http/Offers.java > 80f082410896a50d86c7886736caf79581f5051c > src/test/java/org/apache/aurora/scheduler/http/OffersTest.java PRE-CREATION > > Diff: https://reviews.apache.org/r/50052/diff/ > > > Testing > --- > > Manual, Jenkins, and end_to_end > > > File Attachments > > > CURRENT > > https://reviews.apache.org/media/uploaded/files/2016/07/18/1de4c357-c932-4c84-962f-4209a5b679bc__offers-old.json > NEW > > https://reviews.apache.org/media/uploaded/files/2016/07/19/799bcd1f-f9c8-4b6e-bbaa-ce8022b1dac1__offers-new.json > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 50168: Add rollback functionality to the scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50168/#review143084 --- src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java (line 136) <https://reviews.apache.org/r/50168/#comment208787> Just inline this into the call in the block below. Even better, any reason not to just take the job update key as an arg rather than the JobUpdate? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java (line 126) <https://reviews.apache.org/r/50168/#comment208788> s/complete/completed. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java (lines 129 - 131) <https://reviews.apache.org/r/50168/#comment208790> What if the update is in `ROLLED_FORWARD` and another update has already run and completed since this one (i.e. there's no update in `ROLLING_FORWARD`, but there is a more recent update in `ROLLED_FORWARD`)? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java (line 131) <https://reviews.apache.org/r/50168/#comment208789> s/no/not src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java (line 251) <https://reviews.apache.org/r/50168/#comment208798> I might be missing something, but I don't see a check in this method that if the update is complete, it's in the `ROLLED_FORWARD` state (as described by the javadoc on the interface declaration). src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java (line 271) <https://reviews.apache.org/r/50168/#comment208791> "No job update found for " + key src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java (lines 279 - 280) <https://reviews.apache.org/r/50168/#comment208792> What is the purpose of this call if it's not using the return value? As far as I can tell there are no side effects from the call. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java (line 282) <https://reviews.apache.org/r/50168/#comment208793> Might be useful to call this out as a rollback update in the log (i.e. "Starting rollback update for job " + job). src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java (line 287) <https://reviews.apache.org/r/50168/#comment208794> indentation is off here, this should be aligned w/ `ILockKey.build` from the previous line. src/main/python/apache/aurora/client/cli/update.py (lines 106 - 111) <https://reviews.apache.org/r/50168/#comment208801> This is not going to work for updates that are complete, as `_modify_update` fails if the current state of the update is not an in-progress state: https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/client/cli/update.py#L61-L76 src/main/python/apache/aurora/client/cli/update.py (line 332) <https://reviews.apache.org/r/50168/#comment208800> Maybe something like "Rollback the latest in-progress or completed update." reads a little bit better? - Joshua Cohen On July 21, 2016, 3:37 a.m., Igor Morozov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50168/ > ------- > > (Updated July 21, 2016, 3:37 a.m.) > > > Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and > Zameer Manji. > > > Bugs: AURORA-1721 > https://issues.apache.org/jira/browse/AURORA-1721 > > > Repository: aurora > > > Description > --- > > Add rollback functionality to the scheduler > > Thic change also includes an addition to storage replicated log. I needed a > way to re-insert lock token for an existing job update. > > > Diffs > - > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > 1d66208490aff6ea8af4c737845fa2cf13617529 > api/src/main/thrift/org/apache/aurora/gen/storage.thrift > 9e4213f13255a182df938bea44ca87fa03a25318 > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java > 52c3c6618a3cf1009435ca8a9cece36365913e55 > src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java > d2673e6b328cb1e249fbe91d18e0d9e935636eaa > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java > 39924c62108f6a68f545f90d77ceab4265d41fad > > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java > d0de063fd78e6c4f62fae4a598d1d22f9775772d > > src/main/java/org/apache/aurora/scheduler/thrift/Sc
Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes
> On July 23, 2016, 5:13 p.m., Stephan Erb wrote: > > LGTM! > > > > Minor missing things: > > > > * deprecation notice for the user-facing configuration option > > * rebase > > > > > > Joshua, would be great if you could land this patch once its done. I am > > gone for the week & without proper internet access. Yep, as soon as these changes are made I'll re-land this patch! - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49048/#review143319 --- On July 20, 2016, 5:56 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49048/ > --- > > (Updated July 20, 2016, 5:56 p.m.) > > > Review request for Aurora, Joshua Cohen and Stephan Erb. > > > Bugs: AURORA-1710 > https://issues.apache.org/jira/browse/AURORA-1710 > > > Repository: aurora > > > Description > --- > > AURORA-1710 Make 'tier' required and remove support for 'production' flag in > Job configuration - CLI changes > > > Diffs > - > > RELEASE-NOTES.md 29d224d5596eb060356f1343cf347db79b1ad687 > src/main/python/apache/aurora/client/api/__init__.py > 68baf8fdb90cd26100159401c46c9963c24332b3 > src/main/python/apache/aurora/client/cli/context.py > 9b1511801d031ff48b81c25688a55cb586b8ac66 > src/main/python/apache/aurora/client/config.py > 2fc12559016d406c347adb416a5166cca31c961e > src/test/python/apache/aurora/client/cli/test_command_hooks.py > 2130f1fa71be02a004cdf8e476a270c81a7105d3 > src/test/python/apache/aurora/client/cli/test_context.py > 204ca092adad8bf43c5032a02f61bf303fb0b2fc > src/test/python/apache/aurora/client/cli/test_create.py > 8c27e2b340bb0a5fb5bcb44ef94d433e7f92c76c > src/test/python/apache/aurora/client/cli/test_cron.py > f3c522ed94a2d774865811ceb546bf9df083c14f > src/test/python/apache/aurora/client/cli/test_plugins.py > a545fece5e2b3e0017a61e1be9ac478372b1f34d > src/test/python/apache/aurora/client/cli/test_restart.py > 967d560e5c7eb0ed85b215fb11d9751b8666acb5 > src/test/python/apache/aurora/client/cli/util.py > 7b4558ec7f0fb0fd2902591bc6a90dc15051dd6e > src/test/python/apache/aurora/client/test_config.py > b1a3c1865819899ef19173be0f861783a2631d0a > > Diff: https://reviews.apache.org/r/49048/diff/ > > > Testing > --- > > ``` > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > *** OK (All tests passed) *** > > mesos-master start/running, process 26868 > + RETCODE=0 > + restore_netrc > + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc > + true > Connection to 127.0.0.1 closed. > > real 19m46.324s > user 0m1.496s > sys 0m0.774s > ``` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49048/#review143387 --- Ship it! Below is not a blocker for me. RELEASE-NOTES.md (line 20) <https://reviews.apache.org/r/49048/#comment209161> Given that tier names are configurable by Aurora operators, should we word this differently? - Joshua Cohen On July 25, 2016, 3:30 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49048/ > --- > > (Updated July 25, 2016, 3:30 p.m.) > > > Review request for Aurora, Joshua Cohen and Stephan Erb. > > > Bugs: AURORA-1710 > https://issues.apache.org/jira/browse/AURORA-1710 > > > Repository: aurora > > > Description > --- > > AURORA-1710 Make 'tier' required and remove support for 'production' flag in > Job configuration - CLI changes > > > Diffs > - > > RELEASE-NOTES.md ca98d7ab802114e025f6021d33bc251704c59088 > src/main/python/apache/aurora/client/api/__init__.py > 68baf8fdb90cd26100159401c46c9963c24332b3 > src/main/python/apache/aurora/client/cli/context.py > 9b1511801d031ff48b81c25688a55cb586b8ac66 > src/main/python/apache/aurora/client/config.py > 2fc12559016d406c347adb416a5166cca31c961e > src/test/python/apache/aurora/client/cli/test_command_hooks.py > 2130f1fa71be02a004cdf8e476a270c81a7105d3 > src/test/python/apache/aurora/client/cli/test_context.py > 204ca092adad8bf43c5032a02f61bf303fb0b2fc > src/test/python/apache/aurora/client/cli/test_create.py > 8c27e2b340bb0a5fb5bcb44ef94d433e7f92c76c > src/test/python/apache/aurora/client/cli/test_cron.py > f3c522ed94a2d774865811ceb546bf9df083c14f > src/test/python/apache/aurora/client/cli/test_plugins.py > a545fece5e2b3e0017a61e1be9ac478372b1f34d > src/test/python/apache/aurora/client/cli/test_restart.py > 967d560e5c7eb0ed85b215fb11d9751b8666acb5 > src/test/python/apache/aurora/client/cli/util.py > 7b4558ec7f0fb0fd2902591bc6a90dc15051dd6e > src/test/python/apache/aurora/client/test_config.py > b1a3c1865819899ef19173be0f861783a2631d0a > > Diff: https://reviews.apache.org/r/49048/diff/ > > > Testing > --- > > ``` > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > *** OK (All tests passed) *** > > mesos-master start/running, process 26868 > + RETCODE=0 > + restore_netrc > + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc > + true > Connection to 127.0.0.1 closed. > > real 19m46.324s > user 0m1.496s > sys 0m0.774s > ``` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49048/#review143388 --- @ReviewBot retry - Joshua Cohen On July 25, 2016, 3:30 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49048/ > --- > > (Updated July 25, 2016, 3:30 p.m.) > > > Review request for Aurora, Joshua Cohen and Stephan Erb. > > > Bugs: AURORA-1710 > https://issues.apache.org/jira/browse/AURORA-1710 > > > Repository: aurora > > > Description > --- > > AURORA-1710 Make 'tier' required and remove support for 'production' flag in > Job configuration - CLI changes > > > Diffs > - > > RELEASE-NOTES.md ca98d7ab802114e025f6021d33bc251704c59088 > src/main/python/apache/aurora/client/api/__init__.py > 68baf8fdb90cd26100159401c46c9963c24332b3 > src/main/python/apache/aurora/client/cli/context.py > 9b1511801d031ff48b81c25688a55cb586b8ac66 > src/main/python/apache/aurora/client/config.py > 2fc12559016d406c347adb416a5166cca31c961e > src/test/python/apache/aurora/client/cli/test_command_hooks.py > 2130f1fa71be02a004cdf8e476a270c81a7105d3 > src/test/python/apache/aurora/client/cli/test_context.py > 204ca092adad8bf43c5032a02f61bf303fb0b2fc > src/test/python/apache/aurora/client/cli/test_create.py > 8c27e2b340bb0a5fb5bcb44ef94d433e7f92c76c > src/test/python/apache/aurora/client/cli/test_cron.py > f3c522ed94a2d774865811ceb546bf9df083c14f > src/test/python/apache/aurora/client/cli/test_plugins.py > a545fece5e2b3e0017a61e1be9ac478372b1f34d > src/test/python/apache/aurora/client/cli/test_restart.py > 967d560e5c7eb0ed85b215fb11d9751b8666acb5 > src/test/python/apache/aurora/client/cli/util.py > 7b4558ec7f0fb0fd2902591bc6a90dc15051dd6e > src/test/python/apache/aurora/client/test_config.py > b1a3c1865819899ef19173be0f861783a2631d0a > > Diff: https://reviews.apache.org/r/49048/diff/ > > > Testing > --- > > ``` > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > *** OK (All tests passed) *** > > mesos-master start/running, process 26868 > + RETCODE=0 > + restore_netrc > + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc > + true > Connection to 127.0.0.1 closed. > > real 19m46.324s > user 0m1.496s > sys 0m0.774s > ``` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes
> On July 25, 2016, 3:55 p.m., Joshua Cohen wrote: > > RELEASE-NOTES.md, line 20 > > <https://reviews.apache.org/r/49048/diff/7-8/?file=1440195#file1440195line20> > > > > Given that tier names are configurable by Aurora operators, should we > > word this differently? > > Mehrdad Nurolahzade wrote: > Better? > > The job configuration flag `production` is now deprecated. To achieve the > same scheduling behavior that `production=true` used to provide, users should > elect a `tier` for the job with attributes `preemptible=false` and > `revocable=false`. For example, the `preferred` tier in the default tier > configuation file (`tiers.json`) matches the above criteria. Yes, A+! - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49048/#review143387 --- On July 25, 2016, 3:30 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49048/ > --- > > (Updated July 25, 2016, 3:30 p.m.) > > > Review request for Aurora, Joshua Cohen and Stephan Erb. > > > Bugs: AURORA-1710 > https://issues.apache.org/jira/browse/AURORA-1710 > > > Repository: aurora > > > Description > --- > > AURORA-1710 Make 'tier' required and remove support for 'production' flag in > Job configuration - CLI changes > > > Diffs > - > > RELEASE-NOTES.md ca98d7ab802114e025f6021d33bc251704c59088 > src/main/python/apache/aurora/client/api/__init__.py > 68baf8fdb90cd26100159401c46c9963c24332b3 > src/main/python/apache/aurora/client/cli/context.py > 9b1511801d031ff48b81c25688a55cb586b8ac66 > src/main/python/apache/aurora/client/config.py > 2fc12559016d406c347adb416a5166cca31c961e > src/test/python/apache/aurora/client/cli/test_command_hooks.py > 2130f1fa71be02a004cdf8e476a270c81a7105d3 > src/test/python/apache/aurora/client/cli/test_context.py > 204ca092adad8bf43c5032a02f61bf303fb0b2fc > src/test/python/apache/aurora/client/cli/test_create.py > 8c27e2b340bb0a5fb5bcb44ef94d433e7f92c76c > src/test/python/apache/aurora/client/cli/test_cron.py > f3c522ed94a2d774865811ceb546bf9df083c14f > src/test/python/apache/aurora/client/cli/test_plugins.py > a545fece5e2b3e0017a61e1be9ac478372b1f34d > src/test/python/apache/aurora/client/cli/test_restart.py > 967d560e5c7eb0ed85b215fb11d9751b8666acb5 > src/test/python/apache/aurora/client/cli/util.py > 7b4558ec7f0fb0fd2902591bc6a90dc15051dd6e > src/test/python/apache/aurora/client/test_config.py > b1a3c1865819899ef19173be0f861783a2631d0a > > Diff: https://reviews.apache.org/r/49048/diff/ > > > Testing > --- > > ``` > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > *** OK (All tests passed) *** > > mesos-master start/running, process 26868 > + RETCODE=0 > + restore_netrc > + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc > + true > Connection to 127.0.0.1 closed. > > real 19m46.324s > user 0m1.496s > sys 0m0.774s > ``` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49048/#review143393 --- Ship it! Ship It! - Joshua Cohen On July 25, 2016, 4:25 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49048/ > --- > > (Updated July 25, 2016, 4:25 p.m.) > > > Review request for Aurora, Joshua Cohen and Stephan Erb. > > > Bugs: AURORA-1710 > https://issues.apache.org/jira/browse/AURORA-1710 > > > Repository: aurora > > > Description > --- > > AURORA-1710 Make 'tier' required and remove support for 'production' flag in > Job configuration - CLI changes > > > Diffs > - > > RELEASE-NOTES.md ca98d7ab802114e025f6021d33bc251704c59088 > src/main/python/apache/aurora/client/api/__init__.py > 68baf8fdb90cd26100159401c46c9963c24332b3 > src/main/python/apache/aurora/client/cli/context.py > 9b1511801d031ff48b81c25688a55cb586b8ac66 > src/main/python/apache/aurora/client/config.py > 2fc12559016d406c347adb416a5166cca31c961e > src/test/python/apache/aurora/client/cli/test_command_hooks.py > 2130f1fa71be02a004cdf8e476a270c81a7105d3 > src/test/python/apache/aurora/client/cli/test_context.py > 204ca092adad8bf43c5032a02f61bf303fb0b2fc > src/test/python/apache/aurora/client/cli/test_create.py > 8c27e2b340bb0a5fb5bcb44ef94d433e7f92c76c > src/test/python/apache/aurora/client/cli/test_cron.py > f3c522ed94a2d774865811ceb546bf9df083c14f > src/test/python/apache/aurora/client/cli/test_plugins.py > a545fece5e2b3e0017a61e1be9ac478372b1f34d > src/test/python/apache/aurora/client/cli/test_restart.py > 967d560e5c7eb0ed85b215fb11d9751b8666acb5 > src/test/python/apache/aurora/client/cli/util.py > 7b4558ec7f0fb0fd2902591bc6a90dc15051dd6e > src/test/python/apache/aurora/client/test_config.py > b1a3c1865819899ef19173be0f861783a2631d0a > > Diff: https://reviews.apache.org/r/49048/diff/ > > > Testing > --- > > ``` > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > *** OK (All tests passed) *** > > mesos-master start/running, process 26868 > + RETCODE=0 > + restore_netrc > + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc > + true > Connection to 127.0.0.1 closed. > > real 19m46.324s > user 0m1.496s > sys 0m0.774s > ``` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 50432: AURORA-1741 Fix pystachio binding bug introduced by AURORA-1710
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50432/#review143525 --- I know this is already landed, but it would be nice to follow this up with a test case if possible? - Joshua Cohen On July 26, 2016, 2:36 a.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50432/ > --- > > (Updated July 26, 2016, 2:36 a.m.) > > > Review request for Aurora, David McLaughlin and Maxim Khutornenko. > > > Bugs: AURORA-1741 > https://issues.apache.org/jira/browse/AURORA-1741 > > > Repository: aurora > > > Description > --- > > AURORA-1741 Fix pystachio binding bug introduced by AURORA-1710 > > > Diffs > - > > src/main/python/apache/aurora/client/config.py > 96cd9ddf77cac449d68537cd652bca03ef87d75d > > Diff: https://reviews.apache.org/r/50432/diff/ > > > Testing > --- > > ``` > ./build-support/jenkins/build.sh > > ``` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 50432: AURORA-1741 Fix pystachio binding bug introduced by AURORA-1710
> On July 26, 2016, 2:36 p.m., Joshua Cohen wrote: > > I know this is already landed, but it would be nice to follow this up with > > a test case if possible? > > Mehrdad Nurolahzade wrote: > The logic has two test cases: > (https://github.com/apache/aurora/blob/380307ac1878cb71bc86d2ccd7887192161254cf/src/test/python/apache/aurora/client/test_config.py#L228-L243). Yes, but they weren't failing though? It seems they were not sufficient to catch this problem. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50432/#review143525 --- On July 26, 2016, 2:36 a.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50432/ > --- > > (Updated July 26, 2016, 2:36 a.m.) > > > Review request for Aurora, David McLaughlin and Maxim Khutornenko. > > > Bugs: AURORA-1741 > https://issues.apache.org/jira/browse/AURORA-1741 > > > Repository: aurora > > > Description > --- > > AURORA-1741 Fix pystachio binding bug introduced by AURORA-1710 > > > Diffs > - > > src/main/python/apache/aurora/client/config.py > 96cd9ddf77cac449d68537cd652bca03ef87d75d > > Diff: https://reviews.apache.org/r/50432/diff/ > > > Testing > --- > > ``` > ./build-support/jenkins/build.sh > > ``` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 47853: Isolate the executor's filesystem from the task's.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47853/ --- (Updated July 26, 2016, 4:57 p.m.) Review request for Aurora, Jie Yu, Maxim Khutornenko, and Stephan Erb. Changes --- Update to use new mesos-containerizer launch command to isolate the task's filesystem rather than replicating all of that logic from Mesos into Thermos. A couple of notes: 1) this depends on Mesos 1.0.0, so I won't ship this until that has shipped and we've upgraded Aurora to depend on it. 2) End to end tests are failing for the GPU job which I'm assuming is related to the fact that I've upgraded my vagrant image to Mesos 1.0.0 but have not upgraded Aurora. Repository: aurora Description --- This changes the approach to launching tasks with filesystem images in the unified containerizer. Instead of adding an `Image` to the `MesosContainer`, we instead add the task filesystem as a `Volume` with an associated image. This image is mounted in the mesos directory under the `taskfs` path. The executor, on start up does the following: 1. Creates user/group under the taskfs root. 2. `pivot_root`s into the taskfs, while bind mounting the sandbox under that root as well as mounting procfs. 3. From there, task execution is essentially unchanged minus some slight changes to the environment depending on whether we're running in a pivoted root. Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 1d66208490aff6ea8af4c737845fa2cf13617529 examples/vagrant/upstart/aurora-scheduler.conf 954ddb48e923e0a2a29c415975c4f69afcad37b5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java cbbd6be94aa857b02cd7b45bfb2f0216d9a1cec3 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 0ef3856abc0df5403e3443ac35ba8d6940de8938 src/main/python/apache/aurora/executor/common/sandbox.py 6d8b7f58b639a60cc5a0c0c9ef98dfaaa8a64486 src/main/python/apache/aurora/executor/thermos_task_runner.py 3896e3841562600379705dbf78a6f62728246348 src/main/python/apache/thermos/core/BUILD 1094664e112cc71af37835f32037e9eb6d047202 src/main/python/apache/thermos/core/process.py 1791b5ff9a36eef7470bef9a6ebbafaf0ab05ca3 src/main/python/apache/thermos/core/runner.py fe971edaa2448afaf0fc342e11bc370de96ef5e4 src/main/python/apache/thermos/runner/thermos_runner.py 0d06e8e2ac78d26ba8f63744853eb5ce3f6aced6 src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 500fd435b4c72b25abd8df7eea6b3850edc96e99 src/test/python/apache/aurora/executor/common/test_sandbox.py 63f46e25bdd6fa387dd64975d7b95ee2659f5874 src/test/python/apache/thermos/core/test_process.py 77f644c09116266ce02479b9a80403aa68767bd6 src/test/sh/org/apache/aurora/e2e/Dockerfile 6fdea3d28760f59235c51c5b6913d2ee0172ef1a src/test/sh/org/apache/aurora/e2e/Dockerfile.netcat PRE-CREATION src/test/sh/org/apache/aurora/e2e/http/http_example.aurora bf6ef69401782d6c65a2d72e9270e52d1ad9fb9f src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora edeafbea288c95c19c82aede09717840b569528d src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora 9569eec2a32e0ea5212517c0082fc906036d1e57 src/test/sh/org/apache/aurora/e2e/run-server.sh PRE-CREATION src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 47bd94d1ce2aeffaefb67ac8325fc2f1d21c934c Diff: https://reviews.apache.org/r/47853/diff/ Testing --- Lots of manual testing, e2e tests, etc. I didn't add much coverage on the thermos side of things because it seemed like this was better served by the e2e tests than by doing a bunch of subprocess.check_call mocking. On the e2e front I created a new Dockerfile that sets up a much slimmer filesystem image that explicitly does not include python to ensure that the executor's filesystem is truly isolated. Thanks, Joshua Cohen
Re: Review Request 50478: Improve `executorLost` error message.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50478/#review143663 --- Ship it! Ship It! - Joshua Cohen On July 27, 2016, 1:25 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50478/ > --- > > (Updated July 27, 2016, 1:25 a.m.) > > > Review request for Aurora, Joshua Cohen and Dmitriy Shirchenko. > > > Repository: aurora > > > Description > --- > > Improve `executorLost` error message by including the slave id. > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java > 433fc904e419b533f879ad34284ce043118f33d4 > > Diff: https://reviews.apache.org/r/50478/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 50480: Multiple executor support in Scheduler
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50480/#review143664
---
CHANGELOG (lines 1 - 4)
<https://reviews.apache.org/r/50480/#comment209522>
This file is updated automatically by our release script. You can revert
these changes.
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
(line 165)
<https://reviews.apache.org/r/50480/#comment209523>
Kill this line?
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java
(lines 33 - 34)
<https://reviews.apache.org/r/50480/#comment209524>
Fix indentation. Style for continuation indents should be:
public ExecutorSettings(
ImmutableMap<...> config,
boolean populateDiscoveryInfo) {
// code continues here after blank line above
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java
(lines 40 - 45)
<https://reviews.apache.org/r/50480/#comment209527>
Instead of creating separate methods for get config and checking a config's
existence, how about changing `getExecutorConfig` to return
`Optional` and using `Optional#isPresent` as the indicator of
whether config exists for that name?
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java
(line 61)
<https://reviews.apache.org/r/50480/#comment209525>
s/An map/A map
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java
(line 82)
<https://reviews.apache.org/r/50480/#comment209526>
House style is: `Map foo = Maps.newHashMap()`. It's a legacy of the
project's pre-Java 7 origins. In theory at some point we could just move to
Java 7 diamond syntax (`... = new HashMap<>()`), but until that time, best to
remain consistent.
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java
(lines 97 - 98)
<https://reviews.apache.org/r/50480/#comment209528>
multiExecutors.put(
e.getName(),
new ExecutorConfig(...))
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java
(line 100)
<https://reviews.apache.org/r/50480/#comment209529>
We should probably return an `ImmutableMap` here.
src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 230)
<https://reviews.apache.org/r/50480/#comment209530>
Instead of passing in the task, just to use it to get the executor name,
why not just pass in the executor name?
src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 271)
<https://reviews.apache.org/r/50480/#comment209531>
Same here.
src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java (lines
141 - 143)
<https://reviews.apache.org/r/50480/#comment209532>
Should fit on one line?
src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java (line
158)
<https://reviews.apache.org/r/50480/#comment209533>
Are we guaranteed that the executor config exists here? The previous branch
is checking if executorConfig is set and that the config exists, but won't we
fall into this branch if executorConfig is not set?
- Joshua Cohen
On July 27, 2016, 2:04 a.m., Renan DelValle wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50480/
> -------
>
> (Updated July 27, 2016, 2:04 a.m.)
>
>
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> ---
>
> Adds support for using multiple executors in a single scheduler.
>
> Added warnings for pre-emption performance degradation when increasing
> executor overhead.
>
> Made executor config file require a JSON array.
>
> Modified the way in which Thermos and any custom executors are loaded at
> startup.
>
> Thermos now always exists regardless of any other executors being used.
>
> Jobs sent where no executor configuration is found for them are rejected.
>
>
> Diffs
> -
>
> CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c
> RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702
> docs/operations/configuration.md 0615c545f560597683f727c2425de4cc4e82c364
> src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java
> c43e04aee0df8988ed3af9d463dd6747d6631e3b
>
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
> 93ed3600268f23
Re: Review Request 50530: AURORA-1656 Document tier concept
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50530/#review143793 --- Ship it! Ship It! - Joshua Cohen On July 27, 2016, 7:32 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50530/ > --- > > (Updated July 27, 2016, 7:32 p.m.) > > > Review request for Aurora, Maxim Khutornenko and Stephan Erb. > > > Bugs: AURORA-1656 > https://issues.apache.org/jira/browse/AURORA-1656 > > > Repository: aurora > > > Description > --- > > AURORA-1656 Document tier concept > > > Diffs > - > > docs/features/multitenancy.md 62bcd535d3bf39c9ea7e6d5958f6ae8ba0867c0d > docs/reference/configuration.md 64c076d862453545652fbaa2d3e29f284ddd164d > > Diff: https://reviews.apache.org/r/50530/diff/ > > > Testing > --- > > > Thanks, > > Mehrdad Nurolahzade > >
Review Request 50584: Upgrade to Mesos 1.0.0
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50584/ --- Review request for Aurora, Maxim Khutornenko and Steve Niemitz. Repository: aurora Description --- I also took this as an opportunity to switch us from mesos.native to mesos.executor on the Python side, meaning Docker containers will no longer require all mesos deps. Release notes here: http://mesos.apache.org/blog/mesos-1-0-0-released/ Upgrade notes here: http://mesos.apache.org/documentation/latest/upgrades/ Diffs - 3rdparty/python/BUILD a44624c6cf3dcbdbfcc0504dd5e3dbcbfe6812de RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 Vagrantfile 5aa2128b8645402b9892ee8dc17439bf58ea19ab build-support/packer/build.sh 08938ec13f3054c0c5ec6a75c1515f195533bc6a build-support/python/make-mesos-native-egg 736685f9fc57b2d758edc752dc15bffb426a1879 build.gradle 95401953c51dfabb9ce388d66b5aac109507f74c src/main/java/org/apache/aurora/scheduler/app/AppModule.java e5bafa441cc848eb7eabdfe8c9e22dea3fac2ab7 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 8fe2b7997c0dfcbf9abdb467eea0fc768be6745c src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java e137daa2221c49e6fcd2b62b8ad0ccff1b8cb759 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java cbbd6be94aa857b02cd7b45bfb2f0216d9a1cec3 src/main/python/apache/aurora/executor/BUILD 0be65fc7c180d15f5a94bef268652ef7b868dcf7 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 0ef3856abc0df5403e3443ac35ba8d6940de8938 src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java 448d57a06b80fe1222abbc5149acf4e752d839a5 src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 500fd435b4c72b25abd8df7eea6b3850edc96e99 src/test/sh/org/apache/aurora/e2e/Dockerfile 6fdea3d28760f59235c51c5b6913d2ee0172ef1a Diff: https://reviews.apache.org/r/50584/diff/ Testing --- ./build-support/jenkins/build.sh ran e2e tests. Thanks, Joshua Cohen
Re: Review Request 50584: Upgrade to Mesos 1.0.0
> On July 29, 2016, 7:37 p.m., Maxim Khutornenko wrote: > > build-support/packer/build.sh, line 141 > > <https://reviews.apache.org/r/50584/diff/1/?file=1456885#file1456885line141> > > > > Is there a release note or a jira link that would support mesos.native > > -> mesos.executor transition is safe? I saw no mention of it in any release notes anywhere, but when building the mesos.native egg I saw it had a different name than it previously did (platform was dropped) which led me to dig into it a bit and discuss w/ Steve. He pointed out that mesos.native is now just a thin wrapper on top of mesos.scheduler and mesos.executor, so there's no longer any reason for the executor to pull in mesos.native which drags along all of the unnecessary scheduler deps. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50584/#review144164 ------- On July 28, 2016, 9:18 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50584/ > --- > > (Updated July 28, 2016, 9:18 p.m.) > > > Review request for Aurora, Maxim Khutornenko and Steve Niemitz. > > > Repository: aurora > > > Description > --- > > I also took this as an opportunity to switch us from mesos.native to > mesos.executor on the Python side, meaning Docker containers will no longer > require all mesos deps. > > Release notes here: http://mesos.apache.org/blog/mesos-1-0-0-released/ > Upgrade notes here: http://mesos.apache.org/documentation/latest/upgrades/ > > > Diffs > - > > 3rdparty/python/BUILD a44624c6cf3dcbdbfcc0504dd5e3dbcbfe6812de > RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 > Vagrantfile 5aa2128b8645402b9892ee8dc17439bf58ea19ab > build-support/packer/build.sh 08938ec13f3054c0c5ec6a75c1515f195533bc6a > build-support/python/make-mesos-native-egg > 736685f9fc57b2d758edc752dc15bffb426a1879 > build.gradle 95401953c51dfabb9ce388d66b5aac109507f74c > src/main/java/org/apache/aurora/scheduler/app/AppModule.java > e5bafa441cc848eb7eabdfe8c9e22dea3fac2ab7 > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 8fe2b7997c0dfcbf9abdb467eea0fc768be6745c > > src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java > e137daa2221c49e6fcd2b62b8ad0ccff1b8cb759 > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > cbbd6be94aa857b02cd7b45bfb2f0216d9a1cec3 > src/main/python/apache/aurora/executor/BUILD > 0be65fc7c180d15f5a94bef268652ef7b868dcf7 > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py > 0ef3856abc0df5403e3443ac35ba8d6940de8938 > > src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java > 448d57a06b80fe1222abbc5149acf4e752d839a5 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > 500fd435b4c72b25abd8df7eea6b3850edc96e99 > src/test/sh/org/apache/aurora/e2e/Dockerfile > 6fdea3d28760f59235c51c5b6913d2ee0172ef1a > > Diff: https://reviews.apache.org/r/50584/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > > ran e2e tests. > > > Thanks, > > Joshua Cohen > >
Re: Review Request 50432: AURORA-1741 Fix pystachio binding bug introduced by AURORA-1710
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50432/#review144191 --- src/main/python/apache/aurora/client/config.py <https://reviews.apache.org/r/50432/#comment210196> Isn't this undoing the bug fix to compare w/ `Empty` instead of `None`? - Joshua Cohen On July 29, 2016, 9 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50432/ > --- > > (Updated July 29, 2016, 9 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Bugs: AURORA-1741 > https://issues.apache.org/jira/browse/AURORA-1741 > > > Repository: aurora > > > Description > --- > > AURORA-1741 Fix pystachio binding bug introduced by AURORA-1710 > > > Diffs > - > > src/test/python/apache/aurora/client/test_config.py > 4742fa28e3156e5b20791b80f2db8392f7f2f4bf > > Diff: https://reviews.apache.org/r/50432/diff/ > > > Testing > --- > > ``` > ./build-support/jenkins/build.sh > > ``` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 50617: AURORA-1741 Added missing test cases
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50617/#review144223 --- Ship it! Ship It! - Joshua Cohen On July 29, 2016, 10:46 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50617/ > --- > > (Updated July 29, 2016, 10:46 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Bugs: AURORA-1741 > https://issues.apache.org/jira/browse/AURORA-1741 > > > Repository: aurora > > > Description > --- > > AURORA-1741 Added missing test cases > > > Diffs > - > > src/test/python/apache/aurora/client/test_config.py > 4742fa28e3156e5b20791b80f2db8392f7f2f4bf > > Diff: https://reviews.apache.org/r/50617/diff/ > > > Testing > --- > > `./pants test src/test/python/apache/aurora/client/cli:cli` > > > Thanks, > > Mehrdad Nurolahzade > >
Re: Review Request 50584: Upgrade to Mesos 1.0.0
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50584/ --- (Updated Aug. 1, 2016, 3:05 p.m.) Review request for Aurora, Maxim Khutornenko and Steve Niemitz. Changes --- Update RELEASE-NOTES.md per Stephan's suggestions. Repository: aurora Description --- I also took this as an opportunity to switch us from mesos.native to mesos.executor on the Python side, meaning Docker containers will no longer require all mesos deps. Release notes here: http://mesos.apache.org/blog/mesos-1-0-0-released/ Upgrade notes here: http://mesos.apache.org/documentation/latest/upgrades/ Diffs (updated) - 3rdparty/python/BUILD a44624c6cf3dcbdbfcc0504dd5e3dbcbfe6812de RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 Vagrantfile 5aa2128b8645402b9892ee8dc17439bf58ea19ab build-support/packer/build.sh 08938ec13f3054c0c5ec6a75c1515f195533bc6a build-support/python/make-mesos-native-egg 736685f9fc57b2d758edc752dc15bffb426a1879 build.gradle 95401953c51dfabb9ce388d66b5aac109507f74c src/main/java/org/apache/aurora/scheduler/app/AppModule.java e5bafa441cc848eb7eabdfe8c9e22dea3fac2ab7 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 8fe2b7997c0dfcbf9abdb467eea0fc768be6745c src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java e137daa2221c49e6fcd2b62b8ad0ccff1b8cb759 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java cbbd6be94aa857b02cd7b45bfb2f0216d9a1cec3 src/main/python/apache/aurora/executor/BUILD 0be65fc7c180d15f5a94bef268652ef7b868dcf7 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 0ef3856abc0df5403e3443ac35ba8d6940de8938 src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java 448d57a06b80fe1222abbc5149acf4e752d839a5 src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 500fd435b4c72b25abd8df7eea6b3850edc96e99 src/test/sh/org/apache/aurora/e2e/Dockerfile 6fdea3d28760f59235c51c5b6913d2ee0172ef1a Diff: https://reviews.apache.org/r/50584/diff/ Testing --- ./build-support/jenkins/build.sh ran e2e tests. Thanks, Joshua Cohen
Re: Review Request 47853: Isolate the executor's filesystem from the task's.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47853/ --- (Updated Aug. 1, 2016, 5:22 p.m.) Review request for Aurora, Jie Yu, Maxim Khutornenko, and Stephan Erb. Changes --- rebase on top of Mesos 1.0.0 upgrade changes, fix broken aurora config. Maxim, Stephan, these changes now should be good for review. Repository: aurora Description --- This changes the approach to launching tasks with filesystem images in the unified containerizer. Instead of adding an `Image` to the `MesosContainer`, we instead add the task filesystem as a `Volume` with an associated image. This image is mounted in the mesos directory under the `taskfs` path. The executor, on start up does the following: 1. Creates user/group under the taskfs root. 2. `pivot_root`s into the taskfs, while bind mounting the sandbox under that root as well as mounting procfs. 3. From there, task execution is essentially unchanged minus some slight changes to the environment depending on whether we're running in a pivoted root. Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 1d66208490aff6ea8af4c737845fa2cf13617529 examples/vagrant/upstart/aurora-scheduler.conf 954ddb48e923e0a2a29c415975c4f69afcad37b5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java f9b1c7cf30f93336fb850da09c1f2b7178cbdc17 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py c5159314543cba15ac5e525ad0c31dedd398f8bf src/main/python/apache/aurora/executor/common/sandbox.py 6d8b7f58b639a60cc5a0c0c9ef98dfaaa8a64486 src/main/python/apache/aurora/executor/thermos_task_runner.py 3896e3841562600379705dbf78a6f62728246348 src/main/python/apache/thermos/core/BUILD 1094664e112cc71af37835f32037e9eb6d047202 src/main/python/apache/thermos/core/process.py 1791b5ff9a36eef7470bef9a6ebbafaf0ab05ca3 src/main/python/apache/thermos/core/runner.py fe971edaa2448afaf0fc342e11bc370de96ef5e4 src/main/python/apache/thermos/runner/thermos_runner.py 0d06e8e2ac78d26ba8f63744853eb5ce3f6aced6 src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java bb8a849717a6ca3328ad4638acff5cc44ddd6ac9 src/test/python/apache/aurora/executor/common/test_sandbox.py 63f46e25bdd6fa387dd64975d7b95ee2659f5874 src/test/python/apache/thermos/core/test_process.py 77f644c09116266ce02479b9a80403aa68767bd6 src/test/sh/org/apache/aurora/e2e/Dockerfile 1b91f02725c1a6762cda2aaa587456341df4ba5a src/test/sh/org/apache/aurora/e2e/Dockerfile.netcat PRE-CREATION src/test/sh/org/apache/aurora/e2e/http/http_example.aurora bf6ef69401782d6c65a2d72e9270e52d1ad9fb9f src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora edeafbea288c95c19c82aede09717840b569528d src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora 9569eec2a32e0ea5212517c0082fc906036d1e57 src/test/sh/org/apache/aurora/e2e/run-server.sh PRE-CREATION src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 47bd94d1ce2aeffaefb67ac8325fc2f1d21c934c Diff: https://reviews.apache.org/r/47853/diff/ Testing --- Lots of manual testing, e2e tests, etc. I didn't add much coverage on the thermos side of things because it seemed like this was better served by the e2e tests than by doing a bunch of subprocess.check_call mocking. On the e2e front I created a new Dockerfile that sets up a much slimmer filesystem image that explicitly does not include python to ensure that the executor's filesystem is truly isolated. Thanks, Joshua Cohen
Review Request 50669: Upgrade aurora-packaging for Mesos 1.0.0.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50669/ --- Review request for Aurora and Stephan Erb. Repository: aurora-packaging Description --- Upgrade aurora-packaging for Mesos 1.0.0. Diffs - specs/debian/control 08f64e6e6c6913d4925054c6ff92991382047465 specs/rpm/aurora.spec b68643c4ecda944cac1b86273be9888e7b36a309 test/deb/debian-jessie/provision.sh 26e4804a5d621fdbe90c4889962b3bb5f384375b test/deb/ubuntu-trusty/provision.sh 8105f6ec122f29db8cb039957a79a8c2b758066f test/rpm/centos-7/provision.sh ae1fe693131bebaee0818c61ab022ad3cc244075 Diff: https://reviews.apache.org/r/50669/diff/ Testing --- Built local packages for ubuntu, debian and centos. Thanks, Joshua Cohen
Re: Review Request 50685: Support TBinaryProtocol over HTTP
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50685/#review144450 --- Overall looks good to me. +1 to David's comment as well. src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java (lines 98 - 101) <https://reviews.apache.org/r/50685/#comment210462> Why a list of this class instead of a simple `Map`. I doubt this will be a perf hotspot, but it would allow for constant time lookup of the proper factory type for a given request type rather than having to iterate all of the request types to find a match. We could then have a second map from factory->response mime type? src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java (line 97) <https://reviews.apache.org/r/50685/#comment210463> This could use HTTP 415? - Joshua Cohen On Aug. 2, 2016, 1:29 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50685/ > --- > > (Updated Aug. 2, 2016, 1:29 a.m.) > > > Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Dmitriy > Shirchenko. > > > Bugs: AURORA-1743 > https://issues.apache.org/jira/browse/AURORA-1743 > > > Repository: aurora > > > Description > --- > > This replaces the `TServlet` servlet from thrift with our own servlet which > dispatches thrift responses based on the content type of the request. This > enables a client to use either the thrift json protocol or the binary > protocol when communicating with the scheduler. > > Without this patch the current behaviour is: > - Regardless of content type of the request, assume the request is json and > return json with a `application/x-thrift` content type. > > With this patch the behaviour becomes: > - A missing content type in the request results in a HTTP 400 response. > - A request with a content type of `application/x-thrift` is assumed to be > json and returns json with a `application/vnd.apache.thrift.json` content > type. This maintains backwards compatability with the client. > - A request with a content type of `application/json` returns json with a > `application/vnd.apache.thrift.json` content type. This maintains backwards > compatability with the AJAX UI. > - A request with a content type of `application/vnd.apache.thrift.json` > returns json with a `application/vnd.apache.thrift.json` content type. > - A request with a content type of `application/vnd.apache.thrift.binary` > returns binary thrift with a `application/vnd.apache.thrift.json` content > type. > - Any other content type in a request results in a HTTP 400 response. > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java > cd5adf9655dc3368dbe06bfee15c65182176be2e > > src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java > 31f5cb3bed48eef60c3b2becb2ed285e93f2bd5a > > src/test/java/org/apache/aurora/scheduler/http/api/TContentAwareServletTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/50685/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 47853: Isolate the executor's filesystem from the task's.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47853/ --- (Updated Aug. 2, 2016, 8:55 p.m.) Review request for Aurora, Jie Yu, Maxim Khutornenko, and Stephan Erb. Changes --- Address Stephan's feedback: - Add a note to the help output for --mesos_containerizer_path that its value should match that of -launcher_dir on the mesos agent. - Make user/group adding under the task filesystem more robust to already existing users/groups. - Fail fast if the task is using a filesystem image but --mesos_containerizer_path was not set. - restore the bind mount of the mesos directory into the task's filesystem that was lost in the switch to mesos-containerizer. Repository: aurora Description --- This changes the approach to launching tasks with filesystem images in the unified containerizer. Instead of adding an `Image` to the `MesosContainer`, we instead add the task filesystem as a `Volume` with an associated image. This image is mounted in the mesos directory under the `taskfs` path. The executor, on start up does the following: 1. Creates user/group under the taskfs root. 2. `pivot_root`s into the taskfs, while bind mounting the sandbox under that root as well as mounting procfs. 3. From there, task execution is essentially unchanged minus some slight changes to the environment depending on whether we're running in a pivoted root. Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 1d66208490aff6ea8af4c737845fa2cf13617529 examples/vagrant/upstart/aurora-scheduler.conf 954ddb48e923e0a2a29c415975c4f69afcad37b5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java f9b1c7cf30f93336fb850da09c1f2b7178cbdc17 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py c5159314543cba15ac5e525ad0c31dedd398f8bf src/main/python/apache/aurora/executor/common/sandbox.py 6d8b7f58b639a60cc5a0c0c9ef98dfaaa8a64486 src/main/python/apache/aurora/executor/thermos_task_runner.py 3896e3841562600379705dbf78a6f62728246348 src/main/python/apache/thermos/core/BUILD 1094664e112cc71af37835f32037e9eb6d047202 src/main/python/apache/thermos/core/process.py 1791b5ff9a36eef7470bef9a6ebbafaf0ab05ca3 src/main/python/apache/thermos/core/runner.py fe971edaa2448afaf0fc342e11bc370de96ef5e4 src/main/python/apache/thermos/runner/thermos_runner.py 0d06e8e2ac78d26ba8f63744853eb5ce3f6aced6 src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java bb8a849717a6ca3328ad4638acff5cc44ddd6ac9 src/test/python/apache/aurora/executor/common/test_sandbox.py 63f46e25bdd6fa387dd64975d7b95ee2659f5874 src/test/python/apache/thermos/core/test_process.py 77f644c09116266ce02479b9a80403aa68767bd6 src/test/sh/org/apache/aurora/e2e/Dockerfile 1b91f02725c1a6762cda2aaa587456341df4ba5a src/test/sh/org/apache/aurora/e2e/Dockerfile.netcat PRE-CREATION src/test/sh/org/apache/aurora/e2e/http/http_example.aurora bf6ef69401782d6c65a2d72e9270e52d1ad9fb9f src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora edeafbea288c95c19c82aede09717840b569528d src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora 9569eec2a32e0ea5212517c0082fc906036d1e57 src/test/sh/org/apache/aurora/e2e/run-server.sh PRE-CREATION src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 47bd94d1ce2aeffaefb67ac8325fc2f1d21c934c Diff: https://reviews.apache.org/r/47853/diff/ Testing --- Lots of manual testing, e2e tests, etc. I didn't add much coverage on the thermos side of things because it seemed like this was better served by the e2e tests than by doing a bunch of subprocess.check_call mocking. On the e2e front I created a new Dockerfile that sets up a much slimmer filesystem image that explicitly does not include python to ensure that the executor's filesystem is truly isolated. Thanks, Joshua Cohen
Re: Review Request 47853: Isolate the executor's filesystem from the task's.
> On Aug. 1, 2016, 7:05 p.m., Stephan Erb wrote: > > src/main/python/apache/aurora/executor/common/sandbox.py, lines 200-209 > > <https://reviews.apache.org/r/47853/diff/2/?file=1453189#file1453189line200> > > > > Those two calls have the implicit assumptions: > > > > * the role user does not exist within container. > > * uid and gid as available on the host do not collide with pre-existing > > ones in the container > > > > The first point could be problematic for people migrating from the > > existing Docker support in Aurora, as it requires images with the > > pre-existing users. They do have implicit assumptions, I'm ok with that though, I wasn't building this functionality under the assumption that a Docker image being used via the DockerContainerizer would work out of the box with the new containerizer (at the very least you'll no longer require python in your image). That said, it's fairly ssafe to check for exit code 4 (per the manpage: UID exists) from the useradd call and continue in that case. For groupadd we can pass the -f flag to simply exit 0 if the group exists. > On Aug. 1, 2016, 7:05 p.m., Stephan Erb wrote: > > src/main/python/apache/thermos/core/process.py, lines 408-416 > > <https://reviews.apache.org/r/47853/diff/2/?file=1453192#file1453192line408> > > > > I am slightly confused here. > > > > The way I understand the code `self._sandbox` points to a path of the > > host filesystem. So, if we set `cwd` to that path, what does it point to > > when we are running within the container? Is this related to the bind mount > > that you mention in the description of this patch? Good catch. This is a leftover from my original impl that bind mounted the sandbox path into the task's filesystem. The new mesos-containerizer launch subcommand does not have that behavior though, so I'll need to restore that bind mount. > On Aug. 1, 2016, 7:05 p.m., Stephan Erb wrote: > > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py, lines > > 98-102 > > <https://reviews.apache.org/r/47853/diff/2/?file=1453188#file1453188line98> > > > > Looks like Mesos is able to compute a default value for this using some > > makefile constants: > > https://github.com/apache/mesos/blob/e8ebbe5fe4189ef7ab046da2276a6abee41deeb2/src/slave/flags.cpp#L193-L198 > > > > We should at least document that the expected value here is based on > > the one passed to Mesos. Added a note to the help text. > On Aug. 1, 2016, 7:05 p.m., Stephan Erb wrote: > > src/main/python/apache/aurora/executor/thermos_task_runner.py, lines 268-270 > > <https://reviews.apache.org/r/47853/diff/2/?file=1453190#file1453190line268> > > > > This code is risky to operator error: If the operator forgets to set > > the optional containerizer path, users can launch container images whose > > filesystem isolation will not be applied. > > > > I would prefer if we throw an error (here, or at another appropriate > > place) if we are supposed to use a filesystem image without a containerizer > > path. Added some code to fail fast if task is using an image but path to mesos-containerizer is not provided. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47853/#review144324 --- On Aug. 2, 2016, 8:55 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47853/ > --- > > (Updated Aug. 2, 2016, 8:55 p.m.) > > > Review request for Aurora, Jie Yu, Maxim Khutornenko, and Stephan Erb. > > > Repository: aurora > > > Description > --- > > This changes the approach to launching tasks with filesystem images in the > unified containerizer. Instead of adding an `Image` to the `MesosContainer`, > we instead add the task filesystem as a `Volume` with an associated image. > This image is mounted in the mesos directory under the `taskfs` path. The > executor, on start up does the following: > > 1. Creates user/group under the taskfs root. > 2. `pivot_root`s into the taskfs, while bind mounting the sandbox under that > root as well as mounting procfs. > 3. From there, task execution is essentially unchanged minus some slight > changes to the environment depending on whether we're running in a p
Re: Review Request 50716: Use Docker host network rather than bridging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50716/#review144529 --- Ship it! Ship It! - Joshua Cohen On Aug. 2, 2016, 9:13 p.m., Stephan Erb wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50716/ > --- > > (Updated Aug. 2, 2016, 9:13 p.m.) > > > Review request for Aurora and John Sirois. > > > Repository: aurora-packaging > > > Description > --- > > The default bridge network is known to be slow (1) and potentially > flaky (2). Switching to host networking is a desperate attempt to reduce > flaps in our nightly package builds. > > (1) https://github.com/docker/docker/issues/7857 > (2) https://github.com/docker/docker/issues/11407 > > > Diffs > - > > build-artifact.sh 333c7a4d8e661f608c4949dcbae1401bb1a75b51 > > Diff: https://reviews.apache.org/r/50716/diff/ > > > Testing > --- > > ./build-artifact.sh apache-aurora-0.15.0.tar.gz 0.15.0 > > > Thanks, > > Stephan Erb > >
Re: Review Request 50685: Support TBinaryProtocol over HTTP
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50685/#review144577 --- So, on further reflection, it doesn't necessarily make sense for the response type to be dictated by the `Content-Type` of the request. We should probably use the `Accept` header instead? I don't necessarily know if the use case exists to send json yet want to receive binary, but it's more idiomatic HTTP to support that possibility. - Joshua Cohen On Aug. 3, 2016, 12:10 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50685/ > --- > > (Updated Aug. 3, 2016, 12:10 a.m.) > > > Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Dmitriy > Shirchenko. > > > Bugs: AURORA-1743 > https://issues.apache.org/jira/browse/AURORA-1743 > > > Repository: aurora > > > Description > --- > > This replaces the `TServlet` servlet from thrift with our own servlet which > dispatches thrift responses based on the content type of the request. This > enables a client to use either the thrift json protocol or the binary > protocol when communicating with the scheduler. > > Without this patch the current behaviour is: > - Regardless of content type of the request, assume the request is json and > return json with a `application/x-thrift` content type. > > With this patch the behaviour becomes: > - A request with no content type is assumed to be json and returns json with > a `application/vnd.apache.thrift.json` content type. This maintains backwards > compatability with clients that may not set a content type. > - A request with a content type of `application/x-thrift` is assumed to be > json and returns json with a `application/vnd.apache.thrift.json` content > type. This maintains backwards compatability with the client. > - A request with a content type of `application/json` returns json with a > `application/vnd.apache.thrift.json` content type. This maintains backwards > compatability with the AJAX UI. > - A request with a content type of `application/vnd.apache.thrift.json` > returns json with a `application/vnd.apache.thrift.json` content type. > - A request with a content type of `application/vnd.apache.thrift.binary` > returns binary thrift with a `application/vnd.apache.thrift.json` content > type. > - Any other content type in a request results in a HTTP 415 response. > > > Diffs > - > > RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 > src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java > cd5adf9655dc3368dbe06bfee15c65182176be2e > > src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java > 31f5cb3bed48eef60c3b2becb2ed285e93f2bd5a > > src/test/java/org/apache/aurora/scheduler/http/api/TContentAwareServletTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/50685/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 50480: Multiple executor support in Scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50480/#review144578 --- Ship it! Ship It! - Joshua Cohen On Aug. 2, 2016, 11:38 p.m., Renan DelValle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50480/ > --- > > (Updated Aug. 2, 2016, 11:38 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > Adds support for using multiple executors in a single scheduler. > > Added warnings for pre-emption performance degradation when increasing > executor overhead. > > Made executor config file require a JSON array. > > Modified the way in which Thermos and any custom executors are loaded at > startup. > > Thermos now always exists regardless of any other executors being used. > > Jobs sent where no executor configuration is found for them are rejected. > > Task prefix is now set from json file. > > Fixed non revokable resources not being filtered out from overhead. > > > Diffs > - > > RELEASE-NOTES.md 19d7f7e34652d170a7d8eb41a9c6ffdcc67324eb > docs/operations/configuration.md 0615c545f560597683f727c2425de4cc4e82c364 > src/main/java/org/apache/aurora/GuavaUtils.java > 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > c43e04aee0df8988ed3af9d463dd6747d6631e3b > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > 93ed3600268f231b0e53ceb6b3674ff742d94407 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorConfig.java > 24329a49155b9e8f6d11dd1d09f6f5188d1e2ad9 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java > 8b7f8dcf4d8e0372208e636b3f57fc159272ecc7 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java > e919d3f2e2f86c26c0448029b06277a3a41a6941 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java > b74edf46d35ac99164ec1bcf33edf36556baf9ed > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > f9b1c7cf30f93336fb850da09c1f2b7178cbdc17 > src/main/java/org/apache/aurora/scheduler/mesos/TestExecutorSettings.java > 1dfa97e659c2fca8308cb592f37d14328e4b42bc > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java > ee6fe95eaa41c7952a974ebea069b3de6ed82994 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java > 3b84dbcfde9ae686110409173d742b3fa86ac764 > > src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java > 147327036a872c9ccd03e17daaaf8cb1df489843 > > src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoaderTest.java > 7de7c46e6e1f478e25f7362d1d060b7f765dcb36 > > src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java > d34e12f6e0837fa43ea9b4e2a17db579e0ee10a2 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > bb8a849717a6ca3328ad4638acff5cc44ddd6ac9 > > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java > 7eb1714d14581a6ab25e85d36a1f3e973380c536 > > src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java > fba427bd327e7f63b640c8b8753bfdeec3ee31e7 > src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java > a79b0f413e603374347f8ce5765fb6e71bc9a5b5 > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > c0fe84371ff2f9d47721126a9c356180f7c166de > > src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-missing-field.json > 464617028b1e765a563a3e11f70d66089ede6866 > > src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-multiple-executor.json > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-single-executor.json > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-thermos-executor.json > 114eb4f6c3eaeca3ad976e4907a3ec32c2339a09 > > Diff: https://reviews.apache.org/r/50480/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > Created and killed jobs running a custom executor using a custom client. > > Created and killed thermos jobs with the Aurora Client. > > > Thanks, > > Renan DelValle > >
Re: Review Request 50480: Multiple executor support in Scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50480/#review144579 --- I was about to push this change, but in the process of running e2e tests locally after applying your patch, it occurs to me that perhaps this is something we should have e2e coverage for. Do you think it would be feasible to add something to test_end_to_end.sh that verifies the multiple executor (and transitively the custom executor) support? If it would be a big change, I'd be ok with shipping it in a follow up review. - Joshua Cohen On Aug. 2, 2016, 11:38 p.m., Renan DelValle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50480/ > --- > > (Updated Aug. 2, 2016, 11:38 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > Adds support for using multiple executors in a single scheduler. > > Added warnings for pre-emption performance degradation when increasing > executor overhead. > > Made executor config file require a JSON array. > > Modified the way in which Thermos and any custom executors are loaded at > startup. > > Thermos now always exists regardless of any other executors being used. > > Jobs sent where no executor configuration is found for them are rejected. > > Task prefix is now set from json file. > > Fixed non revokable resources not being filtered out from overhead. > > > Diffs > - > > RELEASE-NOTES.md 19d7f7e34652d170a7d8eb41a9c6ffdcc67324eb > docs/operations/configuration.md 0615c545f560597683f727c2425de4cc4e82c364 > src/main/java/org/apache/aurora/GuavaUtils.java > 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > c43e04aee0df8988ed3af9d463dd6747d6631e3b > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > 93ed3600268f231b0e53ceb6b3674ff742d94407 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorConfig.java > 24329a49155b9e8f6d11dd1d09f6f5188d1e2ad9 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java > 8b7f8dcf4d8e0372208e636b3f57fc159272ecc7 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java > e919d3f2e2f86c26c0448029b06277a3a41a6941 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java > b74edf46d35ac99164ec1bcf33edf36556baf9ed > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > f9b1c7cf30f93336fb850da09c1f2b7178cbdc17 > src/main/java/org/apache/aurora/scheduler/mesos/TestExecutorSettings.java > 1dfa97e659c2fca8308cb592f37d14328e4b42bc > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java > ee6fe95eaa41c7952a974ebea069b3de6ed82994 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java > 3b84dbcfde9ae686110409173d742b3fa86ac764 > > src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java > 147327036a872c9ccd03e17daaaf8cb1df489843 > > src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoaderTest.java > 7de7c46e6e1f478e25f7362d1d060b7f765dcb36 > > src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java > d34e12f6e0837fa43ea9b4e2a17db579e0ee10a2 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > bb8a849717a6ca3328ad4638acff5cc44ddd6ac9 > > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java > 7eb1714d14581a6ab25e85d36a1f3e973380c536 > > src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java > fba427bd327e7f63b640c8b8753bfdeec3ee31e7 > src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java > a79b0f413e603374347f8ce5765fb6e71bc9a5b5 > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > c0fe84371ff2f9d47721126a9c356180f7c166de > > src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-missing-field.json > 464617028b1e765a563a3e11f70d66089ede6866 > > src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-multiple-executor.json > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/configuration/executor
Re: Review Request 47853: Isolate the executor's filesystem from the task's.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47853/ --- (Updated Aug. 3, 2016, 3:34 p.m.) Review request for Aurora, Jie Yu, Maxim Khutornenko, and Stephan Erb. Changes --- Properly handle the case where the gid already exists in the task's filesystem. Repository: aurora Description --- This changes the approach to launching tasks with filesystem images in the unified containerizer. Instead of adding an `Image` to the `MesosContainer`, we instead add the task filesystem as a `Volume` with an associated image. This image is mounted in the mesos directory under the `taskfs` path. The executor, on start up does the following: 1. Creates user/group under the taskfs root. 2. `pivot_root`s into the taskfs, while bind mounting the sandbox under that root as well as mounting procfs. 3. From there, task execution is essentially unchanged minus some slight changes to the environment depending on whether we're running in a pivoted root. Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 1d66208490aff6ea8af4c737845fa2cf13617529 examples/vagrant/upstart/aurora-scheduler.conf 954ddb48e923e0a2a29c415975c4f69afcad37b5 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java f9b1c7cf30f93336fb850da09c1f2b7178cbdc17 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py c5159314543cba15ac5e525ad0c31dedd398f8bf src/main/python/apache/aurora/executor/common/sandbox.py 6d8b7f58b639a60cc5a0c0c9ef98dfaaa8a64486 src/main/python/apache/aurora/executor/thermos_task_runner.py 3896e3841562600379705dbf78a6f62728246348 src/main/python/apache/thermos/core/BUILD 1094664e112cc71af37835f32037e9eb6d047202 src/main/python/apache/thermos/core/process.py 1791b5ff9a36eef7470bef9a6ebbafaf0ab05ca3 src/main/python/apache/thermos/core/runner.py fe971edaa2448afaf0fc342e11bc370de96ef5e4 src/main/python/apache/thermos/runner/thermos_runner.py 0d06e8e2ac78d26ba8f63744853eb5ce3f6aced6 src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java bb8a849717a6ca3328ad4638acff5cc44ddd6ac9 src/test/python/apache/aurora/executor/common/test_sandbox.py 63f46e25bdd6fa387dd64975d7b95ee2659f5874 src/test/python/apache/thermos/core/test_process.py 77f644c09116266ce02479b9a80403aa68767bd6 src/test/sh/org/apache/aurora/e2e/Dockerfile 1b91f02725c1a6762cda2aaa587456341df4ba5a src/test/sh/org/apache/aurora/e2e/Dockerfile.netcat PRE-CREATION src/test/sh/org/apache/aurora/e2e/http/http_example.aurora bf6ef69401782d6c65a2d72e9270e52d1ad9fb9f src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora edeafbea288c95c19c82aede09717840b569528d src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora 9569eec2a32e0ea5212517c0082fc906036d1e57 src/test/sh/org/apache/aurora/e2e/run-server.sh PRE-CREATION src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 47bd94d1ce2aeffaefb67ac8325fc2f1d21c934c Diff: https://reviews.apache.org/r/47853/diff/ Testing --- Lots of manual testing, e2e tests, etc. I didn't add much coverage on the thermos side of things because it seemed like this was better served by the e2e tests than by doing a bunch of subprocess.check_call mocking. On the e2e front I created a new Dockerfile that sets up a much slimmer filesystem image that explicitly does not include python to ensure that the executor's filesystem is truly isolated. Thanks, Joshua Cohen
Re: Review Request 50480: Multiple executor support in Scheduler
> On Aug. 3, 2016, 2:36 a.m., Joshua Cohen wrote: > > I was about to push this change, but in the process of running e2e tests > > locally after applying your patch, it occurs to me that perhaps this is > > something we should have e2e coverage for. > > > > Do you think it would be feasible to add something to test_end_to_end.sh > > that verifies the multiple executor (and transitively the custom executor) > > support? If it would be a big change, I'd be ok with shipping it in a > > follow up review. > > Renan DelValle wrote: > This is a far point and one I considered as well. It would be of great > benefit to add a test but I think it might be a bigger change than it seems > on the surface. One of the biggest pain points to getting this done is being > able to submit tasks with a configurable ExecutorConfig.name using pystachio. > > For now the best we have technical support for is being able to load > multiple executors sucessfully which is one of the test for > ExecutorSettingsLoader. > > That said, I'll be looking into it and would gladly provide a follow up > patch. > > On a slight side note, since we'll be using ExecutorConfig.name to > identify the executor to use, the current default for thermos is > "aurora_executor" (as set on the thrift api). If we want to change this to > "thermos", now's the time to do it. (From what I've seen this constant was > not used anywhere in the Server side code until this patch.) I'm ok with looking into e2e tests in a follow up patch. Would you mind filing a ticket to track that? As far as changing the executor name goes, I'm neutral on that. I'm not sure there's a huge upside, and there are potential downsides to it (e.g. would it cause executor config to change for an otherwise no-op update)? - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50480/#review144579 --- On Aug. 2, 2016, 11:38 p.m., Renan DelValle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50480/ > --- > > (Updated Aug. 2, 2016, 11:38 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > Adds support for using multiple executors in a single scheduler. > > Added warnings for pre-emption performance degradation when increasing > executor overhead. > > Made executor config file require a JSON array. > > Modified the way in which Thermos and any custom executors are loaded at > startup. > > Thermos now always exists regardless of any other executors being used. > > Jobs sent where no executor configuration is found for them are rejected. > > Task prefix is now set from json file. > > Fixed non revokable resources not being filtered out from overhead. > > > Diffs > - > > RELEASE-NOTES.md 19d7f7e34652d170a7d8eb41a9c6ffdcc67324eb > docs/operations/configuration.md 0615c545f560597683f727c2425de4cc4e82c364 > src/main/java/org/apache/aurora/GuavaUtils.java > 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > c43e04aee0df8988ed3af9d463dd6747d6631e3b > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > 93ed3600268f231b0e53ceb6b3674ff742d94407 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorConfig.java > 24329a49155b9e8f6d11dd1d09f6f5188d1e2ad9 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java > 8b7f8dcf4d8e0372208e636b3f57fc159272ecc7 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java > e919d3f2e2f86c26c0448029b06277a3a41a6941 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java > b74edf46d35ac99164ec1bcf33edf36556baf9ed > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > f9b1c7cf30f93336fb850da09c1f2b7178cbdc17 > src/main/java/org/apache/aurora/scheduler/mesos/TestExecutorSettings.java > 1dfa97e659c2fca8308cb592f37d14328e4b42bc > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java > ee6fe95eaa41c7952a974ebea069b3de6ed82994 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java
Re: Review Request 50480: Multiple executor support in Scheduler
> On Aug. 3, 2016, 2:36 a.m., Joshua Cohen wrote: > > I was about to push this change, but in the process of running e2e tests > > locally after applying your patch, it occurs to me that perhaps this is > > something we should have e2e coverage for. > > > > Do you think it would be feasible to add something to test_end_to_end.sh > > that verifies the multiple executor (and transitively the custom executor) > > support? If it would be a big change, I'd be ok with shipping it in a > > follow up review. > > Renan DelValle wrote: > This is a far point and one I considered as well. It would be of great > benefit to add a test but I think it might be a bigger change than it seems > on the surface. One of the biggest pain points to getting this done is being > able to submit tasks with a configurable ExecutorConfig.name using pystachio. > > For now the best we have technical support for is being able to load > multiple executors sucessfully which is one of the test for > ExecutorSettingsLoader. > > That said, I'll be looking into it and would gladly provide a follow up > patch. > > On a slight side note, since we'll be using ExecutorConfig.name to > identify the executor to use, the current default for thermos is > "aurora_executor" (as set on the thrift api). If we want to change this to > "thermos", now's the time to do it. (From what I've seen this constant was > not used anywhere in the Server side code until this patch.) > > Joshua Cohen wrote: > I'm ok with looking into e2e tests in a follow up patch. Would you mind > filing a ticket to track that? > > As far as changing the executor name goes, I'm neutral on that. I'm not > sure there's a huge upside, and there are potential downsides to it (e.g. > would it cause executor config to change for an otherwise no-op update)? > > Renan DelValle wrote: > No problem. > > Yeah, I'm neutral on this as well. The only benefit I see is being able > to test this feature, but I'm not sure about the ripple effects it may have. > > The alternative is using a custom client (which we're in the process of > preparing for release). The downsides here are obvious but it may be the > least painful option. We can continue discussion here: > https://issues.apache.org/jira/browse/AURORA-1744 > > Thanks again for looking over the code, very much appreciated. > > Renan DelValle wrote: > Whoops sorry, crossed some cable here. > Yeah, no huge upsides on changing the name on the Thrift API just wanted > to bring it up in case anyone felt strongly about it. > > Will look into being able to set executor name from Pystachio to test > this feature. Would you mind rebasing this and posting an updated diff. I just tried to merge and I'm getting conflicts with my recently submitted isolation changes. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50480/#review144579 --- On Aug. 2, 2016, 11:38 p.m., Renan DelValle wrote: > > ------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50480/ > --- > > (Updated Aug. 2, 2016, 11:38 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > Adds support for using multiple executors in a single scheduler. > > Added warnings for pre-emption performance degradation when increasing > executor overhead. > > Made executor config file require a JSON array. > > Modified the way in which Thermos and any custom executors are loaded at > startup. > > Thermos now always exists regardless of any other executors being used. > > Jobs sent where no executor configuration is found for them are rejected. > > Task prefix is now set from json file. > > Fixed non revokable resources not being filtered out from overhead. > > > Diffs > - > > RELEASE-NOTES.md 19d7f7e34652d170a7d8eb41a9c6ffdcc67324eb > docs/operations/configuration.md 0615c545f560597683f727c2425de4cc4e82c364 > src/main/java/org/apache/aurora/GuavaUtils.java > 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > c43e04aee0df8988ed3af9d463dd6747d6631e3b > > src/main/java/org/apache/
Re: Review Request 50685: Support TBinaryProtocol over HTTP
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50685/#review144685 --- Ship it! Ship It! - Joshua Cohen On Aug. 3, 2016, 9:36 p.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50685/ > --- > > (Updated Aug. 3, 2016, 9:36 p.m.) > > > Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Dmitriy > Shirchenko. > > > Bugs: AURORA-1743 > https://issues.apache.org/jira/browse/AURORA-1743 > > > Repository: aurora > > > Description > --- > > This replaces the `TServlet` servlet from thrift with our own servlet which > dispatches thrift responses based on the content type of the request. This > enables a client to use either the thrift json protocol or the binary > protocol when communicating with the scheduler. > > Without this patch the current behaviour is: > - Regardless of content type of the request, assume the request is json and > return json with a `application/x-thrift` content type. > > With this patch the behaviour becomes: > - A request with no content type header, or a type of `application/x-thrift` > or `application/json` or `application/vnd.apache.thrift.json` is assumed to > be JSON. > - A request with a content type header of > `application/vnd.apache.thrift.binary` is assumed to be binary. > - A request with an `Accept` header of `application/vnd.apache.thrift.binary` > will have a binary response. > - A request with any other `Accept` header will have a JSON response. > > > Diffs > - > > RELEASE-NOTES.md 19d7f7e34652d170a7d8eb41a9c6ffdcc67324eb > src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java > cd5adf9655dc3368dbe06bfee15c65182176be2e > > src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java > 7dbe48b433262a5b9f7b369d69a0d41e2472b054 > src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java > 31f5cb3bed48eef60c3b2becb2ed285e93f2bd5a > > Diff: https://reviews.apache.org/r/50685/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 50826: Populate the source field of ExecutorInfo.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50826/#review144855 --- This does not compile. We have -Werror set on our builds which means the warning about the field being deprecated causes a build failure. - Joshua Cohen On Aug. 5, 2016, 12:34 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50826/ > --- > > (Updated Aug. 5, 2016, 12:34 a.m.) > > > Review request for Aurora, Joshua Cohen and Dmitriy Shirchenko. > > > Bugs: AURORA-1745 > https://issues.apache.org/jira/browse/AURORA-1745 > > > Repository: aurora > > > Description > --- > > b912e17 stopped populating the source field of the executor. For backwards > compatibility we should continue to populate this field and the `source` > label. > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > 34134430063b2d24a4e20d3f91ab899604edaf89 > > Diff: https://reviews.apache.org/r/50826/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 50826: Populate the source field of ExecutorInfo.
> On Aug. 5, 2016, 12:50 a.m., Joshua Cohen wrote: > > This does not compile. We have -Werror set on our builds which means the > > warning about the field being deprecated causes a build failure. If we do want to maintain this across a release boundary for compatibility sake we'll need to add a @SuppressWarnings annotation and also fix the MesosTaskFactoryImpl tests that break as a result of re-adding this field. Additionally we should add a ticket to remove this after the next release. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50826/#review144855 --- On Aug. 5, 2016, 12:34 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50826/ > --- > > (Updated Aug. 5, 2016, 12:34 a.m.) > > > Review request for Aurora, Joshua Cohen and Dmitriy Shirchenko. > > > Bugs: AURORA-1745 > https://issues.apache.org/jira/browse/AURORA-1745 > > > Repository: aurora > > > Description > --- > > b912e17 stopped populating the source field of the executor. For backwards > compatibility we should continue to populate this field and the `source` > label. > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > 34134430063b2d24a4e20d3f91ab899604edaf89 > > Diff: https://reviews.apache.org/r/50826/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 50826: Populate the source field of ExecutorInfo.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50826/#review144930 --- Ship it! Looks good to me. Would you mind adding something to RELEASE-NOTES.md about ExecutorInfo.source being deprecated in favor of the source label, and that it will be removed from Mesos in an upcoming release? src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 306) <https://reviews.apache.org/r/50826/#comment211092> We should extract sourceName to a var here just like we do in the test. - Joshua Cohen On Aug. 5, 2016, 1:05 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50826/ > --- > > (Updated Aug. 5, 2016, 1:05 a.m.) > > > Review request for Aurora, Joshua Cohen and Dmitriy Shirchenko. > > > Bugs: AURORA-1745 > https://issues.apache.org/jira/browse/AURORA-1745 > > > Repository: aurora > > > Description > --- > > b912e17 stopped populating the source field of the executor. For backwards > compatibility we should continue to populate this field and the `source` > label. > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > 34134430063b2d24a4e20d3f91ab899604edaf89 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > 7484e8b159eb65634f7bd8f0aae61b2e1227e662 > > Diff: https://reviews.apache.org/r/50826/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 50168: Add rollback functionality to the scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50168/#review145144 --- src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java (line 133) <https://reviews.apache.org/r/50168/#comment211312> It feels like we should also be able to roll back an update in `ROLL_FORWARD_AWAITING_PULSE`. This is not currently possible based on the state machine. If a user tried to rollback an update that's stuck waiting for a pulse, they would first need to explicitly pause the update and then roll it back (since the PAUSED->ROLLING_BACK transition is allowed). Do you feel that this is problematic? It's certainly an edge case, but it seems like it could lead to confusion for a user who is desperately trying to roll back an update. We could update `JobUpdateStateMachine` to allow for the transition from `ROLL_FORWARD_AWAITING_PULSE` to `ROLLING_BACK`. - Joshua Cohen On Aug. 3, 2016, 10:16 p.m., Igor Morozov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50168/ > --- > > (Updated Aug. 3, 2016, 10:16 p.m.) > > > Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and > Zameer Manji. > > > Bugs: AURORA-1721 > https://issues.apache.org/jira/browse/AURORA-1721 > > > Repository: aurora > > > Description > --- > > Add rollback functionality to the scheduler > > > Diffs > - > > CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c > RELEASE-NOTES.md 19d7f7e34652d170a7d8eb41a9c6ffdcc67324eb > api/src/main/thrift/org/apache/aurora/gen/api.thrift > b799cce665c99ee20fba84a9ddcb5a895ff5685e > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > b534abf95bab6e1657e3ef993cf34c0d6ec460be > > src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java > 9243c92b11040b68ed6014b3991db69fc08bcddf > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java > f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java > 364c5c753f884a2d89e27802d7bbf3b2b6d3a08e > src/main/python/apache/aurora/client/api/__init__.py > ec2c786d143347a7953f9fe431c6ddc59e1b9854 > src/main/python/apache/aurora/client/cli/update.py > bb526f7bf94d7bfe02fe2786493c85be1bfeb86f > src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java > e157c0dfde5efc418448e138aa008ade742fe816 > src/test/python/apache/aurora/client/api/test_scheduler_client.py > afbd385b7eda64cb1f7d118b695e65e4045eac6c > src/test/python/apache/aurora/client/cli/test_supdate.py > 317b17547e8b33106787fbe43b26cec2da830ba1 > > Diff: https://reviews.apache.org/r/50168/diff/ > > > Testing > --- > > > Thanks, > > Igor Morozov > >
Re: Review Request 50931: Remove unnecessary guice container parameters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50931/#review145300 --- Ship it! Ship It! - Joshua Cohen On Aug. 9, 2016, 7:53 p.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50931/ > --- > > (Updated Aug. 9, 2016, 7:53 p.m.) > > > Review request for Aurora and Joshua Cohen. > > > Repository: aurora > > > Description > --- > > I noticed these configuration parameters have no effect. Both the API and > JAX-RS endpoints like /vars return gipped content. > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java > 12185ec233ac8c9c6b974b60b7e102f48df61c55 > > Diff: https://reviews.apache.org/r/50931/diff/ > > > Testing > --- > > $ curl -I -X GET http://192.168.33.7:8081/vars -H 'Accept-Encoding: gzip, > deflate' > HTTP/1.1 200 OK > Date: Mon, 08 Aug 2016 15:18:12 GMT > Content-Type: text/plain > Vary: Accept-Encoding, User-Agent > Content-Encoding: gzip > Transfer-Encoding: chunked > Server: Jetty(9.3.6.v20151106) > > > Thanks, > > Zameer Manji > >
Re: Review Request 50937: Bump jetty dependency to the latest release.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50937/#review145301 --- Ship it! Ship It! - Joshua Cohen On Aug. 9, 2016, 9:57 p.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50937/ > --- > > (Updated Aug. 9, 2016, 9:57 p.m.) > > > Review request for Aurora and Joshua Cohen. > > > Repository: aurora > > > Description > --- > > A useful fix from the jetty-9.3.10.v20160621 release: > > 623 Add --gzip suffix to 304 responses with ETAGs > > Without this fix adding ETAG support to the scheduler with gzipped requests > and > responses is not possible. > > > Diffs > - > > build.gradle f466b3af1ae9009faf7adbd5a89e8970ac41081c > > Diff: https://reviews.apache.org/r/50937/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 50168: Add rollback functionality to the scheduler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50168/#review145302 --- Ship it! Thanks for adding that transition. This looks good to me, but I want Maxim to take another quick pass before shipping, as he expressed concerns about testing the new transition. - Joshua Cohen On Aug. 9, 2016, 10:41 p.m., Igor Morozov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50168/ > --- > > (Updated Aug. 9, 2016, 10:41 p.m.) > > > Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and > Zameer Manji. > > > Bugs: AURORA-1721 > https://issues.apache.org/jira/browse/AURORA-1721 > > > Repository: aurora > > > Description > --- > > Add rollback functionality to the scheduler > > > Diffs > - > > CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c > RELEASE-NOTES.md 50f9b8372b364773d4ee6b29f031deb13c9f6444 > api/src/main/thrift/org/apache/aurora/gen/api.thrift > b799cce665c99ee20fba84a9ddcb5a895ff5685e > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > b534abf95bab6e1657e3ef993cf34c0d6ec460be > > src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java > 9243c92b11040b68ed6014b3991db69fc08bcddf > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java > f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java > 364c5c753f884a2d89e27802d7bbf3b2b6d3a08e > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java > 7ab739a4dfa292895ad6ba8849e65f5c45ce9770 > src/main/python/apache/aurora/client/api/__init__.py > ec2c786d143347a7953f9fe431c6ddc59e1b9854 > src/main/python/apache/aurora/client/cli/update.py > bb526f7bf94d7bfe02fe2786493c85be1bfeb86f > > src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java > 8d78bae2cbf365a5359d88c0e996b3972a21e50e > src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java > e157c0dfde5efc418448e138aa008ade742fe816 > src/test/python/apache/aurora/client/api/test_scheduler_client.py > afbd385b7eda64cb1f7d118b695e65e4045eac6c > src/test/python/apache/aurora/client/cli/test_supdate.py > 317b17547e8b33106787fbe43b26cec2da830ba1 > > Diff: https://reviews.apache.org/r/50168/diff/ > > > Testing > --- > > > Thanks, > > Igor Morozov > >
Re: Review Request 51192: Moving custom executors documentation to features, adding gorealis to tools
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51192/#review146082 --- Ship it! lgtm modulo the below. docs/operations/configuration.md <https://reviews.apache.org/r/51192/#comment212458> I'd say leave something along these lines this in this file with a link to the full documentation? - Joshua Cohen On Aug. 17, 2016, 10:52 p.m., Renan DelValle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51192/ > --- > > (Updated Aug. 17, 2016, 10:52 p.m.) > > > Review request for Aurora. > > > Repository: aurora > > > Description > --- > > Moved custom executor documentation from docs/operations/configuration.md > since it grew out of proportion. > Added an explaination on how a thrift object needs to be configured in a > createJob call and referenced using a custom Client for now. > > > Diffs > - > > docs/additional-resources/tools.md 64a4f74f38240fa85336105fd4f1c8c6771de852 > docs/features/custom-executors.md PRE-CREATION > docs/operations/configuration.md bbcffac686cddb97f8a137afce1ed50a260b7359 > > Diff: https://reviews.apache.org/r/51192/diff/ > > > Testing > --- > > None, doc changes only. > > > Thanks, > > Renan DelValle > >
Re: Review Request 51264: Reduce static method exposure for Stats.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51264/#review146364 --- Ship it! Ship It! - Joshua Cohen On Aug. 21, 2016, 1:28 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51264/ > --- > > (Updated Aug. 21, 2016, 1:28 a.m.) > > > Review request for Aurora and Joshua Cohen. > > > Repository: aurora > > > Description > --- > > `org.apache.aurora.common.stats.Stats` has several static methods that are > not used in our codebase. This patch deletes the unused methods and reduces > the visability of other static methods where possible. > > > Diffs > - > > commons/src/main/java/org/apache/aurora/common/stats/Stats.java > bb0af69a89e8f8905e7fd01183d0ac7fe2a49db2 > > Diff: https://reviews.apache.org/r/51264/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Review Request 51298: A few executor fixes for filesystem isolation:
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51298/ --- Review request for Aurora and Maxim Khutornenko. Repository: aurora Description --- - Add an option to skip the groupadd/useradd calls into the task's filesystem. - Mount any configured volumes into the task's filesystem. - Clean up http server script used by appc e2e tests. Diffs - src/main/python/apache/aurora/executor/aurora_executor.py dde19a6914c7c7b2178220707f242f61f11f38bd src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 65a495d5c50d91b38c4328bab3bfec667f6a7ba9 src/main/python/apache/aurora/executor/common/sandbox.py 5f091af7636bd94f028f15d63437e305b02f741c src/test/python/apache/aurora/executor/common/test_sandbox.py ce989b1ccda0f1bc7ba9e15dfe4be20116db3491 src/test/python/apache/aurora/executor/test_thermos_executor.py 06601df3bc355a690ff1789b2e2e34484fadefe9 src/test/sh/org/apache/aurora/e2e/run-server.sh 76939888bed2e8138671d97f7bc56fd5641008e4 Diff: https://reviews.apache.org/r/51298/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests Thanks, Joshua Cohen
Re: Review Request 51301: Use a fixed Debian mirror rather than relying on httpredir
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51301/#review146536 --- Ship it! Ship It! - Joshua Cohen On Aug. 22, 2016, 9:05 p.m., Stephan Erb wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51301/ > --- > > (Updated Aug. 22, 2016, 9:05 p.m.) > > > Review request for Aurora and Joshua Cohen. > > > Repository: aurora-packaging > > > Description > --- > > Use a fixed Debian mirror rather than relying on httpredir > > This is another attempt at a stable nightly package build. The currently most > common error is: > > ``` > E: Failed to fetch > http://httpredir.debian.org/debian/pool/main/p/patch/patch_2.7.5-1_amd64.deb > Error reading from server. Remote end closed connection [IP: 5.153.231.35 80] > > E: Unable to fetch some archives, maybe run apt-get update or try with > --fix-missing? > The command '/bin/sh -c apt-get update && apt-get -y install bison > debhelper devscripts dpkg-dev curl git libapr1-dev > libcurl4-openssl-dev libkrb5-dev libsvn-dev python-all-dev > software-properties-common thrift-compiler' returned a non-zero code: 100 > ``` > > > Diffs > - > > builder/deb/debian-jessie/Dockerfile > 22793325749bd2ba439c8df34b30f445571cf1cf > > Diff: https://reviews.apache.org/r/51301/diff/ > > > Testing > --- > > Multiple executions of `./build-artifact.sh builder/deb/debian-jessie > snapshot.tar.gz 0.16.0-snapshot` > > > Thanks, > > Stephan Erb > >
Re: Review Request 51298: A few executor fixes for filesystem isolation:
> On Aug. 22, 2016, 10:41 p.m., Stephan Erb wrote: > > src/main/python/apache/aurora/executor/common/sandbox.py, line 235 > > <https://reviews.apache.org/r/51298/diff/1/?file=1481195#file1481195line235> > > > > ... then this if would not be needed. One could potentially invoke the sandbox creation from elsewhere which could lead to the same case where we would fail to iterate. I figured it was better to make the handling of a `None` value explicit. > On Aug. 22, 2016, 10:41 p.m., Stephan Erb wrote: > > src/main/python/apache/aurora/executor/common/sandbox.py, line 244 > > <https://reviews.apache.org/r/51298/diff/1/?file=1481195#file1481195line244> > > > > When I remember your previous patch correctly, then the current working > > directory of our processes is set to the mesos_directory. This would imply > > that specifying an alternative sandbox mount point could break things. Good catch. I cleaned up the cwd handling, which led to a bit of a rabbit hole of other fixes, but on the bright side, everything seems to work properly as far as bind mounting the sandbox directory into the taskfs. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51298/#review146449 ------- On Aug. 22, 2016, 8:25 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51298/ > --- > > (Updated Aug. 22, 2016, 8:25 p.m.) > > > Review request for Aurora and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > - Add an option to skip the groupadd/useradd calls into the task's filesystem. > - Mount any configured volumes into the task's filesystem. > - Clean up http server script used by appc e2e tests. > > > Diffs > - > > src/main/python/apache/aurora/executor/aurora_executor.py > dde19a6914c7c7b2178220707f242f61f11f38bd > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py > 65a495d5c50d91b38c4328bab3bfec667f6a7ba9 > src/main/python/apache/aurora/executor/common/sandbox.py > 5f091af7636bd94f028f15d63437e305b02f741c > src/test/python/apache/aurora/executor/common/test_sandbox.py > ce989b1ccda0f1bc7ba9e15dfe4be20116db3491 > src/test/python/apache/aurora/executor/test_thermos_executor.py > 06601df3bc355a690ff1789b2e2e34484fadefe9 > src/test/sh/org/apache/aurora/e2e/run-server.sh > 76939888bed2e8138671d97f7bc56fd5641008e4 > > Diff: https://reviews.apache.org/r/51298/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > e2e tests > > > Thanks, > > Joshua Cohen > >
Re: Review Request 51298: A few executor fixes for filesystem isolation:
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51298/ --- (Updated Aug. 23, 2016, 8:35 p.m.) Review request for Aurora and Maxim Khutornenko. Changes --- Address Stephan's review feedback. Repository: aurora Description --- - Add an option to skip the groupadd/useradd calls into the task's filesystem. - Mount any configured volumes into the task's filesystem. - Clean up http server script used by appc e2e tests. Diffs (updated) - src/main/python/apache/aurora/executor/aurora_executor.py dde19a6914c7c7b2178220707f242f61f11f38bd src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 65a495d5c50d91b38c4328bab3bfec667f6a7ba9 src/main/python/apache/aurora/executor/common/sandbox.py 5f091af7636bd94f028f15d63437e305b02f741c src/main/python/apache/aurora/executor/thermos_task_runner.py 1d713ca3d81a2e7be88b787dfcab328d887be24c src/main/python/apache/thermos/core/process.py a296fa715ef6fbb8d5feae356914334437f353f1 src/main/python/apache/thermos/runner/thermos_runner.py 441bacdfe93b1805a03a1216762c74db810a9540 src/test/python/apache/aurora/executor/common/test_sandbox.py ce989b1ccda0f1bc7ba9e15dfe4be20116db3491 src/test/python/apache/aurora/executor/test_thermos_executor.py 06601df3bc355a690ff1789b2e2e34484fadefe9 src/test/python/apache/thermos/core/test_process.py 759f783202803c296ce19bb64c59cbe896d40a43 src/test/sh/org/apache/aurora/e2e/http/http_example.aurora b69ddf129c0015a878b089d85d731bc0c26fd55c src/test/sh/org/apache/aurora/e2e/run-server.sh 76939888bed2e8138671d97f7bc56fd5641008e4 Diff: https://reviews.apache.org/r/51298/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests Thanks, Joshua Cohen
Re: Review Request 51307: Catch IOError.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51307/#review146573 --- Ship it! Ship It! - Joshua Cohen On Aug. 23, 2016, 9:12 p.m., David Robinson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51307/ > --- > > (Updated Aug. 23, 2016, 9:12 p.m.) > > > Review request for Aurora, Joshua Cohen and Maxim Khutornenko. > > > Bugs: AURORA-1752 > https://issues.apache.org/jira/browse/AURORA-1752 > > > Repository: aurora > > > Description > --- > > Catch IOError. > > > Diffs > - > > src/main/python/apache/thermos/monitoring/process_collector_psutil.py > bb7c90206791309772c4bb8e2ccf6e62a3991403 > src/test/python/apache/thermos/monitoring/test_process_collector_psutil.py > PRE-CREATION > > Diff: https://reviews.apache.org/r/51307/diff/ > > > Testing > --- > > $ ./pants test src/test/python/apache/thermos/monitoring > > 13:59:36 00:00 [main] >(To run a reporting server: ./pants server) > 13:59:36 00:00 [setup] > 13:59:36 00:00 [parse] >Executing tasks in goals: test > 13:59:36 00:00 [test] > 13:59:36 00:00 [test-prep-command] > 13:59:36 00:00 [test] > 13:59:36 00:00 [pytest] > 13:59:36 00:00 [run] > == test session starts === > platform linux2 -- Python 2.7.11 -- py-1.4.31 -- > pytest-2.6.4 > plugins: cov, timeout > collected 12 items > > src/test/python/apache/thermos/monitoring/test_disk.py . > > src/test/python/apache/thermos/monitoring/test_detector.py . > > src/test/python/apache/thermos/monitoring/test_process_collector_psutil.py . > > src/test/python/apache/thermos/monitoring/test_resource.py . > > === 12 passed in 0.17 seconds > > 13:59:37 00:01 [complete] >SUCCESS > > > Thanks, > > David Robinson > >
Re: Review Request 51348: Fix thermos killing heuristic to permit setuid(2).
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51348/#review146582 --- Ship it! This makes me a little nervous, but I don't think we're exposing ourselves to anything terrible by doing this. - Joshua Cohen On Aug. 23, 2016, 8:45 p.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51348/ > --- > > (Updated Aug. 23, 2016, 8:45 p.m.) > > > Review request for Aurora, Joshua Cohen and Stephan Erb. > > > Bugs: AURORA-1753 > https://issues.apache.org/jira/browse/AURORA-1753 > > > Repository: aurora > > > Description > --- > > Previously this process killing heuristic would not allow killing of a process > if the uid it was launched with differs from the real uid of the currently > running process. The logic is too conservative because it doesn't factor in > that a process launched as root can use `setuid(2)` to change it's real uid. > > This patch fixes the heuristic by permitting killing of a process launched as > root but the real uid is now not root. > > > Diffs > - > > src/main/python/apache/thermos/core/helper.py > dda40ed71bf8d26255cdb76eae29a5978a120d41 > src/test/python/apache/thermos/core/test_helper.py > 35397abd3ec769788f166088e5455c28bb120459 > > Diff: https://reviews.apache.org/r/51348/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 51366: Unset PYTHONPATH before calling pants
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51366/#review146658 --- Ship it! Ship It! - Joshua Cohen On Aug. 24, 2016, 12:07 p.m., Stephan Erb wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51366/ > --- > > (Updated Aug. 24, 2016, 12:07 p.m.) > > > Review request for Aurora and John Sirois. > > > Bugs: AURORA-1717 > https://issues.apache.org/jira/browse/AURORA-1717 > > > Repository: aurora > > > Description > --- > > Our tests are started via `./pants test.pytest` and are then calling `./pants > binary` within some test setup routines. Looks like that the `PYTHONPATH` can > be tainted for the second run. Unsetting it seems to prevent test failures of > the kind: > > ``` > Traceback (most recent call last): > File > "/home/jenkins/.cache/pants/setup/bootstrap-Linux-x86_64/0.0.80/bin/pants", > line 7, in > from pants.bin.pants_exe import main > ImportError: No module named pants.bin.pants_exe > ``` > > > Diffs > - > > pants b1ca3823b5b31cceccc5b9e4cb9a7ef79305d9fd > > Diff: https://reviews.apache.org/r/51366/diff/ > > > Testing > --- > > > Thanks, > > Stephan Erb > >
Re: Review Request 51298: A few executor fixes for filesystem isolation:
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51298/ --- (Updated Aug. 25, 2016, 6:51 p.m.) Review request for Aurora and Maxim Khutornenko. Changes --- Review feedback: - Fix support for .thermos_profile. - Remove process_log_directory option in favor of cleaner separation of sandbox root/container sandbox root. Repository: aurora Description --- - Add an option to skip the groupadd/useradd calls into the task's filesystem. - Mount any configured volumes into the task's filesystem. - Clean up http server script used by appc e2e tests. Diffs (updated) - src/main/python/apache/aurora/executor/aurora_executor.py dde19a6914c7c7b2178220707f242f61f11f38bd src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 65a495d5c50d91b38c4328bab3bfec667f6a7ba9 src/main/python/apache/aurora/executor/common/sandbox.py 5f091af7636bd94f028f15d63437e305b02f741c src/main/python/apache/aurora/executor/thermos_task_runner.py 1d713ca3d81a2e7be88b787dfcab328d887be24c src/main/python/apache/thermos/core/process.py a296fa715ef6fbb8d5feae356914334437f353f1 src/main/python/apache/thermos/core/runner.py dcfc190f7ed529e4f816e02b2d969077c60b008f src/main/python/apache/thermos/runner/thermos_runner.py 441bacdfe93b1805a03a1216762c74db810a9540 src/test/python/apache/aurora/executor/common/test_sandbox.py ce989b1ccda0f1bc7ba9e15dfe4be20116db3491 src/test/python/apache/aurora/executor/test_thermos_executor.py 06601df3bc355a690ff1789b2e2e34484fadefe9 src/test/python/apache/thermos/core/test_process.py 759f783202803c296ce19bb64c59cbe896d40a43 src/test/sh/org/apache/aurora/e2e/http/http_example.aurora b69ddf129c0015a878b089d85d731bc0c26fd55c src/test/sh/org/apache/aurora/e2e/run-server.sh 76939888bed2e8138671d97f7bc56fd5641008e4 src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 0404d0e0a1a1054a5ed0c0b9f5f5be9fb3ecc113 Diff: https://reviews.apache.org/r/51298/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests Thanks, Joshua Cohen
Re: Review Request 51298: A few executor fixes for filesystem isolation:
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51298/ --- (Updated Aug. 25, 2016, 6:54 p.m.) Review request for Aurora and Maxim Khutornenko. Changes --- Minor cleanup missed in previous revision. Repository: aurora Description --- - Add an option to skip the groupadd/useradd calls into the task's filesystem. - Mount any configured volumes into the task's filesystem. - Clean up http server script used by appc e2e tests. Diffs (updated) - src/main/python/apache/aurora/executor/aurora_executor.py dde19a6914c7c7b2178220707f242f61f11f38bd src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 65a495d5c50d91b38c4328bab3bfec667f6a7ba9 src/main/python/apache/aurora/executor/common/sandbox.py 5f091af7636bd94f028f15d63437e305b02f741c src/main/python/apache/aurora/executor/thermos_task_runner.py 1d713ca3d81a2e7be88b787dfcab328d887be24c src/main/python/apache/thermos/core/process.py a296fa715ef6fbb8d5feae356914334437f353f1 src/main/python/apache/thermos/core/runner.py dcfc190f7ed529e4f816e02b2d969077c60b008f src/main/python/apache/thermos/runner/thermos_runner.py 441bacdfe93b1805a03a1216762c74db810a9540 src/test/python/apache/aurora/executor/common/test_sandbox.py ce989b1ccda0f1bc7ba9e15dfe4be20116db3491 src/test/python/apache/aurora/executor/test_thermos_executor.py 06601df3bc355a690ff1789b2e2e34484fadefe9 src/test/python/apache/thermos/core/test_process.py 759f783202803c296ce19bb64c59cbe896d40a43 src/test/sh/org/apache/aurora/e2e/http/http_example.aurora b69ddf129c0015a878b089d85d731bc0c26fd55c src/test/sh/org/apache/aurora/e2e/run-server.sh 76939888bed2e8138671d97f7bc56fd5641008e4 src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 0404d0e0a1a1054a5ed0c0b9f5f5be9fb3ecc113 Diff: https://reviews.apache.org/r/51298/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests Thanks, Joshua Cohen
Re: Review Request 51298: A few executor fixes for filesystem isolation:
leted. This is a > > problem similar to the thermos_profile issue mentioned above. I'm not sure what you're getting at here? The `cwd` arg is passed on to mesos-containerizer as the value for the `--working_directory` flag. That value is used to `chdir` after the pivot_root/chroot logic is run. I've also thoroughly verified that this directory is respected by the running task (original by the `create_datafile` process in the e2e test, now replaced by the `setup_env` process). > On Aug. 24, 2016, 11:21 a.m., Stephan Erb wrote: > > src/main/python/apache/thermos/runner/thermos_runner.py, lines 139-144 > > <https://reviews.apache.org/r/51298/diff/2/?file=1482579#file1482579line139> > > > > Can this be changed in a meaningful way? I have always assumed that the > > observer UI has the hardcoded assumption that logs can be found within > > `sandbox/.logs`. You're right, this was working only by coincidence (because the value being passed was a bind mount of the sandbox). I cleaned this up which uncovered a few more bugs (e.g. the runner was ensuring the sandbox directory existed, so when we passed in /mnt/mesos/sandbox as the value, that path was created on the host filesystem). > On Aug. 24, 2016, 11:21 a.m., Stephan Erb wrote: > > src/main/python/apache/aurora/executor/common/sandbox.py, line 36 > > <https://reviews.apache.org/r/51298/diff/1-2/?file=1481195#file1481195line36> > > > > Please extend this doc string slightly to mention that this path is > > within the host filesystem. Done. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51298/#review146619 --- On Aug. 25, 2016, 6:54 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51298/ > --- > > (Updated Aug. 25, 2016, 6:54 p.m.) > > > Review request for Aurora and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > - Add an option to skip the groupadd/useradd calls into the task's filesystem. > - Mount any configured volumes into the task's filesystem. > - Clean up http server script used by appc e2e tests. > > > Diffs > - > > src/main/python/apache/aurora/executor/aurora_executor.py > dde19a6914c7c7b2178220707f242f61f11f38bd > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py > 65a495d5c50d91b38c4328bab3bfec667f6a7ba9 > src/main/python/apache/aurora/executor/common/sandbox.py > 5f091af7636bd94f028f15d63437e305b02f741c > src/main/python/apache/aurora/executor/thermos_task_runner.py > 1d713ca3d81a2e7be88b787dfcab328d887be24c > src/main/python/apache/thermos/core/process.py > a296fa715ef6fbb8d5feae356914334437f353f1 > src/main/python/apache/thermos/core/runner.py > dcfc190f7ed529e4f816e02b2d969077c60b008f > src/main/python/apache/thermos/runner/thermos_runner.py > 441bacdfe93b1805a03a1216762c74db810a9540 > src/test/python/apache/aurora/executor/common/test_sandbox.py > ce989b1ccda0f1bc7ba9e15dfe4be20116db3491 > src/test/python/apache/aurora/executor/test_thermos_executor.py > 06601df3bc355a690ff1789b2e2e34484fadefe9 > src/test/python/apache/thermos/core/test_process.py > 759f783202803c296ce19bb64c59cbe896d40a43 > src/test/sh/org/apache/aurora/e2e/http/http_example.aurora > b69ddf129c0015a878b089d85d731bc0c26fd55c > src/test/sh/org/apache/aurora/e2e/run-server.sh > 76939888bed2e8138671d97f7bc56fd5641008e4 > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > 0404d0e0a1a1054a5ed0c0b9f5f5be9fb3ecc113 > > Diff: https://reviews.apache.org/r/51298/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > e2e tests > > > Thanks, > > Joshua Cohen > >
Re: Review Request 51298: A few executor fixes for filesystem isolation:
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51298/ --- (Updated Aug. 25, 2016, 7:28 p.m.) Review request for Aurora and Maxim Khutornenko. Changes --- Fix broken test. Repository: aurora Description --- - Add an option to skip the groupadd/useradd calls into the task's filesystem. - Mount any configured volumes into the task's filesystem. - Clean up http server script used by appc e2e tests. Diffs (updated) - src/main/python/apache/aurora/executor/aurora_executor.py dde19a6914c7c7b2178220707f242f61f11f38bd src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 65a495d5c50d91b38c4328bab3bfec667f6a7ba9 src/main/python/apache/aurora/executor/common/sandbox.py 5f091af7636bd94f028f15d63437e305b02f741c src/main/python/apache/aurora/executor/thermos_task_runner.py 1d713ca3d81a2e7be88b787dfcab328d887be24c src/main/python/apache/thermos/core/process.py a296fa715ef6fbb8d5feae356914334437f353f1 src/main/python/apache/thermos/core/runner.py dcfc190f7ed529e4f816e02b2d969077c60b008f src/main/python/apache/thermos/runner/thermos_runner.py 441bacdfe93b1805a03a1216762c74db810a9540 src/test/python/apache/aurora/executor/common/test_sandbox.py ce989b1ccda0f1bc7ba9e15dfe4be20116db3491 src/test/python/apache/aurora/executor/test_thermos_executor.py 06601df3bc355a690ff1789b2e2e34484fadefe9 src/test/python/apache/thermos/core/test_process.py 759f783202803c296ce19bb64c59cbe896d40a43 src/test/sh/org/apache/aurora/e2e/http/http_example.aurora b69ddf129c0015a878b089d85d731bc0c26fd55c src/test/sh/org/apache/aurora/e2e/run-server.sh 76939888bed2e8138671d97f7bc56fd5641008e4 src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 0404d0e0a1a1054a5ed0c0b9f5f5be9fb3ecc113 Diff: https://reviews.apache.org/r/51298/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests Thanks, Joshua Cohen
Re: Review Request 51384: Introduce UpdateMetadata fields in JobUpdateRequest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51384/#review146835 --- src/main/java/org/apache/aurora/scheduler/updater/UpdateInprogressException.java (line 22) <https://reviews.apache.org/r/51384/#comment213570> s/inprogress/inProgress here and below. src/main/python/apache/aurora/client/cli/options.py (line 89) <https://reviews.apache.org/r/51384/#comment213579> You're going to want to pass the optional `maxsplit=1` arg here to handle the case where someone sets metadata to something like: foo=bar=baz. That said, I'm not convinced we actually need to expose this as a user-visible option. For the time being I think it's fine for the client to just generate the update id and send it without user intervention. If/when a use case for user configurable metadata arises it'll be easy enough to add that support then. src/main/python/apache/aurora/client/cli/options.py (line 232) <https://reviews.apache.org/r/51384/#comment213582> do you think we should package-prefix this to avoid potentially conflicting with other metadata values in the future (i.e. org.apache.aurora.client.update_id)? src/main/python/apache/aurora/client/cli/update.py (lines 218 - 226) <https://reviews.apache.org/r/51384/#comment213585> I feel like this method should either be renamed or be side effect free. As it stands, I would not expect a call to `_generated_id_if_needed` to also mutate the `metadata` param. So, either just return the id without setting it (leaving it up to the caller) or rename to something like `_populate_id_if_needed`? src/main/python/apache/aurora/client/cli/update.py (line 237) <https://reviews.apache.org/r/51384/#comment213587> Try to avoid abbreviations in variable names, so maybe `start_update_result` instead? src/main/python/apache/aurora/client/cli/update.py (line 239) <https://reviews.apache.org/r/51384/#comment213588> s/in/in_ src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml (line 207) <https://reviews.apache.org/r/51384/#comment213589> call this prefix `jum_` for consistency with the others? src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql (lines 312 - 317) <https://reviews.apache.org/r/51384/#comment213564> You need to create a migration for this table. See https://github.com/apache/aurora/blob/master/docs/development/db-migration.md for details. src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java (line 313) <https://reviews.apache.org/r/51384/#comment213591> s/Inprogress/InProgress throughout src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java (line 1609) <https://reviews.apache.org/r/51384/#comment213592> Should this be in a finally to ensure a failure here doesn't break the rest of the tests? src/test/python/apache/aurora/client/cli/test_options.py (line 78) <https://reviews.apache.org/r/51384/#comment213593> line limit is 100 characters. I'm surprised checkstyle didn't pick this up. src/test/python/apache/aurora/client/cli/test_supdate.py (line 156) <https://reviews.apache.org/r/51384/#comment213594> Looks like there's a lot of lines beyond 100 chars here as well, can you fix? We'll have to figure out why checkstyle isn't calling those out as problematic. - Joshua Cohen On Aug. 24, 2016, 7:41 p.m., Santhosh Kumar Shanmugham wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51384/ > ------- > > (Updated Aug. 24, 2016, 7:41 p.m.) > > > Review request for Aurora, David McLaughlin, Joshua Cohen, and Maxim > Khutornenko. > > > Bugs: AURORA-1711 > https://issues.apache.org/jira/browse/AURORA-1711 > > > Repository: aurora > > > Description > --- > > Allow custom metadata to be stored with the job updates. This feature > is to help case the reconciliation on job update request on the clients. > > Today when a client's requests are proxied via a middle-man, > if the middle-man times out before the scheduler responds with success, > the client is left the only option of retry. In the case of updates, > since multiple updates are not allowed in parallel, the retries fail, > with INVALID_REQUEST. Although the original update is already accepted > by the scheduler and is in-progress. > > With this feature, clients can reconcile the job updates, > - by marking updates them with a unique id in the
Re: Review Request 51438: containerizer docu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51438/#review146848 --- Thanks for fixing this! docs/features/containers.md (line 35) <https://reviews.apache.org/r/51438/#comment213602> Would you mind also fixing this in the actual hello_docker.aurora file mentioned here? https://github.com/apache/aurora/blob/master/examples/jobs/docker/hello_docker.aurora - Joshua Cohen On Aug. 25, 2016, 8:25 p.m., Tarun Gogineni wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51438/ > --- > > (Updated Aug. 25, 2016, 8:25 p.m.) > > > Review request for Aurora and Joshua Cohen. > > > Bugs: AURORA-1754 > https://issues.apache.org/jira/browse/AURORA-1754 > > > Repository: aurora > > > Description > --- > > containerizer documentation changed to non-deprecated syntax > > > Diffs > - > > docs/features/containers.md 729a0ad2b606f47fb39902d02a7f9771b3704f39 > > Diff: https://reviews.apache.org/r/51438/diff/ > > > Testing > --- > > none needed - documentation fix > > > Thanks, > > Tarun Gogineni > >
Re: Review Request 51298: A few executor fixes for filesystem isolation:
> On Aug. 24, 2016, 11:21 a.m., Stephan Erb wrote: > > src/main/python/apache/thermos/core/process.py, line 437 > > <https://reviews.apache.org/r/51298/diff/2/?file=1482578#file1482578line437> > > > > In the `taskfs_isolated` case, `cwd` will be set to a path within the > > container, but we use it before the pivot root is completed. This is a > > problem similar to the thermos_profile issue mentioned above. > > Joshua Cohen wrote: > I'm not sure what you're getting at here? The `cwd` arg is passed on to > mesos-containerizer as the value for the `--working_directory` flag. That > value is used to `chdir` after the pivot_root/chroot logic is run. I've also > thoroughly verified that this directory is respected by the running task > (original by the `create_datafile` process in the e2e test, now replaced by > the `setup_env` process). > > Stephan Erb wrote: > Oh sorry, I highlighted the wrong line. I was referring to the cwd used > for the launching the subprocess. Ahh! Gotcha! In practice it doesn't matter since the command executed is not concerned w/ cwd, but I'll update it to use the host sandbox directory just in case. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51298/#review146619 --- On Aug. 25, 2016, 7:28 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51298/ > --- > > (Updated Aug. 25, 2016, 7:28 p.m.) > > > Review request for Aurora and Maxim Khutornenko. > > > Repository: aurora > > > Description > --- > > - Add an option to skip the groupadd/useradd calls into the task's filesystem. > - Mount any configured volumes into the task's filesystem. > - Clean up http server script used by appc e2e tests. > > > Diffs > - > > src/main/python/apache/aurora/executor/aurora_executor.py > dde19a6914c7c7b2178220707f242f61f11f38bd > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py > 65a495d5c50d91b38c4328bab3bfec667f6a7ba9 > src/main/python/apache/aurora/executor/common/sandbox.py > 5f091af7636bd94f028f15d63437e305b02f741c > src/main/python/apache/aurora/executor/thermos_task_runner.py > 1d713ca3d81a2e7be88b787dfcab328d887be24c > src/main/python/apache/thermos/core/process.py > a296fa715ef6fbb8d5feae356914334437f353f1 > src/main/python/apache/thermos/core/runner.py > dcfc190f7ed529e4f816e02b2d969077c60b008f > src/main/python/apache/thermos/runner/thermos_runner.py > 441bacdfe93b1805a03a1216762c74db810a9540 > src/test/python/apache/aurora/executor/common/test_sandbox.py > ce989b1ccda0f1bc7ba9e15dfe4be20116db3491 > src/test/python/apache/aurora/executor/test_thermos_executor.py > 06601df3bc355a690ff1789b2e2e34484fadefe9 > src/test/python/apache/thermos/core/test_process.py > 759f783202803c296ce19bb64c59cbe896d40a43 > src/test/sh/org/apache/aurora/e2e/http/http_example.aurora > b69ddf129c0015a878b089d85d731bc0c26fd55c > src/test/sh/org/apache/aurora/e2e/run-server.sh > 76939888bed2e8138671d97f7bc56fd5641008e4 > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > 0404d0e0a1a1054a5ed0c0b9f5f5be9fb3ecc113 > > Diff: https://reviews.apache.org/r/51298/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > e2e tests > > > Thanks, > > Joshua Cohen > >
Re: Review Request 51298: A few executor fixes for filesystem isolation:
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51298/ --- (Updated Aug. 25, 2016, 9:26 p.m.) Review request for Aurora and Maxim Khutornenko. Changes --- Set cwd explicitly to the host sandbox for ths subprocess call. Repository: aurora Description --- - Add an option to skip the groupadd/useradd calls into the task's filesystem. - Mount any configured volumes into the task's filesystem. - Clean up http server script used by appc e2e tests. Diffs (updated) - src/main/python/apache/aurora/executor/aurora_executor.py dde19a6914c7c7b2178220707f242f61f11f38bd src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 65a495d5c50d91b38c4328bab3bfec667f6a7ba9 src/main/python/apache/aurora/executor/common/sandbox.py 5f091af7636bd94f028f15d63437e305b02f741c src/main/python/apache/aurora/executor/thermos_task_runner.py 1d713ca3d81a2e7be88b787dfcab328d887be24c src/main/python/apache/thermos/core/process.py a296fa715ef6fbb8d5feae356914334437f353f1 src/main/python/apache/thermos/core/runner.py dcfc190f7ed529e4f816e02b2d969077c60b008f src/main/python/apache/thermos/runner/thermos_runner.py 441bacdfe93b1805a03a1216762c74db810a9540 src/test/python/apache/aurora/executor/common/test_sandbox.py ce989b1ccda0f1bc7ba9e15dfe4be20116db3491 src/test/python/apache/aurora/executor/test_thermos_executor.py 06601df3bc355a690ff1789b2e2e34484fadefe9 src/test/python/apache/thermos/core/test_process.py 759f783202803c296ce19bb64c59cbe896d40a43 src/test/sh/org/apache/aurora/e2e/http/http_example.aurora b69ddf129c0015a878b089d85d731bc0c26fd55c src/test/sh/org/apache/aurora/e2e/run-server.sh 76939888bed2e8138671d97f7bc56fd5641008e4 src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 0404d0e0a1a1054a5ed0c0b9f5f5be9fb3ecc113 Diff: https://reviews.apache.org/r/51298/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests Thanks, Joshua Cohen
Re: Review Request 51298: A few executor fixes for filesystem isolation:
> On Aug. 25, 2016, 9:48 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, lines 165-168
> > <https://reviews.apache.org/r/51298/diff/5/?file=1486010#file1486010line165>
> >
> > The `pop` is unnecessary. Luckily the Python interpreter is already
> > protecting us from this mistake
> >
> > ```
> > >>> def f(x, **kwargs):
> > ... print x
> > ...
> > >>> f(x=1, **{'x': 2})
> > Traceback (most recent call last):
> > File "", line 1, in
> > TypeError: f() got multiple values for keyword argument 'x'
> > ```
I'm not popping it for fear of it being set twice, I'm poppinig it because I
want to be sure we pass `None` for the user value for this sandbox.
> On Aug. 25, 2016, 9:48 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, lines 253-257
> > <https://reviews.apache.org/r/51298/diff/5/?file=1486010#file1486010line253>
> >
> > That `os.path.join(mesos_directory, container_path)` looks slightly
> > strange. Either we have to stip the leading `/` of container path for this
> > one as well, or the `join` is not necessary as (from the docs): 'If a
> > component is an absolute path, all previous components are thrown away and
> > joining continues from the absolute path component.'
> >
> > Optional style nitpick: I feel the function could be easier to
> > understand if we get rid of the list comprehension and move the computation
> > of the two paths to the `for` loop below, so that declaration and usage are
> > closer together.
So this was based on some confusion on my part. I was under the mistaken
impression that Mesos would always mount volumes relative to `$MESOS_DIRECTORY`
(thus why I was trying to join the paths). It was only working due to the fact
that I was not stripping the leading slash from the container path, which as
you point out causes us to throw away all previous path parts, meaning we were
mounting that absolute path which is how Mesos *actually* mounts volumes into
the container's mount namespace. Now that I've cleared that up everything works
as expected without the os.path.join.
As for the rest, thanks for the nudge, that definitely was not the cleanest
code.
> On Aug. 25, 2016, 9:48 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, lines 262-263
> > <https://reviews.apache.org/r/51298/diff/5/?file=1486010#file1486010line262>
> >
> > If you feel fancy, you could shorten this via `lstrip`:
> >
> > ```
> > >>> '/foo/bar'.lstrip('/')
> > 'foo/bar'
> > >>> 'foo/bar'.lstrip('/')
> > 'foo/bar'
> > ```
Ahh, nice, thanks!
> On Aug. 25, 2016, 9:48 p.m., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/process.py, lines 482-483
> > <https://reviews.apache.org/r/51298/diff/5/?file=1486012#file1486012line482>
> >
> > We are now sourcing the thermos profile twice: Once within the
> > container image and once on the outside.
> >
> > Probably does no harm.
I've cleaned this up so that we only source it once regardless.
- Joshua
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51298/#review146860
---
On Aug. 25, 2016, 9:26 p.m., Joshua Cohen wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51298/
> ---
>
> (Updated Aug. 25, 2016, 9:26 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> ---
>
> - Add an option to skip the groupadd/useradd calls into the task's filesystem.
> - Mount any configured volumes into the task's filesystem.
> - Clean up http server script used by appc e2e tests.
>
>
> Diffs
> -
>
> src/main/python/apache/aurora/executor/aurora_executor.py
> dde19a6914c7c7b2178220707f242f61f11f38bd
> src/main/python/apache/aurora/executor/bin/thermos_executor_main.py
> 65a495d5c50d91b38c4328bab3bfec667f6a7ba9
> src/main/python/apache/aurora/executor/common/sandbox.py
> 5f091af7636bd94f028f15d63437e305b02f741c
> src/main/python/apache/aurora/executor/thermos_task_
Re: Review Request 51298: A few executor fixes for filesystem isolation:
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51298/ --- (Updated Aug. 26, 2016, 3:52 a.m.) Review request for Aurora and Maxim Khutornenko. Changes --- Latest round of review feedback. Repository: aurora Description --- - Add an option to skip the groupadd/useradd calls into the task's filesystem. - Mount any configured volumes into the task's filesystem. - Clean up http server script used by appc e2e tests. Diffs (updated) - src/main/python/apache/aurora/executor/aurora_executor.py dde19a6914c7c7b2178220707f242f61f11f38bd src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 65a495d5c50d91b38c4328bab3bfec667f6a7ba9 src/main/python/apache/aurora/executor/common/sandbox.py 5f091af7636bd94f028f15d63437e305b02f741c src/main/python/apache/aurora/executor/thermos_task_runner.py 1d713ca3d81a2e7be88b787dfcab328d887be24c src/main/python/apache/thermos/core/process.py a296fa715ef6fbb8d5feae356914334437f353f1 src/main/python/apache/thermos/core/runner.py dcfc190f7ed529e4f816e02b2d969077c60b008f src/main/python/apache/thermos/runner/thermos_runner.py 441bacdfe93b1805a03a1216762c74db810a9540 src/test/python/apache/aurora/executor/common/test_sandbox.py ce989b1ccda0f1bc7ba9e15dfe4be20116db3491 src/test/python/apache/aurora/executor/test_thermos_executor.py 06601df3bc355a690ff1789b2e2e34484fadefe9 src/test/python/apache/thermos/core/test_process.py 759f783202803c296ce19bb64c59cbe896d40a43 src/test/sh/org/apache/aurora/e2e/http/http_example.aurora b69ddf129c0015a878b089d85d731bc0c26fd55c src/test/sh/org/apache/aurora/e2e/run-server.sh 76939888bed2e8138671d97f7bc56fd5641008e4 src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 0404d0e0a1a1054a5ed0c0b9f5f5be9fb3ecc113 Diff: https://reviews.apache.org/r/51298/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests Thanks, Joshua Cohen
Re: Review Request 51438: containerizer docu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51438/#review146999 --- Ship it! Thanks! - Joshua Cohen On Aug. 26, 2016, 6:14 p.m., Tarun Gogineni wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51438/ > --- > > (Updated Aug. 26, 2016, 6:14 p.m.) > > > Review request for Aurora and Joshua Cohen. > > > Bugs: AURORA-1754 > https://issues.apache.org/jira/browse/AURORA-1754 > > > Repository: aurora > > > Description > --- > > containerizer documentation and example changed to non-deprecated syntax > > > Diffs > - > > docs/features/containers.md 729a0ad2b606f47fb39902d02a7f9771b3704f39 > examples/jobs/docker/hello_docker.aurora > 47adb3932323120e62649f43353719b76c48a963 > > Diff: https://reviews.apache.org/r/51438/diff/ > > > Testing > --- > > new aurora file was tested > > > Thanks, > > Tarun Gogineni > >
Re: Review Request 51484: Re-enable python style check in the integration build.
> On Aug. 29, 2016, 4:39 p.m., John Sirois wrote: > > I'm taking a look presently. The seperate script call-out should no longer > > be needed since `./pants test...` implies `./pants compile...` which is all > > the seperate script does. > > John Sirois wrote: > Aha - yes, the compile dependency was historically leeched from the jvm > stack which is now not installed. LGTM. > > John Sirois wrote: > Filed pants issue to fix this: > https://github.com/pantsbuild/pants/issues/3819 I was just talking to Stu Hood in pants slack about this. He came to the same conclusion (removing the JVM backend caused test to no longer be linked to compile). He suggested we might want to install a plugin to redirect pythonstyle to the test goal instead. I think that this solution is probably ok for my use cases (checking style before posting a review and having ReviewBot catch style issues for me so I don't have to call them out myself on other reviews), but wanted to raise the option for those who might like to have checkstyle run during the local dev flow of edit/test/repeat. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51484/#review147139 --- On Aug. 28, 2016, 4:40 a.m., Santhosh Kumar Shanmugham wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51484/ > --- > > (Updated Aug. 28, 2016, 4:40 a.m.) > > > Review request for Aurora, John Sirois and Zameer Manji. > > > Repository: aurora > > > Description > --- > > Re-enable python style check in the integration build. > > pants test does not appear to invoke the python checkstyle. Re-enable > it by explicitly calling in the integration build script. Also fix the > few issues that have already been commited. > > > Diffs > - > > build-support/jenkins/build.sh 1de144628f0e687eb3a191b0aa819bae018a8f37 > src/main/python/apache/aurora/client/cli/context.py > f1a256a8d09d23d8d4d4ee7d264be0fe376398c4 > src/main/python/apache/aurora/client/cli/update.py > 23aaa2c1b67599420408633733e4581553f7151b > src/main/python/apache/thermos/core/process.py > 78e7d788f14a1611031e3c6e255c77768daabac4 > src/test/python/apache/aurora/executor/common/test_sandbox.py > b30a5bc733ac5ccace0b93b68e2ee6f04ce31184 > src/test/python/apache/thermos/core/test_process.py > 49f52d947471c8ea8daae1708f4d26afa4c6ee0a > > Diff: https://reviews.apache.org/r/51484/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > > > Thanks, > > Santhosh Kumar Shanmugham > >
Re: Review Request 51484: Re-enable python style check in the integration build.
> On Aug. 29, 2016, 4:39 p.m., John Sirois wrote: > > I'm taking a look presently. The seperate script call-out should no longer > > be needed since `./pants test...` implies `./pants compile...` which is all > > the seperate script does. > > John Sirois wrote: > Aha - yes, the compile dependency was historically leeched from the jvm > stack which is now not installed. LGTM. > > John Sirois wrote: > Filed pants issue to fix this: > https://github.com/pantsbuild/pants/issues/3819 > > Joshua Cohen wrote: > I was just talking to Stu Hood in pants slack about this. He came to the > same conclusion (removing the JVM backend caused test to no longer be linked > to compile). He suggested we might want to install a plugin to redirect > pythonstyle to the test goal instead. > > I think that this solution is probably ok for my use cases (checking > style before posting a review and having ReviewBot catch style issues for me > so I don't have to call them out myself on other reviews), but wanted to > raise the option for those who might like to have checkstyle run during the > local dev flow of edit/test/repeat. > > John Sirois wrote: > Yeah - I'm happy to follow-up with a plugin to shim checkstyle into the > `./pants test...` workflow, but in the past custom pants plugins for Aurora > were frowned upon. Yeah, I'm fine to commit this change and only add the plugin if someone feels strongly that hooking back into `./pants test ...` is critical. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51484/#review147139 --- On Aug. 28, 2016, 4:40 a.m., Santhosh Kumar Shanmugham wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51484/ > --- > > (Updated Aug. 28, 2016, 4:40 a.m.) > > > Review request for Aurora, John Sirois and Zameer Manji. > > > Repository: aurora > > > Description > --- > > Re-enable python style check in the integration build. > > pants test does not appear to invoke the python checkstyle. Re-enable > it by explicitly calling in the integration build script. Also fix the > few issues that have already been commited. > > > Diffs > - > > build-support/jenkins/build.sh 1de144628f0e687eb3a191b0aa819bae018a8f37 > src/main/python/apache/aurora/client/cli/context.py > f1a256a8d09d23d8d4d4ee7d264be0fe376398c4 > src/main/python/apache/aurora/client/cli/update.py > 23aaa2c1b67599420408633733e4581553f7151b > src/main/python/apache/thermos/core/process.py > 78e7d788f14a1611031e3c6e255c77768daabac4 > src/test/python/apache/aurora/executor/common/test_sandbox.py > b30a5bc733ac5ccace0b93b68e2ee6f04ce31184 > src/test/python/apache/thermos/core/test_process.py > 49f52d947471c8ea8daae1708f4d26afa4c6ee0a > > Diff: https://reviews.apache.org/r/51484/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > > > Thanks, > > Santhosh Kumar Shanmugham > >
Re: Review Request 51384: Introduce UpdateMetadata fields in JobUpdateRequest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51384/#review147184 --- Looks good to me, only one issue remaining from my perspective. src/test/python/apache/aurora/client/cli/test_supdate.py (line 153) <https://reviews.apache.org/r/51384/#comment214283> since we're mocking this anyway, no real need to generate an actual uuid, can just have the mock return something like 'some-mocked-uuid'. That'll make it a little bit more clear, in the event of a failure, that the value was a mock and not coming from live code. src/main/python/apache/aurora/client/cli/update.py (line 193) <https://reviews.apache.org/r/51384/#comment214284> Am I missing something? Shouldn't this be a set of `Metadata` structs? rather than a set of strings which is what this will currently pass? - Joshua Cohen On Aug. 28, 2016, 3:58 a.m., Santhosh Kumar Shanmugham wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51384/ > --- > > (Updated Aug. 28, 2016, 3:58 a.m.) > > > Review request for Aurora, David McLaughlin, Joshua Cohen, and Maxim > Khutornenko. > > > Bugs: AURORA-1711 > https://issues.apache.org/jira/browse/AURORA-1711 > > > Repository: aurora > > > Description > --- > > Allow custom metadata to be stored with the job updates. This feature > is to help case the reconciliation on job update request on the clients. > > Today when a client's requests are proxied via a middle-man, > if the middle-man times out before the scheduler responds with success, > the client is left the only option of retry. In the case of updates, > since multiple updates are not allowed in parallel, the retries fail, > with INVALID_REQUEST. Although the original update is already accepted > by the scheduler and is in-progress. > > With this feature, clients can reconcile the job updates, > - by marking updates them with a unique id in the metadata field > - scheduler returns the in-progress job update in its response > - client can match the client-generated ids to reconcile its state. > > > Diffs > - > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > c5765b70501c101f0535b4eed94e9948c36808f9 > src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java > d2673e6b328cb1e249fbe91d18e0d9e935636eaa > > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java > a3b04949f1726f110638e4f93ef45947cdb9e7f8 > > src/main/java/org/apache/aurora/scheduler/storage/db/migration/V008_CreateUpdateMetadataTable.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/storage/db/views/DbJobUpdate.java > 78703e92c932cd5e93ff0b70f2a0b6928f6b4003 > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > 929d021a336c6a3438613c9340c84a1096dc9069 > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java > 594bb6219294dcc77d48dcad14e2a6f9caa0c534 > > src/main/java/org/apache/aurora/scheduler/updater/UpdateInProgressException.java > PRE-CREATION > src/main/python/apache/aurora/client/api/__init__.py > 9149c3018ae58d405f284fcbd4076d251ccc8192 > src/main/python/apache/aurora/client/cli/context.py > f1a256a8d09d23d8d4d4ee7d264be0fe376398c4 > src/main/python/apache/aurora/client/cli/options.py > 1245ff15a69a4b4347672f7b556985521e813a00 > src/main/python/apache/aurora/client/cli/update.py > 23aaa2c1b67599420408633733e4581553f7151b > src/main/python/apache/aurora/client/hooks/hooked_api.py > 542f165add0f1d01a782fe6d6089bff3e736fb82 > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml > 8563630a179cb6d1e3b0e22c730ccbfe1c9291e2 > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql > a40830fd668aa650c22a48cbc757b45aef27e961 > > src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java > 08530397ff75081bde6f07f9d53317b5486e0da4 > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > 779dc302602ae8842084807ca868a491ea99b676 > src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java > 04551f17999d742c53dfb1b36286b119b448550e > src/test/python/apache/aurora/client/cli/test_supdate.py > 92fb039fb7d3e368d7c0dfa5ebdb465c7f112416 > src/test/python/apache/aurora/client/cli/util.py > aac9f9c802e4ad1f06cee8cf3f56749760b33af5 > > Diff: https://reviews.apache.org/r/51384/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > ./pants test src/test/python/apache/aurora/client/cli:cli > > > Thanks, > > Santhosh Kumar Shanmugham > >
Review Request 51500: Configure ssh for e2e tests once globally, rather than as part of a specific test case.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51500/ --- Review request for Aurora and Stephan Erb. Repository: aurora Description --- Configure ssh for e2e tests once globally, rather than as part of a specific test case. Diffs - src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 1a668dd9e98472bdf0deda840868813a5c3dcef2 Diff: https://reviews.apache.org/r/51500/diff/ Testing --- ran e2e tests on a clean vagrant environment. Thanks, Joshua Cohen
Review Request 51502: Clean up leaking of mounts into the host's mtab.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51502/ --- Review request for Aurora and Stephan Erb. Repository: aurora Description --- Clean up leaking of mounts into the host's mtab. Diffs - src/main/python/apache/aurora/executor/common/sandbox.py cedcab68f31bb0a514ab42ccebc88ab2c7470d1c src/test/python/apache/aurora/executor/common/test_sandbox.py 2ba3341273efe54d1a79fe48be50d4253530dcbc src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 7c32c94edf88d6dc0fbd7f838bea2b8077da0235 Diff: https://reviews.apache.org/r/51502/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests Thanks, Joshua Cohen
Re: Review Request 51502: Clean up leaking of mounts into the host's mtab.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51502/ --- (Updated Aug. 29, 2016, 8:28 p.m.) Review request for Aurora and Stephan Erb. Changes --- Fix missed test case. Repository: aurora Description --- Clean up leaking of mounts into the host's mtab. Diffs (updated) - src/main/python/apache/aurora/executor/common/sandbox.py cedcab68f31bb0a514ab42ccebc88ab2c7470d1c src/test/python/apache/aurora/executor/common/test_sandbox.py 2ba3341273efe54d1a79fe48be50d4253530dcbc src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 7c32c94edf88d6dc0fbd7f838bea2b8077da0235 Diff: https://reviews.apache.org/r/51502/diff/ Testing --- ./build-support/jenkins/build.sh e2e tests Thanks, Joshua Cohen
Re: Review Request 51502: Clean up leaking of mounts into the host's mtab.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51502/#review147208 --- @ReviewBot retry - Joshua Cohen On Aug. 29, 2016, 8:28 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51502/ > --- > > (Updated Aug. 29, 2016, 8:28 p.m.) > > > Review request for Aurora and Stephan Erb. > > > Repository: aurora > > > Description > --- > > Clean up leaking of mounts into the host's mtab. > > > Diffs > - > > src/main/python/apache/aurora/executor/common/sandbox.py > cedcab68f31bb0a514ab42ccebc88ab2c7470d1c > src/test/python/apache/aurora/executor/common/test_sandbox.py > 2ba3341273efe54d1a79fe48be50d4253530dcbc > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > 7c32c94edf88d6dc0fbd7f838bea2b8077da0235 > > Diff: https://reviews.apache.org/r/51502/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > e2e tests > > > Thanks, > > Joshua Cohen > >
Re: Review Request 51384: Introduce UpdateMetadata fields in JobUpdateRequest.
> On Aug. 29, 2016, 6:52 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/client/cli/update.py, line 193
> > <https://reviews.apache.org/r/51384/diff/4/?file=1487511#file1487511line193>
> >
> > Am I missing something? Shouldn't this be a set of `Metadata` structs?
> > rather than a set of strings which is what this will currently pass?
>
> Santhosh Kumar Shanmugham wrote:
> You are totally correct. Is there any way to make the python tests do
> type-checking?
>
> This however makes the "assert mock_calls" on mock_api to fail. python's
> assert, TestCase.assertEquals and testfixtures.compare all are unable to deal
> with Metadata object. Any ideas around this would be welcome.
I think what I would recommend is updating `_job_update_request` to take a dict
of metadata key/value pairs and performing the thrift conversion there. That
has the benefit of encapsulating knowledge of the thrift API within the API
client. It also means you should be able to assert on the dict that's passed in
test_supdate?
That said, simple equality on the generated thrift types should work? E.g.
$ PEX_INTERPRETER=1 aurora
>>> from gen.apache.aurora.api.ttypes import Metadata
>>> a = Metadata('foo', 'bar')
>>> b = Metadata('foo', 'bar')
>>> a == b
True
>>> c = Metadata('baz', 'qux')
>>> a == c
False
- Joshua
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51384/#review147184
---
On Aug. 28, 2016, 3:58 a.m., Santhosh Kumar Shanmugham wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51384/
> ---
>
> (Updated Aug. 28, 2016, 3:58 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Maxim
> Khutornenko.
>
>
> Bugs: AURORA-1711
> https://issues.apache.org/jira/browse/AURORA-1711
>
>
> Repository: aurora
>
>
> Description
> ---
>
> Allow custom metadata to be stored with the job updates. This feature
> is to help case the reconciliation on job update request on the clients.
>
> Today when a client's requests are proxied via a middle-man,
> if the middle-man times out before the scheduler responds with success,
> the client is left the only option of retry. In the case of updates,
> since multiple updates are not allowed in parallel, the retries fail,
> with INVALID_REQUEST. Although the original update is already accepted
> by the scheduler and is in-progress.
>
> With this feature, clients can reconcile the job updates,
> - by marking updates them with a unique id in the metadata field
> - scheduler returns the in-progress job update in its response
> - client can match the client-generated ids to reconcile its state.
>
>
> Diffs
> -
>
> api/src/main/thrift/org/apache/aurora/gen/api.thrift
> c5765b70501c101f0535b4eed94e9948c36808f9
> src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa
>
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
> a3b04949f1726f110638e4f93ef45947cdb9e7f8
>
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V008_CreateUpdateMetadataTable.java
> PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbJobUpdate.java
> 78703e92c932cd5e93ff0b70f2a0b6928f6b4003
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
> 929d021a336c6a3438613c9340c84a1096dc9069
>
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
> 594bb6219294dcc77d48dcad14e2a6f9caa0c534
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateInProgressException.java
> PRE-CREATION
> src/main/python/apache/aurora/client/api/__init__.py
> 9149c3018ae58d405f284fcbd4076d251ccc8192
> src/main/python/apache/aurora/client/cli/context.py
> f1a256a8d09d23d8d4d4ee7d264be0fe376398c4
> src/main/python/apache/aurora/client/cli/options.py
> 1245ff15a69a4b4347672f7b556985521e813a00
> src/main/python/apache/aurora/client/cli/update.py
> 23aaa2c1b67599420408633733e4581553f7151b
> src/main/python/apache/aurora/client/hooks/hooked_api.py
> 542f165add0f1d01a782fe6d6089bff3e736fb82
>
> src/main/resources/org/apache/aurora
Re: Review Request 51513: Add support for ETags in the Aurora API.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51513/#review147255 --- src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java (lines 139 - 142) <https://reviews.apache.org/r/51513/#comment214370> Am I reading this correctly: you're buffering the entire response in memory so we can calculate the hash for the etag? Do you envision this causing memory pressure for larger responses (especially given that the goal here is to optimize for the case where responses are large enough that sending them on the wire and processing them on the client side is causing a perf problem)? At the very least, perhaps we should only buffer the response if the `If-None-Match` header is set? - Joshua Cohen On Aug. 30, 2016, 1:12 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51513/ > --- > > (Updated Aug. 30, 2016, 1:12 a.m.) > > > Review request for Aurora, Joshua Cohen and Stephan Erb. > > > Bugs: AURORA-1757 > https://issues.apache.org/jira/browse/AURORA-1757 > > > Repository: aurora > > > Description > --- > > This patch enhances the Aurora API by producing an `ETag` header for each API > request. It also consumes etag values in API requests via the `If-None-Match` > header and produces a HTTP 304 if the request would produce a body with the > same > etag value. > > > Diffs > - > > RELEASE-NOTES.md 1819eaa20cf5014228643a1e120316d646cc2824 > > src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java > 1634cb88ac09c778c5bb277ca902f4ca35dd6c9d > src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java > 0a3ff05586c87e0ab2cc20470e99b5dd609f7039 > > Diff: https://reviews.apache.org/r/51513/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 51384: Introduce UpdateMetadata fields in JobUpdateRequest.
> On Aug. 29, 2016, 6:52 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/client/cli/update.py, line 193
> > <https://reviews.apache.org/r/51384/diff/4/?file=1487511#file1487511line193>
> >
> > Am I missing something? Shouldn't this be a set of `Metadata` structs?
> > rather than a set of strings which is what this will currently pass?
>
> Santhosh Kumar Shanmugham wrote:
> You are totally correct. Is there any way to make the python tests do
> type-checking?
>
> This however makes the "assert mock_calls" on mock_api to fail. python's
> assert, TestCase.assertEquals and testfixtures.compare all are unable to deal
> with Metadata object. Any ideas around this would be welcome.
>
> Joshua Cohen wrote:
> I think what I would recommend is updating `_job_update_request` to take
> a dict of metadata key/value pairs and performing the thrift conversion
> there. That has the benefit of encapsulating knowledge of the thrift API
> within the API client. It also means you should be able to assert on the dict
> that's passed in test_supdate?
>
> That said, simple equality on the generated thrift types should work? E.g.
>
> $ PEX_INTERPRETER=1 aurora
> >>> from gen.apache.aurora.api.ttypes import Metadata
> >>> a = Metadata('foo', 'bar')
> >>> b = Metadata('foo', 'bar')
> >>> a == b
> True
> >>> c = Metadata('baz', 'qux')
> >>> a == c
> False
>
> Santhosh Kumar Shanmugham wrote:
> Items of a set are not getting deep equality check. Is there an
> assertDeepEquals() ?
>
> $ PEX_INTERPRETER=1 aurora
> >>> from gen.apache.aurora.api.ttypes import Metadata
> >>> a = Metadata('foo', 'bar')
> >>> b = Metadata('foo', 'bar')
> >>> a == b
> True
> >>> {a} == {b}
> False
> >>> set([a]) == set([b])
> False
> >>> set([a]) == set([a])
> True
Ahh, the thrift python compiler did not generate `__hash__` methods until this
commit:
https://github.com/apache/thrift/commit/e841b3dac619a5e5d3523d059d48db1a12e41360,
which does not seem to have made it into an official release yet (not that it
would matter if it did, since we're pinned to an older version of thrift for
reasons I cannot recall at the moment).
So I think you'd have to iterate the sets to check for equality (which would be
made more complicated by the fact that ordering is not guaranteed). All the
more reason to just bury the set of thrift structs internal to the API impl and
just pass a dict instead?
- Joshua
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51384/#review147184
-------
On Aug. 28, 2016, 3:58 a.m., Santhosh Kumar Shanmugham wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51384/
> ---
>
> (Updated Aug. 28, 2016, 3:58 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Maxim
> Khutornenko.
>
>
> Bugs: AURORA-1711
> https://issues.apache.org/jira/browse/AURORA-1711
>
>
> Repository: aurora
>
>
> Description
> ---
>
> Allow custom metadata to be stored with the job updates. This feature
> is to help case the reconciliation on job update request on the clients.
>
> Today when a client's requests are proxied via a middle-man,
> if the middle-man times out before the scheduler responds with success,
> the client is left the only option of retry. In the case of updates,
> since multiple updates are not allowed in parallel, the retries fail,
> with INVALID_REQUEST. Although the original update is already accepted
> by the scheduler and is in-progress.
>
> With this feature, clients can reconcile the job updates,
> - by marking updates them with a unique id in the metadata field
> - scheduler returns the in-progress job update in its response
> - client can match the client-generated ids to reconcile its state.
>
>
> Diffs
> -
>
> api/src/main/thrift/org/apache/aurora/gen/api.thrift
> c5765b70501c101f0535b4eed94e9948c36808f9
> src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java
> d2673e6b328cb1e249fbe91d18e0d9e93563
Re: Review Request 51506: Enable -zk_use_curator by default and deprecate.
> On Aug. 30, 2016, 6:23 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java, > > line 78 > > <https://reviews.apache.org/r/51506/diff/1/?file=1488296#file1488296line78> > > > > I suggest against placing a version number into the warning message. > > We've had a few past examples when we did not follow up on deprecation > > warnings as promised. +1 to this. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51506/#review147326 --- On Aug. 29, 2016, 11:37 p.m., John Sirois wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51506/ > --- > > (Updated Aug. 29, 2016, 11:37 p.m.) > > > Review request for Aurora, Joshua Cohen, Stephan Erb, and Zameer Manji. > > > Bugs: AURORA-1669 > https://issues.apache.org/jira/browse/AURORA-1669 > > > Repository: aurora > > > Description > --- > > The flag is noted as deprecated for removal in 0.17.0. > > RELEASE-NOTES.md >| 2 ++ > docs/reference/scheduler-configuration.md >| 4 ++-- > examples/vagrant/upstart/aurora-scheduler.conf >| 1 - > > src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java > | 11 +-- > 4 files changed, 13 insertions(+), 5 deletions(-) > > > Diffs > - > > RELEASE-NOTES.md 1819eaa20cf5014228643a1e120316d646cc2824 > docs/reference/scheduler-configuration.md > c0c39442725c970e5f9811cd7b4ab3104364c671 > examples/vagrant/upstart/aurora-scheduler.conf > dd60981564286e5b25546737fdd0ce08b0168ed4 > > src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java > 36ad18c49c5693031136440ab163070f9ffa9405 > > Diff: https://reviews.apache.org/r/51506/diff/ > > > Testing > --- > > Locally green: > ``` > ./gradlew -Pq build > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > ``` > > > Thanks, > > John Sirois > >
Re: Review Request 51531: Minor improvements to the custom executor docs
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51531/#review147329 --- Ship it! Ship It! - Joshua Cohen On Aug. 30, 2016, 5:45 p.m., Stephan Erb wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51531/ > --- > > (Updated Aug. 30, 2016, 5:45 p.m.) > > > Review request for Aurora and Joshua Cohen. > > > Repository: aurora > > > Description > --- > > * Add link from README.md to features/custom-executors.md > * Add short introduction paragraph to features/custom-executors.md > > > Diffs > - > > RELEASE-NOTES.md 1819eaa20cf5014228643a1e120316d646cc2824 > docs/README.md 7cea18f6a62a8dc51b593243709d841b32d3238b > docs/features/custom-executors.md 7438938d29901e0d69389288ac592f06c3c698c0 > docs/operations/configuration.md 15ec042ff4035abc07e1a73fe52f418695ff56f0 > > Diff: https://reviews.apache.org/r/51531/diff/ > > > Testing > --- > > Rendered version available at > https://github.com/StephanErb/aurora/tree/exec-doc-links/docs > > > Thanks, > > Stephan Erb > >
Re: Review Request 51513: Add support for ETags in the Aurora API.
> On Aug. 30, 2016, 1:32 a.m., Joshua Cohen wrote: > > src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java, > > lines 139-142 > > <https://reviews.apache.org/r/51513/diff/1/?file=1488357#file1488357line139> > > > > Am I reading this correctly: you're buffering the entire response in > > memory so we can calculate the hash for the etag? Do you envision this > > causing memory pressure for larger responses (especially given that the > > goal here is to optimize for the case where responses are large enough that > > sending them on the wire and processing them on the client side is causing > > a perf problem)? > > > > At the very least, perhaps we should only buffer the response if the > > `If-None-Match` header is set? > > Zameer Manji wrote: > If the `If-None-Match` header is not set, we need to compute the `ETag` > header for the response anyways so the client could send it in a future > request. > If it is set, we need to compute it to compare against the value of the > `If-None-Match` to determine if we should send a 304 or not. > > I understand your concern with buffering responses. I have no information > right now if this will change memory allocation patterns or not, but I doubt > it will cause a huge impact. We don't even `flush` the output stream of the > response here so nothing says we aren't already buffering the response. > > If the memory performance is a big concern, I could wrap the outputstream > of the response and have that compute the hash as bytes are given. If we can stream the bytes into the hash calculation rather than buffering, that would be ideal. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51513/#review147255 --- On Aug. 30, 2016, 1:12 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51513/ > --- > > (Updated Aug. 30, 2016, 1:12 a.m.) > > > Review request for Aurora, Joshua Cohen and Stephan Erb. > > > Bugs: AURORA-1757 > https://issues.apache.org/jira/browse/AURORA-1757 > > > Repository: aurora > > > Description > --- > > This patch enhances the Aurora API by producing an `ETag` header for each API > request. It also consumes etag values in API requests via the `If-None-Match` > header and produces a HTTP 304 if the request would produce a body with the > same > etag value. > > > Diffs > - > > RELEASE-NOTES.md 1819eaa20cf5014228643a1e120316d646cc2824 > > src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java > 1634cb88ac09c778c5bb277ca902f4ca35dd6c9d > src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java > 0a3ff05586c87e0ab2cc20470e99b5dd609f7039 > > Diff: https://reviews.apache.org/r/51513/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
