New failover implementation#8566
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 implements a new failover mechanism for SSSD, introducing prioritized server groups, parallelized candidate server discovery, and a transaction-based API for automated retries. It also provides a minimal provider implementation to demonstrate the new architecture. Critical logic bugs were identified in the server group resolution logic, where duplicate detection causes premature loop exit, and in the address change detection function, which currently returns inverted results.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
alexey-tikhonov
added
Waiting for review
no-backport
|
@pbrezina, is it expected CI fails to build? |
0570a63 to
2a2c475
Compare
|
@pbrezina, |
|
FreeBSD CI doesn't have required headers installed: While 'minimal' isn't going to be merged in main repo branches, this 'fail to build' can hide other issues. |
e75bb95 to
f84c987
Compare
|
Now it is fixed. There were missing headers in noinst_HEADERS and some other problems. I reordered the commits and every commit for testing only is clearly marked to not go to master. Reviewer needs to pay attention only to the "failover" commit, other commits are just for testing and a demostration. |
|
@pbrezina, would it be difficult to include a 'system' test using "minimal" provider and covering any failover scenario? If it's difficult then disregard as test would be discarded eventually. |
Periodic refreshes are also not yet implemented, right? |
…ider (cherry picked from commit 0f5f3b6)
so it can be directly modified
…can easily pass new fctx
So it can be modified later.
…kup and user authentication
…pec file Add the sssd-minimal provider package to the spec file following the same pattern as other providers (ldap, ipa, ad, etc.). This packages the libsss_minimal.so library that was added in recent commits. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
And also disable codeql for the minimal provider. The provider is for testing only, it does not make sense to fix any issue there.
|
I fixed the issues, I will start working on remaining tickets when this is merged. |
This crafts and implements the new failover interface, it does not provide complete implementation of the failover mechanism yet. It brings the code to a state were the public and private interfaces are stable, working and testable so the following tasks can be split and work on in parallel. What is missing at this state: - server configuration and discovery (failover_server_group/batch/vtable_op) - server selection mechanism (sss_failover_vtable_op_server_next) - kerberos authentication - sharing servers between IPA/AD LDAP and KDC - online/offline callbacks (resolve callback should not be needed) But especially it is possible to start refactoring SSSD code to start using the new failover implementation.
|
Well, I won't pretend I've read all "+8,002" new lines of code in the same fashion I usually do during review... It's tempting to say "we only merge to feature branch, not risky", but being realistic I'm not sure we will re-review before merging to 'master' :) Nonetheless, I think it's ok to merge. |
|
@pbrezina, I've rebased https://github.com/SSSD/sssd/commits/failover/ |
|
Looks like I'm missing something... |
Sorry, I missed |
It is rebased. |
|
Merged, let's start opening PRs against it. We need to keep rebasing it regurarly. |
4 participants
This pull request is intended to be a start of a "failover" feature branch where other developers will be able to contribute.
The main failover logic works, compiles and can be tested using a "minimal" provider that is included as an example. The purpose of the "minimal" provider is only to test the failover without the need to port full provider code and itwill be removed prior pushing the contents to the master branch. See how to set it up in
minimal-provider-notes.txtand see the switch to new failover in commitminimal: switch to new failover for service lookup and user authentication- this is the minimal set of changes to get it working, but the real port should get and will require more refactoring.The work is still not finished and there is missing functionality. This functionality, however, can be implemented in small areas of code and should not require larger changes or glues in the whole code base, so this is ready for review. Remaining work is tracked at [1]. Feel free to take any of these tickets and open new tickets when you find something missing.
When reviewing, you can start with
src/providers/failover/readme.mdthat provides high level documentation of the code. And of course do not forget the design page [2].Thanks, Pavel