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

Don't remove sector data when moving data into a shared path #7494

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Oct 11, 2021

Fixes #7484

Example of what could happen:

  • Worker A has access to paths 1 and 2
  • Worker B has access to path 1
  • Some sector is stored in path 2
  • Worker B wants that data moved to path 1:
    • (in most cases worker A should do the move locally, but there are some edge-cases here)
    • Worker B fetches the sector from A/2
    • B stores that data in path 1
    • Worker B tells Worker A to remove its copies of that sector
    • Worker A looks for it's copies of that sector, and removes it from both paths since it happens to have access to both

@magik6k magik6k requested a review from a team as a code owner October 11, 2021 19:10
@magik6k magik6k force-pushed the fix/move-shared-data-loss branch from 732da7a to f352c18 Compare October 11, 2021 19:11
@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #7494 (f352c18) into master (3a3edbb) will decrease coverage by 0.00%.
The diff coverage is 47.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7494      +/-   ##
==========================================
- Coverage   39.88%   39.88%   -0.01%     
==========================================
  Files         631      632       +1     
  Lines       66907    66918      +11     
==========================================
+ Hits        26684    26688       +4     
+ Misses      35607    35606       -1     
- Partials     4616     4624       +8     
Impacted Files Coverage Δ
extern/sector-storage/manager.go 64.94% <0.00%> (ø)
extern/sector-storage/stores/http_handler.go 73.59% <0.00%> (ø)
extern/sector-storage/stores/index.go 73.44% <ø> (+1.65%) ⬆️
extern/sector-storage/worker_local.go 62.08% <0.00%> (ø)
extern/sector-storage/stores/mocks/index.go 16.48% <14.28%> (ø)
extern/sector-storage/stores/mocks/store.go 27.58% <27.58%> (ø)
extern/sector-storage/stores/remote.go 58.35% <53.84%> (-0.03%) ⬇️
extern/sector-storage/stores/local.go 59.26% <100.00%> (+0.46%) ⬆️
extern/sector-storage/stores/mocks/pf.go 100.00% <100.00%> (ø)
chain/events/observer.go 71.64% <0.00%> (-6.72%) ⬇️
... and 24 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 3a3edbb...f352c18. Read the comment docs.

@jennijuju jennijuju added the SPX label Oct 12, 2021
@jennijuju jennijuju requested a review from arajasek October 12, 2021 18:53
@jennijuju jennijuju added this to the v1.13.1 milestone Oct 14, 2021
@magik6k magik6k merged commit a079516 into master Oct 15, 2021
@magik6k magik6k deleted the fix/move-shared-data-loss branch October 15, 2021 07:16
@neondragon
Copy link

This appears to have successfully prevented the problem reoccurring.

@Aloxaf
Copy link
Contributor

Aloxaf commented Oct 28, 2021

This PR seems also fix #5996 and #6121?

@RobQuistNL
Copy link
Contributor

And it fixes #7545

@RobQuistNL RobQuistNL mentioned this pull request Nov 5, 2021
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shared storage across workers can result in sealing sectors being deleted
6 participants