Skip to content

Add BigLDAP topology#251

Draft
thalman wants to merge 1 commit into
SSSD:masterfrom
thalman:big_ldap
Draft

Add BigLDAP topology#251
thalman wants to merge 1 commit into
SSSD:masterfrom
thalman:big_ldap

Conversation

@thalman
Copy link
Copy Markdown
Contributor

@thalman thalman commented May 28, 2026

This patch expects that ldap container/vm has ldap backup 'largedata'. This backup is restored when BigLDAP topology is used.

Restore process is quite fast (~2s).

This patch expects that ldap container/vm has ldap backup 'largedata'.
This backup is restored when BigLDAP topology is used.

Restore process is quite fast (~2s).
@thalman thalman requested a review from pbrezina May 28, 2026 12:49
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new BigLDAPTopologyController and its corresponding topology mark BigLDAP to support LDAP topologies with large amounts of users and groups. The feedback points out several issues in the topology_setup implementation of BigLDAPTopologyController, including bypassing the self.provisioned check, hardcoding service management commands and instance names instead of using existing abstractions, and missing error handling decorators. A refactoring suggestion is provided to address these issues.

Comment on lines +428 to +434
def topology_setup(self, client: ClientHost, ldap: LDAPHost) -> None:
ldap.conn.run(
"systemctl stop dirsrv@localhost.service && "
"dsctl slapd-localhost bak2db largedata && "
"systemctl start dirsrv@localhost.service"
)
super().topology_setup()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There are several issues with the current implementation of topology_setup in BigLDAPTopologyController:

  1. Bypassing self.provisioned: The setup commands are executed even if self.provisioned is True. This defeats the purpose of pre-provisioned topologies (which are meant to skip setup/provisioning steps to save time).
  2. Hardcoded Service Management: Hardcoding systemctl stop dirsrv@localhost.service and systemctl start dirsrv@localhost.service bypasses the LDAPHost service management abstraction (ldap.stop() and ldap.start()), which respects the configured ldap_service_name.
  3. Hardcoded Instance Name: Hardcoding slapd-localhost assumes the instance name is always localhost. We can dynamically extract the instance name from ldap._ldap_service_name.
  4. Missing Error Handling Decorator: Other topology controllers use @BackupTopologyController.restore_vanilla_on_error to ensure the environment is restored to a clean state if setup fails.

We can address all of these by refactoring the method.

Suggested change
def topology_setup(self, client: ClientHost, ldap: LDAPHost) -> None:
ldap.conn.run(
"systemctl stop dirsrv@localhost.service && "
"dsctl slapd-localhost bak2db largedata && "
"systemctl start dirsrv@localhost.service"
)
super().topology_setup()
@BackupTopologyController.restore_vanilla_on_error
def topology_setup(self, client: ClientHost, ldap: LDAPHost) -> None:
if not self.provisioned:
instance = ldap._ldap_service_name.split("@")[-1].split(".")[0]
ldap.stop()
ldap.conn.run(f"dsctl {instance} bak2db largedata")
ldap.start()
super().topology_setup()

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.

1 participant