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

chore: deps: update go-libp2p to v0.21 #8970

Merged
merged 15 commits into from
Aug 19, 2022

Conversation

MarcoPolo
Copy link
Contributor

@MarcoPolo MarcoPolo commented Jul 5, 2022

Related Issues

Proposed Changes

Draft PR to help update lotus to use the latest release of go-libp2p.

Also adds observability to the resource manager via prometheus so operators can debug blocked resources better. See https://ipfs.marcopolo.io/d/MgmGIjjnk/resource-manager?orgId=1 for a live example.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green
  • We should merge Update go libp2p v0.21 go-fil-markets#744 first and we can update the go-fil-markets dep here as well.
  • Merge and release Chore: update to go-libp2p 0.21 libp2p/go-libp2p-kad-dht#784 (currently referencing this commit directly)

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #8970 (daf1731) into master (75d78de) will increase coverage by 5.45%.
The diff coverage is 29.09%.

❗ Current head daf1731 differs from pull request most recent head b5caf08. Consider uploading reports for the commit b5caf08 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8970      +/-   ##
==========================================
+ Coverage   35.19%   40.65%   +5.45%     
==========================================
  Files         703      707       +4     
  Lines       78466    78671     +205     
==========================================
+ Hits        27620    31987    +4367     
+ Misses      45851    41209    -4642     
- Partials     4995     5475     +480     
Impacted Files Coverage Δ
api/types.go 52.77% <ø> (ø)
node/impl/net/rcmgr.go 0.00% <0.00%> (ø)
node/modules/lp2p/smux.go 9.09% <ø> (ø)
node/modules/lp2p/rcmgr.go 25.65% <42.10%> (-7.68%) ⬇️
chain/events/state/fastapi.go 75.00% <0.00%> (-25.00%) ⬇️
storage/pipeline/currentdealinfo.go 71.42% <0.00%> (-4.77%) ⬇️
storage/wdpost/wdpost_run_faults.go 68.30% <0.00%> (-2.82%) ⬇️
chain/consensus/filcns/mine.go 66.66% <0.00%> (-2.39%) ⬇️
storage/sealer/sched_assigner_common.go 78.40% <0.00%> (-2.28%) ⬇️
... and 149 more

@jennijuju
Copy link
Member

jennijuju commented Aug 8, 2022

@MarcoPolo thank you for opening the draft PR! We noticed that https://github.com/libp2p/go-libp2p/releases/tag/v0.21.0 is now published, im wondering if you are planning to update this branch to the stable release and open PR for review?

it'd be great if we can have rcmgr autoscale in lotus & and have lotus nodes exposes opencensus metrics as well

@MarcoPolo
Copy link
Contributor Author

@MarcoPolo thank you for opening the draft PR! We noticed that https://github.com/libp2p/go-libp2p/releases/tag/v0.21.0 is now published, im wondering if you are planning to update this branch to the stable release and open PR for review?

it'd be great if we can have rcmgr autoscale in lotus & and have lotus nodes exposes opencensus metrics as well

Yup! planning on doing that today :)

I'll probably split this into two PRs. One with just the dep updates and the other that enables the opencensus metrics.

@MarcoPolo
Copy link
Contributor Author

We should merge filecoin-project/go-fil-markets#744 first and we can update the go-fil-markets dep here as well.

@MarcoPolo MarcoPolo marked this pull request as ready for review August 11, 2022 20:43
@MarcoPolo MarcoPolo requested a review from a team as a code owner August 11, 2022 20:43
@jennijuju
Copy link
Member

thanks @MarcoPolo

pinged market maintainer for t he review there, tho we probably should do separate PR for updating the market version!

@jennijuju jennijuju changed the title chore: deps: update go-libp2p to v0.21-RC chore: deps: update go-libp2p to v0.21 Aug 11, 2022
@jennijuju jennijuju requested a review from vyzo August 11, 2022 21:04
@MarcoPolo
Copy link
Contributor Author

thanks @MarcoPolo

pinged market maintainer for t he review there, tho we probably should do separate PR for updating the market version!

We need to update go-fil-markets before merging this PR since there was a breaking change in go-libp2p-core and the current go-fil-markets wouldn't work with this PR since it updates go-libp2p-core.

You get this error if you try:

go: finding module for package github.com/libp2p/go-libp2p-core/mux
github.com/filecoin-project/lotus/node imports
        github.com/filecoin-project/go-fil-markets/retrievalmarket/network imports
        github.com/libp2p/go-libp2p-core/mux: module github.com/libp2p/go-libp2p-core@latest found (v0.19.1), but does not contain package github.com/libp2p/go-libp2p-core/mux

if err != nil {
return nil, fmt.Errorf("error creating resource manager stats reporter: %w", err)
}
view.Register(obs.DefaultViews...)
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 registers the resource managers opencensus metrics

@jennijuju
Copy link
Member

there was a breaking change

ah- ack!

@jacobheun
Copy link
Contributor

It looks like go-fil-markets and dht had releases for this change, what else is needed here? We have some overlap with this changeset on Boost.

Copy link
Contributor

@magik6k magik6k 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.

Will merge with tagged markets

@magik6k magik6k merged commit ba67431 into filecoin-project:master Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants