Skip to content

test(packages): add plugin loading test with tidb-server for enterprise profile - #1055

Merged
ti-chi-bot[bot] merged 1 commit into
mainfrom
test/tidb-plugin-loading
Aug 13, 2026
Merged

test(packages): add plugin loading test with tidb-server for enterprise profile#1055
ti-chi-bot[bot] merged 1 commit into
mainfrom
test/tidb-plugin-loading

Conversation

@wuhuizuo

Copy link
Copy Markdown
Contributor

What

For the enterprise profile of the tidb component, after building the audit/whitelist plugins, verify they can actually be loaded by the packaged tidb-server:

  • start tidb-server with -plugin-dir=./bin -plugin-load=audit-1,whitelist-1
  • wait 30s, then assert the process is still running via pgrep -x tidb-server
  • clean up with pkill -9 -x tidb-server

Notes

  • The builder image (centos7) does not ship killall, so pkill/pgrep are used instead (both verified present in ghcr.io/pingcap-qe/cd/builders/tidb:*).
  • If plugin loading fails, bootstrapSessionImpl returns an error and tidb-server exits, so the pgrep assertion catches broken plugins.
  • Applied to all three version-range routers that build plugins (>=8.4.0, 7.1.08.4.0, 6.1.07.1.0).

@ti-chi-bot ti-chi-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 fixed sleep 30 can 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 if tidb-server is running every few seconds and times out after a maximum wait period. For example:
    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
    This reduces flakiness and fails faster if startup never succeeds.

Code Improvements

  • Lack of error handling on pgrep and pkill:
    If pgrep fails (no process found), the script should explicitly exit with an error, rather than continuing silently. Similarly, pkill should 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 pkill fails:
    If pkill -9 -x tidb-server fails 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-server output fully:
    Currently, the output is piped to tee ./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 30 with polling loop checking for tidb-server process.
  • Add explicit error handling on pgrep and pkill.
  • 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.

@ti-chi-bot ti-chi-bot Bot added the size/S label Aug 13, 2026
@wuhuizuo

Copy link
Copy Markdown
Contributor Author

/approve

@ti-chi-bot

ti-chi-bot Bot commented Aug 13, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added the approved label Aug 13, 2026
@ti-chi-bot
ti-chi-bot Bot merged commit 8d87667 into main Aug 13, 2026
4 checks passed
@ti-chi-bot
ti-chi-bot Bot deleted the test/tidb-plugin-loading branch August 13, 2026 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant