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: sealing: StartEpochSealingBuffer triggers packing on timer #7905

Merged
merged 8 commits into from
Feb 9, 2022

Conversation

ZenGround0
Copy link
Contributor

@ZenGround0 ZenGround0 commented Jan 9, 2022

Proposed Changes

Improvements to snap deals integration beyond the existing mvp PR

  • use the config's StartEpochSealingBuffer param as a way to enforce that sectors are packed for sealing / updating no matter how many deals they have if the nearest deal start date is close enough to the present. This solves an existing problem predating snap deals (FYI @jennijuju)
  • Config option for disabling automatic creation of new sectors for handling incoming deals. Forces user to trigger inclusion by marking sectors for snap upgrade.
    - [ ] Recycle assigned deals in updating sectors that have been aborted so that these deals can be reassigned to other sectors

@jennijuju jennijuju added this to the v1.14.0 milestone Jan 10, 2022
@jennijuju
Copy link
Member

jennijuju commented Jan 10, 2022

Nice - I assume it is intentional that this will apply to regular deal sectors too?

the remaining two todos would be really nice to have - I think we should aim this for lotus v1.15.0 (Jan 18). (I feel like its a bit tight to get this implemented and tested in v1.14.0, which code freezes in 2 days. its a super nice enhancement to the currently sealing pipeline - so okay to wait for v1.15.0)

@jennijuju jennijuju modified the milestones: v1.14.0, v1.15.0 Jan 10, 2022
@jennijuju
Copy link
Member

Please do **not ** merge this PR without a pr to https://lotus.filecoin.io/docs/storage-providers/config/

@ZenGround0
Copy link
Contributor Author

ZenGround0 commented Jan 10, 2022

It turns out that recycling deals from a failed sector is a more involved change than I had thought and therefore is going to have to wait beyond v 1.15.0. I am going to file an issue which I'll link to from back here describing the problem a bit more.

--edit--
This is well covered in filecoin-project/boost#1345 where I've identified some problems

@ZenGround0 ZenGround0 force-pushed the feat/snap-deals-add-ons branch 2 times, most recently from d6be162 to ee98879 Compare January 10, 2022 10:24
Base automatically changed from feat/snap-deals to master January 11, 2022 17:46
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, just 2 small nits

node/config/types.go Outdated Show resolved Hide resolved
sealTime := time.Unix(sector.CreationTime, 0).Add(cfg.WaitDealsDelay)

// check deal age, start sealing when the deal closest to starting is within slack time
safeSealTime := sealTime
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this copied variable, can just use sealTime directly

@BlocksOnAChain
Copy link
Contributor

@ZenGround0 can we merge it for 1.15.0, since I think we got positive feedback from Magik?
CC: @jennijuju

@ZenGround0 ZenGround0 marked this pull request as ready for review February 8, 2022 17:40
@ZenGround0 ZenGround0 requested a review from a team as a code owner February 8, 2022 17:40
@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #7905 (485568d) into master (899ae8a) will decrease coverage by 0.04%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    filecoin-project/lotus#7905      +/-   ##
==========================================
- Coverage   39.12%   39.08%   -0.05%     
==========================================
  Files         660      660              
  Lines       71459    71474      +15     
==========================================
- Hits        27961    27938      -23     
- Misses      38681    38710      +29     
- Partials     4817     4826       +9     
Impacted Files Coverage Δ
extern/storage-sealing/input.go 53.89% <38.46%> (-0.66%) ⬇️
node/config/def.go 97.48% <100.00%> (+0.01%) ⬆️
node/modules/storageminer.go 63.56% <100.00%> (+0.05%) ⬆️
chain/actors/builtin/miner/diff.go 48.52% <0.00%> (-10.30%) ⬇️
markets/retrievaladapter/client_blockstore.go 62.50% <0.00%> (-6.25%) ⬇️
miner/warmup.go 70.83% <0.00%> (-4.17%) ⬇️
chain/types/tipset_key.go 85.96% <0.00%> (-3.51%) ⬇️
extern/sector-storage/sched_worker.go 77.32% <0.00%> (-2.33%) ⬇️
storage/miner.go 69.31% <0.00%> (-2.28%) ⬇️
miner/miner.go 56.06% <0.00%> (-1.97%) ⬇️
... and 17 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 899ae8a...485568d. Read the comment docs.

@jennijuju jennijuju merged commit 7319420 into master Feb 9, 2022
@jennijuju jennijuju deleted the feat/snap-deals-add-ons branch February 9, 2022 02:06
@jennijuju
Copy link
Member

oh btw! bad pr title!! reminder to follow the one in pr template next time

@jennijuju jennijuju changed the title StartEpochSealingBuffer triggers packing on timer feat: sealing: StartEpochSealingBuffer triggers packing on timer Feb 9, 2022
@ZenGround0
Copy link
Contributor Author

ZenGround0 commented Feb 9, 2022

@jennijuju You'll need to help me read the template better because I tried my best to follow it.

PR type: :

The title is feat: sealing: * description of change being made* right?

PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test

feat

area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps

sealing

Are the underscores what you are talking about? Double checking they're not being used in other PRs.

@jennijuju
Copy link
Member

jennijuju commented Feb 9, 2022

(just to close the loop from slack) what you said is all ✅

when I left the comment, the title was " StartEpochSealingBuffer triggers packing on timer " and I update it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants