fix: stop the right containers for minimize-downtime flag - #4454
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5b08f48. Configure here.
|
@aldy505 Would you please address issues by bots? They're right, |
I'm a bit... confused between the behavior of |
|
Force and stop are not necessary for What's the reason for switching to |
EDIT: oh right, and we need to reload the image tag, therefore dc stop and dc rm does not suffice |
| echo "Relay container not found, skipping restart." | ||
| fi | ||
| # Start the whole setup, except nginx, relay, and web. | ||
| start_service_and_wait_ready --remove-orphans $($dc config --services | grep -v -E '^(nginx|relay|web)$') |
There was a problem hiding this comment.
Correct me if I'm wrong, but web container should always be recreated with new image after an upgrade, why does this has been added to exclusions?
|
|
||
| # Start the whole setup, except nginx and relay. | ||
| # Start the whole setup, except nginx, relay, and web. | ||
| start_service_and_wait_ready --remove-orphans $($dc config --services | grep -v -E '^(nginx|relay)$') |
There was a problem hiding this comment.
Bug: The comment in wrap-up.sh states the web service should be excluded from the initial start, but the grep regex on the next line does not exclude it.
Severity: MEDIUM
Suggested Fix
Update the grep regular expression on line 5 of install/wrap-up.sh to include web in the list of excluded services. The regex should be changed from '^(nginx|relay)$' to '^(nginx|web|relay)$' to match the intent described in the comment.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: install/wrap-up.sh#L5
Potential issue: In the `--minimize-downtime` upgrade path, the script
`install/wrap-up.sh` is intended to perform a staged service startup. A comment on line
4 indicates that the `web` service should be excluded from the initial
`start_service_and_wait_ready` call. However, the `grep` command's regex on line 5
(`'^(nginx|relay)$'`) fails to exclude `web`. This causes the `web` service to be
started and force-recreated prematurely, which contradicts the intended staged startup
logic designed to minimize downtime. While this may not cause a complete failure, it
deviates from the expected upgrade process.
| # Stop everything but relay and nginx | ||
| $dc rm -fsv $($dc config --services | grep -v -E '^(nginx|relay)$') | ||
| # Stop everything unless databases, relay, web, and nginx | ||
| $dc down $($dc config --services | grep -v -E '^(nginx|web|relay|smtp|memcached|redis|postgres|pgbouncer|kafka|clickhouse|seaweedfs)$') |
There was a problem hiding this comment.
podman down, stops and removes dependencies like postgres as well.
2 participants
The
--minimize-downtimeshould not remove all databases (postgres, kafka, clickhouse, etc) onturn-things-off. Tested this on my instance with:nightly. Works well as long as you don't have any wrongdoings ondocker-compose.override.yml(learned this the hard way)