Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[Aurora ReviewBot]

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



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

   
self._clock.converge(threads=[hct.threaded_health_checker])
   
self._clock.assert_waiting(hct.threaded_health_checker, amount=1)
 
   assert hct._total_latency == 0
   assert 
hct.metrics.sample()['total_latency_secs'] == 0
 
   # start the health check (during health check it 
is still 0)
   epsilon = 0.001
   self._clock.tick(1.0 + epsilon)
   
self._clock.converge(threads=[hct.threaded_health_checker])
   
self._clock.assert_waiting(hct.threaded_health_checker, amount=0.5)
   assert hct._total_latency == 0
   assert 
hct.metrics.sample()['total_latency_secs'] == 0
   assert hct.metrics.sample()['checks'] == 0
 
   # finish the health check
   self._clock.tick(0.5 + epsilon)
   
self._clock.converge(threads=[hct.threaded_health_checker])
   
self._clock.assert_waiting(hct.threaded_health_checker, amount=1)  # 
interval_secs
 > assert hct._total_latency == 0.5
 E AssertionError: assert 0.5009 
== 0.5
 E  +  where 0.5009 = 
._total_latency
 
 
src/test/python/apache/aurora/executor/common/test_health_checker.py:174: 
AssertionError
 -- Captured stderr call --
 [] Time now: 0.0
 [] Time now: 0.0
 [] Time now: 1.0
 [] Time now: 1.001
 [] Time now: 1.001
 [] Time now: 1.501
 [] Time now: 1.502
  generated xml file: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/test-results/415337499eb72578eab327a6487c1f5c9452b3d6.xml
 
  1 failed, 663 passed, 5 skipped, 1 warnings in 
201.06 seconds 
 
FAILURE


16:37:05 04:20   [complete]
   FAILURE


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

- Aurora ReviewBot


On March 29, 2016, 4:18 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45396/
> ---
> 
> (Updated March 29, 2016, 4:18 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In instances where the root filesystem is read-only, it is desirable to
> have the executor/runner extract themselves into the sandbox.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   config/legacy_untested_classes.txt 144b258b7a7f73131f07826a0fdcac04834d87db 
>   docs/operations/configuration.md f9e8844914b7af2b0057ca5f1d7d4391a63ca142 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  949c299bdbc54f976db994266fb97f3099256f13 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModuleTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45396/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[George Sirois]

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

(Updated March 29, 2016, 4:18 p.m.)


Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.


Changes
---

Fixes for style nits.


Repository: aurora


Description
---

In instances where the root filesystem is read-only, it is desirable to
have the executor/runner extract themselves into the sandbox.


Diffs (updated)
-

  RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
  config/legacy_untested_classes.txt 144b258b7a7f73131f07826a0fdcac04834d87db 
  docs/operations/configuration.md f9e8844914b7af2b0057ca5f1d7d4391a63ca142 
  
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
 949c299bdbc54f976db994266fb97f3099256f13 
  
src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModuleTest.java
 PRE-CREATION 

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


Testing
---


Thanks,

George Sirois

Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[Joshua Cohen]

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


Ship it!




LGTM, thanks for adding the tests!

If you don't mind fixing the two style nits below I can commit this.


src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
 (line 112)


nit: +blank line before



src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModuleTest.java
 (line 71)


same style nit here, blank line after method signature that's broken up 
over multiple lines.


- Joshua Cohen


On March 29, 2016, 5:49 a.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45396/
> ---
> 
> (Updated March 29, 2016, 5:49 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In instances where the root filesystem is read-only, it is desirable to
> have the executor/runner extract themselves into the sandbox.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   config/legacy_untested_classes.txt 144b258b7a7f73131f07826a0fdcac04834d87db 
>   docs/operations/configuration.md f9e8844914b7af2b0057ca5f1d7d4391a63ca142 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  949c299bdbc54f976db994266fb97f3099256f13 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModuleTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45396/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[Aurora ReviewBot]

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


Ship it!




Master (f28f41a) 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 March 29, 2016, 5:49 a.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45396/
> ---
> 
> (Updated March 29, 2016, 5:49 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In instances where the root filesystem is read-only, it is desirable to
> have the executor/runner extract themselves into the sandbox.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   config/legacy_untested_classes.txt 144b258b7a7f73131f07826a0fdcac04834d87db 
>   docs/operations/configuration.md f9e8844914b7af2b0057ca5f1d7d4391a63ca142 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  949c299bdbc54f976db994266fb97f3099256f13 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModuleTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45396/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[George Sirois]

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

(Updated March 29, 2016, 5:49 a.m.)


Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.


Repository: aurora


Description
---

In instances where the root filesystem is read-only, it is desirable to
have the executor/runner extract themselves into the sandbox.


Diffs (updated)
-

  RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
  config/legacy_untested_classes.txt 144b258b7a7f73131f07826a0fdcac04834d87db 
  docs/operations/configuration.md f9e8844914b7af2b0057ca5f1d7d4391a63ca142 
  
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
 949c299bdbc54f976db994266fb97f3099256f13 
  
src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModuleTest.java
 PRE-CREATION 

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


Testing
---


Thanks,

George Sirois

Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[Joshua Cohen]

> On March 29, 2016, 12:03 a.m., Joshua Cohen wrote:
> > Are there any concerns about any other unintended side effect of setting 
> > $HOME to the sandbox? I suppose it's opt-in for now, so that allays most 
> > fears.
> 
> George Sirois wrote:
> I couldn't think of any (nor did I discover any in testing, both with and 
> without Docker), but making it opt-in should allow us to gain experience with 
> running it in production environments without impacting existing deployments. 
> The reason I used HOME (as opposed to PEX_ROOT) is that the executor strips 
> PEX_ROOT before forking the runner, which has the same issue. We could add 
> some plumbing to get around that, but it seems like if you're running with a 
> read-only root FS, you'd want HOME to be a writable location anyway.

Yeah, I'm not coming up with anything either other than potentially causing 
tasks to exceed their disk space allotment due to ~/.pex/install now being 
counted against them. That's arguably the right thing to do though, operators 
should just be cognizant of that when enabling this (or if/when we make it the 
default).


- Joshua


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


On March 28, 2016, 6:21 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45396/
> ---
> 
> (Updated March 28, 2016, 6:21 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In instances where the root filesystem is read-only, it is desirable to
> have the executor/runner extract themselves into the sandbox.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   docs/deploying-aurora-scheduler.md 03bfdbab927c924486b04c42df2ad236c0f414a0 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  949c299bdbc54f976db994266fb97f3099256f13 
> 
> Diff: https://reviews.apache.org/r/45396/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[George Sirois]

> On March 29, 2016, 12:03 a.m., Joshua Cohen wrote:
> > Are there any concerns about any other unintended side effect of setting 
> > $HOME to the sandbox? I suppose it's opt-in for now, so that allays most 
> > fears.

I couldn't think of any (nor did I discover any in testing, both with and 
without Docker), but making it opt-in should allow us to gain experience with 
running it in production environments without impacting existing deployments. 
The reason I used HOME (as opposed to PEX_ROOT) is that the executor strips 
PEX_ROOT before forking the runner, which has the same issue. We could add some 
plumbing to get around that, but it seems like if you're running with a 
read-only root FS, you'd want HOME to be a writable location anyway.


- George


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


On March 28, 2016, 6:21 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45396/
> ---
> 
> (Updated March 28, 2016, 6:21 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In instances where the root filesystem is read-only, it is desirable to
> have the executor/runner extract themselves into the sandbox.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   docs/deploying-aurora-scheduler.md 03bfdbab927c924486b04c42df2ad236c0f414a0 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  949c299bdbc54f976db994266fb97f3099256f13 
> 
> Diff: https://reviews.apache.org/r/45396/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[George Sirois]

> On March 29, 2016, 12:03 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java,
> >  line 112
> > 
> >
> > Given that this is a static method, it should be pretty easy to test, 
> > would you mind adding a test case for this behavior (probably requires 
> > refactoring the method to take the arg values as parameters, rather than 
> > referencing them directly).

Sure, will do.


- George


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


On March 28, 2016, 6:21 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45396/
> ---
> 
> (Updated March 28, 2016, 6:21 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In instances where the root filesystem is read-only, it is desirable to
> have the executor/runner extract themselves into the sandbox.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   docs/deploying-aurora-scheduler.md 03bfdbab927c924486b04c42df2ad236c0f414a0 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  949c299bdbc54f976db994266fb97f3099256f13 
> 
> Diff: https://reviews.apache.org/r/45396/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[Joshua Cohen]

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



Are there any concerns about any other unintended side effect of setting $HOME 
to the sandbox? I suppose it's opt-in for now, so that allays most fears.


src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
 (line 112)


Given that this is a static method, it should be pretty easy to test, would 
you mind adding a test case for this behavior (probably requires refactoring 
the method to take the arg values as parameters, rather than referencing them 
directly).


- Joshua Cohen


On March 28, 2016, 6:21 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45396/
> ---
> 
> (Updated March 28, 2016, 6:21 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In instances where the root filesystem is read-only, it is desirable to
> have the executor/runner extract themselves into the sandbox.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   docs/deploying-aurora-scheduler.md 03bfdbab927c924486b04c42df2ad236c0f414a0 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  949c299bdbc54f976db994266fb97f3099256f13 
> 
> Diff: https://reviews.apache.org/r/45396/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[Aurora ReviewBot]

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



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

However, it appears that it might lack test coverage.

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

- Aurora ReviewBot


On March 28, 2016, 6:21 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45396/
> ---
> 
> (Updated March 28, 2016, 6:21 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In instances where the root filesystem is read-only, it is desirable to
> have the executor/runner extract themselves into the sandbox.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   docs/deploying-aurora-scheduler.md 03bfdbab927c924486b04c42df2ad236c0f414a0 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  949c299bdbc54f976db994266fb97f3099256f13 
> 
> Diff: https://reviews.apache.org/r/45396/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[Steve Niemitz]

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




RELEASE-NOTES.md (line 14)


In the future I think we should revisit this and make this the default 
behavior.  The only reason I think it shouldn't be enabled by default now is 
because the mesos.native lib inside the pex is massive, and duplicating it into 
every sandbox is 1) slow, and 2) a waste of space.

However, once my patch lands, the lib goes down to ~8 MB, which I think is 
worth duplicating, especially to get rid of any weird race conditions with 
multiple executors launching.


- Steve Niemitz


On March 28, 2016, 6:21 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45396/
> ---
> 
> (Updated March 28, 2016, 6:21 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In instances where the root filesystem is read-only, it is desirable to
> have the executor/runner extract themselves into the sandbox.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   docs/deploying-aurora-scheduler.md 03bfdbab927c924486b04c42df2ad236c0f414a0 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  949c299bdbc54f976db994266fb97f3099256f13 
> 
> Diff: https://reviews.apache.org/r/45396/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 45396: Adds the ability to set HOME to the sandbox before running the executor.

[Steve Niemitz]

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


Ship it!




Ship It!

- Steve Niemitz


On March 28, 2016, 6:21 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45396/
> ---
> 
> (Updated March 28, 2016, 6:21 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Steve Niemitz, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In instances where the root filesystem is read-only, it is desirable to
> have the executor/runner extract themselves into the sandbox.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 34f28a165aae4ae24fa95ef19b4972e088fd63a0 
>   docs/deploying-aurora-scheduler.md 03bfdbab927c924486b04c42df2ad236c0f414a0 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  949c299bdbc54f976db994266fb97f3099256f13 
> 
> Diff: https://reviews.apache.org/r/45396/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> George Sirois
> 
>