[ES-8436] feat: Change Dockerfile base images to Wolfi #1871
Conversation
favilo
changed the title
Running the docker image with a mounted `/rally/.rally` directory that already contains the configuration file didn't work. So I had to change how we parse the logging config and rally.ini files. Now we dealy the parsing of the environment variables to read time, to make running from docker and outside of docker compatible with each other.
I'd been installing `esrally` to the wrong location, now it's correctly in venv/bin
favilo
force-pushed
the
docker-refresh
branch
from
77c16e5 to
c8f8d41
Compare
and possibly break users' workflows.
There was a problem hiding this comment.
This looks good. I like the increased portability the delayed resolution of ${CONFIG_DIR} and ${LOG_PATH} variables offer but I think this should get proper description in here and here. It could also be useful to mention it in Docker section, perhaps here?
| # the logging path might contain backslashes that we need to escape | ||
| log_path = io.escape_path(log_path) |
There was a problem hiding this comment.
We're dropping backlash escaping which was meant to help with Windows judging by #829. But we never officially supported Windows so I'm fine with it.
There was a problem hiding this comment.
Oh, how did I not see this? I am happy to add this logic back to the code.
There was a problem hiding this comment.
Actually, on second thought, this would only have been valuable when we were writing the file out to disk for deserialization later. Since we are doing the replacement on the fly, it should still work, even on Windows
docker/Dockerfiles/Dockerfile-dev
Outdated
| # Build stage 0 `builder`: | ||
| # Install Rally from source inside a virtualenv | ||
| ################################################################################ | ||
| FROM docker.elastic.co/wolfi/python:3.11.7-dev AS builder |
There was a problem hiding this comment.
This is probably a good opportunity to define the rule going forward. Will we keep this version aligned with the version of the nightlies (3.11.x), or perhaps we should bump it up all the way to the newest supported version (3.12.x)?
There was a problem hiding this comment.
I think I tried 3.12 and it wasn't playing nice, but I could also be remembering something else. I don't mind pinning it to the newest version we support.
docs/docker.rst
Outdated
| make the files more portable. For example:: | ||
|
|
||
| * In ``rally.ini``, you can set ``root.dir = ${CONFIG_DIR}/benchmarks`` instead of hard-coding the path as ``/rally/.rally/benchmarks`` | ||
| * In ``logging.json``, you can set ``"filename": "${LOG_PATH}/rally.log"`` instead of hard-coding the path as ``"filename": "/rally/.rally/logs/rally.log"`` |
There was a problem hiding this comment.
This bulleted list does not look good in the preview. Double colon perhaps?
Running the docker image with a mounted
/rally/.rallydirectory that already contains the configuration file didn't work. So I had to change how we parse the logging config and rally.ini files.Now we dealy the parsing of the environment variables to read time, to make running from docker and outside of docker compatible with each other.
This will require changes to the buildkite scripts to allow pushing a regular docker image for use with nightlies, and potentially rebuilding older images when vulnerabilities are found.