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

[Merged by Bors] - Fix broken Nethermind integration tests #4836

Closed
wants to merge 14 commits into from

Conversation

macladson
Copy link
Member

Issue Addressed

CI is currently blocked by persistently failing integration tests.

Proposed Changes

Use latest Nethermind release and apply the appropriate fixes as there have been breaking changes.
Also increase the timeout since I had some local timeouts.

@michaelsproul
Copy link
Member

Thanks @macladson! I hijacked your branch and pushed some changes to get the latest version of Nethermind working.

Enabling Shapella from genesis seems to have done the trick. I nicked some Nethermind config JSON from src/Nethermind/Nethermind.Blockchain.Test/Specs/shanghai_from_genesis.json.

@michaelsproul michaelsproul added ready-for-review The code is ready for review infra-ci labels Oct 13, 2023
@@ -68,7 +68,8 @@ impl NethermindEngine {
.join("bin")
.join("Release")
.join("net7.0")
.join("Nethermind.Runner")
.join("linux-x64")
Copy link
Member

Choose a reason for hiding this comment

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

this part of the path didn't exist on my machine, lets see if it exists on CI 🤞

Copy link
Member

Choose a reason for hiding this comment

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

It no longer runs on macos though :(

Copy link
Member

Choose a reason for hiding this comment

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

path on my machine: testing/execution_engine_integration/execution_clients/nethermind/src/Nethermind/Nethermind.Runner/bin/Release/net7.0/nethermind

Copy link
Member

Choose a reason for hiding this comment

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

I'm deleting it. I think maybe this was why the job was failing

@jimmygchen
Copy link
Member

I ran into a panic locally due to "would clobber existing tag", and seems like I was running into this issue
.

Adding the --force flag below this line fixed it for me, but could well just be a local issue:

output_to_result(
Command::new("git")
.arg("fetch")
.arg("--tags")

@michaelsproul
Copy link
Member

michaelsproul commented Oct 15, 2023

Ah, the CI issue is due to us being out of disk space:

You are running out of disk space. The runner will stop working when the machine runs out of disk space. Free space left: 0 MB

Received request to deprovision: The request was cancelled by the remote provider.

From: https://github.com/sigp/lighthouse/actions/runs/6514115416

@michaelsproul
Copy link
Member

Pushed a new commit to run the tests on the self-hosted runners.

let tests_dir = execution_clients_dir.join("nethermind/src/tests");
if let Err(e) = fs::remove_dir_all(tests_dir) {
eprintln!("Error while deleting folder: {}", e);
}
Copy link
Member

@jimmygchen jimmygchen Oct 18, 2023

Choose a reason for hiding this comment

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

Wow just realized these are 2.44 GB on my machine, perhaps we don't even need to clone them in the first place? I think we can optionally exclude submodule during checkout:

output_to_result(
Command::new("git")
.arg("submodule")
.arg("update")
.arg("--init")
.arg("--recursive")

Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Changes look good! I only have a small comment above wrt excluding the submodule checkout, but happy to address it in a separate PR, as this is already better than what we had

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 18, 2023
@jimmygchen
Copy link
Member

bors+

@jimmygchen
Copy link
Member

Oops missed an r!
bors r+

bors bot pushed a commit that referenced this pull request Oct 18, 2023
## Issue Addressed

CI is currently blocked by persistently failing integration tests.

## Proposed Changes

Use latest Nethermind release and apply the appropriate fixes as there have been breaking changes.
Also increase the timeout since I had some local timeouts.


Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: antondlr <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
@bors
Copy link

bors bot commented Oct 18, 2023

Build failed:

@jimmygchen
Copy link
Member

bors retry

@jimmygchen
Copy link
Member

bors r+

@bors
Copy link

bors bot commented Oct 18, 2023

Already running a review

bors bot pushed a commit that referenced this pull request Oct 18, 2023
## Issue Addressed

CI is currently blocked by persistently failing integration tests.

## Proposed Changes

Use latest Nethermind release and apply the appropriate fixes as there have been breaking changes.
Also increase the timeout since I had some local timeouts.


Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: antondlr <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
@bors
Copy link

bors bot commented Oct 18, 2023

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Oct 18, 2023
## Issue Addressed

CI is currently blocked by persistently failing integration tests.

## Proposed Changes

Use latest Nethermind release and apply the appropriate fixes as there have been breaking changes.
Also increase the timeout since I had some local timeouts.


Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: antondlr <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
@@ -317,17 +317,21 @@ jobs:
./doppelganger_protection.sh success genesis.json
execution-engine-integration-ubuntu:
name: execution-engine-integration-ubuntu
runs-on: ubuntu-latest
runs-on: ${{ github.repository == 'sigp/lighthouse' && fromJson('["self-hosted", "linux", "CI", "small"]') || 'ubuntu-latest' }}
Copy link
Member

Choose a reason for hiding this comment

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

I've just realized that we're one runner short of running the entire workflow in parallel after adding this one, it may be worth considering adding one more runner instance.

@bors
Copy link

bors bot commented Oct 18, 2023

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Oct 18, 2023
## Issue Addressed

CI is currently blocked by persistently failing integration tests.

## Proposed Changes

Use latest Nethermind release and apply the appropriate fixes as there have been breaking changes.
Also increase the timeout since I had some local timeouts.


Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: antondlr <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
@bors
Copy link

bors bot commented Oct 18, 2023

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title Fix broken Nethermind integration tests [Merged by Bors] - Fix broken Nethermind integration tests Oct 18, 2023
@bors bors bot closed this Oct 18, 2023
Gua00va pushed a commit to Gua00va/lighthouse that referenced this pull request Oct 18, 2023
## Issue Addressed

CI is currently blocked by persistently failing integration tests.

## Proposed Changes

Use latest Nethermind release and apply the appropriate fixes as there have been breaking changes.
Also increase the timeout since I had some local timeouts.


Co-authored-by: Michael Sproul <[email protected]>
Co-authored-by: antondlr <[email protected]>
Co-authored-by: Jimmy Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra-ci ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants