Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

br: Add TLS support for PProf of BR #824

Merged
merged 10 commits into from
Mar 10, 2021
Merged

br: Add TLS support for PProf of BR #824

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e317dc4
br: pprof now supports TLS
YuJuncen Mar 8, 2021
08a0f4b
tests: remove a TODO
YuJuncen Mar 8, 2021
be58b8d
br: fix dynamic pprof listen addr
YuJuncen Mar 8, 2021
43f1b86
utils: extract common part of startPProf*
YuJuncen Mar 8, 2021
ecefa8c
Merge branch 'master' into tlspprof
YuJuncen Mar 9, 2021
ebe1b28
Merge branch 'master' into tlspprof
YuJuncen Mar 10, 2021
9ad2cb2
br: refactor pprof
YuJuncen Mar 10, 2021
fbe6bb8
Merge branch 'tlspprof' of https://github.com/YuJuncen/br into tlspprof
YuJuncen Mar 10, 2021
f4ccd82
br: refine some code
YuJuncen Mar 10, 2021
2f051c3
br: use tidbutils.TLS
YuJuncen Mar 10, 2021
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
11 changes: 8 additions & 3 deletions cmd/br/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,16 +151,21 @@ func Init(cmd *cobra.Command) (err error) {
}

// Initialize the pprof server.
// TODO: Support TLS.
statusAddr, e := cmd.Flags().GetString(FlagStatusAddr)
if e != nil {
err = e
return
}
tlsConfig := task.TLSConfig{}
useTLS := !(tlsConfig.ParseFromFlags(cmd.Flags()) != nil || tlsConfig.Cert == "" || tlsConfig.Key == "")
startPProf := func(addr string) { utils.StartPProfListener(addr) }
if useTLS {
startPProf = func(addr string) { utils.StartPProfListenerTLS(addr, tlsConfig.Cert, tlsConfig.Key) }
}
if statusAddr != "" {
utils.StartPProfListener(statusAddr)
startPProf(statusAddr)
} else {
utils.StartDynamicPProfListener()
utils.StartDynamicPProfListener(startPProf)
}
})
return errors.Trace(err)
Expand Down
8 changes: 4 additions & 4 deletions pkg/utils/dyn_pprof_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@ const startPProfSignal = syscall.SIGUSR1
var signalChan = make(chan os.Signal, 1)

// StartDynamicPProfListener starts the listener that will enable pprof when received `startPProfSignal`.
func StartDynamicPProfListener() {
func StartDynamicPProfListener(start func(addr string)) {
Copy link
Member

Choose a reason for hiding this comment

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

Prefer pass arguments explicitly instead of a closure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function is an internal function, I think if we pass arguments here, we probably need to provide TLS config explicitly here, like StartDynamicPProfListener(cert, key string). At the implementation might would be like:

if cert == "" || key == "" {
  StartPProfListener("0.0.0.0:0")
} else {
  StartPProfListener("0.0.0.0:0", cert, key)
}

We got more args, and function overloading-like magic... Without more information, I'm afraid I would prefer the current style...

Copy link
Member

Choose a reason for hiding this comment

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

We can move the if condition into the StartPProfListener.

func StartPProfListener(addr, cert, key string) error {
    if cert == "" || key == "" {
    } else {
    }
}

Copy link
Collaborator Author

@YuJuncen YuJuncen Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, I think those would make a if statement within the function, whose condition is the new parameter we added. And in my opinion this is also control coupling or another form of function overloading.

I think make another function(StartPProfListenerTLS) and let user to decide how to start the pprof listener by callback in the StartDynamicPProfListener would be more flexible.

I'm wondering if there are some style guides preferring plain arguments or some bad things happened with closures...? If so, I will surely remove those.

signal.Notify(signalChan, startPProfSignal)
go onSignalStartPProf(signalChan)
go onSignalStartPProf(signalChan, start)
}

func onSignalStartPProf(signals <-chan os.Signal) {
func onSignalStartPProf(signals <-chan os.Signal, start func(addr string)) {
for sig := range signals {
if sig == startPProfSignal {
log.Info("signal received, starting pprof...", zap.Stringer("signal", sig))
StartPProfListener("0.0.0.0:0")
start("0.0.0.0:0")
}
}
}
46 changes: 33 additions & 13 deletions pkg/utils/pprof.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,12 @@ var (
mu sync.Mutex
)

// StartPProfListener forks a new goroutine listening on specified port and provide pprof info.
func StartPProfListener(statusAddr string) {
func listen(statusAddr string) net.Listener {
mu.Lock()
defer mu.Unlock()
if startedPProf != "" {
log.Warn("Try to start pprof when it has been started, nothing will happen", zap.String("address", startedPProf))
return
return nil
}
failpoint.Inject("determined-pprof-port", func(v failpoint.Value) {
port := v.(int)
Expand All @@ -39,19 +38,40 @@ func StartPProfListener(statusAddr string) {
listener, err := net.Listen("tcp", statusAddr)
if err != nil {
log.Warn("failed to start pprof", zap.String("addr", statusAddr), zap.Error(err))
return
return nil
}
startedPProf = listener.Addr().String()
log.Info("bound pprof to addr", zap.String("addr", startedPProf))
_, _ = fmt.Fprintf(os.Stderr, "bound pprof to addr %s\n", startedPProf)
return listener
}

go func() {
if e := http.Serve(listener, nil); e != nil {
log.Warn("failed to serve pprof", zap.String("addr", startedPProf), zap.Error(e))
mu.Lock()
startedPProf = ""
mu.Unlock()
return
}
}()
// StartPProfListenerTLS forks a new goroutine listening on specified port and provide pprof info, with TLS.
func StartPProfListenerTLS(statusAddr, certFile, keyFile string) {
if listener := listen(statusAddr); listener != nil {
go func() {
if e := http.ServeTLS(listener, nil, certFile, keyFile); e != nil {
log.Warn("failed to serve pprof", zap.String("addr", startedPProf), zap.Error(e))
mu.Lock()
startedPProf = ""
mu.Unlock()
return
}
}()
}
}

// StartPProfListener forks a new goroutine listening on specified port and provide pprof info.
func StartPProfListener(statusAddr string) {
if listener := listen(statusAddr); listener != nil {
go func() {
if e := http.Serve(listener, nil); e != nil {
log.Warn("failed to serve pprof", zap.String("addr", startedPProf), zap.Error(e))
mu.Lock()
startedPProf = ""
mu.Unlock()
return
}
}()
}
YuJuncen marked this conversation as resolved.
Show resolved Hide resolved
}
3 changes: 1 addition & 2 deletions tests/br_other/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,7 @@ echo "starting pprof..."

# give the former backup some time to write down lock file (and start pprof server).
sleep 1
# TODO Support TLS.
run_curl "http://localhost:$PPROF_PORT/debug/pprof/trace?seconds=1" &>/dev/null
run_curl "https://localhost:$PPROF_PORT/debug/pprof/trace?seconds=1" &>/dev/null
echo "pprof started..."

run_curl https://$PD_ADDR/pd/api/v1/config/schedule | grep '"disable": false'
Expand Down