Add BigLDAP topology#251
Conversation
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).
There was a problem hiding this comment.
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.
| 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() |
There was a problem hiding this comment.
There are several issues with the current implementation of topology_setup in BigLDAPTopologyController:
- Bypassing
self.provisioned: The setup commands are executed even ifself.provisionedisTrue. This defeats the purpose of pre-provisioned topologies (which are meant to skip setup/provisioning steps to save time). - Hardcoded Service Management: Hardcoding
systemctl stop dirsrv@localhost.serviceandsystemctl start dirsrv@localhost.servicebypasses theLDAPHostservice management abstraction (ldap.stop()andldap.start()), which respects the configuredldap_service_name. - Hardcoded Instance Name: Hardcoding
slapd-localhostassumes the instance name is alwayslocalhost. We can dynamically extract the instance name fromldap._ldap_service_name. - Missing Error Handling Decorator: Other topology controllers use
@BackupTopologyController.restore_vanilla_on_errorto ensure the environment is restored to a clean state if setup fails.
We can address all of these by refactoring the method.
| 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() |
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).