Univention Bugzilla – Bug 33224
Performance and debugging of udm-cli
Last modified: 2023-01-05 13:17:27 CET
Created attachment 5597 [details] Improve UDM CLI debugging UDM-CLI forks a udm-cli-server to reduce that startup cost. When that conversion was done, a simple protocol was implemented to transfer the result of the server back to the client. This protocol simply uses repr() and eval() to serialize and de-serialize the data. This has several drawbacks: 1. The result is first completely aggregated in the client, then serialized, and only then transferred to the CLI frontent. For large result sets, this needs lots of ram and takes very long without any progress report. In an environment with several hundred computers nothing happened for several seconds. 2. This complicated debugging, as all trace-backs happen in another process and are only partly transferred to the front-end CLI process. 3. eval() is insecure, as it allows executing arbitrary Python code. This is less an issue here, since UDM-CLI-Server is controlled by us, but at least its bad style. It would help if /usr/share/pyshared/univention/admincli/admin.py would implement a main() function to make it callable from CLI. The attached patch implements said request plus some cleanup. parse_commandline() is not yet used, see Bug #31768.
Can you remove the cleanup issues from the patch and split the cleanup to a separate bug?
Created attachment 5684 [details] Improve UDM CLI debugging + cleanups $ git log -33 --oneline --reverse 15a9cbb Bug #21585: Fix listing policy references efa54e3 Bug #33224: Fix typo in comment 1c4de0a Bug #33224: Remove arg handling 823154e Bug #33224: Remove custom attribute handling 9116db8 Bug #33224: Convert array to streaming to stdout 5f914ee Bug #33224: Make module callable ee7ed2c Bug #33224: Strip both sides 2e2659c Bug #33224: Simplify buffer concatenation 2eb110b Bug #33224: Convert UCR type early 2632624 Bug #33224: Use path concatenation e4a464e Bug #33224: Just read the whole file dea7fac Bug #33224: Use instanceof() 84ac60a Bug #33224: Catch specific exception aca0007 Bug #33224: Just use .keys() directly e7e4fb8 Bug #33224: Use getattr() 3ac7f7e Bug #33224: Replace .has_key() 18214dc Bug #33224: Use else after loop 531d495 Bug #33224: Use dict.setdefault() 4b3d6a1 Bug #33224: Iterate directly through list 9303ce1 Bug #33224: Use list comprehension 6282db5 Bug #33224: Use True/False 11ec307 Bug #33224: Refactore duplicate policy code e780925 Bug #33224: Remove unused imports 494aad4 Bug #33224: Shorten imports 57ddb3b Bug #33224: Use named constants 11bae0c Bug #33224: Rework splitting d8fe273 Bug #33224: Check for multiple conditions fe29003 Bug #33224: Refactore credential check 5133a80 Bug #33224: Small Python code cleanup 5a555f3 Bug #33224: Convert string concatenation to list 35b471f Bug #33224: pep8 compliance 1150009 Bug #33224: pep8 exception renaming 0fe04d9 Bug #33224: WIP Convert to argparse As I had to touch every line doing output, I decided to first remove the deprecated code for custom attributes and --arg handling instead of converting them too and then removing that code. If that code should still be kept, it must be converted too, is is basically a conversion from out.append(...) to print >> out, ... The last patch about argparse was just a test and should not be applied as is. udm-modules/modules/univention/admincli/admin.py | 2046 ++++------ udm-modules/univention-cli-server | 40 2 files changed, 1038 insertions(+), 1048 deletions(-) Without that patch and without the spitting of long lines a lot more code was removed ...
One more optimization would be to use SCM_RIGHTS to pass the file descriptor from udm-cli to the udm-cli-server through the UNIX Domain socket, which would allow the continuously running server to output directly to the command line client. But <http://docs.python.org/3.3/library/socket.html#socket.socket.sendmsg> and <http://docs.python.org/3.3/library/socket.html#socket.socket.sendmsg> are only available since Python-3.3. For Python-2 there is <https://gitorious.org/python-fdsend/pages/Home> and <http://yans.pl.sophia.inria.fr/code/python-passfd/summary>.
This minimal addition helps a lot when UDM-CLI-Server fails to start: /usr/share/pyshared/univention/admincli/admin.py: $ if __name__ == '__main__': import sys print '\n'.join(doit(sys.argv))
Good patch. I needed this. Stefan, can you reassign TM to something UCS 4 based?
univention-cli-server:272 > else: # parent > os.waitpid(pid, os.P_NOWAIT) os.P_* is for spawn(), while os.W* is for wait*() os.P_NOWAIT == os.WNOHANG > 120 »···ud.debug(ud.ADMIN, ud.INFO, 'daemon [%s] forked to background' % os.getpid()) message is wrong -> "server is running as process %d" daemonize is missing - double-fork - close-all-fds() - reopen FD0-2 - chdir - umask
See also Bug #40422 comment 5.
This issue has been filled against UCS 4.1. The maintenance with bug and security fixes for UCS 4.1 has ended on 5st of April 2018. Customers still on UCS 4.1 are encouraged to update to UCS 4.3. Please contact your partner or Univention for any questions. If this issue still occurs in newer UCS versions, please use "Clone this bug" or simply reopen the issue. In this case please provide detailed information on how this issue is affecting you.
(In reply to Philipp Hahn from comment #2) > Created attachment 5684 [details] > Improve UDM CLI debugging + cleanups A lot of the commits are already in the latest code. The missing one are in MR: https://git.knut.univention.de/univention/ucs/-/merge_requests/461. It's marked as draft, as comment 3 and comment 6 needs to be done.
The hunks from the patches have been applied accordingly. Via the use of SCM_RIGHTS (comment 3) the stdout and stderr file descriptors are passed from udm-cli to the udm-cli-server through the UNIX Domain socket. Bug #4498 changes that the stderr file descriptor is actually used and error messages are printed to stderr. univention-directory-manager-modules.yaml 90eb6831b02b | YAML Bug #33224, Bug #4498 univention-directory-manager-modules (15.0.13-3) b28fea72f5b9 | Bug #33224: pass stdout and stderr of CLI client to CLI server 69d102ae7020 | Bug #33224: fix debug message aca3adc04790 | Bug #33224: use correct os.WNOHANG e37ba5329373 | Bug #33224: Extract common implementation to get policy into separate function. 814e855c91c7 | Bug #33224: refactor flags 65e83f76831a | Bug #33224: Use modern any() and tuple() containment test instead of multiple equal comparisons. b5aea3ff7215 | Bug #33224: use True/False instead of 1/0. 27ed390c0ebd | Bug #33224: Use for-loop to check for usable credential to reduce code duplication. d46544b2eda7 | Bug #33224: stream output of UDM CLI
Verified: * Code review * Package update * Jenkins tests show no regression * Documentation changes not required * No interoperability issues expected with non-updated systems * Advisory
FYI: the performance improved for a system with 23927 users/user objects: Previously: # time udm users/user list real 37m20,388s user 3m20,251s sys 5m51,376s Afterwards: real 1m40,081s user 0m0,043s sys 0m0,022s
<https://errata.software-univention.de/#/?erratum=5.0x406>