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/sector storage unseal #7730

Merged
merged 1 commit into from
Dec 8, 2021
Merged

Feat/sector storage unseal #7730

merged 1 commit into from
Dec 8, 2021

Conversation

ZenGround0
Copy link
Contributor

@ZenGround0 ZenGround0 commented Dec 2, 2021

This extends sector-storage to do unseal with snap deals replicas (+sector key) and to do sector key generation with snap deals replicas (+ unsealed data)

The snap deals test is extended to exercise both unsealing of data in a snap deals replica and removing and regenerating the sector key.

@ZenGround0 ZenGround0 requested a review from a team as a code owner December 2, 2021 15:57
@ZenGround0 ZenGround0 force-pushed the feat/sector-storage-unseal branch from 3757800 to dcd348a Compare December 2, 2021 16:15
@ZenGround0 ZenGround0 requested a review from magik6k December 2, 2021 20:05
@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #7730 (1978317) into next (91f6d4b) will increase coverage by 0.04%.
The diff coverage is 50.00%.

❗ Current head 1978317 differs from pull request most recent head a5be808. Consider uploading reports for the commit a5be808 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             next    #7730      +/-   ##
==========================================
+ Coverage   39.55%   39.59%   +0.04%     
==========================================
  Files         646      646              
  Lines       68606    68687      +81     
==========================================
+ Hits        27137    27197      +60     
- Misses      36784    36795      +11     
- Partials     4685     4695      +10     
Impacted Files Coverage Δ
api/api_storage.go 0.00% <ø> (ø)
extern/sector-storage/mock/mock.go 61.34% <0.00%> (-0.70%) ⬇️
extern/sector-storage/sealtasks/task.go 81.81% <ø> (ø)
extern/sector-storage/storiface/worker.go 53.84% <ø> (ø)
extern/sector-storage/manager.go 62.24% <48.78%> (-0.83%) ⬇️
extern/sector-storage/ffiwrapper/sealer_cgo.go 59.61% <52.94%> (+0.81%) ⬆️
extern/sector-storage/worker_local.go 63.08% <71.42%> (+0.20%) ⬆️
extern/sector-storage/manager_calltracker.go 59.03% <0.00%> (-3.53%) ⬇️
chain/vm/mkactor.go 48.48% <0.00%> (-3.04%) ⬇️
chain/stmgr/searchwait.go 66.02% <0.00%> (-1.29%) ⬇️
... and 18 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 91f6d4b...a5be808. Read the comment docs.

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 reasonable, just 2 questions

extern/sector-storage/ffiwrapper/sealer_cgo.go Outdated Show resolved Hide resolved
Comment on lines +621 to +624
// NOTE: We set allowFetch to false in so that we always execute on a worker
// with direct access to the data. We want to do that because this step is
// generally very cheap / fast, and transferring data is not worth the effort
selector := newExistingSelector(m.index, sector.ID, storiface.FTUnsealed|storiface.FTUpdate|storiface.FTUpdateCache|storiface.FTCache, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have benchmarks on how fast this is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I'll add some basic benchmarking to the unit test so that @cryptonemo can generate a report on the big sectors.

- Unsealing replica update with sector key works and tested
- Sector key generation added and tested
@ZenGround0 ZenGround0 force-pushed the feat/sector-storage-unseal branch from 1978317 to a5be808 Compare December 3, 2021 20:22
@ZenGround0 ZenGround0 merged commit ac31651 into next Dec 8, 2021
@ZenGround0 ZenGround0 deleted the feat/sector-storage-unseal branch December 8, 2021 15:10
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.

2 participants