Uh oh!
There was an error while loading. Please reload this page.
HBASE-29386: SnapshotProcedure and EnableTableProcedure can cause a deadlock - #7084
Conversation
| } | ||
| @Test | ||
| public void testItCanEnableTableWhileSnapshotProcedureIsRunning() throws Exception { |
There was a problem hiding this comment.
This test fails without my changes, both procedures will run forever. This also functions as the reproduction of the phenomenon.
For reproducibility, I manually set the table state to ENABLING, and then kick off the EnableTableProcedure at the stage it gets stuck on (after subprocesses finish)
| this.shouldForceLockRelease = shouldForceLockRelease; | ||
| } | ||
| public boolean shouldForceLockRelease() { |
There was a problem hiding this comment.
The Procedure class has a holdLock method which returns true for SnapshotProcedure. This prevents us from releasing the lock even on suspension. I believe that at the stage where the deadlock could happen, it's safe to release the lock. We haven't done any snapshotting yet, so I believe there shouldn't be any weird data inconsistencies from region splits/merges.
There was a problem hiding this comment.
Shouldn't we just modify the SnapshotProcedure.holdLock() behaviour? Afterall, this is contrary to what we say in that method comment:
protected boolean holdLock(MasterProcedureEnv env) { // In order to avoid enabling/disabling/modifying/deleting table during snapshot, // we don't release lock during suspend return true; }
And you are already modifying SnapshotProcedure execution logic to reflect the special condition when the lock should be released, so better keep this there and avoid changing ProcedureSuspendedException?
There was a problem hiding this comment.
I don't think we should. If we did, we'd have potential inconsistencies. As I mention in my comment, this bypass allows us to release the lock prior to us actually snapshotting any data. Once we start snapshotting (anything in at or past SNAPSHOT_SNAPSHOT_ONLINE_REGIONS) it is unsafe to release the lock.
I think the comment still holds. We want to avoid enabling/disabling/modifying/deleting a table during a snapshot. The procedure suspension occurs prior to us starting to snapshot any data, at SNAPSHOT_WRITE_SNAPSHOT_INFO.
There was a problem hiding this comment.
Alternatively, maybe a less confusing option would be to return a non-static value for holdLock in SnapshotProcedure
This forces you to manage additional state at each stage though, which I why I opted for this implementation which I thought was safer
There was a problem hiding this comment.
I think the comment still holds. We want to avoid enabling/disabling/modifying/deleting a table during a snapshot.
We may want to avoid, but now we are allowing a case where we suspend and release the lock.
Alternatively, maybe a less confusing option would be to return a non-static value for
holdLockinSnapshotProcedure
That's what I meant by "modify the SnapshotProcedure.holdLock() behaviour". You are already modifying SnapshotProcedure behaviour to cause a release lock suspension via a flag in the ProcedureSuspendedException. We could keep this "flag" internally in SnapshotProcedure and use it to define what SnapshotProcedure.holdLock() returns.
This forces you to manage additional state at each stage though, which I why I opted for this implementation which I thought was safer
True. Yet, it seems more intuitive to me. But that's a personal opinion, just wanted to make sure it was also considered. Up to you to decide on the "final" solution. If going with the original, just please amend the comment in holdLock to explain the scenario where we release the lock.
| TableState tableState = | ||
| env.getMasterServices().getTableStateManager().getTableState(snapshotTable); | ||
| if (tableState.isEnabled()) { | ||
| SnapshotDescriptionUtils.writeSnapshotInfo(snapshot, workingDir, workingDirFS); |
There was a problem hiding this comment.
Move inside the if statements to avoid unnecessary I/O in case we need to suspend
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
charlesconnell
commented
Jun 10, 2025
Another option, which is simpler to reason about, is to simply fail the Failing a procedure is simpler and doesn't introduce more edge cases in the procedure executor state machine. However, obviously, it's a better user experience if your procedures get executed eventually. |
Agreed. The reason I opted for supsending the procedure is because this is already something we do if we are splitting or merging regions. So this procedure will already suspend and resume in the case the table state isn't optimal. I don't think I have a very strong opinion one way or another, though. Enabling/disabling tables happens often enough that it might be fine to simply fail and force the user to manually handle the failure. cc @Apache9 Don't know if you have any strong opinions here either way. |
rmdmattingly
commented
Jun 10, 2025
Do you mean "happens |
hgromer
commented
Jun 10, 2025
Yes I did; I am happy to fail the procedure if that's the general consensus. That being said, I do think modern backups start to stress this system out. Any call to the BackupAdmin triggers an enable table procedure, so we're more likely to get into this state. If we are okay with failing, then the user will have to manually kick off another snapshot on failure. Another possible solution would be to reset the state of the snapshot procedure, so that it needs to run from the beginning if (tableState.isEnabled()) {
// action
} elseif (tableState.isDisabled) {
// action
} else {
// set up suspension timeout/persistencesetNextState(SnapshotState.SNAPSHOT_PREPARE);
} |
hgromer
commented
Jun 10, 2025
going to proceed with the strategy of simply failing |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Apache9
left a comment
There was a problem hiding this comment.
When implementing this SnapshotProcedure, we decided to use shared lock and holdLock = true to prevent other table procedure jumps in in the middle of our execution while not hurting the availability because exclusive lock will also prevent region assigning.
In HBASE-28683, we introduced a new way to only allow one procedure to run at the same time for the same table, so maybe a possible way is to make SnapshotProcedure also acquire exclusive lock and set holdLock = false, so it will not be executed at the same time with Enable and Disable procedure.
Thanks.
| setNextState(SnapshotState.SNAPSHOT_SNAPSHOT_CLOSED_REGIONS); | ||
| } else { | ||
| setState(ProcedureState.FAILED); | ||
| throw new ProcedureAbortedException( |
There was a problem hiding this comment.
Just use setFailure and then return Flow.NO_MORE_STATE.
There was a problem hiding this comment.
Will do, thank you for the suggestion
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hgromer
commented
Jun 11, 2025
That's good to know, thank you for the context. The issue here is that the EnableTableProcedure will release the lock after it's subprocedures finish, which allows the SnapshotProcedure to execute on a table that is enabling. The SnapshotProcedure then gets stuck continuously re-running the same state over again, and will refuse to release the lock, creating a deadlock With all this said, I think it makes the most sense, for simplicity's sake, to fail the procedure if the table is in an invalid state. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Apache-HBase
commented
Jun 24, 2025
🎊 +1 overall
This message was automatically generated. |
hgromer
commented
Jun 24, 2025
@Apache9 I think this PR is good to go, please let me know if you think everything makes sense! |
| () -> procExec.getProcedure(mergeProcId).getState() == ProcedureState.RUNNABLE); | ||
| SnapshotProcedure sp = new SnapshotProcedure(procExec.getEnvironment(), snapshotProto); | ||
| long snapshotProcId = procExec.submitProcedure(sp); | ||
| TEST_UTIL.waitFor(2000, 1000, () -> procExec.getProcedure(snapshotProcId) != null |
There was a problem hiding this comment.
Mind explaining a bit why we need to remove the WAITING_TIMEOUT state check?
There was a problem hiding this comment.
Sure; the SnapshotProcedure will never reach the state where it is running at the same time that the merge procedures are running because it will now require an exclusive locks of the table to run. So when we kick off the SnapshotProcedure, it will sit in the TableProcedureWaitingQueue as RUNNABLE before being scheduled. Therefore, this code path will never be executed. We can clean this code up too if we'd like
…adlock (#7084) Co-authored-by: Hernan Gelaf-Romer <hgelafromer@hubspot.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Ray Mattingly <rmattingly@apache.org> Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org>
…adlock (#7084) Co-authored-by: Hernan Gelaf-Romer <hgelafromer@hubspot.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Ray Mattingly <rmattingly@apache.org> Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org>
…adlock (#7084) Co-authored-by: Hernan Gelaf-Romer <hgelafromer@hubspot.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Ray Mattingly <rmattingly@apache.org> Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org>
…adlock (apache#7084) (apache#7123) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Ray Mattingly <rmattingly@apache.org> Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> Co-authored-by: Hernan Romer <nanug33@gmail.com> Co-authored-by: Hernan Gelaf-Romer <hgelafromer@hubspot.com>
hgromer
commented
Jul 1, 2025
I think we need to revert this PR. This change can cause another deadlock, which can interfere with server crash procedures. The SCP acquires a server exclusive lock, so it can run in parallel to a snapshot procedure. However the SCP will schedule SplitWALRemoteProcedure which do acquire table locks. The SplitWALRemoteProcedure won't run until the snapshot procedure finishes, however the snapshot procedure will get stuck at state SNAPSHOT_SNAPSHOT_SPLIT_REGIONS waiting for the server to go online. This puts us in a state where the children of the SCP will never finish, which means the SCP will never finish, which also blocks the snapshot procedure from finishing. |
Apache9
commented
Jul 1, 2025
The holdLock is false for SnapshotProcedure, so unless it is blocked while execution, it will release the table lock when it can not go on. So I do not think it is the root cause for your problem. And why SplitWALRemoteProcedure requires table locks? I haven't seen any table lock related code in SplitWALRemoteProcedure. And the log you provided is not about SCP, it says the region is SPLITTING? I think you have messed up SplitWALRemoteProcedure and SnapshotRegionProcedure? Thanks. |
I may have conflated the locking behavior of |
Apache9
commented
Jul 2, 2025
Any updates here? It is a bit strange that, we should have check in both SnapshotProcedure and SplitTableTableProcedure/MergeTableRegionsProcedure, to prevent they execute concurrently. So why you can see a region in SPLITTING state while executing SnapshotProcedure? |
hgromer
commented
Jul 2, 2025
The log line indicates that the server is in a SPLITTING state, not the region. The SnapshotProcedure will check here if the target server is online and suspend the procedure if it isn't. We were in a tricky scenario where a test cluster was trying to run various SCP, and those SCP were essentially blocking the SnapshotProcedure. I had though the SnapshotProcedure was preventing the SCP from proceeding, but it turns out SCP was failing when running a SplitWALRemoteProcedure due to memory issues I'm still attempting to track down exactly, the entire host's memory was being eaten up by RecoveredEditsOutputSink#append. We did have a disproportionately high number of regions per host on the cluster, which likely was the cause. Importantly, the SnapshotProcedure was not the cause of these stuck SCP |
Apache9
commented
Jul 2, 2025
Your direct memory limit is 1.75GB, which seems too small? What is your heap size? If you enable off heap blockcache and off heap memstore, you need to configure a higher direct memory limitation, which may be greater than your heap size. |
Apache9
commented
Jul 2, 2025
Anyway, seems we are good to close this issue and go with the 2.6.3 release. Thanks guys! |
hgromer
commented
Jul 2, 2025
Yes, we were running on very small instance types. A test ran awry, which caused us to run into memory issues. Thank you for jumping in here, and apologies for the back and forth |
…adlock (apache#7084) (apache#7122) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Ray Mattingly <rmattingly@apache.org> Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> Co-authored-by: Hernan Romer <nanug33@gmail.com> Co-authored-by: Hernan Gelaf-Romer <hgelafromer@hubspot.com>
…adlock (apache#7084) (apache#7122) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Ray Mattingly <rmattingly@apache.org> Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> Co-authored-by: Hernan Romer <nanug33@gmail.com> Co-authored-by: Hernan Gelaf-Romer <hgelafromer@hubspot.com>
…adlock (apache#7084) (apache#7122) (#195) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Ray Mattingly <rmattingly@apache.org> Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> Co-authored-by: Ray Mattingly <rmattingly@apache.org> Co-authored-by: Hernan Gelaf-Romer <hgelafromer@hubspot.com>
No description provided.