tests: isolate next gen upstream TiKV wal-sync dirs#4685
tests: isolate next gen upstream TiKV wal-sync dirs#4685ti-chi-bot[bot] merged 2 commits intopingcap:masterfrom
Conversation
Fixes flaky next-gen integration startup where upstream TiKV instances shared one rfengine wal-sync-dir path. Generate per-instance upstream TiKV configs with isolated wal-sync-dir and add a regression test. Ref pingcap#4684 Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/release-note-label-needed
📝 WalkthroughWalkthroughThe integration test bootstrap script is modified to isolate upstream TiKV instances by generating per-instance configuration files with unique wal-sync directories (tikv1, tikv2, tikv3) instead of sharing a single path. A 30-second sleep waiting for keyspace pre-allocation is also removed from the startup sequence. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 start_tidb_cluster_nextgen script to ensure that each TiKV instance uses a unique wal-sync-dir path, preventing potential conflicts. A new integration test was added to verify this configuration. The reviewer suggested using the //go:embed directive in the test file to improve readability and eliminate runtime file I/O dependencies.
| package utils_test | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestNextGenUpstreamTiKVWalSyncDirIsPerInstance(t *testing.T) { | ||
| _, currentFile, _, ok := runtime.Caller(0) | ||
| if !ok { | ||
| t.Fatal("failed to get test file path") | ||
| } | ||
|
|
||
| scriptPath := filepath.Join(filepath.Dir(currentFile), "start_tidb_cluster_nextgen") | ||
| content, err := os.ReadFile(scriptPath) | ||
| if err != nil { | ||
| t.Fatalf("failed to read %s: %v", scriptPath, err) | ||
| } | ||
|
|
||
| script := string(content) | ||
|
|
||
| sharedWalDir := `wal-sync-dir = "$TEST_DATA_DIR/wal-sync/upstream/tikv/raft-wal"` | ||
| if strings.Contains(script, sharedWalDir) { | ||
| t.Fatalf("upstream TiKV instances must not share rfengine wal-sync-dir: %q", sharedWalDir) | ||
| } | ||
|
|
||
| perInstanceWalDir := `wal-sync-dir = "$TEST_DATA_DIR/wal-sync/upstream/tikv$idx/raft-wal"` | ||
| if !strings.Contains(script, perInstanceWalDir) { | ||
| t.Fatalf("expected per-instance rfengine wal-sync-dir snippet: %q", perInstanceWalDir) | ||
| } | ||
| } |
There was a problem hiding this comment.
For improved readability and robustness, you can use the //go:embed directive (available in Go 1.16+) to embed the script file directly into your test binary at compile time. This simplifies the code by removing the runtime file loading logic, makes the test self-contained, and prevents potential runtime I/O errors.
package utils_test
import (
_ "embed"
"strings"
"testing"
)
//go:embed start_tidb_cluster_nextgen
var script string
func TestNextGenUpstreamTiKVWalSyncDirIsPerInstance(t *testing.T) {
sharedWalDir := `wal-sync-dir = "$TEST_DATA_DIR/wal-sync/upstream/tikv/raft-wal"`
if strings.Contains(script, sharedWalDir) {
t.Fatalf("upstream TiKV instances must not share rfengine wal-sync-dir: %q", sharedWalDir)
}
perInstanceWalDir := `wal-sync-dir = "$TEST_DATA_DIR/wal-sync/upstream/tikv$idx/raft-wal"`
if !strings.Contains(script, perInstanceWalDir) {
t.Fatalf("expected per-instance rfengine wal-sync-dir snippet: %q", perInstanceWalDir)
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration_tests/_utils/start_tidb_cluster_nextgen (1)
1-2:⚠️ Potential issue | 🟡 MinorMissing copyright header causes CI failure.
The pipeline reports
check-copyright failed. Add the appropriate copyright header at the top of this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration_tests/_utils/start_tidb_cluster_nextgen` around lines 1 - 2, Add the project's standard copyright/license header to this script file by inserting the same multi-line header used across the repo immediately after the existing shebang (#!/usr/bin/env bash) in start_tidb_cluster_nextgen; ensure it includes the correct year and owner and matches the formatting of other files so check-copyright passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration_tests/_utils/start_tidb_cluster_nextgen_test.go`:
- Around line 1-9: Add the repository's standard copyright header at the top of
this file before the package declaration (package utils_test); ensure the header
matches the project's required format and years/owner, placed above the import
block so the CI check-copyright passes.
---
Outside diff comments:
In `@tests/integration_tests/_utils/start_tidb_cluster_nextgen`:
- Around line 1-2: Add the project's standard copyright/license header to this
script file by inserting the same multi-line header used across the repo
immediately after the existing shebang (#!/usr/bin/env bash) in
start_tidb_cluster_nextgen; ensure it includes the correct year and owner and
matches the formatting of other files so check-copyright passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 187cd87b-2342-4cb4-b596-5ad8ad1e35a2
📒 Files selected for processing (2)
tests/integration_tests/_utils/start_tidb_cluster_nextgentests/integration_tests/_utils/start_tidb_cluster_nextgen_test.go
| package utils_test | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "strings" | ||
| "testing" | ||
| ) |
There was a problem hiding this comment.
Missing copyright header causes CI failure.
The pipeline reports check-copyright failed. Add the repository's standard copyright header before the package declaration.
Example fix structure
+// Copyright <year> PingCAP, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// ... (full header per repo standard)
+
package utils_test🧰 Tools
🪛 GitHub Actions: PR Build and Unit Test
[error] 1-1: check-copyright failed: The copyright information of this file is incorrect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration_tests/_utils/start_tidb_cluster_nextgen_test.go` around
lines 1 - 9, Add the repository's standard copyright header at the top of this
file before the package declaration (package utils_test); ensure the header
matches the project's required format and years/owner, placed above the import
block so the CI check-copyright passes.
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
ti-chi-bot
Bot
added
size/S
|
/test next-gen |
ti-chi-bot
Bot
added
release-note-none
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
/test pull-cdc-storage-integration-heavy-next-gen |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wk989898, wlwilliamx 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 |
[LGTM Timeline notifier]Timeline:
|
3 participants
What problem does this PR solve?
Issue Number: close #4684
In next-gen integration startup, all upstream TiKV instances shared the same
rfengine.wal-sync-dir. This can cause unstable startup behavior and lead to flaky failures when bootstrapping upstream system TiDB.What is changed and how it works?
[rfengine]with a sharedwal-sync-dirfrom commonupstream-tikv.toml.upstream-tikv1.toml,upstream-tikv2.toml,upstream-tikv3.toml).[rfengine]settings and set isolatedwal-sync-dirfor each TiKV.Check List
Tests
Test commands:
go test ./tests/integration_tests/_utils -run TestNextGenUpstreamTiKVWalSyncDirIsPerInstance -count=1go test ./tests/integration_tests/_utils -count=1bash -n tests/integration_tests/_utils/start_tidb_cluster_nextgenQuestions
Will it cause performance regression or break compatibility?
No.
Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note