CI: set keyspace in test for nextgen#12640
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
Nextgen TiDB refuses to start without a keyspace name (or standby mode), which broke pull_dm_integration_test_next_gen. Pass -keyspace-name=SYSTEM when the pipeline sets NEXT_GEN=1.
dveeden
force-pushed
the
ci_nextgen_keyspace
branch
from
964566a to
4ce5b80
Compare
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
size/XS
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the run_tidb_server utility to support Nextgen TiDB by conditionally adding the -keyspace-name=SYSTEM flag when the NEXT_GEN environment variable is set. Feedback indicates that this change may cause subsequent client connections in the script to fail if they are not updated to include the keyspace name. Additionally, the shell script syntax used to include the optional arguments is noted as fragile and could lead to execution errors.
| # The Jenkins pull_dm_integration_test_next_gen pipeline sets NEXT_GEN=1. | ||
| EXTRA_ARGS="" | ||
| if [ "${NEXT_GEN:-0}" = "1" ]; then | ||
| EXTRA_ARGS="-keyspace-name=SYSTEM" |
There was a problem hiding this comment.
Starting TiDB with the -keyspace-name=SYSTEM flag typically requires clients to specify the keyspace in their connection string (e.g., using root@SYSTEM). The mysql commands later in this script (lines 46, 56, etc.) currently use -uroot, which will likely fail to connect when the server is in this mode. You should update these commands to be keyspace-aware when NEXT_GEN is enabled.
There was a problem hiding this comment.
@gemini could you give some references for those statements?
There was a problem hiding this comment.
When TiDB is started with the -keyspace-name flag, it operates in a multi-tenancy mode where data is isolated by keyspace. In this mode, standard MySQL clients connecting to the server must specify the keyspace in the username to authenticate and access the correct data. This is documented in the TiDB Multi-tenancy documentation. Specifically, the mysql command line tool requires the format user@keyspace to connect to a specific keyspace. Since your integration tests use mysql -uroot, the connection will fail because it lacks the keyspace context.
| echo "Starting TiDB on port ${PORT}" | ||
| bin/tidb-server \ | ||
| -P ${PORT} \ | ||
| ${EXTRA_ARGS} \ |
There was a problem hiding this comment.
Placing an optional variable on its own line with a trailing backslash is fragile in shell scripts. If the variable is empty, the line consists only of a backslash, which can cause the command to fail if any trailing whitespace is accidentally added after it. It is generally safer to append optional arguments to a line containing a mandatory argument.
|
@dveeden: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/cc @OliverS929 |
1 participant
What problem does this PR solve?
Issue Number: close #xxx
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note