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

Only run the lightwalletd full sync on the main branch #5316

Closed
teor2345 opened this issue Oct 3, 2022 · 4 comments · Fixed by #5393
Closed

Only run the lightwalletd full sync on the main branch #5316

teor2345 opened this issue Oct 3, 2022 · 4 comments · Fixed by #5393
Assignees
@gustavovalverde
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-bug Category: This is a bug I-cost Zebra infrastructure costs I-slow Problems with performance or responsiveness

Comments

@teor2345
Copy link
Contributor

teor2345 commented Oct 3, 2022 •
edited
Loading

Motivation

In PR #5164, we made lightwalletd sync all the way to the tip in its full sync test.

This increases that test's time from 1 hour to 4 hours, which makes the CI we run on each PR change increase from 3 hours to 6 hours.

This increases Google Cloud costs, and makes PR testing and merges slower for engineers.

Designs

We're already running the full zebrad sync only on the main branch.
We can copy and paste the workflow code to the full lightwalletd sync, so this should be an easy change.

We might also need to update the patch workflows or the branch protection rules.
@teor2345 teor2345 added C-bug Category: This is a bug A-devops Area: Pipelines, CI/CD and Dockerfiles S-needs-triage Status: A bug report needs triage P-Medium ⚡ I-slow Problems with performance or responsiveness I-cost Zebra infrastructure costs labels Oct 3, 2022
@teor2345
Copy link
Contributor Author

teor2345 commented Oct 3, 2022

@mpguerra PR #5164 will double the time it takes to run CI on every PR change (from 3 hours to 6 hours).

So we might want to schedule this work early in the next sprint, or right after PR #5164 merges.

@mpguerra
Copy link
Contributor

mpguerra commented Oct 3, 2022

Motivation

In PR #5164, we made lightwalletd sync all the way to the tip in its full sync test.

What was it doing before and why did we change it?

@teor2345
Copy link
Contributor Author

teor2345 commented Oct 3, 2022

Motivation

In PR #5164, we made lightwalletd sync all the way to the tip in its full sync test.

What was it doing before and why did we change it?

We were skipping all the blocks after 1.7 million, because we needed to add some extra test code to test those blocks. But this meant that we weren't testing those blocks at all. So we didn't find issues like #5307 until now.

@mpguerra
Copy link
Contributor

mpguerra commented Oct 6, 2022

Hey team! Please add your planning poker estimate with Zenhub @arya2 @conradoplg @dconnolly @gustavovalverde @oxarbitrage @teor2345 @upbqdn

gustavovalverde added a commit that referenced this issue Oct 12, 2022
@gustavovalverde
ci(sync): only run the lightwalletd full sync on the main branch
c6c621c 
Previous behavior:
In PR #5164, we made lightwalletd sync all the way to the tip in its full
sync test.

This increases that test's time from 1 hour to 4 hours, which makes the CI
we run on each PR change increase from 3 hours to 6 hours.

Expected behavior:
Run the lightwalletd full sync just on `main` or if a state disk for the
actual version is not found.

Solution:
Add the `github.event_name == 'push' && github.ref_name == 'main'` condition
to the `lightwalletd-full-sync` test.

Fixes #5316
@gustavovalverde gustavovalverde mentioned this issue Oct 12, 2022
Merged
5 tasks
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Oct 19, 2022
@mergify mergify bot closed this as completed in #5393 Oct 19, 2022
mergify bot pushed a commit that referenced this issue Oct 19, 2022
@gustavovalverde @teor2345
ci(sync): only run the lightwalletd full sync on the main branch (#…
dae1d92 
…5393)

* ci(sync): only run the `lightwalletd` full sync on the `main` branch

Previous behavior:
In PR #5164, we made lightwalletd sync all the way to the tip in its full
sync test.

This increases that test's time from 1 hour to 4 hours, which makes the CI
we run on each PR change increase from 3 hours to 6 hours.

Expected behavior:
Run the lightwalletd full sync just on `main` or if a state disk for the
actual version is not found.

Solution:
Add the `github.event_name == 'push' && github.ref_name == 'main'` condition
to the `lightwalletd-full-sync` test.

Fixes #5316

* Allow lwd full syncs to be triggered manually (#5400)

* Limit checkpoint and lwd full sync concurrency

* Add a patch job for lightwalletd-full-sync

Co-authored-by: teor <teor@riseup.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gustavovalverde gustavovalverde

Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-bug Category: This is a bug I-cost Zebra infrastructure costs I-slow Problems with performance or responsiveness
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ci(sync): only run the lightwalletd full sync on the main branch ZcashFoundation/zebra
3 participants
@mpguerra @teor2345 @gustavovalverde