feat: sealing: Sector upgrade queue #8330

magik6k · 2022-03-16T16:44:27Z

Related Issues

Closes #8231

Proposed Changes

Add a new Available sector state for CC sectors available for upgrading ('available storage')
After sectors are marked for upgrade, they go into the Available state instead of SnapDealsWaitDeals
When a deal arrives to the sealing pipeline, and it can't be matched to any sectors:
- We'll try to see if any Available sectors can accept it - we'll select a candidate with lowest initial pledge (most rational to upgrade?)
- If we don't find any good available sectors, we'll just create a new CC sector if that's not disabled in the config

Additional Info

TODO:

Test on a real setup
Add a config which would make newly sealed sectors automatically become Available (maybe a separate PR)
When considering Active sectors run all snapdeal checks
Support reverting from Available to Proving

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

ZenGround0

Looking good, keep me posted when this gets out of draft

ZenGround0 · 2022-03-16T17:51:52Z

extern/storage-sealing/sealing.go

@@ -106,6 +106,8 @@ type Sealing struct {
 	assignedPieces map[abi.SectorID][]cid.Cid
 	creating       *abi.SectorNumber // used to prevent a race where we could create a new sector more than once

+	available map[abi.SectorID]struct{}


I think this will become stale if the miner restarts. Are there existing patterns for traversing sector infos to repopulate this for all infos in Available state?

The Available state handler gets called on restart, so this will get repopulated

woah cool, is this the same for all state handlers?

extern/storage-sealing/states_proving.go

extern/storage-sealing/input.go

itests/ccupgrade_test.go

extern/storage-sealing/input.go

ZenGround0

Unsure about a few things but looks ready for minerX testing.

ZenGround0 · 2022-03-16T19:56:35Z

documentation/en/default-lotus-miner-config.toml

+  #
+  # type: bool
+  # env var: LOTUS_SEALING_MAKECCSECTORSAVAILABLE
+  #MakeCCSectorsAvailable = false


ZenGround0 · 2022-03-16T19:58:41Z

extern/storage-sealing/fsm.go

@@ -136,6 +137,7 @@ var fsmPlanners = map[SectorState]func(events []statemachine.Event, state *Secto

 	FinalizeSector: planOne(
 		on(SectorFinalized{}, Proving),
+		on(SectorFinalizedAvailable{}, Available),


What's the point of FinalizedAvailable? Why not instead go to Available from proving if the MakeCCSectorsAvailable config is set? That way we can avoid coupling with CommitFinalize.

You can't really distinguish CC sectors already in the proving state from CC sectors entering the proving state (that's because on restart we run all state handlers for current states of all sectors). If we were to do that without tracking existing CC sectors, all CC sectors would get converted to Available, which I'm quite sure is not what SPs will want.

Other things we could do instead of this event:

Additional state deciding this

Now we need 2 events, and an additional state

Add a boolean to SectorInfo to track which CC sectors are Proving already, and shouldn't be auto-converted to Available

Pretty sure this still requires an additional event to update this variable. Also adds more bits of private per-sector state, which ideally wouldn't exist

Thanks makes sense. That run all state handlers on restart fact is pretty important.

ZenGround0 · 2022-03-16T20:02:56Z

extern/storage-sealing/input.go

+
+	log.Infow("Upgrading sector", "number", candidate.Number, "type", "deal", "proofType", sp, "expiration", bestExpiration, "pledge", types.FIL(bestPledge))
+	delete(m.available, candidate)
+	m.creating = &candidate.Number


Why set creating I thought this was only needed when making a new sector, not dealing with an existing one.

This var is used to synchronize sector event processing - basically we want to make sure that we don't re-enter tryGetDealSector before the sector we've just asked to enter WaitDeals* adds itself to m.openSectors.

Renamed the var to something which makes more sense given that we now also use it for snapdeals

ZenGround0 · 2022-03-16T20:10:03Z

extern/storage-sealing/sealing.go

@@ -63,7 +64,7 @@ type SealingAPI interface {
 	StateMinerInfo(context.Context, address.Address, TipSetToken) (miner.MinerInfo, error)
 	StateMinerAvailableBalance(context.Context, address.Address, TipSetToken) (big.Int, error)
 	StateMinerSectorAllocated(context.Context, address.Address, abi.SectorNumber, TipSetToken) (bool, error)
-	StateMinerActiveSectors(context.Context, address.Address, TipSetToken) ([]*miner.SectorOnChainInfo, error)
+	StateMinerActiveSectors(context.Context, address.Address, TipSetToken) (bitfield.BitField, error)


ZenGround0 · 2022-03-16T20:10:20Z

extern/storage-sealing/sealing.go

@@ -106,6 +106,8 @@ type Sealing struct {
 	assignedPieces map[abi.SectorID][]cid.Cid
 	creating       *abi.SectorNumber // used to prevent a race where we could create a new sector more than once

+	available map[abi.SectorID]struct{}


woah cool, is this the same for all state handlers?

codecov · 2022-03-16T20:40:50Z

Codecov Report

Merging #8330 (60ba133) into master (159da73) will decrease coverage by 0.04%.
The diff coverage is 62.50%.

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

@@            Coverage Diff             @@
##           master    #8330      +/-   ##
==========================================
- Coverage   40.38%   40.33%   -0.05%     
==========================================
  Files         679      679              
  Lines       73898    73985      +87     
==========================================
  Hits        29842    29842              
- Misses      38845    38903      +58     
- Partials     5211     5240      +29

Impacted Files	Coverage Δ
cmd/lotus-miner/info.go	`60.11% <ø> (ø)`
extern/storage-sealing/mocks/api.go	`0.00% <0.00%> (ø)`
extern/storage-sealing/states_failed.go	`0.00% <0.00%> (ø)`
extern/storage-sealing/states_sealing.go	`38.10% <0.00%> (-0.17%)`	⬇️
extern/storage-sealing/fsm_events.go	`50.62% <50.00%> (+0.62%)`	⬆️
storage/adapter_storage_miner.go	`50.00% <50.00%> (-0.22%)`	⬇️
extern/storage-sealing/input.go	`54.61% <58.94%> (+1.04%)`	⬆️
extern/storage-sealing/upgrade_queue.go	`32.14% <83.33%> (-13.10%)`	⬇️
extern/storage-sealing/fsm.go	`57.04% <100.00%> (-0.73%)`	⬇️
extern/storage-sealing/sealing.go	`78.88% <100.00%> (+0.23%)`	⬆️
... and 27 more

jennijuju · 2022-03-16T21:22:49Z

COOOOOOOOOLLLL

jennijuju · 2022-03-16T21:23:43Z

oh yess this is cool we should have it in v1.15.1

feat: sealing: Sector upgrade queue

aef2ec5

magik6k requested a review from a team as a code owner March 16, 2022 16:44

feat: sealing: Linter did have a good point

193c848

magik6k marked this pull request as draft March 16, 2022 16:47

magik6k added 3 commits March 16, 2022 17:48

itests: TestTooManyMarkedForUpgrade is not applicable anymore

8600ae2

sealing: Support abort uprade for Available sectors

628102a

itests: Test reverting Available sectors to Proving

799d3eb

ZenGround0 reviewed Mar 16, 2022

View reviewed changes

magik6k added 8 commits March 16, 2022 19:29

sealing: Add MakeCCSectorsAvailable config

4ded059

sealing: Use bitfields in sectorActive

4d47394

sealing: Run more checks when considering Available sectors

e7ee5b5

sealing: Address snap queue review

724d80c

sealing: Fix build

aac1f5a

sealing: Enforce minimum expiration in tryGetUpgradeSector

3f23cf0

sealing: regenerate mocks

2961f3b

sealing: fix lint issues

3b2a1ea

magik6k marked this pull request as ready for review March 16, 2022 19:33

ZenGround0 approved these changes Mar 16, 2022

View reviewed changes

sealing: Rename Sealing.creating to a less confusing name

60ba133

miner cli: Treat Activating sectors like Proving in sectors list

a431fdb

jennijuju added the release/backport label Mar 16, 2022

jennijuju added this to the v1.15.1 milestone Mar 16, 2022

magik6k merged commit a3bdd29 into master Mar 16, 2022

magik6k deleted the feat/snap-queue branch March 16, 2022 21:24

magik6k mentioned this pull request Mar 16, 2022

backport: v1.15.1: feat: sealing: Sector upgrade queue #8333

Merged

5 tasks

jennijuju removed the release/backport label Apr 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: sealing: Sector upgrade queue #8330

feat: sealing: Sector upgrade queue #8330

magik6k commented Mar 16, 2022 •

edited

Loading

ZenGround0 left a comment

ZenGround0 Mar 16, 2022

magik6k Mar 16, 2022

ZenGround0 Mar 16, 2022

magik6k Mar 16, 2022

ZenGround0 left a comment

ZenGround0 Mar 16, 2022

ZenGround0 Mar 16, 2022

magik6k Mar 16, 2022

ZenGround0 Mar 16, 2022

ZenGround0 Mar 16, 2022

magik6k Mar 16, 2022

ZenGround0 Mar 16, 2022

ZenGround0 Mar 16, 2022

codecov bot commented Mar 16, 2022 •

edited

Loading

jennijuju commented Mar 16, 2022

jennijuju commented Mar 16, 2022

feat: sealing: Sector upgrade queue #8330

feat: sealing: Sector upgrade queue #8330

Conversation

magik6k commented Mar 16, 2022 • edited Loading

Related Issues

Proposed Changes

Additional Info

Checklist

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 16, 2022 • edited Loading

Codecov Report

jennijuju commented Mar 16, 2022

jennijuju commented Mar 16, 2022

magik6k commented Mar 16, 2022 •

edited

Loading

codecov bot commented Mar 16, 2022 •

edited

Loading