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

Docker setup: Wait for genesis file to exist before starting nodes #949

Merged
merged 4 commits into from
Nov 24, 2022
Merged

Docker setup: Wait for genesis file to exist before starting nodes #949

merged 4 commits into from
Nov 24, 2022

Conversation

mediremi-antithesis
Copy link
Contributor

@mediremi-antithesis mediremi-antithesis commented Nov 21, 2022

Description

The Docker image entrypoint docker/local/polygon-edge.sh had a race condition where if the init container took too long to generate the genesis.json genesis file, nodes would fail with Error: open /genesis/genesis.json: no such file or directory.

This fix ensures that nodes wait for this file to exist (by polling for its existence in a loop) prior to starting up.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

Manually tested by running docker-compose up

@github-actions
Copy link

github-actions bot commented Nov 21, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@mediremi-antithesis
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

Copy link
Contributor

@epikichi epikichi left a comment

Choose a reason for hiding this comment

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

Thanks for the identifying this behavior and adding a remediation! This change looks good functionally.

I want to see your thoughts on implementing the depends_on long syntax condition to handle this race scenario on the compose level for better init feedback. We should be able to set the dependency to wait for the service_completed_successfully condition to avoid premature node starts and uncaught init exceptions.

This approach does bring up an inevitable downstream condition where if the genesis.json already exists in the defined volumes (make run-local; make stop-local; make run-local to simply reproduce), then the init service will exit with status code 1 and fail the condition. Adding a catch for this scenario in the polygon-edge.sh file should be sufficient for that case.

Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@mediremi-antithesis
Copy link
Contributor Author

I didn't know about the depends_on long syntax but that definitely sounds like a cleaner way of implementing this.

Unfortunately I use podman-compose instead of docker-compose, which doesn't support the long syntax version of depends_on, so I'm unable to test it. So please feel free to make that change and push to this branch.

I have made the change you suggested regarding the init command, which will now only attempt to generate the genesis file if it does not already exist.

@epikichi
Copy link
Contributor

I didn't know about the depends_on long syntax but that definitely sounds like a cleaner way of implementing this.

Unfortunately I use podman-compose instead of docker-compose, which doesn't support the long syntax version of depends_on, so I'm unable to test it. So please feel free to make that change and push to this branch.

I have made the change you suggested regarding the init command, which will now only attempt to generate the genesis file if it does not already exist.

Thanks for the amendments! Made a quick https://github.com/mediremi-antithesis/polygon-edge/pull/1 to add in the changes as suggested.

adding conditional dependency for compose to wait for successful  node
Copy link
Contributor

@epikichi epikichi left a comment

Choose a reason for hiding this comment

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

Tested and worked with the mentioned above changes regarding docker compose conditions. LGTM!

@codecov
Copy link

codecov bot commented Nov 24, 2022

Codecov Report

Merging #949 (0360f1a) into develop (f80b615) will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #949      +/-   ##
===========================================
+ Coverage    52.89%   52.91%   +0.01%     
===========================================
  Files          129      129              
  Lines        17113    17113              
===========================================
+ Hits          9052     9055       +3     
+ Misses        7413     7411       -2     
+ Partials       648      647       -1     
Impacted Files Coverage Δ
syncer/client.go 63.20% <0.00%> (+1.41%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@ZeljkoBenovic ZeljkoBenovic left a comment

Choose a reason for hiding this comment

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

LGTM 🏅

@epikichi epikichi merged commit e22c827 into 0xPolygon:develop Nov 24, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug fix Functionality that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants