Skip to content
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

ci: Tame the doge #632

Merged
merged 1 commit into from
Dec 7, 2023
Merged

ci: Tame the doge #632

merged 1 commit into from
Dec 7, 2023

Conversation

kim
Copy link
Contributor

@kim kim commented Dec 6, 2023

Description of Changes

Incantation to restart containers in a controlled way.

API and ABI breaking changes

If you use docker-compose, you shouldn't. Use docker compose.

Expected complexity level and risk

1

Fixes #630

@kim kim force-pushed the kim/doge branch 2 times, most recently from 5f75801 to 6f8444b Compare December 6, 2023 13:32
@kim kim marked this pull request as ready for review December 6, 2023 13:36
Comment on lines 20 to 22
# These cannot run in parallel, obviously
- name: Run restarting smoketests
run: test/run-smoke-tests.sh zz_docker-restart-module zz_docker-restart-repeating-reducer zz_docker-restart-sql
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were parallel restarts the main issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I just fixed that as a drive-by

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out the runner script already handles this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, turns out turns out we need to run them as a separate task, for whatever reason.

Comment on lines +90 to +91
# The health probe runs inside the container, but that doesn't mean we can
# reach it from outside. Ping until we get through.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How useful is the health check then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because sometimes, potentially related to the phase of the moon, it will be reachable immediately. I should move the sleep to the end of the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also useful for folks using docker for local development? I think you get slightly more helpful output from docker commands when you lost the log stream f.ex.

#
# The health probe runs inside the container, but that doesn't mean we can
# reach it from outside. Ping until we get through.
ping
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct in asserting that the minimal set of changes needed to remove the flakiness from those tests were

  1. Not running them in parallel
  2. This ping utility

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also restart. stop followed by start works only some of the time.

@kim kim force-pushed the kim/doge branch 2 times, most recently from 9dc3a4c to 8ac2f18 Compare December 7, 2023 09:46
One does not simply restart a docker container. But when you do,
SpacetimeDB also restarts.
@kim kim merged commit 4b9f30f into master Dec 7, 2023
5 checks passed
@kim kim deleted the kim/doge branch December 7, 2023 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix disabled smoketests
2 participants