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

feat(zetacore): add upgrade tracker #2095

Merged
merged 7 commits into from
May 16, 2024
Merged

Conversation

gartnera
Copy link
Member

@gartnera gartnera commented Apr 29, 2024

Description

When we start deploying every change from develop on "developnet", we will need the ability to run each store upgrade/migration function individually as the are added.

Introduce the upgrade version tracker which will manage tracking these upgrades. You must now assign a monotonically increasing index for each store upgrade. We store the maximum index on the disk and skip any store upgrades with indexes less than the stored value.

This tracker is only fully used when you explicitly enable it via environment variable. Otherwise, we assume you're doing a major version upgrade (v14 -> v16) and apply all upgrades.

Blocked by resolution of #2077. Update: I am fixing that in this PR too.

Part of DEVOP-642

More About developnet

developnet will be a stateful network where every change from develop is automatically built and deployed via governance proposals. We will use the git commit timestamp for the version number each build, so upgrades will look like this:

  • v0.0.1714761000-develop
  • v0.0.1714764425-develop
  • v0.0.1714771234-develop

Since each change will be deployed individually, we must adjust the upgrade handler logic to only run the specific migration logic for each change rather than all migrations every upgrade. An upgrade handler must be defined for every upgrade even if there are no migrations being run.

It is expected that developnet will occasionally need to have it's state reset due to breakages. We will try to minimize the need to reset it's state over time by improving CI checks.

How Has This Been Tested?

Checklist:

  • I have added unit tests that prove my fix feature works
  • three way upgrade tests pass
  • think about returning error vs panic

@gartnera gartnera changed the title feat(zetacore): add develop store upgrde tracker feat(zetacore): add develop store upgrade tracker Apr 29, 2024
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 48.23529% with 44 lines in your changes are missing coverage. Please review.

Project coverage is 68.24%. Comparing base (d9a5d44) to head (a45ab9c).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2095      +/-   ##
===========================================
- Coverage    72.34%   68.24%   -4.11%     
===========================================
  Files          249      259      +10     
  Lines        14007    14927     +920     
===========================================
+ Hits         10134    10187      +53     
- Misses        3412     4278     +866     
- Partials       461      462       +1     
Files Coverage Δ
app/app.go 2.31% <0.00%> (ø)
app/upgrade_tracker.go 95.34% <95.34%> (ø)
app/setup_handlers.go 0.00% <0.00%> (ø)

... and 7 files with indirect coverage changes

Copy link

github-actions bot commented May 1, 2024

!!!WARNING!!!
nosec detected in the following files: app/upgrade_tracker.go

Be very careful about using #nosec in code. It can be a quick way to suppress security warnings and move forward with development, it should be employed with caution. Suppressing warnings with #nosec can hide potentially serious vulnerabilities. Only use #nosec when you're absolutely certain that the security issue is either a false positive or has been mitigated in another way.

Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203
Broad #nosec annotations should be avoided, as they can hide other vulnerabilities. The CI will block you from merging this PR until you remove #nosec annotations that do not target specific rules.

Pay extra attention to the way #nosec is being used in the files listed above.

@github-actions github-actions bot added the nosec label May 1, 2024
@gartnera gartnera changed the title feat(zetacore): add develop store upgrade tracker feat(zetacore): add upgrade tracker May 1, 2024
@gartnera gartnera marked this pull request as ready for review May 1, 2024 18:24
Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

Blocked by resolution of #2077. I could fix that in this PR too.

Is it what #2101 is about? @kingpinXD

app/setup_handlers.go Outdated Show resolved Hide resolved
app/setup_handlers.go Outdated Show resolved Hide resolved
app/setup_handlers.go Outdated Show resolved Hide resolved
app/setup_handlers.go Outdated Show resolved Hide resolved
app/setup_handlers.go Outdated Show resolved Hide resolved
app/setup_handlers.go Outdated Show resolved Hide resolved
app/setup_handlers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kingpinXD kingpinXD left a comment

Choose a reason for hiding this comment

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

@gartnera , it would be helpful it ,you could add details of how this upgrade tracker is going to be used , and also why its needed .
Some context would be helpful in reviewing this PR .

@gartnera
Copy link
Member Author

gartnera commented May 3, 2024

@gartnera , it would be helpful it ,you could add details of how this upgrade tracker is going to be used , and also why its needed . Some context would be helpful in reviewing this PR .

@kingpinXD I added a bit more info in the PR description, please let me know which areas need additional clarification.

@kingpinXD
Copy link
Contributor

kingpinXD commented May 7, 2024

expected that developnet will occasionally need to have it's state reset due to breakages. We will try to minimize the need to reset it's state over time by improving CI checks.

Thanks that was quite helpful , I am wondering though , why cant we use the existing releases that we have , to ru upgrade developnet , without having to manage it through a tracker .

For example we could apply updates 1 to 14 , one by one after every X number of blocks to reach a v14 network , if needed .The gov proposals can be managed via an external script without baking anything into the zetacored binary

I would imagine development to be very similar to athens 3 , just that the users would be internal .

@gartnera
Copy link
Member Author

gartnera commented May 7, 2024

Thanks that was quite helpful , I am wondering though , why cant we use the existing releases that we have , to ru upgrade developnet , without having to manage it through a tracker .

because

Since each change will be deployed individually, we must adjust the upgrade handler logic to only run the specific migration logic for each change rather than all migrations every upgrade. An upgrade handler must be defined for every upgrade even if there are no migrations being run.

The existing upgrade handler logic will not tolerate the migrations being run multiple times. We must be more granular in how we track and apply upgrades for develop builds.

@kingpinXD
Copy link
Contributor

kingpinXD commented May 8, 2024

Yeah, I am just saying, that can't Developnet use only the existing releases?
I don't see a reason for testing an upgrade when there is no actual release for it

@gartnera
Copy link
Member Author

gartnera commented May 8, 2024

Yeah, I am just saying, that can't Developnet use only the existing releases?

We could but we would have to reset the state of the network every deployment. That would increase the difficulty of connecting to btc/eth testnets. Not impossible, but we would require much more protocol support.

I don't see a reason for testing an upgrade when there is no actual release for it

The main purpose of developnet is giving internal and external developers the ability to interact with the latest code from develop. Our team is specifically interested in detecting breaking changes to metrics for example.

@gartnera gartnera force-pushed the develop-version-upgrades branch from b822b3c to 0262f99 Compare May 9, 2024 18:11
@gartnera gartnera added the no-changelog Skip changelog CI check label May 9, 2024
@CharlieMc0
Copy link
Member

Yeah, I am just saying, that can't Developnet use only the existing releases? I don't see a reason for testing an upgrade when there is no actual release for it

To add to this. The last release v15 was Mar 20 which means we haven't ran the latest code on any public networks for almost two months. The goal with DevelopNet is to decrease the delay between when the Protocol team commits code and when internal and external devs see it running on a long(ish) lived public network.

@CharlieMc0
Copy link
Member

@gartnera can you fix the merge conflict?

@kingpinXD @lumtis anything else blocking this from moving forward?

@gartnera gartnera force-pushed the develop-version-upgrades branch from f1b2d8d to 044d312 Compare May 13, 2024 18:28
@gartnera
Copy link
Member Author

@gartnera can you fix the merge conflict?

done

app/setup_handlers.go Outdated Show resolved Hide resolved
@gartnera gartnera force-pushed the develop-version-upgrades branch from 9490300 to a6c3fe1 Compare May 13, 2024 20:29
Copy link
Contributor

@kingpinXD kingpinXD left a comment

Choose a reason for hiding this comment

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

looks good overall

app/upgrade_tracker.go Show resolved Hide resolved
app/upgrade_tracker.go Outdated Show resolved Hide resolved
app/upgrade_tracker.go Outdated Show resolved Hide resolved
app/upgrade_tracker.go Show resolved Hide resolved
@gartnera gartnera force-pushed the develop-version-upgrades branch from f3e2743 to de99a89 Compare May 14, 2024 17:25
app/setup_handlers.go Show resolved Hide resolved
app/setup_handlers.go Show resolved Hide resolved
app/setup_handlers.go Show resolved Hide resolved
app/upgrade_tracker.go Outdated Show resolved Hide resolved
app/upgrade_tracker.go Show resolved Hide resolved
app/upgrade_tracker.go Outdated Show resolved Hide resolved
@gartnera gartnera force-pushed the develop-version-upgrades branch from de99a89 to a342b84 Compare May 15, 2024 16:24
@gartnera gartnera requested a review from skosito May 15, 2024 16:26
@gartnera gartnera force-pushed the develop-version-upgrades branch from a342b84 to a45ab9c Compare May 16, 2024 17:41
@gartnera gartnera merged commit 2e7dac2 into develop May 16, 2024
19 of 21 checks passed
@gartnera gartnera deleted the develop-version-upgrades branch May 16, 2024 18:10
@gartnera
Copy link
Member Author

works when #2198 is applied

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Skip changelog CI check nosec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants