Bug 14466 - ctdb disable/enable can fail due to race condition
Summary: ctdb disable/enable can fail due to race condition
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: CTDB (show other bugs)
Version: 4.12.6
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-15 07:22 UTC by Martin Schwenke
Modified: 2020-08-31 08:14 UTC (History)
1 user (show)

See Also:


Attachments
Patch for 4.13, 4.12, 4.11 (48.93 KB, patch)
2020-08-19 04:52 UTC, Martin Schwenke
amitay: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Schwenke 2020-08-15 07:22:40 UTC
The recovery daemon's main loop pulls flags from all nodes and pushes them out using update_local_flags().  These flags are saved in the nodemap variable as the recovery master's canonical view of node states.  Later, main loop re-fetches the flags from remote nodes using get_remote_nodemaps() and then conducts a consistency check.  If the flags on the remote node are different then this is "fixed":

                                        DEBUG(DEBUG_ERR,("Use flags 0x%02x from local recmaster node for cluster update of node %d flags\n", nodemap->nodes[i].flags, i));
                                        update_flags_on_all_nodes(
                                                rec,
                                                nodemap->nodes[i].pnn,
                                                nodemap->nodes[i].flags);
                                        ctdb_set_culprit(rec, nodemap->nodes[j].pnn);
                                        do_recovery(rec, mem_ctx, pnn, nodemap, 
                                                    vnnmap);
                                        return;

However, if a node is disabled or enabled (via the ctdb tool) after the call to update_local_flags() but before the call to get_remote_nodemaps() then the recovery master will note the inconsistency and "fix" it.  This can cause the ctdb tool command to fail because the node's state is reverted to its previous state and the tool never find the node in the desired state.

Retrieving flags of a node at 2 different times in main loop and comparing them is wrong.  The flags should only be retrieved once.  If pushing the flags out fails in update_local_flags() then an error is already flagged, so all of the appropriate error handling is in place at this point.
Comment 1 Martin Schwenke 2020-08-19 04:52:05 UTC
Created attachment 16174 [details]
Patch for 4.13, 4.12, 4.11

The patch includes 12 commits for the fix and 10 other recent staging commits that simplify the fix.  BUG tags are only in the last 12 - when I discussed this recently with Karolin she said this is fine because only the last commit really needs a BUG tag.

The same patch applies to v4-13-test, v4-12-test, v4-11-test.

I've run the CTDB testsuite over all of these, including running tests on a virtual cluster.  Looks good.  Although some of the commits have been reworked in the last couple of months to make the fix more obvious, the logic behind the fix has been in my test branch for over 2 years...  and is finally seeing daylight!
Comment 2 Amitay Isaacs 2020-08-20 02:53:05 UTC
Thanks for testing all the branches. We need to consolidate our testing.
Comment 3 Amitay Isaacs 2020-08-20 02:53:56 UTC
Hi Karolin,

This is ready for the branches v4-11, v4-12 and v4-13.

Thanks.
Comment 4 Karolin Seeger 2020-08-25 11:26:42 UTC
(In reply to Amitay Isaacs from comment #3)
Hi Amitay,

pushed to autobuild-v4-{13,12,11}-test.
Comment 5 Karolin Seeger 2020-08-31 08:14:50 UTC
(In reply to Karolin Seeger from comment #4)
Pushed to all branches.
Closing out bug report.

Thanks!