-
Notifications
You must be signed in to change notification settings - Fork 313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inject build_flavor in track templates #1750
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach looks good to me and in tests I did it worked well. Left a very minor comment.
docs/advanced.rst
Outdated
Extensions | ||
"""""""""" | ||
|
||
Rally also provides a few extensions to Jinja2: | ||
|
||
* ``now``: a global variable that represents the current date and time when the template is evaluated by Rally. | ||
* ``build_flavor``: a global variable that holds build flavor reported by Elasticsearch cluster targetted by Rally. | ||
* ``days_ago()``: a `filter <http://jinja.pocoo.org/docs/dev/templates/#filters>`_ that you can use for date calculations. | ||
|
||
You can find an example in the ``http_logs`` track:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a bit confusing, since the example only talks about now
and days_ago
. Not sure if we may want to add an example with bulld_flavour as well? and specify that the http_log example is for the other two?..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I haven't thought about this example below. I moved build_flavor
variable description lower in 29351b4. I don't have an example that would make sense outside of serverless context, so I'm not adding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Here's an attempt at injecting
build_flavor
in track templates. It is part of Rally modifications for serverless.The idea is to handle
build_flavor
as Jinja2 internal variable, similarly tonow
(doc).Internal variables take precedence over user-supplied parameters and are excluded from a list of parameters exposed by a track (see here). As a result, today if a user provides
now
parameter via--track-params
it is not recognised as a valid parameter even if it is present in a track definition. I found current error slightly misleading so this PR adds a dedicated error message when user-provided parameter collides with any of the variables defined internally.Internal variables are not used today when rendering index settings, index templates, component templates and composable templates. This PR changes that adding
build_flavor
internal variable alone to these as well. That's necessary to support scenarios such as elastic/rally-tracks#406 wherebuild_flavor
could be used in component template definition.With this approach
build_flavor
cannot be determined within custom runner or custom parameter source Python code. Their interface simply does not expose the necessary data structures. To workaround this we would need to injectbuild_flavor
as an extra operation parameter which I did not find compelling. I don't think it's a blocker becausebuild_flavor
-driven customisation can still be achieved at the track definition level.The
build_flavor
value is taken from Elasticsearch REST API call in an early stage of benchmark preparation flow, i.e.setup()
inBenchmarkCoordinator
(see #1748 diagram) and saved to configuration here. This call does not happen in case of source builds. If configuration entry is missing,default
value ofbuild_flavor
is assumed.This is a breaking change because
build_flavor
becomes reserved, so the plan is toreleasebump up version to2.8.1
and2.9.0
before merging it.