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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: rds deploy crash #1068

Merged
merged 8 commits into from
Jul 6, 2023
Merged

fix: rds deploy crash #1068

merged 8 commits into from
Jul 6, 2023

Conversation

oddgrd
Copy link
Contributor

@oddgrd oddgrd commented Jul 3, 2023

Description of change

We were incorrectly assuming the state to be running in the logs received from a deployment, but they could also be loading (and they will be loading for a while in the case of rds). This lead to the client reporting that the deployment crashed since it received the running state in the logs stream, but the deployment was still loading (an unexpected state since we received running in logs).

Closes #1029

TODO:

  • We're not getting the logs from runtime during loading, although I think we used to (I remember seeing the "deploying <db_type>, this could take a while..." output). 馃 This means we get "loading service" from the deployer run function, and then nothing until loading is complete.
  • Should we use rwlock for the logger state? Does it matter?

How has this been tested? (if applicable)

Tested by deploying tide rds example to unstable, as well as some other examples (axum hello world, poem postgres).

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
cargo-shuttle/src/lib.rs Outdated Show resolved Hide resolved
runtime/src/logger.rs Show resolved Hide resolved
Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>
Copy link
Contributor

@iulianbarbu iulianbarbu left a comment

Choose a reason for hiding this comment

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

LGTM @oddgrd ! This is a great fix.

runtime/src/logger.rs Show resolved Hide resolved
@iulianbarbu
Copy link
Contributor

iulianbarbu commented Jul 5, 2023

Related to the TODOs:

  1. The missing logs sound like a regression, but I don't see why runtime logs are not sent to the deployer. I would double-check this separately though (seems like this PR doesn't depend on the load phase logs at least).
  2. I think the RW lock is the safest choice, but I also think we can use the Mutex as well for now. I don't see how the runtime can have contention read/write-wise on the logger, to make the use of Mutex inefficient. In case that might happen (by having multiple runtime tasks doing >= INFO level logging) then the RW lock would help.

@oddgrd oddgrd merged commit d3aafff into shuttle-hq:main Jul 6, 2023
30 checks passed
@oddgrd oddgrd deleted the fix/rds-deploy-crash branch July 6, 2023 08:18
AlphaKeks pushed a commit to AlphaKeks/shuttle that referenced this pull request Jul 21, 2023
* misc: debug prints

* fix: debug rds bug

* feat: get state from runtime::LogItem

* feat: debug logs in client

* refactor: improve run.rs debug logs

* refactor: comment, unwrap to expect

* refactor: debug string in cargo-shuttle

Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>

---------

Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>
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.

[Bug]: Deployment fails when using shuttle-aws-rds
2 participants