Repository navigation
Conversation
…RemoteFinalize The move selector only preferred a worker when the sector was already in one of the destination paths, so a move from a sealing path to a long-term storage path on the same worker was never preferred. With DisallowRemoteFinalize only preferred workers are accepted, so the fetch task for FinalizeSector stayed in the scheduler queue forever. Prefer a worker that has the sector in one of its paths and also has a destination path. Fixes filecoin-project#9946
# Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issues
Fixes #9946
Proposed Changes
moveSelector.Oknow prefers a worker that has the sector in one of its paths and also has a destination path. Before, it preferred a worker only when the sector was already in one of the destination paths.workerPathsinOkis now a set, since the per-path count isn't used anymore.TestMoveSelectorPrefersLocalMove. It runs the selector with remote finalize allowed and disallowed, for a worker with both the sealing path and a long-term storage path, a worker with only the storage path, and a worker with only the sealing path.Additional Info
DisallowRemoteFinalizeis documented as forcing Finalize tasks "to run on workers with local access to both long-term storage and the sealing path containing the sector". When it's set,newMoveSelectorgetsallowRemote = falseandOkreturns(ok && s.allowRemote) || pref, so only preferred workers pass.prefwas set only when a path returned byStorageBestAlloc, which is a long-term storage path, already had the sector. After sealing the sector is in a sealing path, so no worker was ever preferred and theseal/v0/fetchrequest fromFinalizeSectorstayed in the queue. That is the state in the issue:sched-diagshows the fetch request, and every worker has both the sealing path with the sector (5ba861fb...) and the storage path (70ad46e2...).The comment in the selector already said the preferred case covers "a local move", so the code now does what the comment says.
With remote finalize allowed, which is the default, this also changes which worker gets the move. A worker that can move the sector locally is now preferred over one that has to fetch it from another machine. Before, every worker with a long-term storage path was equal and the one with the lowest utilization won. I think the new behavior is the intended one, but it is a change, so I'm calling it out.
#10697 is a separate problem in the same selector, when the storage path is full. I'm sending a separate PR for it. The two change different parts of
Okand merge without conflicts.Tests:
go test ./storage/sealer/...passes.selector_move.gochange reverted,TestMoveSelectorPrefersLocalMovefails: with remote finalize disallowed the worker with both paths is rejected, and with it allowed that worker isn't preferred.gofmt,go vetand golangci-lint v1.64.8 report no issues for./storage/sealer/....Checklist
Before you mark the PR ready for review, please make sure that: