diff --git a/.github/workflows/make-integration-tests.yml b/.github/workflows/make-integration-tests.yml index 32c28ad7d..0d5662076 100644 --- a/.github/workflows/make-integration-tests.yml +++ b/.github/workflows/make-integration-tests.yml @@ -24,6 +24,10 @@ on: description: 'runner type' required: true type: string + use-valgrind: + required: true + default: false + type: boolean jobs: test-agent: @@ -77,7 +81,7 @@ jobs: run: | docker exec \ -e PHPS=${{matrix.php}} \ - -e INTEGRATION_ARGS="-license ${{secrets.NR_TEST_LICENSE}} -collector ${{secrets.NR_COLLECTOR_HOST}} -agent agent/modules/newrelic.so" \ + -e INTEGRATION_ARGS="-license ${{secrets.NR_TEST_LICENSE}} -collector ${{secrets.NR_COLLECTOR_HOST}} -agent agent/modules/newrelic.so ${{ inputs.use-valgrind && '-valgrind /usr/bin/valgrind' }}" \ -e APP_supportability=${{secrets.APP_SUPPORTABILITY}} \ -e ACCOUNT_supportability=${{secrets.ACCOUNT_SUPPORTABILITY}} \ -e ACCOUNT_supportability_trusted=${{secrets.ACCOUNT_SUPPORTABILITY_TRUSTED}} \ diff --git a/.github/workflows/run-integration-tests.yml b/.github/workflows/run-integration-tests.yml index 1294a9973..ef847f564 100644 --- a/.github/workflows/run-integration-tests.yml +++ b/.github/workflows/run-integration-tests.yml @@ -26,7 +26,11 @@ on: options: - ubuntu-22.04-arm - ubuntu-24.04-arm - + use-valgrind: + description: 'Use valgrind?' + required: true + default: false + type: boolean jobs: build-test-runner-amd64: uses: ./.github/workflows/make-for-platform-on-arch.yml @@ -78,6 +82,7 @@ jobs: with: origin: ${{ inputs.origin }} ref: ${{ inputs.ref }} + use-valgrind: ${{ inputs.use-valgrind }} arch: amd64 runs-on: ubuntu-latest continue-on-error: false @@ -88,6 +93,7 @@ jobs: with: origin: ${{ inputs.origin }} ref: ${{ inputs.ref }} + use-valgrind: ${{ inputs.use-valgrind }} arch: arm64 runs-on: ${{inputs.runner-type}} continue-on-error: true diff --git a/daemon/cmd/integration_runner/main.go b/daemon/cmd/integration_runner/main.go index f1a65a402..60c0247d8 100644 --- a/daemon/cmd/integration_runner/main.go +++ b/daemon/cmd/integration_runner/main.go @@ -344,6 +344,10 @@ func main() { ctx = integration.NewContext(*flagPHP, *flagCGI) ctx.Valgrind = *flagValgrind + if ctx.Valgrind != "" && *flagWorkers != 1 { + fmt.Fprintf(os.Stderr, "cannot run valgrind on multiple workers\n") + os.Exit(1) + } ctx.Timeout = *flagTimeout // Settings common to all tests. diff --git a/daemon/internal/newrelic/integration/transaction.go b/daemon/internal/newrelic/integration/transaction.go index be2819aa7..b69b4b004 100644 --- a/daemon/internal/newrelic/integration/transaction.go +++ b/daemon/internal/newrelic/integration/transaction.go @@ -48,7 +48,7 @@ func PhpTx(src Script, env, settings map[string]string, ctx *Context) (Tx, error args := phpArgs(nil, filepath.Base(src.Name()), false, settings) - if ctx.Valgrind != "" { + if ctx.Valgrind != "" && settings["newrelic.appname"] != "skipif" { txn = &ValgrindCLI{ CLI: CLI{ Path: ctx.PHP, @@ -81,6 +81,7 @@ func PhpTx(src Script, env, settings map[string]string, ctx *Context) (Tx, error // See: https://tools.ietf.org/html/rfc3875 func CgiTx(src Script, env, settings map[string]string, headers http.Header, ctx *Context) (Tx, error) { var err error + var txn Tx req := &http.Request{ Method: env["REQUEST_METHOD"], @@ -105,34 +106,69 @@ func CgiTx(src Script, env, settings map[string]string, headers http.Header, ctx return nil, fmt.Errorf("unable to create cgi request: %v", err) } - tx := &CGI{ - request: req, - handler: &cgi.Handler{ - Path: ctx.CGI, - Dir: src.Dir(), - Args: phpArgs(nil, "", false, settings), - }, - } + if ctx.Valgrind != "" { + tx := &ValgrindCGI{ + CGI: CGI{ + request: req, + handler: &cgi.Handler{ + Path: ctx.CGI, + Dir: src.Dir(), + Args: phpArgs(nil, "", false, settings), + }, + }, + Valgrind: ctx.Valgrind, + Timeout: ctx.Timeout, + } + tx.handler.Env = append(tx.handler.Env, + "SCRIPT_FILENAME="+scriptFile, + "SCRIPT_NAME=/"+filepath.Base(src.Name()), + "REDIRECT_STATUS=200") + tx.handler.Env = append(tx.handler.Env, flatten(env)...) + + // If the environment includes a REQUEST_URI and doesn't include an explicit + // PATH_INFO, we'll set it here to avoid problems with Go's CGI package + // attempting to guess what the PATH_INFO should be. + if ruri, ok := env["REQUEST_URI"]; ok { + if _, ok := env["PATH_INFO"]; !ok { + tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri) + } + } - tx.handler.Env = append(tx.handler.Env, - "SCRIPT_FILENAME="+scriptFile, - "SCRIPT_NAME=/"+filepath.Base(src.Name()), - "REDIRECT_STATUS=200") - tx.handler.Env = append(tx.handler.Env, flatten(env)...) - - // If the environment includes a REQUEST_URI and doesn't include an explicit - // PATH_INFO, we'll set it here to avoid problems with Go's CGI package - // attempting to guess what the PATH_INFO should be. - if ruri, ok := env["REQUEST_URI"]; ok { - if _, ok := env["PATH_INFO"]; !ok { - tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri) + if !src.IsFile() { + return withTempFile(tx, src.Name(), src.Bytes()), nil + } + txn = tx + } else { + tx := &CGI{ + request: req, + handler: &cgi.Handler{ + Path: ctx.CGI, + Dir: src.Dir(), + Args: phpArgs(nil, "", false, settings), + }, + } + tx.handler.Env = append(tx.handler.Env, + "SCRIPT_FILENAME="+scriptFile, + "SCRIPT_NAME=/"+filepath.Base(src.Name()), + "REDIRECT_STATUS=200") + tx.handler.Env = append(tx.handler.Env, flatten(env)...) + + // If the environment includes a REQUEST_URI and doesn't include an explicit + // PATH_INFO, we'll set it here to avoid problems with Go's CGI package + // attempting to guess what the PATH_INFO should be. + if ruri, ok := env["REQUEST_URI"]; ok { + if _, ok := env["PATH_INFO"]; !ok { + tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri) + } } - } - if !src.IsFile() { - return withTempFile(tx, src.Name(), src.Bytes()), nil + if !src.IsFile() { + return withTempFile(tx, src.Name(), src.Bytes()), nil + } + txn = tx } - return tx, nil + + return txn, nil } // phpArgs builds the command line for a PHP process. diff --git a/daemon/internal/newrelic/integration/valgrind.go b/daemon/internal/newrelic/integration/valgrind.go index 6ced42d31..b1ef069d1 100644 --- a/daemon/internal/newrelic/integration/valgrind.go +++ b/daemon/internal/newrelic/integration/valgrind.go @@ -10,6 +10,7 @@ import ( "fmt" "io/ioutil" "net/http" + "net/http/httptest" "sync" "time" @@ -24,6 +25,12 @@ type ValgrindCLI struct { Timeout time.Duration } +type ValgrindCGI struct { + CGI + Valgrind string + Timeout time.Duration +} + func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) { if len(tx.Path) == 0 { return nil, []byte("skip: executable not specified"), nil @@ -34,8 +41,13 @@ func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) { // valgrind reports to their tests in the same way we use the appname // to link the transaction data. Failing that, perhaps we could use // the valgrind process's pid instead. - valgrindMu.Lock() - defer valgrindMu.Unlock() + // + // We prevent concurrent invocations by disallowing valgrind when + // the threads argument is greater than 1. Previously, we used the + // commented out locks below. This was changed to allow subprocesses + // (i.e. curl) to run without deadlocking. + //valgrindMu.Lock() + //defer valgrindMu.Unlock() cmd := valgrind.Memcheck(tx.Valgrind, "--quiet") cmd.Args = append(cmd.Args, "--xml=yes") @@ -105,6 +117,87 @@ func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) { return nil, output, err } +func (tx *ValgrindCGI) Execute() (http.Header, []byte, error) { + if len(tx.handler.Path) == 0 { + return nil, []byte("skip: executable not specified"), nil + } + + // For now, we don't have a mechanism to handle concurrent invocations + // of Valgrind. In the future, we could use the appname to connect + // valgrind reports to their tests in the same way we use the appname + // to link the transaction data. Failing that, perhaps we could use + // the valgrind process's pid instead. + // + // We prevent concurrent invocations by disallowing valgrind when + // the threads argument is greater than 1. Previously, we used the + // commented out locks below. This was changed to allow subprocesses + // (i.e. curl) to run without deadlocking. + //valgrindMu.Lock() + //defer valgrindMu.Unlock() + + cmd := valgrind.Memcheck(tx.Valgrind, "--quiet") + cmd.Args = append(cmd.Args, "--xml=yes") + cmd.Args = append(cmd.Args, "--xml-socket="+valgrindLn.Addr().String()) + cmd.Args = append(cmd.Args, "--") + cmd.Args = append(cmd.Args, tx.handler.Path) + if len(tx.handler.Args) > 0 { + cmd.Args = append(cmd.Args, tx.handler.Args...) + } + + log.Debugf("command: %v", cmd) + + // Replace the handler with valgrind + tx.handler.Path = cmd.Path + // The first Arg is the Path. + // ServeHTTP will re-add Path + // to the front of Args + tx.handler.Args = cmd.Args[1:] + + ch := make(chan resultOrError, 1) + go func() { + var result resultOrError + result.R, result.E = acceptOneReport(tx.Timeout) + ch <- result + }() + + resp := httptest.NewRecorder() + tx.handler.ServeHTTP(resp, tx.request) + + vgOutput := <-ch + + // Append the output from Valgrind to the test output. + // + // TODO: Eventually we want to report valgrind output separately, so we can + // treat valgrind errors similarly to failed test expectations. i.e. We'd + // like to add them to Test.Failures. After all, each test has an implicit + // expectation that it will not exhibit memory bugs! + // + // TODO: Once the former is in place, we should be able to parse the memory + // leak reports produced by the Zend Memory Manager from the test output and + // report them the same way as valgrind errors. + output := resp.Body.Bytes() + if vgOutput.R != nil && len(vgOutput.R.Errors) > 0 { + // Safe to ignore the error here, Report.MarshalText() never fails. + data, _ := vgOutput.R.MarshalText() + output = append(output, '\n') + output = append(output, data...) + } + + // Ensure a non-nil error is returned when valgrind detects errors. + // Otherwise, the test could be marked as passing if it does not have + // any expectations on the test output. This sucks. + // + // TODO: Remove this when valgrind errors can be treated as failed test + // expectations. + err := vgOutput.E + if err == nil && vgOutput.R != nil && len(vgOutput.R.Errors) > 0 { + err = fmt.Errorf("detected %d memory errors", len(vgOutput.R.Errors)) + } + + return resp.HeaderMap, output, err + +} + // resultOrError is a poor man's sum type. type resultOrError struct { R *valgrind.Report