Skip to content

test(dump): add comprehensive tests for stack trace and coredump functionalities #5434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cheyang merged 13 commits into from
Jan 30, 2026
Merged

test(dump): add comprehensive tests for stack trace and coredump functionalities #5434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
748b8bf
test(dump): add comprehensive tests for stack trace and coredump func…
adity1raut Jan 21, 2026
a5adf37
test(dump): add comprehensive tests for stack trace and coredump func…
adity1raut Jan 21, 2026
9d84db2
test(dump): add comprehensive tests for stack trace and coredump func…
adity1raut Jan 21, 2026
260ac21
test(dump): enhance stack trace and coredump tests for concurrency an…
adity1raut Jan 21, 2026
e403662
Solve falling CI
adity1raut Jan 21, 2026
888be95
Resolve
adity1raut Jan 22, 2026
e65906e
REsolve
adity1raut Jan 22, 2026
d5c4189
REsolve
adity1raut Jan 23, 2026
838810a
Resolve
adity1raut Jan 23, 2026
0309050
REsolve The config
adity1raut Jan 26, 2026
5e55bb8
Resolve
adity1raut Jan 26, 2026
d4fe51c
Solve
adity1raut Jan 26, 2026
da0e1e4
Solve
adity1raut Jan 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 40 additions & 36 deletions pkg/dump/dump.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,19 @@ import (
"os"
"os/signal"
"runtime"
"sync"
"sync/atomic"
"syscall"
"time"

"github.com/go-logr/logr"
ctrl "sigs.k8s.io/controller-runtime"
)

var log logr.Logger
var log = ctrl.Log.WithName("dump")
var initialized int32 // Changed to atomic int32

var initialized bool

var dumpfile string = "%s/%s-%s.txt"
var dumpfileMutex sync.RWMutex
var dumpfile string = "%s-%s.txt"

func StackTrace(all bool) string {
buf := make([]byte, 10240)
Expand All @@ -52,39 +53,36 @@ func StackTrace(all bool) string {
}

func InstallgoroutineDumpGenerator() {
if !initialized {
log = ctrl.Log.WithName("dump")
log.Info("Register goroutine dump generator")

signals := make(chan os.Signal, 1)

signal.Notify(signals, syscall.SIGQUIT)

go func() {
for {
sig := <-signals

switch sig {
case syscall.SIGQUIT:
t := time.Now()
timestamp := fmt.Sprint(t.Format("20060102150405"))
log.Info("User uses kill -3 to generate goroutine dump")
// coredump("/tmp/go_" + timestamp + ".txt")
coredump(fmt.Sprintf(dumpfile, "/tmp", "go", timestamp))
// case syscall.SIGTERM:
// fmt.Println("User told me to exit")
// os.Exit(0)
default:
continue
}
}

}()

initialized = true
} else {
if !atomic.CompareAndSwapInt32(&initialized, 0, 1) {
log.Info("Do nothing for installing grouting dump.")
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

Typo in log message: "grouting" should be "goroutine".

Suggested change
log.Info("Do nothing for installing grouting dump.")
log.Info("Do nothing for installing goroutine dump.")

Copilot uses AI. Check for mistakes.
Comment thread
adity1raut marked this conversation as resolved.
return
}

log.Info("Register goroutine dump generator")
Comment on lines +56 to +61
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jan 26, 2026

Choose a reason for hiding this comment

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

critical

There's a potential panic here. The package-level log variable is used in log.Info on line 59, but it's only initialized on line 63. If InstallgoroutineDumpGenerator is called more than once, atomic.CompareAndSwapInt32 will return false, and log.Info will be called on a nil logr.Logger, causing a panic.

To fix this, log should be initialized before its first use. The best approach is to initialize it at the package level (e.g., var log = ctrl.Log.WithName("dump")) and remove the initialization from this function. A valid alternative that keeps changes within this function is to move the initialization to the top.

Additionally, there's a typo in the log message on line 59: "grouting" should be "goroutine".

Suggested change
if !atomic.CompareAndSwapInt32(&initialized, 0, 1) {
log.Info("Do nothing for installing grouting dump.")
return
}
log = ctrl.Log.WithName("dump")
log.Info("Register goroutine dump generator")
log = ctrl.Log.WithName("dump")
if !atomic.CompareAndSwapInt32(&initialized, 0, 1) {
log.Info("Do nothing for installing goroutine dump.")
return
}
log.Info("Register goroutine dump generator")

Comment on lines +56 to +61
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jan 26, 2026

Choose a reason for hiding this comment

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

high

There is a race condition here. If two goroutines call InstallgoroutineDumpGenerator concurrently, one might execute log.Info on line 59 before the other has initialized the package-level log variable on line 63. While logr.Logger's zero value doesn't panic, this will result in a lost log message.

To fix this, you should initialize the log variable at the package level, where it is declared, instead of inside this function.

// At package level (e.g. line 33)
var log = ctrl.Log.WithName("dump")

// Then, remove the initialization from this function:
func InstallgoroutineDumpGenerator() {
	if !atomic.CompareAndSwapInt32(&initialized, 0, 1) {
		log.Info("Do nothing for installing grouting dump.")
		return
	}

	log.Info("Register goroutine dump generator")
    // ... rest of the function
}


signals := make(chan os.Signal, 1)

signal.Notify(signals, syscall.SIGQUIT)

go func() {
for {
sig := <-signals

switch sig {
case syscall.SIGQUIT:
t := time.Now()
timestamp := fmt.Sprint(t.Format("20060102150405"))
log.Info("User uses kill -3 to generate goroutine dump")
dumpfileMutex.RLock()
filename := fmt.Sprintf(dumpfile, "/tmp", "go", timestamp)
Comment thread
adity1raut marked this conversation as resolved.
dumpfileMutex.RUnlock()
Comment on lines +76 to +78
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jan 26, 2026

Choose a reason for hiding this comment

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

medium

The dumpfileMutex seems unnecessary. It's used to protect reads of the dumpfile variable, but dumpfile is a format string that is initialized once at the package level and never modified. This lock adds unnecessary complexity and a slight performance overhead. I recommend removing the lock here and also removing the dumpfileMutex variable declaration on line 37.

Suggested change
dumpfileMutex.RLock()
filename := fmt.Sprintf(dumpfile, "/tmp", "go", timestamp)
dumpfileMutex.RUnlock()
filename := fmt.Sprintf(dumpfile, "/tmp", "go", timestamp)

Comment thread
adity1raut marked this conversation as resolved.
coredump(filename)
default:
continue
}
}

}()
}

func coredump(fileName string) {
Expand All @@ -98,3 +96,9 @@ func coredump(fileName string) {
log.Info(stdout)

}

// ResetForTesting resets the package state for testing purposes
// This should only be used in tests
func ResetForTesting() {
Comment on lines +100 to +102
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

The ResetForTesting function is exported but lacks any safeguards to prevent its use in production code. Consider either making this function unexported (resetForTesting) or adding build constraints to ensure it's only available in test builds. This would prevent accidental misuse in production code that could reset the initialized state and cause unexpected behavior.

Suggested change
// ResetForTesting resets the package state for testing purposes
// This should only be used in tests
func ResetForTesting() {
// resetForTesting resets the package state for testing purposes
// This should only be used in tests
func resetForTesting() {

Copilot uses AI. Check for mistakes.
atomic.StoreInt32(&initialized, 0)
}
28 changes: 28 additions & 0 deletions pkg/dump/dump_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
Copyright 2023 The Fluid Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package dump

import (
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)

func TestDump(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Dump Suite")
}
Loading
Loading