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

fix(ci): Temporarily finish full sync at 99% #4457

Merged
merged 4 commits into from
May 24, 2022
Merged

fix(ci): Temporarily finish full sync at 99% #4457

merged 4 commits into from
May 24, 2022

Conversation

teor2345
Copy link
Collaborator

@teor2345 teor2345 commented May 22, 2022

Motivation

As a workaround for #4456, we can just finish the full sync slightly early.

This removes coverage for:

  • finishing sync
  • gossiped blocks
  • mempool

But allows us to test lightwalletd.

Solution

  • finish full sync at 99% in CI
  • finish full sync at 99% in Rust

Review

This PR is urgent because it blocks lightwalletd testing.

Reviewer Checklist

  • Full sync doesn't time out

Follow Up Work

We should revert this change once #4456 is fixed.

@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code A-devops Area: Pipelines, CI/CD and Dockerfiles P-Critical 🚑 I-slow Problems with performance or responsiveness I-integration-fail Continuous integration fails, including build and test failures C-testing Category: These are tests lightwalletd any work associated with lightwalletd labels May 22, 2022
@teor2345 teor2345 self-assigned this May 22, 2022
@teor2345 teor2345 changed the title fix(ci): finish full sync at 99.5% fix(ci): temporarily finish full sync at 99.5% May 22, 2022
@teor2345 teor2345 marked this pull request as ready for review May 22, 2022 23:38
@teor2345 teor2345 requested review from a team as code owners May 22, 2022 23:38
@teor2345 teor2345 requested review from gustavovalverde and conradoplg and removed request for a team May 22, 2022 23:38
@teor2345 teor2345 changed the title fix(ci): temporarily finish full sync at 99.5% fix(ci): Temporarily finish full sync at 99.5% May 22, 2022
@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #4457 (43e8ecb) into main (9425cb3) will increase coverage by 0.05%.
The diff coverage is n/a.

❗ Current head 43e8ecb differs from pull request most recent head 939701d. Consider uploading reports for the commit 939701d to get more accurate results

@@            Coverage Diff             @@
##             main    #4457      +/-   ##
==========================================
+ Coverage   78.88%   78.93%   +0.05%     
==========================================
  Files         304      304              
  Lines       37285    37285              
==========================================
+ Hits        29412    29432      +20     
+ Misses       7873     7853      -20     

@conradoplg
Copy link
Contributor

It failed by a few minutes. I'm retrying it in case we got bad luck, but it seems we'll need to make it a bit faster, or change the percentage.

@gustavovalverde
Copy link
Member

The full sync completes, but the next step (Create image from the state disk) is also included in the 6 hours timeout. I'm thinking on how to solve this.

I was thinking on decoupling this task from the job, but then the instance will be kept alive indefinitely if the deletion step is not executed. I'm seeing if we can put a timer on the instance itself to auto-delete after 6h:30m

zebrad/tests/common/sync.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 changed the title fix(ci): Temporarily finish full sync at 99.5% fix(ci): Temporarily finish full sync at 99.2% May 23, 2022
@teor2345
Copy link
Collaborator Author

It failed by a few minutes. I'm retrying it in case we got bad luck, but it seems we'll need to make it a bit faster, or change the percentage.

I changed it to 99.2% for now

@teor2345 teor2345 changed the title fix(ci): Temporarily finish full sync at 99.2% fix(ci): Temporarily finish full sync at 99% May 24, 2022
zebrad/tests/common/sync.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Collaborator Author

This works, and it gives us the temporary speed increase we need, so I'm just going to merge it to unblock other PRs.

We can revert it later.

@teor2345 teor2345 merged commit 49406f3 into main May 24, 2022
@teor2345 teor2345 deleted the full-sync-995 branch May 24, 2022 07:59
teor2345 added a commit that referenced this pull request May 24, 2022
teor2345 added a commit that referenced this pull request May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles A-rust Area: Updates to Rust code C-bug Category: This is a bug C-testing Category: These are tests I-integration-fail Continuous integration fails, including build and test failures I-slow Problems with performance or responsiveness lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants