test(packages): add plugin loading test with tidb-server for enterprise profile - #1055
Conversation
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR adds a runtime test to verify that the audit and whitelist plugins for the enterprise profile of the tidb component can be loaded successfully by the packaged tidb-server. The approach is to start tidb-server with the plugins, wait 30 seconds, check if the process is still alive, and then forcibly kill it. This test is applied consistently across three version ranges that build these plugins. Overall, the approach is straightforward and effective for catching plugin loading failures early in the CI pipeline.
Critical Issues
- Reliance on fixed sleep duration (lines added in packages.yaml.tmpl, e.g., ~line 973, 1128, 1278):
Using a fixedsleep 30can lead to flaky tests if the server takes longer to start or fails quickly but the check occurs after 30 seconds.
Suggested fix: Replace fixed sleep with a polling loop that checks iftidb-serveris running every few seconds and times out after a maximum wait period. For example:This reduces flakiness and fails faster if startup never succeeds.bin/tidb-server -plugin-dir=./bin -plugin-load=audit-1,whitelist-1 | tee ./loading-plugin.log & for i in {1..30}; do if pgrep -x tidb-server; then break fi sleep 1 done if ! pgrep -x tidb-server; then echo "tidb-server failed to start with plugins loaded" exit 1 fi
Code Improvements
-
Lack of error handling on
pgrepandpkill:
Ifpgrepfails (no process found), the script should explicitly exit with an error, rather than continuing silently. Similarly,pkillshould not mask errors.
Suggested fix:pgrep -x tidb-server || { echo "tidb-server not running after plugin load"; exit 1; } pkill -9 -x tidb-server || echo "Warning: failed to kill tidb-server"
This makes failures more explicit.
-
Potential leftover processes if
pkillfails:
Ifpkill -9 -x tidb-serverfails for some reason, the server process could linger, causing interference in subsequent tests. Consider adding checks or retries for cleanup.
Best Practices
-
Add comments explaining why 30 seconds is chosen and the rationale for the approach:
The PR notes mention waiting 30s, but a comment in the script itself would help future maintainers understand the reason for this timing. -
Consider redirecting or capturing
tidb-serveroutput fully:
Currently, the output is piped totee ./loading-plugin.log. Confirm this log is properly archived or inspected on failures to aid debugging. -
Testing coverage:
This runtime plugin loading test should be complemented by unit tests verifying plugin build correctness if not already present. -
DRY (Don't Repeat Yourself):
The same script block is repeated for three version ranges. Consider extracting this logic into a reusable script or make target to reduce duplication and ease future maintenance. -
Style / Formatting:
The added scripts are shell snippets inside YAML; ensure consistent indentation and quoting style for maintainability. For example, quoting variables and commands can improve robustness.
Summary of actionable changes:
- Replace fixed
sleep 30with polling loop checking fortidb-serverprocess. - Add explicit error handling on
pgrepandpkill. - Add comments in scripts explaining wait time and error handling rationale.
- Extract repeated script blocks into a shared script or make target.
- Verify logs are archived/available for debugging failures.
- Add cleanup robustness (check if process is killed).
Implementing these changes will improve test reliability, maintainability, and diagnostics.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What
For the
enterpriseprofile of thetidbcomponent, after building the audit/whitelist plugins, verify they can actually be loaded by the packagedtidb-server:tidb-serverwith-plugin-dir=./bin -plugin-load=audit-1,whitelist-1pgrep -x tidb-serverpkill -9 -x tidb-serverNotes
killall, sopkill/pgrepare used instead (both verified present inghcr.io/pingcap-qe/cd/builders/tidb:*).bootstrapSessionImplreturns an error and tidb-server exits, so thepgrepassertion catches broken plugins.8.4.0, 6.1.07.1.0).