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

fix: fsm: shutdown removed sectors FSMs #10363

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Feb 28, 2023

Related Issues

Fixes #9968

Proposed Changes

  • Don't log stuff to removed sectors logs
  • Shutdown statemachines for removed sectors
  • Add an envvar to keep removed sector FSMs running - LOTUS_KEEP_REMOVED_FSM_ACTIVE, this makes it possible to move sectors out of the Removed state with update-state in the extremely unlikely case when that's needed.

Additional Info

Could use testing

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • 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, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@rjan90
Copy link
Contributor

rjan90 commented Mar 5, 2023

Can confirm that the PR does not log stuff for removed sectors anymore.

Before the PR, if one tried to remove an already removed sector, i.e lotus-miner sectors remove --really-do-it 49, that sectors log would end up adding a state machine error in the log:

12.     2023-03-05 10:16:49 +0100 CET:  [event;sealing.SectorRemove]    {"User":{}}
13.     2023-03-05 10:16:49 +0100 CET:  [event;sealing.SectorRemoved]   {"User":{}}
14.     2023-03-05 10:16:49 +0100 CET:  [error;<nil>]   state machine error: %!s(<nil>)
15.     2023-03-05 10:17:08 +0100 CET:  [event;sealing.SectorRemove]    {"User":{}}
16.     2023-03-05 10:17:08 +0100 CET:  [error;<nil>]   state machine error: %!s(<nil>)

After cherry-picking the PR, when one tries to remove an already removed sector, it just error with normal shutdown of the state machine, and no entires are added to the log:

lotus-miner sectors remove --really-do-it 49
ERROR: normal shutdown of state machine

@rjan90 rjan90 added this to the v1.21.0 milestone Mar 5, 2023
@magik6k magik6k marked this pull request as ready for review March 8, 2023 09:26
@magik6k magik6k requested a review from a team as a code owner March 8, 2023 09:26
@ZenGround0
Copy link
Contributor

ZenGround0 commented Mar 8, 2023

Im seeing the splitstore test failures that I thought I fixed with #10366, investigating

--edit--
PR CI is just out of date, after rebase tests pass locally

@magik6k magik6k merged commit 52ae215 into master Mar 8, 2023
@magik6k magik6k deleted the fix/fsm-shutdown-removed-sectors branch March 8, 2023 18:32
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.

Removed/Terminated sectors still get Event Logs (state machine error: %!s(<nil>))
3 participants