failover: implement offline detection, reuse active connection and connection expiration - #8982
Open
pbrezina wants to merge 29 commits into
Open
failover: implement offline detection, reuse active connection and connection expiration#8982pbrezina wants to merge 29 commits into
pbrezina wants to merge 29 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a connection state machine (enum sss_failover_state) to track the failover context's status (DISCONNECTED, CONNECTED, or OFFLINE). It implements offline marking capabilities, updates transactions to return ERR_OFFLINE when no servers are available, and significantly improves robustness by adding numerous out-of-memory (OOM) checks for talloc_reference operations. I have no additional feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
pbrezina
force-pushed
the
fo-offline
branch
3 times, most recently
from
July 21, 2026 16:59
7d9edf0 to
4b7e387
Compare
This makes sure that active_server is set to NULL and failover state is set to offline when we can not connect to any server. It also sets the fctx->connection to NULL so it can not be used anymore. Only the one failover context is set to an error, offline state. Other contexts may still be functional so it is possible to distinguish between e.g. KDC being down, we can not authenticate but id can still work.
This should not happen, but let's be defensive.
Technically, it may return NULL if allocation fails so we should check it everywhere.
The vtable_op should be only responsible for selecting a server and obtaining a connection. If the server and connection should be stored in the failover context should be left on the caller. The active server and connection is now set only in the transaction code and only if reusable connection was selected requested, otherwise we do not touch the global state.
ERR_NO_MORE_SERVERS is translated to ERR_OFFLINE as the failover context is set to an offline state and this is a clearer message.
This was previously omitted.
This is a helper to get reference to the active server.
pbrezina
changed the title
The original idea was that kinit and connect will be always called and the decision to reuse existing connection will be here. But this is unnecessary difficult, it can be reused as long as the connection works and the very simple logic has been implemented in the transaction code. The LDAP connection code will however implement expiration and idle timers in subsequent commits.
Be more explicit on the exact place where the current connection was dropped. This simplifies the logic and state maintenance. Also add debug messages around reference handling.
This is just for information purpose (backend may choose to set a flag in the connection), the actually disconnect must happen in a talloc destructor.
It makes sense to unify the naming pattern as "sss_failover_active_server_*" when we have multiple functions now.
It makes sense to unify the naming pattern as "sss_failover_connection_*" when we have multiple functions now.
These helpers compare what we want to remove to what is stored and only removes the active server or connection if there is a match to avoid removing active server or connection if it was set to something else in the mean time.
This is not strictly needed as the connection is terminated by the sdap_handle destructor which is called automatically when the connection is being freed, but it is better to be explicit about it.
This will be used to determine whether the connection is idle or busy.
Member
Author
|
@alexey-tikhonov @justin-stephenson I think this is ready for review. It is a pile of commits, but they are well organized and very small. It's actually less then 1000 lines of code. |
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
3 participants
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.
This PR implements three missing functionality, they all depends on the previous patches so I'm sending it in single PR.