Add Quick startup #2436
Add Quick startup #2436
Conversation
| if [ ! "$QUICK_STARTUP" = "1" ]; then | ||
| until $(curl --output /dev/null -f --silent http://localhost:$ALERTMANAGER_PORT) || [ $TRIES -eq $RETRIES ]; do | ||
| ((TRIES = TRIES + 1)) | ||
| sleep 5 |
There was a problem hiding this comment.
You could probably reduce this to 1 second and increase the retries count.
There was a problem hiding this comment.
BTW, curl has all the built-in functionality for retrying. No real need for this loop.
There was a problem hiding this comment.
Looks good, so I "approve", but I do think you should rethink the new fast option as something good enough to be the default - not some "nice option you shouldn't use in practice" like you suggested it is. If that requires changing start-all.sh to check something after starting all the processes and not start and ignore what happened - then so be it.
start-all.sh could even make the waiting experience more pleasant by adding a few printouts. For example, imagine that Grafana really does take 30 seconds to become responsive and we can't make it any faster. Then start-all.sh could print "Starting Prometheus and Grafana" and then just 1.5 seconds later print "Waiting for servers to become responsive..." and 30 seconds later we'll get "Grafana is up".
| @@ -104,6 +104,7 @@ Options: | |||
| --limit container,param - Allow to set a specific Docker parameter for a container, where container can be: | |||
| prometheus, grafana, alertmanager, loki, sidecar, grafanarender | |||
| --archive data-directory - Treat data directory as an archive. This disables Prometheus time-to-live (infinite retention), and would run a minimal mode | |||
| --quick-startup - If set, the script will not validate that each of the processes start correctly. | |||
There was a problem hiding this comment.
A thought:
start-all could have two modes of operations
- This "--quick-startup" means start-all doesn't wait at all for anything to come up.
- The other option is to run each service with --quick-startup, but then wait for all of them together.
The existing mode of operation, starting each background process in order and waiting for each one separately, should probably not exist at all. If you don't think option 1 should become the default, option 2 can. The current situation (wait in order) isn't useful at all. Or so I think.
| @@ -216,6 +217,9 @@ for arg; do | |||
| --no-alertmanager) | |||
| SKIP_ALERTMANAGER=1 | |||
| ;; | |||
| --quick-startup) | |||
| QUICK_STARTUP=1 | |||
| ;; | |||
There was a problem hiding this comment.
Something looks off in the indentation here, but maybe it's just github showing it wrongly?
| @@ -681,7 +688,7 @@ if [ "$SKIP_ALERTMANAGER" = "1" ]; then | |||
| AM_ADDRESS="127.0.0.1:9093" | |||
| else | |||
| echo "Wait for alert manager container to start" | |||
| AM_ADDRESS=$(./start-alertmanager.sh $ALERTMANAGER_PORT $ALERT_MANAGER_DIR -D "$DOCKER_PARAM" $LIMITS $VOLUMES $PARAMS $ALERTMANAGER_COMMAND $BIND_ADDRESS_CONFIG $ALERT_MANAGER_RULE_CONFIG) | |||
| AM_ADDRESS=$(./start-alertmanager.sh $ALERTMANAGER_PORT "$QUICK_STARTUP_CMD" $ALERT_MANAGER_DIR -D "$DOCKER_PARAM" $LIMITS $VOLUMES $PARAMS $ALERTMANAGER_COMMAND $BIND_ADDRESS_CONFIG $ALERT_MANAGER_RULE_CONFIG) | |||
There was a problem hiding this comment.
Are you sure you wanted the quotes around "$QUICK_STARTUP_CMD"? It means if this variable is empty (as it is by default), and empty string parameter is passed to the script. Which maybe doesn't affect it, but it's strange nonetheless.
|
You forgot "Fixes #2433" |
This series adds an option to a quicker startup process; it does not wait for each container process to start.
This method of starting the containers will not stop if a container fails to start correctly.
When experimenting on my laptop without Prometheus data,
startup time reduced from 45s to 1.5s.
To use the quick-startup, use the
--quick-startupcommand line flag with start-all.sh or set ``QUICK_STARTUP=1``` in the env file.