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

introduce MaxStagingDealsBytes - reject new deals if our staging deals area is full #7276

Merged
merged 4 commits into from
Sep 6, 2021

Conversation

nonsense
Copy link
Member

@nonsense nonsense commented Sep 6, 2021

This PR is introducing a MaxStagingDealsBytes config - if the markets repository deal-staging directory gets too large, the markets node can now be configured to reject new incoming deals.

A stopgap for #7241


MaxStagingDealsBytes should be added under the [Dealmaking] section of the markets' repo config.toml


TODO:

  • add a test as part of the itests suite (this PR has already been tested on a devnet)

@nonsense nonsense requested review from dirkmc and magik6k September 6, 2021 13:03
@codecov
Copy link

codecov bot commented Sep 6, 2021

Codecov Report

Merging #7276 (992cc3f) into master (212400b) will increase coverage by 0.03%.
The diff coverage is 68.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7276      +/-   ##
==========================================
+ Coverage   39.09%   39.12%   +0.03%     
==========================================
  Files         614      614              
  Lines       64895    64938      +43     
==========================================
+ Hits        25369    25408      +39     
- Misses      35127    35136       +9     
+ Partials     4399     4394       -5     
Impacted Files Coverage Δ
itests/kit/deals.go 82.98% <56.66%> (-4.82%) ⬇️
node/modules/storageminer.go 62.31% <80.00%> (+0.17%) ⬆️
itests/kit/ensemble.go 91.92% <100.00%> (+0.02%) ⬆️
itests/kit/node_opts.go 67.30% <100.00%> (+2.72%) ⬆️
node/builder_miner.go 94.81% <100.00%> (ø)
markets/loggers/loggers.go 89.28% <0.00%> (-10.72%) ⬇️
extern/sector-storage/manager_calltracker.go 57.70% <0.00%> (-4.85%) ⬇️
chain/stmgr/call.go 67.87% <0.00%> (-3.64%) ⬇️
storage/wdpost_sched.go 75.24% <0.00%> (-1.99%) ⬇️
chain/vm/vm.go 59.47% <0.00%> (-1.12%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 212400b...992cc3f. Read the comment docs.

@nonsense nonsense changed the title introduce MaxStagingDealsGiB - reject new deals if our staging deals area is full introduce MaxStagingDealsBytes - reject new deals if our staging deals area is full Sep 6, 2021
@nonsense nonsense marked this pull request as ready for review September 6, 2021 14:55
@nonsense nonsense requested a review from a team as a code owner September 6, 2021 14:55
@benjaminh83
Copy link

I'm not sure this fix will be really effective on making sure the market node does not crash. My .lotusmarket folder is currently able to handle 18TiB of data, and I would have no issue with it being filled if it was needed as a buffer. Issue here is the market node that stops working probably when it has x number of transfers all stuck at 0B transferred.
So setting a limit at x MiB on the deals-staging directory will be a pretty arbitrary number for a miner... Like how will we know how high we can set the limit before the market node gets shaky?

Also this is a pretty easy way to do a DoS on our miner. Estuary sends a miner 100 deals like a railgun and transfers 0B or very few and stream reset. Our folder filles up and we are now rejecting all deals from all clients until we purge the market node.
Wouldn't be better to look at what client IDs are actually transferring and if it keeps on initiating new deal transfers while it already has transfers at 0B, then reject the individual ID, and not just reject all incoming deals..

@benjaminh83
Copy link

Off cause, solving the issue of a single machine miner getting the disk full that also houses .lotus and .lotusminer, then it makes perfectly sense.
But tmpfs files has prior to the market split always lived inside the .lotusminer path, so its not a new thing that we should watch out for the available storage.
So maybe the throttle I'm referring to is more about making sure the lotus market node does not crash because of too many transfers, where this is more about not running out of space because of deals.

@nonsense
Copy link
Member Author

nonsense commented Sep 6, 2021

@benjaminh83 this config is about preventing the miner from getting out of disk space. If you run a sealing node and a markets node on the same host, at the moment it is very easy for the markets node to fill the disk space by accepting too many deals.

This PR is not attempting to address all the issues you are describing.


The sealing subsystem uses the scratch space and does accounting with respect to available disk space.

There is no such functionality on the markets service, so this PR is basically adding a config to say "do not use more than X GiB for staging deals". I hope that makes sense.

@jennijuju jennijuju added the P2 P2: Should be resolved label Sep 6, 2021
@jennijuju jennijuju added this to the v1.11.3 milestone Sep 6, 2021
@jennijuju
Copy link
Member

Would be really nice to get this in v1.11.3 as a stop gap, but okay to postpone to a later release

@magik6k magik6k merged commit 3a3a4fc into master Sep 6, 2021
@magik6k magik6k deleted the nonsense/reject-deal-too-many-staging-deals branch September 6, 2021 17:01
@@ -533,6 +537,16 @@ func BasicDealFilter(user dtypes.StorageDealFilter) func(onlineOk dtypes.Conside
return false, "miner error", err
}

diskUsageBytes, err := r.DiskUsage(r.Path() + "/deal-staging")
Copy link
Contributor

Choose a reason for hiding this comment

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

should be filepath.Join, also ideally we'd not hardcode 'deal-staging'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 P2: Should be resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants