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.

Sign up for GitHub

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

Jump to bottom

fix: update CI to avoid running out of memory #1206

Merged
merged 1 commit into from
Jul 6, 2021
Merged

fix: update CI to avoid running out of memory #1206

merged 1 commit into from
Jul 6, 2021

Conversation

smartcontracts
Copy link
Contributor

Description
Resolves most common issues with CI running out of memory by waiting for the Sequencer before starting the integration tests. Also adds a stats.sh script that will log current memory usage of different parts of the system once per second while the integration tests are running.

Metadata

@changeset-bot
Copy link

changeset-bot bot commented Jul 2, 2021 •
edited
Loading

⚠️ No Changeset found

Latest commit: 1f8661e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2021 •
edited
Loading

Codecov Report

Merging #1206 (773154a) into develop (e615144) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1206   +/-   ##
========================================
  Coverage    86.05%   86.05%           
========================================
  Files           49       49           
  Lines         1936     1936           
  Branches       307      307           
========================================
  Hits          1666     1666           
  Misses         270      270

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e615144...773154a. Read the comment docs.

@smartcontracts smartcontracts force-pushed the sc/fix-ci-oom branch 2 times, most recently from 2e2339e to 9be5d82 Compare July 3, 2021 18:40
@smartcontracts smartcontracts changed the title fix: resolve issues with CI running out of memory and add stats script fix: update CI to avoid running out of memory Jul 3, 2021
smartcontracts
smartcontracts commented Jul 3, 2021
View reviewed changes
.github/workflows/integration.yml
@@ -38,7 +38,13 @@ jobs:

- name: Bring the stack up
working-directory: ./ops
run: docker-compose up -d
run: |
./scripts/stats.sh &
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that running this once here is good enough. It appears to run continuously throughout the rest of the workflow.

.github/workflows/integration.yml
./scripts/stats.sh &
docker-compose up -d

- name: Wait for the Sequencer node
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the step that helps avoid memory issues. The problem was that the integration tests would start before the docker setup was finished starting. This doesn't fix /all/ memory issues but definitely fixes the most common issue. I having the stats.sh script will help debug future issues.

.github/workflows/integration.yml
@@ -99,11 +105,11 @@ jobs:
uses: jwalton/gh-docker-logs@v1
with:
images: 'ethereumoptimism/builder,ethereumoptimism/hardhat,ethereumoptimism/deployer,ethereumoptimism/data-transport-layer,ethereumoptimism/l2geth,ethereumoptimism/message-relayer,ethereumoptimism/batch-submitter,ethereumoptimism/l2geth,ethereumoptimism/integration-tests'
dest: './logs'
dest: '~/logs'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this because it was easier to reliably stuff things into ~/logs from other scripts (namely from stats.sh).

@smartcontracts smartcontracts marked this pull request as ready for review July 3, 2021 18:43
@smartcontracts smartcontracts force-pushed the sc/fix-ci-oom branch 3 times, most recently from cbc9dea to 46aee66 Compare July 6, 2021 10:45
@smartcontracts smartcontracts merged commit b7a2614 into develop Jul 6, 2021
@smartcontracts smartcontracts deleted the sc/fix-ci-oom branch July 6, 2021 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elenadimitrova elenadimitrova elenadimitrova approved these changes

@snario snario snario approved these changes

Assignees
No one assigned
Labels
A-ops Area: ops
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate Exit Code 137 (out of memory) in Sync Tests
4 participants
@smartcontracts @codecov-commenter @elenadimitrova @snario