aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mohit Jaggi <mohitja...@gmail.com>
Subject Re: Review Request 62692: Move job environment validation to the scheduler.
Date Wed, 11 Oct 2017 20:20:59 GMT


> On Sept. 29, 2017, 5:38 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/app/AppModule.java
> > Lines 108 (patched)
> > <https://reviews.apache.org/r/62692/diff/1/?file=1839853#file1839853line108>
> >
> >     I would find this easier to read and more flexible:
> >     
> >     ```
> >     -allowed_job_environments=prod,devel,test,^staging\d+*
> >     ```
> >     
> >     Then you can do:
> >     ```
> >     -allowed_job_environments=.*
> >     ```
> 
> Mauricio Garavaglia wrote:
>     I'd expect \.* to be the default then, right?
> 
> Bill Farner wrote:
>     If starting from scratch, yes.  In this case, it's more friendly to prevent surprises
by maintaining existing behavior by default.
> 
> Mauricio Garavaglia wrote:
>     The existing behavior is that the scheduler API ignores whatever environment comes
in, but the CLI restricts it to the list there. Are you suggesting that now the default, for
the API, should be to only allow 'prod,devel,test,^staging\\d+'? How is that maintaining existing
behavior by default?
> 
> Bill Farner wrote:
>     My mistake!  I believed the scheduler was already restricting this, but i stand corrected.
 I support your suggestion for `".*"` as the default.
> 
> Stephan Erb wrote:
>     I am not convinced we would need to enforce backwards compatibility here. 
>     
>     Most Aurora users will use the default client and would thus not be affected if we
use the same enforcement at the scheduler side. For those clusers with custom clients, I think
it is OK to ask the operator to add/update the scheduler flag when deploying the latest scheduler
version. Yes, it is a breaking change but at least one that is very easy to handle if we call
it out in the RELEASE-NOTES.md.
> 
> Mauricio Garavaglia wrote:
>     Sure, the default can be kept the same. I still found the whole environment validation
extremely arbitrary, with no clear implication for the cluster operator, and as such is not
clear what's its purpose other than preserve an old behavior.
>     Anyway, regarding the validation format, is that something like a comma separated
list of Regexps? or is that just a Regexp?
> 
> Stephan Erb wrote:
>     I believe having just one regex would be the most flexible: `prod,devel,test,^staging\d+*`
could be written as `prod|devel|test|^staging\d+*`

A single regexp can do an OR of patterns, so we don't need a comma separated list. One might
be tempted to attempt a more "user-friendly" format but regexp is a pretty standard construct
and I would suggest not re-inventing the wheel. Yet, I don't have a strong opinion and would
like to see this and 319 closed quickly :-)


- Mohit


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


On Oct. 1, 2017, 3:36 p.m., Mauricio Garavaglia wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62692/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2017, 3:36 p.m.)
> 
> 
> Review request for Aurora and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Moves the job environment validation to the scheduler, which can be enabled with the
scheduler require_predefined_environments flag. This allows to have a consistent behavior
when using the CLI and the API. In order to preserve backward compatibility, the validation
is kept in the CLI and for the API it needs to be manually enabled in the scheduler.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 081dff2bda626f01ba222628b8d7d8afebbb0004

>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 186fa1b3a4780c0536fb486d50a33133258110cd

>   src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java 754fde0fdc976b673d78ae15d8ccd8c85b792373

>   src/main/python/apache/aurora/client/config.py 70c2c980309e18de576b251087cdfea00ac06b75

>   src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
50d7499f4332a3feb0e2301cb707f2cea6bb2e98 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 6b4b17f8dafd5c2d751dcda3080b476335f8aec0

> 
> 
> Diff: https://reviews.apache.org/r/62692/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Mauricio Garavaglia
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message