Re: Review Request 49478: Fixing e2e tests failing due to mesos-slave state.

2016-06-30 Thread Joshua Cohen 

---
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 49478: Fixing e2e tests failing due to mesos-slave state.

2016-06-30 Thread Aurora ReviewBot 

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


Ship it!




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

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

- Aurora ReviewBot


On 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 49478: Fixing e2e tests failing due to mesos-slave state.

2016-06-30 Thread Maxim Khutornenko 

---
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.


Changes
---

Removed spaces and moved rm after stopping slave.


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 (updated)
-

  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 49478: Fixing e2e tests failing due to mesos-slave state.

2016-06-30 Thread Aurora ReviewBot 

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



Master (be2174b) is red with this patch.
  ./build-support/jenkins/build.sh

   with temporary_dir() as checkpoint_root:
 te = AuroraExecutor(
 >   
runner_provider=make_provider(checkpoint_root),
 
sandbox_provider=DefaultTestSandboxProvider())
 
 
src/test/python/apache/aurora/executor/test_thermos_executor.py:580: 
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
src/test/python/apache/aurora/executor/test_thermos_executor.py:193: in 
make_provider
 pex_location=thermos_runner_path(),
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
 build = True
 
 def thermos_runner_path(build=True):
   if not build:
 return getattr(thermos_runner_path, 'value', 
None)
 
   if not hasattr(thermos_runner_path, 'value'):
 pex_dir = safe_mkdtemp()
 >   assert subprocess.call(["./pants", 
"--pants-distdir=%s" % pex_dir, "binary",
   
"src/main/python/apache/thermos/runner:thermos_runner"]) == 0
 E   assert 1 == 0
 E+  where 1 = (['./pants', '--pants-distdir=/tmp/user/10021/tmpvyKaKZ', 
'binary', 'src/main/python/apache/thermos/runner:thermos_runner'])
 E+where  = subprocess.call
 
 
src/test/python/apache/aurora/executor/test_thermos_executor.py:185: 
AssertionError
 -- Captured stderr call --
 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
  generated xml file: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/test-results/415337499eb72578eab327a6487c1f5c9452b3d6.xml
 
  16 failed, 638 passed, 6 skipped, 1 warnings, 8 
error in 102.35 seconds 
 
FAILURE


   Waiting for background workers to finish.
23:24:28 02:26   [complete]
   FAILURE


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

- Aurora ReviewBot


On June 30, 2016, 11:15 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:15 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
> 
>

Review Request 49478: Fixing e2e tests failing due to mesos-slave state.

2016-06-30 Thread Maxim Khutornenko 

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

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 49399: Fix Process log configuration handling.

2016-06-30 Thread John Sirois 


> On June 30, 2016, 7:39 a.m., Stephan Erb wrote:
> > Your fix looks correct. Also thanks for investing the time for the test!
> > 
> > However, I kind of feel that the overall feature is more complicated than 
> > it should be. Two ideas below, feedback welcome.

My thinking is outlined below.
Although I don't think it affects the discussion materially I wanted to point 
out a premise I'm working under is that the pystachio schema should be the 
single source of default values and enum values.


> On June 30, 2016, 7:39 a.m., Stephan Erb wrote:
> > docs/reference/configuration.md, line 144
> > 
> >
> > Have you considered using `RotatePolicy()` as the default rather than 
> > `Empty`? This could relieve us from having to handle the assigned and 
> > unassigned case in the backend, and thus reduce overall code complexity.

I did. The downside here is that the configuration becomes confusing to read. A 
user can now see this in the Observer UI (or the Aurora UI as an escaped json 
string):
```json
{
"processes": [
{
"daemon": false, 
"name": "hello", 
"max_failures": 1, 
"ephemeral": false, 
"min_duration": 5, 
"cmdline": "\nwhile true; do\n  echo hello world\n  
sleep 10\ndone\n  ", 
"logger": {
"destination": "console", 
"rotate": {
"backups": 5, 
"log_size": 104857600
}, 
"mode": "standard"
}, 
"final": false
}
], 
"name": "hello", 
"finalization_wait": 30, 
"max_failures": 1, 
"max_concurrency": 0, 
"resources": {
"gpu": 0, 
"disk": 134217728, 
"ram": 134217728, 
"cpu": 1.0
}, 
"constraints": [
{
"order": [
"hello"
]
}
]
}
```

Flags aside (discussed below), the problem would be alleviated by the pystachio 
schema using a single `mode = Choice(Standard, Rotate)` field (Choice did not 
exist when the feature was written).  Here `Standard` would be an empty 
`Struct` and `Rotate` would be a rename of `Rotate` policy, so, in full:
```python
class Logger(Struct):
  destination = Default(LoggerDestination, LoggerDestination('file'))
  mode = Default(Choice(Standard, Rotate), Standard())
```

Perhaps this to allow a deprecation period?:
```python
Rotate = RotatePolicy

class Logger(Struct):
  destination = Default(LoggerDestination, LoggerDestination('file'))
  mode = Default(Choice(LoggerMode, Standard, Rotate), Standard())
  rotate = RotatePolicy
```

The code would remain complex (grow more complex) in the short term while 
handling old and new styles, but once the deprecated `rotate` field and 
`LoggerMode` `Choice` are removed, the code would then be able to simplify from 
what exists today.  Perhaps more importantly, the configuration for end users 
would simplify - the `mode` field would no longer be ~redundant.


> On June 30, 2016, 7:39 a.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py, lines 
> > 125-146
> > 
> >
> > I feel like we can reduce the cyclomatic complexity in 
> > `_build_process_logger_args` if we assign default arugments here, rather 
> > than later in the code.

I think this would be less defaulting than you envisioned due to the same odd 
UI issue as shown above in tak config json presents in CLI help output.
I could clearly default `--runner-logger-destination` using 
~`default=Logger().destination().get()`, but then you run into both UI and 
seperation of concerns issues for the next 3.


- John


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


On June 29, 2016, 5 p.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49399/
> ---
> 
> (Updated June 29, 2016, 5 p.m.)
> 
> 
> Review request for Aurora, George Sirois, Martin Hrabovcin, and Stephan Erb.
> 
> 
> Bugs: AURORA-1724
> https://issues.apache.org/jira/browse/AURORA-1724
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Previously flagged configuration of Process logging mode would
> blow up and claimed defaulting of the rotation policy did not
> occur.
> 
>  docs/operations/configuration.md|   9 +-
>  docs/reference/configuration.md |  33 
> 
>

Re: Review Request 49399: Fix Process log configuration handling.

2016-06-30 Thread Stephan Erb 


> On June 30, 2016, 12:58 a.m., John Sirois wrote:
> > src/main/python/apache/thermos/core/runner.py, line 750
> > 
> >
> > NB: The trailing comma here on the LHS formed a 1-tuple leading to the 
> > initial bug report (wrong type).

D'oh! Good catch


- Stephan


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


On June 30, 2016, 1 a.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49399/
> ---
> 
> (Updated June 30, 2016, 1 a.m.)
> 
> 
> Review request for Aurora, George Sirois, Martin Hrabovcin, and Stephan Erb.
> 
> 
> Bugs: AURORA-1724
> https://issues.apache.org/jira/browse/AURORA-1724
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Previously flagged configuration of Process logging mode would
> blow up and claimed defaulting of the rotation policy did not
> occur.
> 
>  docs/operations/configuration.md|   9 +-
>  docs/reference/configuration.md |  33 
> 
>  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py |  21 
> ++---
>  src/main/python/apache/thermos/core/runner.py   |  40 
> ++---
>  src/main/python/apache/thermos/testing/runner.py|  16 
> ++--
>  src/test/python/apache/thermos/core/BUILD   |   2 +
>  src/test/python/apache/thermos/core/test_runner_log_config.py   | 230 
> ++
>  7 files changed, 293 insertions(+), 58 deletions(-)
> 
> 
> Diffs
> -
> 
>   docs/operations/configuration.md e332f864aeb25790f860bc1961a7a2c49b29004e 
>   docs/reference/configuration.md c4b1d387435e4b8fd59a296bd5d6d203bb50cbde 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 203fc47d74840889a1192dc867fef5584b704685 
>   src/main/python/apache/thermos/core/runner.py 
> 3ebf86ebd12ed3b68f543d4b9a45615e4681ba7f 
>   src/main/python/apache/thermos/testing/runner.py 
> 8b6ba730acedf284a61e4bde60ab480950f92ede 
>   src/test/python/apache/thermos/core/BUILD 
> acfb79b6c411744be7f8b3bb364f0c8d049f1694 
>   src/test/python/apache/thermos/core/test_runner_log_config.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49399/diff/
> 
> 
> Testing
> ---
> 
> Manually tested the cases in
>   https://issues.apache.org/jira/browse/AURORA-1724.
> 
> Also ran the following green locally:
> ```
> ./pants test src/test/python/apache/thermos/core
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ```
> 
> Doc edits are rendered here:
>   
> https://github.com/jsirois/aurora/blob/jsirois/issues/AURORA-1724/docs/reference/configuration.md#logger
>   
> https://github.com/jsirois/aurora/blob/jsirois/issues/AURORA-1724/docs/operations/configuration.md#process-logs
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Re: Review Request 49399: Fix Process log configuration handling.

2016-06-30 Thread Stephan Erb 

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



Your fix looks correct. Also thanks for investing the time for the test!

However, I kind of feel that the overall feature is more complicated than it 
should be. Two ideas below, feedback welcome.


docs/reference/configuration.md (line 140)


Have you considered using `RotatePolicy()` as the default rather than 
`Empty`? This could relieve us from having to handle the assigned and 
unassigned case in the backend, and thus reduce overall code complexity.



src/main/python/apache/aurora/executor/bin/thermos_executor_main.py (lines 120 
- 139)


I feel like we can reduce the cyclomatic complexity in 
`_build_process_logger_args` if we assign default arugments here, rather than 
later in the code.


- Stephan Erb


On June 30, 2016, 1 a.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49399/
> ---
> 
> (Updated June 30, 2016, 1 a.m.)
> 
> 
> Review request for Aurora, George Sirois, Martin Hrabovcin, and Stephan Erb.
> 
> 
> Bugs: AURORA-1724
> https://issues.apache.org/jira/browse/AURORA-1724
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Previously flagged configuration of Process logging mode would
> blow up and claimed defaulting of the rotation policy did not
> occur.
> 
>  docs/operations/configuration.md|   9 +-
>  docs/reference/configuration.md |  33 
> 
>  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py |  21 
> ++---
>  src/main/python/apache/thermos/core/runner.py   |  40 
> ++---
>  src/main/python/apache/thermos/testing/runner.py|  16 
> ++--
>  src/test/python/apache/thermos/core/BUILD   |   2 +
>  src/test/python/apache/thermos/core/test_runner_log_config.py   | 230 
> ++
>  7 files changed, 293 insertions(+), 58 deletions(-)
> 
> 
> Diffs
> -
> 
>   docs/operations/configuration.md e332f864aeb25790f860bc1961a7a2c49b29004e 
>   docs/reference/configuration.md c4b1d387435e4b8fd59a296bd5d6d203bb50cbde 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 203fc47d74840889a1192dc867fef5584b704685 
>   src/main/python/apache/thermos/core/runner.py 
> 3ebf86ebd12ed3b68f543d4b9a45615e4681ba7f 
>   src/main/python/apache/thermos/testing/runner.py 
> 8b6ba730acedf284a61e4bde60ab480950f92ede 
>   src/test/python/apache/thermos/core/BUILD 
> acfb79b6c411744be7f8b3bb364f0c8d049f1694 
>   src/test/python/apache/thermos/core/test_runner_log_config.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49399/diff/
> 
> 
> Testing
> ---
> 
> Manually tested the cases in
>   https://issues.apache.org/jira/browse/AURORA-1724.
> 
> Also ran the following green locally:
> ```
> ./pants test src/test/python/apache/thermos/core
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ```
> 
> Doc edits are rendered here:
>   
> https://github.com/jsirois/aurora/blob/jsirois/issues/AURORA-1724/docs/reference/configuration.md#logger
>   
> https://github.com/jsirois/aurora/blob/jsirois/issues/AURORA-1724/docs/operations/configuration.md#process-logs
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Re: Review Request 49413: Fixup install docs to match 0.13.0+ packaging.

2016-06-30 Thread Stephan Erb 

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


Ship it!




Ship It!

- Stephan Erb


On June 30, 2016, 2:10 a.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49413/
> ---
> 
> (Updated June 30, 2016, 2:10 a.m.)
> 
> 
> Review request for Aurora and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> docs/operations/installation.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> Diffs
> -
> 
>   docs/operations/installation.md d3fb529929aaf8975579930d50a59e155d1e4357 
> 
> Diff: https://reviews.apache.org/r/49413/diff/
> 
> 
> Testing
> ---
> 
> Came across the issue testing the 0.13.0 and 0.14.0 releases.
> 
> Rendered here:
>   
> https://github.com/jsirois/aurora/blob/jsirois/docs/installation/fix/docs/operations/installation.md#centos-7-2
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Re: Review Request 49399: Fix Process log configuration handling.

2016-06-30 Thread Martin Hrabovcin 

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


Ship it!




Nice integration test

- Martin Hrabovcin


On June 29, 2016, 11 p.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49399/
> ---
> 
> (Updated June 29, 2016, 11 p.m.)
> 
> 
> Review request for Aurora, George Sirois, Martin Hrabovcin, and Stephan Erb.
> 
> 
> Bugs: AURORA-1724
> https://issues.apache.org/jira/browse/AURORA-1724
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Previously flagged configuration of Process logging mode would
> blow up and claimed defaulting of the rotation policy did not
> occur.
> 
>  docs/operations/configuration.md|   9 +-
>  docs/reference/configuration.md |  33 
> 
>  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py |  21 
> ++---
>  src/main/python/apache/thermos/core/runner.py   |  40 
> ++---
>  src/main/python/apache/thermos/testing/runner.py|  16 
> ++--
>  src/test/python/apache/thermos/core/BUILD   |   2 +
>  src/test/python/apache/thermos/core/test_runner_log_config.py   | 230 
> ++
>  7 files changed, 293 insertions(+), 58 deletions(-)
> 
> 
> Diffs
> -
> 
>   docs/operations/configuration.md e332f864aeb25790f860bc1961a7a2c49b29004e 
>   docs/reference/configuration.md c4b1d387435e4b8fd59a296bd5d6d203bb50cbde 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 203fc47d74840889a1192dc867fef5584b704685 
>   src/main/python/apache/thermos/core/runner.py 
> 3ebf86ebd12ed3b68f543d4b9a45615e4681ba7f 
>   src/main/python/apache/thermos/testing/runner.py 
> 8b6ba730acedf284a61e4bde60ab480950f92ede 
>   src/test/python/apache/thermos/core/BUILD 
> acfb79b6c411744be7f8b3bb364f0c8d049f1694 
>   src/test/python/apache/thermos/core/test_runner_log_config.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49399/diff/
> 
> 
> Testing
> ---
> 
> Manually tested the cases in
>   https://issues.apache.org/jira/browse/AURORA-1724.
> 
> Also ran the following green locally:
> ```
> ./pants test src/test/python/apache/thermos/core
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ```
> 
> Doc edits are rendered here:
>   
> https://github.com/jsirois/aurora/blob/jsirois/issues/AURORA-1724/docs/reference/configuration.md#logger
>   
> https://github.com/jsirois/aurora/blob/jsirois/issues/AURORA-1724/docs/operations/configuration.md#process-logs
> 
> 
> Thanks,
> 
> John Sirois
> 
>