From d86ccf6b4952667ce44886067682209b40828afc Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 13 Feb 2025 19:00:13 +0100 Subject: [PATCH 01/28] Add library for spawning a daemon --- cmd/cmd.go | 4 ++ cmd/selftest/print_stdin.go | 26 ++++++++++++ cmd/selftest/selftest.go | 15 +++++++ libs/daemon/daemon.go | 75 +++++++++++++++++++++++++++++++++++ libs/daemon/daemon_unix.go | 24 +++++++++++ libs/daemon/daemon_windows.go | 15 +++++++ 6 files changed, 159 insertions(+) create mode 100644 cmd/selftest/print_stdin.go create mode 100644 cmd/selftest/selftest.go create mode 100644 libs/daemon/daemon.go create mode 100644 libs/daemon/daemon_unix.go create mode 100644 libs/daemon/daemon_windows.go diff --git a/cmd/cmd.go b/cmd/cmd.go index 5d835409f..290b5da7a 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -75,5 +75,9 @@ func New(ctx context.Context) *cobra.Command { cli.AddCommand(sync.New()) cli.AddCommand(version.New()) + cli.AddCommand(&cobra.Command{ + Use: "stream-", + }) + return cli } diff --git a/cmd/selftest/print_stdin.go b/cmd/selftest/print_stdin.go new file mode 100644 index 000000000..a864f6c4a --- /dev/null +++ b/cmd/selftest/print_stdin.go @@ -0,0 +1,26 @@ +package selftest + +import ( + "os" + + "github.com/spf13/cobra" +) + +const ( + PrintStdinParentPid = "DATABRICKS_CLI_PRINT_STDIN_PARENT_PID" +) + +// TODO CONTINUE: Write command that wait for each other via the PID. +// Ensure to check the process name as the PID otherwise can be reused pretty +// quick. + +func newPrintStdin() *cobra.Command { + return &cobra.Command{ + Use: "print-stdin", + RunE: func(cmd *cobra.Command, args []string) error { + if os.Getenv(PrintStdinParentPid) != "" { + return nil + } + }, + } +} diff --git a/cmd/selftest/selftest.go b/cmd/selftest/selftest.go new file mode 100644 index 000000000..5c487e783 --- /dev/null +++ b/cmd/selftest/selftest.go @@ -0,0 +1,15 @@ +package selftest + +import ( + "github.com/spf13/cobra" +) + +func New() *cobra.Command { + cmd := &cobra.Command{ + Use: "selftest", + Short: "Non functional CLI commands that are useful for testing", + } + + cmd.AddCommand(newPrintStdin()) + return cmd +} diff --git a/libs/daemon/daemon.go b/libs/daemon/daemon.go new file mode 100644 index 000000000..aeafa0847 --- /dev/null +++ b/libs/daemon/daemon.go @@ -0,0 +1,75 @@ +package daemon + +import ( + "fmt" + "io" + "os" + "os/exec" + "strconv" +) + +type Daemon struct { + // If provided, the child process will create a pid file at this path. + PidFilePath string + + // Environment variables to set in the child process. + Env []string + + // Arguments to pass to the child process. + Args []string + + cmd *exec.Cmd + stdin io.WriteCloser +} + +func (d *Daemon) Start() error { + cli, err := os.Executable() + if err != nil { + return err + } + + d.cmd = exec.Command(cli, d.Args...) + d.cmd.Env = d.Env + d.cmd.SysProcAttr = sysProcAttr() + + d.stdin, err = d.cmd.StdinPipe() + if err != nil { + return fmt.Errorf("failed to get stdin pipe: %w", err) + } + + err = d.cmd.Start() + if err != nil { + return err + } + + if d.PidFilePath != "" { + err = os.WriteFile(d.PidFilePath, []byte(strconv.Itoa(d.cmd.Process.Pid)), 0o644) + if err != nil { + return fmt.Errorf("failed to write pid file: %w", err) + } + } + + return nil +} + +func (d *Daemon) Release() error { + if d.PidFilePath != "" { + err := os.Remove(d.PidFilePath) + if err != nil { + return fmt.Errorf("failed to remove pid file: %w", err) + } + } + + if d.stdin != nil { + err := d.stdin.Close() + if err != nil { + return fmt.Errorf("failed to close stdin: %w", err) + } + } + + if d.cmd == nil { + return nil + } + + return d.cmd.Process.Release() +} diff --git a/libs/daemon/daemon_unix.go b/libs/daemon/daemon_unix.go new file mode 100644 index 000000000..75bafdb84 --- /dev/null +++ b/libs/daemon/daemon_unix.go @@ -0,0 +1,24 @@ +//go:build linux || darwin + +package daemon + +import "syscall" + +// References: +// 1. linux: https://go.dev/src/syscall/exec_linux.go +// 2. macos (arm): https://go.dev/src/syscall/exec_libc2.go +func sysProcAttr() *syscall.SysProcAttr { + return &syscall.SysProcAttr{ + // Create a new session for the child process. This ensures that the daemon + // is not terminated when the parent session is closed. This can happen + // for example when a ssh session is terminated. + // TODO: Test this. + Setsid: true, + Noctty: true, + + // Start a new process group for the child process. This ensures that + // termination signals to the parent's process group are not propagated to + // the child process. + Setpgid: true, + } +} diff --git a/libs/daemon/daemon_windows.go b/libs/daemon/daemon_windows.go new file mode 100644 index 000000000..ab88391fc --- /dev/null +++ b/libs/daemon/daemon_windows.go @@ -0,0 +1,15 @@ +//go:build windows + +package daemon + +import ( + "syscall" + + "golang.org/x/sys/windows" +) + +func sysProcAttr() *syscall.SysProcAttr { + return &syscall.SysProcAttr{ + CreationFlags: windows.CREATE_NEW_PROCESS_GROUP | windows.DETACHED_PROCESS | windows.CREATE_NO_WINDOW, + } +} From 6148681ed12d423bac1854e5b2d025ef6b8dd1b6 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Feb 2025 19:23:02 +0100 Subject: [PATCH 02/28] implement --- acceptance/acceptance_test.go | 1 + acceptance/bin/wait_pid | 42 +++++++++++ acceptance/bundle/debug/out.stderr.txt | 2 +- .../daemon/child-detaches/out.parentchild.txt | 13 ++++ acceptance/daemon/child-detaches/output.txt | 2 + acceptance/daemon/child-detaches/script | 6 ++ acceptance/help/output.txt | 4 ++ cmd/cmd.go | 2 + cmd/selftest/child.go | 70 +++++++++++++++++++ cmd/selftest/parent.go | 48 +++++++++++++ cmd/selftest/print_stdin.go | 26 ------- cmd/selftest/selftest.go | 3 +- libs/daemon/daemon.go | 48 ++++++++++--- libs/daemon/daemon_unix.go | 7 -- libs/daemon/daemon_windows.go | 3 +- libs/process/wait.go | 15 ++++ libs/process/wait_test.go | 13 ++++ libs/process/wait_unix.go | 36 ++++++++++ libs/process/wait_unix_test.go | 18 +++++ libs/process/wait_windows.go | 44 ++++++++++++ 20 files changed, 359 insertions(+), 44 deletions(-) create mode 100755 acceptance/bin/wait_pid create mode 100644 acceptance/daemon/child-detaches/out.parentchild.txt create mode 100644 acceptance/daemon/child-detaches/output.txt create mode 100644 acceptance/daemon/child-detaches/script create mode 100644 cmd/selftest/child.go create mode 100644 cmd/selftest/parent.go delete mode 100644 cmd/selftest/print_stdin.go create mode 100644 libs/process/wait.go create mode 100644 libs/process/wait_test.go create mode 100644 libs/process/wait_unix.go create mode 100644 libs/process/wait_unix_test.go create mode 100644 libs/process/wait_windows.go diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index d99ad2991..69c87ad31 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -77,6 +77,7 @@ func TestInprocessMode(t *testing.T) { require.Equal(t, 1, testAccept(t, true, "selftest/server")) } +// TODO: Maybe add flag to mark tests that cannot be completely debugged in test.toml. func testAccept(t *testing.T, InprocessMode bool, singleTest string) int { repls := testdiff.ReplacementsContext{} cwd, err := os.Getwd() diff --git a/acceptance/bin/wait_pid b/acceptance/bin/wait_pid new file mode 100755 index 000000000..f1375bf51 --- /dev/null +++ b/acceptance/bin/wait_pid @@ -0,0 +1,42 @@ +#!/bin/bash + + +# wait in bash only works for processes that are direct children to the calling +# shell. This script is more general purpose. +wait_pid() { + local pid=$1 + local max_attempts=100 # 100 * 0.1 seconds = 10 seconds + local attempt=0 + local sleep_time=0.1 + + while [ $attempt -lt $max_attempts ]; do + if [[ "$OSTYPE" == "msys"* || "$OSTYPE" == "cygwin"* ]]; then + # Windows approach + if ! tasklist | grep -q $pid; then + echo "Process has ended" + return 0 + fi + else + # Linux/macOS approach + if ! kill -0 $pid 2>/dev/null; then + echo "Process has ended" + return 0 + fi + fi + + sleep $sleep_time + attempt=$((attempt + 1)) + done + + echo "Timeout: Process $pid did not end within 10 seconds" + return 1 +} + +# Usage +if [ $# -eq 0 ]; then + echo "Usage: $0 " + exit 1 +fi + +wait_pid $1 +exit $? diff --git a/acceptance/bundle/debug/out.stderr.txt b/acceptance/bundle/debug/out.stderr.txt index e5867e008..031d58082 100644 --- a/acceptance/bundle/debug/out.stderr.txt +++ b/acceptance/bundle/debug/out.stderr.txt @@ -72,7 +72,7 @@ 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=metadata.AnnotatePipelines 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Using Terraform from DATABRICKS_TF_EXEC_PATH at [TERRAFORM] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize -10:07:59 Debug: Using Terraform CLI config from DATABRICKS_TF_CLI_CONFIG_FILE at [DATABRICKS_TF_CLI_CONFIG_FILE] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize +10:07:59 Debug: DATABRICKS_TF_PROVIDER_VERSION as 1.62.0 does not match the current version 1.65.1, ignoring DATABRICKS_TF_CLI_CONFIG_FILE pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Environment variables for Terraform: ...redacted... pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit 10:07:59 Debug: No script defined for postinit, skipping pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit diff --git a/acceptance/daemon/child-detaches/out.parentchild.txt b/acceptance/daemon/child-detaches/out.parentchild.txt new file mode 100644 index 000000000..36d2e9722 --- /dev/null +++ b/acceptance/daemon/child-detaches/out.parentchild.txt @@ -0,0 +1,13 @@ +Parent process has started +Started the child process +Provided input: Hello from the other side +Released the child process +Parent process is exiting + +==================== + +All output from this point on is from the child process +Parent process has exited +Received input from parent process: +Hello from the other side + diff --git a/acceptance/daemon/child-detaches/output.txt b/acceptance/daemon/child-detaches/output.txt new file mode 100644 index 000000000..30e9c5331 --- /dev/null +++ b/acceptance/daemon/child-detaches/output.txt @@ -0,0 +1,2 @@ +waiting for child process to finish +Process has ended diff --git a/acceptance/daemon/child-detaches/script b/acceptance/daemon/child-detaches/script new file mode 100644 index 000000000..77b5fda61 --- /dev/null +++ b/acceptance/daemon/child-detaches/script @@ -0,0 +1,6 @@ +export DATABRICKS_CLI_SELFTEST_CHILD_OUTPUT_FILE="out.parentchild.txt" + +$CLI selftest parent &> out.parentchild.txt + +echo "waiting for child process to finish" +wait_pid $(cat ./child.pid) diff --git a/acceptance/help/output.txt b/acceptance/help/output.txt index 18434251d..af9f167ba 100644 --- a/acceptance/help/output.txt +++ b/acceptance/help/output.txt @@ -129,6 +129,7 @@ Additional Commands: configure Configure authentication help Help about any command labs Manage Databricks Labs installations + selftest Non functional CLI commands that are useful for testing version Retrieve information about the current version of this CLI Flags: @@ -139,4 +140,7 @@ Flags: -t, --target string bundle target to use (if applicable) -v, --version version for databricks +Additional help topics: + databricks stream- + Use "databricks [command] --help" for more information about a command. diff --git a/cmd/cmd.go b/cmd/cmd.go index 290b5da7a..5178db6fd 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -12,6 +12,7 @@ import ( "github.com/databricks/cli/cmd/fs" "github.com/databricks/cli/cmd/labs" "github.com/databricks/cli/cmd/root" + "github.com/databricks/cli/cmd/selftest" "github.com/databricks/cli/cmd/sync" "github.com/databricks/cli/cmd/version" "github.com/databricks/cli/cmd/workspace" @@ -74,6 +75,7 @@ func New(ctx context.Context) *cobra.Command { cli.AddCommand(labs.New(ctx)) cli.AddCommand(sync.New()) cli.AddCommand(version.New()) + cli.AddCommand(selftest.New()) cli.AddCommand(&cobra.Command{ Use: "stream-", diff --git a/cmd/selftest/child.go b/cmd/selftest/child.go new file mode 100644 index 000000000..166e29aa9 --- /dev/null +++ b/cmd/selftest/child.go @@ -0,0 +1,70 @@ +package selftest + +import ( + "errors" + "fmt" + "io" + "os" + "strconv" + + "github.com/databricks/cli/libs/daemon" + "github.com/databricks/cli/libs/process" + "github.com/spf13/cobra" +) + +// TODO: Look into the release function and ensure whether I need to call it. +const () + +// TODO CONTINUE: Write command that wait for each other via the PID. +// Ensure to check the process name as the PID otherwise can be reused pretty +// quick. +// +// Implement dummy child and parent commands, and write acceptance tests to account +// for all variations. +// +// Ensure that a robust timeout mechanism exists for the telemetry process. We +// do not want the daemons to hang indefinitely. Can this also be tested? +// +// TODO: One set of tests will be asserting that the tests have the right +// properties. A thread on my personal slack account will help with that. +// The other set of tests will assert on the functional behaviour, that the +// parent and child process are indeed indpenedent, and that the child process +// does not block the parent process. +// +// All this requires some PID handler which get the process information based on +// the PID and some "name", since PIDs can be reused, being a source of flakyness. +// +// TODO: Make sure to acknowledge the risk of failing when people try to delete +// the binary in windows. +// +// TODO: Ensure that child stdout / stderr are not sent to the parent process. + +func newChildCommand() *cobra.Command { + return &cobra.Command{ + Use: "child", + RunE: func(cmd *cobra.Command, args []string) error { + parentPid, err := strconv.Atoi(os.Getenv(daemon.DatabricksCliParentPid)) + if err != nil { + return fmt.Errorf("failed to parse parent PID: %w", err) + } + + err = process.Wait(parentPid) + if err != nil && !errors.As(err, &process.ErrProcessNotFound{}) { + return fmt.Errorf("failed to wait for parent process: %w", err) + } + + fmt.Println("\n====================") + fmt.Println("\nAll output from this point on is from the child process") + fmt.Println("Parent process has exited") + + in, err := io.ReadAll(os.Stdin) + if err != nil { + return fmt.Errorf("failed to read from stdin: %w", err) + } + + fmt.Println("Received input from parent process:") + fmt.Println(string(in)) + return nil + }, + } +} diff --git a/cmd/selftest/parent.go b/cmd/selftest/parent.go new file mode 100644 index 000000000..7198f3d91 --- /dev/null +++ b/cmd/selftest/parent.go @@ -0,0 +1,48 @@ +package selftest + +import ( + "fmt" + "os" + + "github.com/databricks/cli/libs/daemon" + "github.com/spf13/cobra" +) + +const OutputFile = "DATABRICKS_CLI_SELFTEST_CHILD_OUTPUT_FILE" + +func newParentCommand() *cobra.Command { + return &cobra.Command{ + Use: "parent", + RunE: func(cmd *cobra.Command, args []string) error { + fmt.Println("Parent process has started") + + d := daemon.Daemon{ + Env: os.Environ(), + Args: []string{"selftest", "child"}, + LogFile: os.Getenv(OutputFile), + PidFilePath: "child.pid", + } + + err := d.Start() + if err != nil { + return fmt.Errorf("failed to start child process: %w", err) + } + fmt.Println("Started the child process") + + err = d.WriteInput([]byte("Hello from the other side\n")) + if err != nil { + return fmt.Errorf("failed to write to child process: %w", err) + } + fmt.Println("Provided input: Hello from the other side") + + err = d.Release() + if err != nil { + return fmt.Errorf("failed to release child process: %w", err) + } + fmt.Println("Released the child process") + + fmt.Println("Parent process is exiting") + return nil + }, + } +} diff --git a/cmd/selftest/print_stdin.go b/cmd/selftest/print_stdin.go deleted file mode 100644 index a864f6c4a..000000000 --- a/cmd/selftest/print_stdin.go +++ /dev/null @@ -1,26 +0,0 @@ -package selftest - -import ( - "os" - - "github.com/spf13/cobra" -) - -const ( - PrintStdinParentPid = "DATABRICKS_CLI_PRINT_STDIN_PARENT_PID" -) - -// TODO CONTINUE: Write command that wait for each other via the PID. -// Ensure to check the process name as the PID otherwise can be reused pretty -// quick. - -func newPrintStdin() *cobra.Command { - return &cobra.Command{ - Use: "print-stdin", - RunE: func(cmd *cobra.Command, args []string) error { - if os.Getenv(PrintStdinParentPid) != "" { - return nil - } - }, - } -} diff --git a/cmd/selftest/selftest.go b/cmd/selftest/selftest.go index 5c487e783..e05bf02de 100644 --- a/cmd/selftest/selftest.go +++ b/cmd/selftest/selftest.go @@ -10,6 +10,7 @@ func New() *cobra.Command { Short: "Non functional CLI commands that are useful for testing", } - cmd.AddCommand(newPrintStdin()) + cmd.AddCommand(newChildCommand()) + cmd.AddCommand(newParentCommand()) return cmd } diff --git a/libs/daemon/daemon.go b/libs/daemon/daemon.go index aeafa0847..ff81643b3 100644 --- a/libs/daemon/daemon.go +++ b/libs/daemon/daemon.go @@ -1,6 +1,7 @@ package daemon import ( + "context" "fmt" "io" "os" @@ -8,16 +9,26 @@ import ( "strconv" ) +const DatabricksCliParentPid = "DATABRICKS_CLI_PARENT_PID" + type Daemon struct { + // TODO: remove this. + ctx context.Context + // If provided, the child process will create a pid file at this path. + // TODO: Can we remove this? PidFilePath string // Environment variables to set in the child process. Env []string - // Arguments to pass to the child process. + // Arguments to pass to the child process. The main executable is always the CLI + // binary itself. Args []string + // Log file to write the child process's output to. + LogFile string + cmd *exec.Cmd stdin io.WriteCloser } @@ -29,9 +40,30 @@ func (d *Daemon) Start() error { } d.cmd = exec.Command(cli, d.Args...) + + // Set environment variable so that the child process know's it's parent's PID. + d.Env = append(d.Env, fmt.Sprintf("%s=%d", DatabricksCliParentPid, os.Getpid())) d.cmd.Env = d.Env + d.cmd.SysProcAttr = sysProcAttr() + // By default redirect stdout and stderr to /dev/null. + // TODO: Test that by default stdout and stderr do not leak to the parent process. + d.cmd.Stdout = nil + d.cmd.Stderr = nil + + // If a log file is provided, redirect stdout and stderr to the log file. + if d.LogFile != "" { + f, err := os.OpenFile(d.LogFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) + if err != nil { + return fmt.Errorf("failed to open log file: %w", err) + } + defer f.Close() + + d.cmd.Stdout = f + d.cmd.Stderr = f + } + d.stdin, err = d.cmd.StdinPipe() if err != nil { return fmt.Errorf("failed to get stdin pipe: %w", err) @@ -52,14 +84,12 @@ func (d *Daemon) Start() error { return nil } -func (d *Daemon) Release() error { - if d.PidFilePath != "" { - err := os.Remove(d.PidFilePath) - if err != nil { - return fmt.Errorf("failed to remove pid file: %w", err) - } - } +func (d *Daemon) WriteInput(b []byte) error { + _, err := d.stdin.Write(b) + return err +} +func (d *Daemon) Release() error { if d.stdin != nil { err := d.stdin.Close() if err != nil { @@ -71,5 +101,7 @@ func (d *Daemon) Release() error { return nil } + // This does not seem to be strictly necessary, but the docs recommend + // adding it if Wait is not called. Thus we add it here to be safe. return d.cmd.Process.Release() } diff --git a/libs/daemon/daemon_unix.go b/libs/daemon/daemon_unix.go index 75bafdb84..b9a7023a7 100644 --- a/libs/daemon/daemon_unix.go +++ b/libs/daemon/daemon_unix.go @@ -12,13 +12,6 @@ func sysProcAttr() *syscall.SysProcAttr { // Create a new session for the child process. This ensures that the daemon // is not terminated when the parent session is closed. This can happen // for example when a ssh session is terminated. - // TODO: Test this. Setsid: true, - Noctty: true, - - // Start a new process group for the child process. This ensures that - // termination signals to the parent's process group are not propagated to - // the child process. - Setpgid: true, } } diff --git a/libs/daemon/daemon_windows.go b/libs/daemon/daemon_windows.go index ab88391fc..bccf22e4b 100644 --- a/libs/daemon/daemon_windows.go +++ b/libs/daemon/daemon_windows.go @@ -10,6 +10,7 @@ import ( func sysProcAttr() *syscall.SysProcAttr { return &syscall.SysProcAttr{ - CreationFlags: windows.CREATE_NEW_PROCESS_GROUP | windows.DETACHED_PROCESS | windows.CREATE_NO_WINDOW, + HideWindow: true, + CreationFlags: windows.CREATE_NEW_PROCESS_GROUP | windows.DETACHED_PROCESS, } } diff --git a/libs/process/wait.go b/libs/process/wait.go new file mode 100644 index 000000000..251ef1db6 --- /dev/null +++ b/libs/process/wait.go @@ -0,0 +1,15 @@ +package process + +import "fmt" + +type ErrProcessNotFound struct { + Pid int +} + +func (e ErrProcessNotFound) Error() string { + return fmt.Sprintf("process with pid %d does not exist", e.Pid) +} + +func Wait(pid int) error { + return waitForPid(pid) +} diff --git a/libs/process/wait_test.go b/libs/process/wait_test.go new file mode 100644 index 000000000..72a5ee4aa --- /dev/null +++ b/libs/process/wait_test.go @@ -0,0 +1,13 @@ +package process + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TODO: Test this in windows. Setup an IDE. +func TestWait(t *testing.T) { + err := Wait(1000000) + assert.EqualError(t, err, "process with pid 1000000 does not exist") +} diff --git a/libs/process/wait_unix.go b/libs/process/wait_unix.go new file mode 100644 index 000000000..03f548f97 --- /dev/null +++ b/libs/process/wait_unix.go @@ -0,0 +1,36 @@ +//go:build linux || darwin + +package process + +import ( + "errors" + "os" + "syscall" + "time" +) + +func waitForPid(pid int) error { + p, err := os.FindProcess(pid) + if err != nil { + return err + } + + // Initial existence check. + if err := p.Signal(syscall.Signal(0)); err != nil { + if errors.Is(err, os.ErrProcessDone) { + return ErrProcessNotFound{Pid: pid} + } + return err + } + + // Polling loop until process exits + for { + if err := p.Signal(syscall.Signal(0)); err != nil { + if errors.Is(err, os.ErrProcessDone) { + return nil + } + return err + } + time.Sleep(100 * time.Millisecond) + } +} diff --git a/libs/process/wait_unix_test.go b/libs/process/wait_unix_test.go new file mode 100644 index 000000000..98fbd0c79 --- /dev/null +++ b/libs/process/wait_unix_test.go @@ -0,0 +1,18 @@ +package process + +import ( + "runtime" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestWaitForPidUnix(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Skipping test on windows") + } + + // Out of bounds pid. Should return an error. + err := waitForPid(1000000) + assert.EqualError(t, err, "process with pid 1000000 does not exist") +} diff --git a/libs/process/wait_windows.go b/libs/process/wait_windows.go new file mode 100644 index 000000000..2139601de --- /dev/null +++ b/libs/process/wait_windows.go @@ -0,0 +1,44 @@ +//go:build windows + +package process + +import ( + "errors" + "fmt" + "time" + + "golang.org/x/sys/windows" +) + +func waitForPid(pid int) error { + handle, err := windows.OpenProcess( + windows.SYNCHRONIZE|windows.PROCESS_QUERY_INFORMATION, + false, + uint32(pid), + ) + if errors.Is(err, windows.ERROR_INVALID_PARAMETER) { + return ErrProcessDoesNotExist{Pid: pid} + } + if err != nil { + return fmt.Errorf("OpenProcess failed: %v", err) + } + defer windows.CloseHandle(handle) + + // Wait forever for the process to exit. Wait for 5 minutes max. + ret, err := windows.WaitForSingleObject(handle, uint32(5*time.Minute.Milliseconds())) + if err != nil { + return fmt.Errorf("Wait failed: %v", err) + } + + switch ret { + case windows.WAIT_OBJECT_0: + return nil // Process exited + case 0x00000102: + // Standard library does not have have a constant defined for this + // so we use the hex value directly. This is the WAIT_TIMEOUT value. + // ref: https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobject#return-value + return fmt.Errorf("process wait timed out") + default: + return fmt.Errorf("unexpected process wait return value: %d", ret) + } +} From 533e7be51b2ab41e5518a3ca1b4f2e14a224a8d2 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Feb 2025 19:24:11 +0100 Subject: [PATCH 03/28] - --- libs/process/wait_unix_test.go | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100644 libs/process/wait_unix_test.go diff --git a/libs/process/wait_unix_test.go b/libs/process/wait_unix_test.go deleted file mode 100644 index 98fbd0c79..000000000 --- a/libs/process/wait_unix_test.go +++ /dev/null @@ -1,18 +0,0 @@ -package process - -import ( - "runtime" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestWaitForPidUnix(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("Skipping test on windows") - } - - // Out of bounds pid. Should return an error. - err := waitForPid(1000000) - assert.EqualError(t, err, "process with pid 1000000 does not exist") -} From d15ff75f3b510891bb1e7dbad7c0d9b3100e4f99 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Feb 2025 19:24:51 +0100 Subject: [PATCH 04/28] - --- acceptance/bundle/debug/out.stderr.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/bundle/debug/out.stderr.txt b/acceptance/bundle/debug/out.stderr.txt index 031d58082..e5867e008 100644 --- a/acceptance/bundle/debug/out.stderr.txt +++ b/acceptance/bundle/debug/out.stderr.txt @@ -72,7 +72,7 @@ 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=metadata.AnnotatePipelines 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Using Terraform from DATABRICKS_TF_EXEC_PATH at [TERRAFORM] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize -10:07:59 Debug: DATABRICKS_TF_PROVIDER_VERSION as 1.62.0 does not match the current version 1.65.1, ignoring DATABRICKS_TF_CLI_CONFIG_FILE pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize +10:07:59 Debug: Using Terraform CLI config from DATABRICKS_TF_CLI_CONFIG_FILE at [DATABRICKS_TF_CLI_CONFIG_FILE] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Environment variables for Terraform: ...redacted... pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit 10:07:59 Debug: No script defined for postinit, skipping pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit From e970a129af21e815cbd86e224869bf6b51ec2cc2 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Feb 2025 19:26:06 +0100 Subject: [PATCH 05/28] - --- acceptance/help/output.txt | 1 - cmd/selftest/selftest.go | 5 +++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/acceptance/help/output.txt b/acceptance/help/output.txt index af9f167ba..7310c5794 100644 --- a/acceptance/help/output.txt +++ b/acceptance/help/output.txt @@ -129,7 +129,6 @@ Additional Commands: configure Configure authentication help Help about any command labs Manage Databricks Labs installations - selftest Non functional CLI commands that are useful for testing version Retrieve information about the current version of this CLI Flags: diff --git a/cmd/selftest/selftest.go b/cmd/selftest/selftest.go index e05bf02de..3291e0633 100644 --- a/cmd/selftest/selftest.go +++ b/cmd/selftest/selftest.go @@ -6,8 +6,9 @@ import ( func New() *cobra.Command { cmd := &cobra.Command{ - Use: "selftest", - Short: "Non functional CLI commands that are useful for testing", + Use: "selftest", + Short: "Non functional CLI commands that are useful for testing", + Hidden: true, } cmd.AddCommand(newChildCommand()) From 3c4443c13f3983748a0f843b2890f9ba473dead6 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Feb 2025 19:27:53 +0100 Subject: [PATCH 06/28] cleanup --- acceptance/help/output.txt | 3 --- cmd/cmd.go | 4 ---- cmd/selftest/child.go | 10 +--------- 3 files changed, 1 insertion(+), 16 deletions(-) diff --git a/acceptance/help/output.txt b/acceptance/help/output.txt index 7310c5794..18434251d 100644 --- a/acceptance/help/output.txt +++ b/acceptance/help/output.txt @@ -139,7 +139,4 @@ Flags: -t, --target string bundle target to use (if applicable) -v, --version version for databricks -Additional help topics: - databricks stream- - Use "databricks [command] --help" for more information about a command. diff --git a/cmd/cmd.go b/cmd/cmd.go index 5178db6fd..4f5337fd3 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -77,9 +77,5 @@ func New(ctx context.Context) *cobra.Command { cli.AddCommand(version.New()) cli.AddCommand(selftest.New()) - cli.AddCommand(&cobra.Command{ - Use: "stream-", - }) - return cli } diff --git a/cmd/selftest/child.go b/cmd/selftest/child.go index 166e29aa9..622c7f79e 100644 --- a/cmd/selftest/child.go +++ b/cmd/selftest/child.go @@ -15,12 +15,7 @@ import ( // TODO: Look into the release function and ensure whether I need to call it. const () -// TODO CONTINUE: Write command that wait for each other via the PID. -// Ensure to check the process name as the PID otherwise can be reused pretty -// quick. -// -// Implement dummy child and parent commands, and write acceptance tests to account -// for all variations. +// TODO CONTINUE: // // Ensure that a robust timeout mechanism exists for the telemetry process. We // do not want the daemons to hang indefinitely. Can this also be tested? @@ -31,9 +26,6 @@ const () // parent and child process are indeed indpenedent, and that the child process // does not block the parent process. // -// All this requires some PID handler which get the process information based on -// the PID and some "name", since PIDs can be reused, being a source of flakyness. -// // TODO: Make sure to acknowledge the risk of failing when people try to delete // the binary in windows. // From 42c526f40ffc357ccca43b413d1df55d74c08e30 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Feb 2025 19:28:20 +0100 Subject: [PATCH 07/28] - --- cmd/selftest/child.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/cmd/selftest/child.go b/cmd/selftest/child.go index 622c7f79e..0f40e9e09 100644 --- a/cmd/selftest/child.go +++ b/cmd/selftest/child.go @@ -12,9 +12,6 @@ import ( "github.com/spf13/cobra" ) -// TODO: Look into the release function and ensure whether I need to call it. -const () - // TODO CONTINUE: // // Ensure that a robust timeout mechanism exists for the telemetry process. We From ecbbee1bb3566f166fb5f33eabf59e1a252f3c59 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Feb 2025 19:38:33 +0100 Subject: [PATCH 08/28] more test --- acceptance/bundle/debug/out.stderr.txt | 2 +- acceptance/daemon/child-detaches/script | 2 ++ acceptance/daemon/child-discards-output/output.txt | 9 +++++++++ acceptance/daemon/child-discards-output/script | 9 +++++++++ libs/daemon/daemon.go | 11 +++-------- libs/process/wait_test.go | 1 - libs/process/wait_windows.go | 2 +- 7 files changed, 25 insertions(+), 11 deletions(-) create mode 100644 acceptance/daemon/child-discards-output/output.txt create mode 100644 acceptance/daemon/child-discards-output/script diff --git a/acceptance/bundle/debug/out.stderr.txt b/acceptance/bundle/debug/out.stderr.txt index e5867e008..031d58082 100644 --- a/acceptance/bundle/debug/out.stderr.txt +++ b/acceptance/bundle/debug/out.stderr.txt @@ -72,7 +72,7 @@ 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=metadata.AnnotatePipelines 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Using Terraform from DATABRICKS_TF_EXEC_PATH at [TERRAFORM] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize -10:07:59 Debug: Using Terraform CLI config from DATABRICKS_TF_CLI_CONFIG_FILE at [DATABRICKS_TF_CLI_CONFIG_FILE] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize +10:07:59 Debug: DATABRICKS_TF_PROVIDER_VERSION as 1.62.0 does not match the current version 1.65.1, ignoring DATABRICKS_TF_CLI_CONFIG_FILE pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Environment variables for Terraform: ...redacted... pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit 10:07:59 Debug: No script defined for postinit, skipping pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit diff --git a/acceptance/daemon/child-detaches/script b/acceptance/daemon/child-detaches/script index 77b5fda61..0e83daade 100644 --- a/acceptance/daemon/child-detaches/script +++ b/acceptance/daemon/child-detaches/script @@ -4,3 +4,5 @@ $CLI selftest parent &> out.parentchild.txt echo "waiting for child process to finish" wait_pid $(cat ./child.pid) + +rm ./child.pid diff --git a/acceptance/daemon/child-discards-output/output.txt b/acceptance/daemon/child-discards-output/output.txt new file mode 100644 index 000000000..5fe488ade --- /dev/null +++ b/acceptance/daemon/child-discards-output/output.txt @@ -0,0 +1,9 @@ +Parent process has started +Started the child process +Provided input: Hello from the other side +Released the child process +Parent process is exiting + +wait for child process to finish +Process has ended +the child process should not have written any output. diff --git a/acceptance/daemon/child-discards-output/script b/acceptance/daemon/child-discards-output/script new file mode 100644 index 000000000..94ef19c70 --- /dev/null +++ b/acceptance/daemon/child-discards-output/script @@ -0,0 +1,9 @@ +$CLI selftest parent + +echo " +wait for child process to finish" +wait_pid $(cat ./child.pid) + +echo "the child process should not have written any output." + +rm ./child.pid diff --git a/libs/daemon/daemon.go b/libs/daemon/daemon.go index ff81643b3..1c7dc7e9a 100644 --- a/libs/daemon/daemon.go +++ b/libs/daemon/daemon.go @@ -1,7 +1,6 @@ package daemon import ( - "context" "fmt" "io" "os" @@ -12,11 +11,7 @@ import ( const DatabricksCliParentPid = "DATABRICKS_CLI_PARENT_PID" type Daemon struct { - // TODO: remove this. - ctx context.Context - // If provided, the child process will create a pid file at this path. - // TODO: Can we remove this? PidFilePath string // Environment variables to set in the child process. @@ -48,7 +43,6 @@ func (d *Daemon) Start() error { d.cmd.SysProcAttr = sysProcAttr() // By default redirect stdout and stderr to /dev/null. - // TODO: Test that by default stdout and stderr do not leak to the parent process. d.cmd.Stdout = nil d.cmd.Stderr = nil @@ -101,7 +95,8 @@ func (d *Daemon) Release() error { return nil } - // This does not seem to be strictly necessary, but the docs recommend - // adding it if Wait is not called. Thus we add it here to be safe. + // The docs recommend calling Release if Wait is not called. This does not + // seem to be necessary since the functionality works without it. However, we + // we add it just to be safe. return d.cmd.Process.Release() } diff --git a/libs/process/wait_test.go b/libs/process/wait_test.go index 72a5ee4aa..bc9407a8e 100644 --- a/libs/process/wait_test.go +++ b/libs/process/wait_test.go @@ -6,7 +6,6 @@ import ( "github.com/stretchr/testify/assert" ) -// TODO: Test this in windows. Setup an IDE. func TestWait(t *testing.T) { err := Wait(1000000) assert.EqualError(t, err, "process with pid 1000000 does not exist") diff --git a/libs/process/wait_windows.go b/libs/process/wait_windows.go index 2139601de..c7a8d9ac7 100644 --- a/libs/process/wait_windows.go +++ b/libs/process/wait_windows.go @@ -17,7 +17,7 @@ func waitForPid(pid int) error { uint32(pid), ) if errors.Is(err, windows.ERROR_INVALID_PARAMETER) { - return ErrProcessDoesNotExist{Pid: pid} + return ErrProcessNotFound{Pid: pid} } if err != nil { return fmt.Errorf("OpenProcess failed: %v", err) From 2c7126991b32ed3911121cccf73c40514e121607 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Feb 2025 19:39:18 +0100 Subject: [PATCH 09/28] - --- acceptance/acceptance_test.go | 1 - acceptance/bundle/debug/out.stderr.txt | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index a9e37efa4..c7b1151ab 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -83,7 +83,6 @@ func TestInprocessMode(t *testing.T) { require.Equal(t, 1, testAccept(t, true, "selftest/server")) } -// TODO: Maybe add flag to mark tests that cannot be completely debugged in test.toml. func testAccept(t *testing.T, InprocessMode bool, singleTest string) int { repls := testdiff.ReplacementsContext{} cwd, err := os.Getwd() diff --git a/acceptance/bundle/debug/out.stderr.txt b/acceptance/bundle/debug/out.stderr.txt index 031d58082..e5867e008 100644 --- a/acceptance/bundle/debug/out.stderr.txt +++ b/acceptance/bundle/debug/out.stderr.txt @@ -72,7 +72,7 @@ 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=metadata.AnnotatePipelines 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Using Terraform from DATABRICKS_TF_EXEC_PATH at [TERRAFORM] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize -10:07:59 Debug: DATABRICKS_TF_PROVIDER_VERSION as 1.62.0 does not match the current version 1.65.1, ignoring DATABRICKS_TF_CLI_CONFIG_FILE pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize +10:07:59 Debug: Using Terraform CLI config from DATABRICKS_TF_CLI_CONFIG_FILE at [DATABRICKS_TF_CLI_CONFIG_FILE] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Environment variables for Terraform: ...redacted... pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit 10:07:59 Debug: No script defined for postinit, skipping pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit From a9ccc3285b1e38a7deb5977b65560814089e0d36 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Feb 2025 19:48:58 +0100 Subject: [PATCH 10/28] -' --- cmd/selftest/child.go | 16 ---------------- go.mod | 3 ++- 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/cmd/selftest/child.go b/cmd/selftest/child.go index 0f40e9e09..ebb319d34 100644 --- a/cmd/selftest/child.go +++ b/cmd/selftest/child.go @@ -12,22 +12,6 @@ import ( "github.com/spf13/cobra" ) -// TODO CONTINUE: -// -// Ensure that a robust timeout mechanism exists for the telemetry process. We -// do not want the daemons to hang indefinitely. Can this also be tested? -// -// TODO: One set of tests will be asserting that the tests have the right -// properties. A thread on my personal slack account will help with that. -// The other set of tests will assert on the functional behaviour, that the -// parent and child process are indeed indpenedent, and that the child process -// does not block the parent process. -// -// TODO: Make sure to acknowledge the risk of failing when people try to delete -// the binary in windows. -// -// TODO: Ensure that child stdout / stderr are not sent to the parent process. - func newChildCommand() *cobra.Command { return &cobra.Command{ Use: "child", diff --git a/go.mod b/go.mod index 2e2505361..b4157c61b 100644 --- a/go.mod +++ b/go.mod @@ -37,6 +37,8 @@ require ( gopkg.in/yaml.v3 v3.0.1 ) +require golang.org/x/sys v0.30.0 + require ( cloud.google.com/go/auth v0.4.2 // indirect cloud.google.com/go/auth/oauth2adapt v0.2.2 // indirect @@ -72,7 +74,6 @@ require ( go.opentelemetry.io/otel/trace v1.24.0 // indirect golang.org/x/crypto v0.31.0 // indirect golang.org/x/net v0.33.0 // indirect - golang.org/x/sys v0.30.0 // indirect golang.org/x/time v0.5.0 // indirect google.golang.org/api v0.182.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240521202816-d264139d666e // indirect From 403c1b2a0c34de06c246b3442fb21c18e54ee5ce Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 17 Feb 2025 23:02:17 +0100 Subject: [PATCH 11/28] - --- cmd/selftest/child.go | 1 + libs/daemon/daemon.go | 13 ++++++-- libs/process/testdata/parent.sh | 6 ++++ libs/process/wait_test.go | 58 +++++++++++++++++++++++++++++++-- 4 files changed, 73 insertions(+), 5 deletions(-) create mode 100755 libs/process/testdata/parent.sh diff --git a/cmd/selftest/child.go b/cmd/selftest/child.go index ebb319d34..b1584970b 100644 --- a/cmd/selftest/child.go +++ b/cmd/selftest/child.go @@ -12,6 +12,7 @@ import ( "github.com/spf13/cobra" ) +// TODO: Manually test that indeed latency is not added. func newChildCommand() *cobra.Command { return &cobra.Command{ Use: "child", diff --git a/libs/daemon/daemon.go b/libs/daemon/daemon.go index 1c7dc7e9a..89b66e952 100644 --- a/libs/daemon/daemon.go +++ b/libs/daemon/daemon.go @@ -17,8 +17,10 @@ type Daemon struct { // Environment variables to set in the child process. Env []string - // Arguments to pass to the child process. The main executable is always the CLI - // binary itself. + // Path to executable to run. If empty, the current executable is used. + Executable string + + // Arguments to pass to the child process. Args []string // Log file to write the child process's output to. @@ -34,7 +36,12 @@ func (d *Daemon) Start() error { return err } - d.cmd = exec.Command(cli, d.Args...) + executable := d.Executable + if executable == "" { + executable = cli + } + + d.cmd = exec.Command(executable, d.Args...) // Set environment variable so that the child process know's it's parent's PID. d.Env = append(d.Env, fmt.Sprintf("%s=%d", DatabricksCliParentPid, os.Getpid())) diff --git a/libs/process/testdata/parent.sh b/libs/process/testdata/parent.sh new file mode 100755 index 000000000..be80c0ee4 --- /dev/null +++ b/libs/process/testdata/parent.sh @@ -0,0 +1,6 @@ +#!/bin/bash +# Launch background process. +(sleep 5; echo "abc" > $2) & + +# Save PID of the background process to the file specified by the first argument. +echo -n $! > $1 diff --git a/libs/process/wait_test.go b/libs/process/wait_test.go index bc9407a8e..8db9de781 100644 --- a/libs/process/wait_test.go +++ b/libs/process/wait_test.go @@ -1,12 +1,66 @@ package process import ( + "os" + "os/exec" + "path/filepath" + "strconv" "testing" + "time" + "github.com/databricks/cli/internal/testutil" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestWait(t *testing.T) { - err := Wait(1000000) - assert.EqualError(t, err, "process with pid 1000000 does not exist") + t.Parallel() + tmpDir := t.TempDir() + + pidFile := filepath.Join(tmpDir, "child.pid") + outputFile := filepath.Join(tmpDir, "output.txt") + + // For this test, we cannot start the background process in this test itself + // and instead have to use parent.sh as an intermediary. + // + // This is because in Unix if we start the background process in this test itself, + // the background process will be a child of this test process and thus would + // need to be reaped by this test process (using the Wait function / syscall). + // Otherwise waitForPid will forever wait for the background process to finish. + // + // If we rely on an intermediate script to start the background process, the + // background process is reasigned to the init process (PID 1) once the parent + // exits and thus we can successfully wait for it in this test using waitForPid function. + cmd := exec.Command("./testdata/parent.sh", pidFile, outputFile) + err := cmd.Start() + require.NoError(t, err) + + // Wait 5 seconds for the parent bash script to write the child's PID to the file. + var childPid int + require.Eventually(t, func() bool { + b, err := os.ReadFile(pidFile) + if err != nil { + return false + } + + childPid, err = strconv.Atoi(string(b)) + require.NoError(t, err) + return true + }, 2*time.Second, 100*time.Millisecond) + + // The output file should not exist yet since the background process should + // still be running. + assert.NoFileExists(t, outputFile) + + // Wait for the background process to finish. + err = waitForPid(childPid) + assert.NoError(t, err) + + // The output file should exist now since the background process has finished. + testutil.AssertFileContents(t, outputFile, "abc\n") + + // Since the background process has finished, waiting for it again should + // return an error. + err = waitForPid(childPid) + assert.Regexp(t, "process with pid .* does not exist", err.Error()) } From 07141ba7ad79c3f26613f9a89e8b9464e6fadaa4 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 10:44:10 +0100 Subject: [PATCH 12/28] skip wait test on windows --- libs/process/{wait_test.go => wait_unix_test.go} | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) rename libs/process/{wait_test.go => wait_unix_test.go} (94%) diff --git a/libs/process/wait_test.go b/libs/process/wait_unix_test.go similarity index 94% rename from libs/process/wait_test.go rename to libs/process/wait_unix_test.go index 8db9de781..680e8b5c3 100644 --- a/libs/process/wait_test.go +++ b/libs/process/wait_unix_test.go @@ -4,6 +4,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strconv" "testing" "time" @@ -13,7 +14,11 @@ import ( "github.com/stretchr/testify/require" ) -func TestWait(t *testing.T) { +func TestWaitUnix(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Skipping test on Windows") + } + t.Parallel() tmpDir := t.TempDir() From a4009ed6fad6a95e25d8fbc56f360e1b5ec0284d Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 13:14:29 +0100 Subject: [PATCH 13/28] use script to wait --- acceptance/bin/wait_pid | 4 +- acceptance/bundle/debug/out.stderr.txt | 2 +- .../daemon/child-detaches/out.parentchild.txt | 18 ++--- acceptance/daemon/child-detaches/output.txt | 4 +- acceptance/daemon/child-detaches/script | 5 +- .../daemon/child-discards-output/output.txt | 15 ++-- .../daemon/child-discards-output/script | 6 +- cmd/selftest/child.go | 21 ++---- cmd/selftest/parent.go | 10 +-- libs/process/wait.go | 15 ---- libs/process/wait_unix.go | 36 ---------- libs/process/wait_unix_test.go | 71 ------------------- libs/process/wait_windows.go | 44 ------------ 13 files changed, 32 insertions(+), 219 deletions(-) delete mode 100644 libs/process/wait.go delete mode 100644 libs/process/wait_unix.go delete mode 100644 libs/process/wait_unix_test.go delete mode 100644 libs/process/wait_windows.go diff --git a/acceptance/bin/wait_pid b/acceptance/bin/wait_pid index f1375bf51..6e28c144c 100755 --- a/acceptance/bin/wait_pid +++ b/acceptance/bin/wait_pid @@ -13,13 +13,13 @@ wait_pid() { if [[ "$OSTYPE" == "msys"* || "$OSTYPE" == "cygwin"* ]]; then # Windows approach if ! tasklist | grep -q $pid; then - echo "Process has ended" + echo "[wait_pid] process has ended" return 0 fi else # Linux/macOS approach if ! kill -0 $pid 2>/dev/null; then - echo "Process has ended" + echo "[wait_pid] process has ended" return 0 fi fi diff --git a/acceptance/bundle/debug/out.stderr.txt b/acceptance/bundle/debug/out.stderr.txt index e5867e008..031d58082 100644 --- a/acceptance/bundle/debug/out.stderr.txt +++ b/acceptance/bundle/debug/out.stderr.txt @@ -72,7 +72,7 @@ 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=metadata.AnnotatePipelines 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Using Terraform from DATABRICKS_TF_EXEC_PATH at [TERRAFORM] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize -10:07:59 Debug: Using Terraform CLI config from DATABRICKS_TF_CLI_CONFIG_FILE at [DATABRICKS_TF_CLI_CONFIG_FILE] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize +10:07:59 Debug: DATABRICKS_TF_PROVIDER_VERSION as 1.62.0 does not match the current version 1.65.1, ignoring DATABRICKS_TF_CLI_CONFIG_FILE pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Environment variables for Terraform: ...redacted... pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit 10:07:59 Debug: No script defined for postinit, skipping pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit diff --git a/acceptance/daemon/child-detaches/out.parentchild.txt b/acceptance/daemon/child-detaches/out.parentchild.txt index 36d2e9722..e4223edfd 100644 --- a/acceptance/daemon/child-detaches/out.parentchild.txt +++ b/acceptance/daemon/child-detaches/out.parentchild.txt @@ -1,13 +1,7 @@ -Parent process has started -Started the child process -Provided input: Hello from the other side -Released the child process -Parent process is exiting - -==================== - -All output from this point on is from the child process -Parent process has exited -Received input from parent process: -Hello from the other side +[parent] started child +[parent] input sent to child: Hello from the other side +[parent] exiting +[child][wait_pid] process has ended +[child] Parent process has exited +[child] input from parent: Hello from the other side diff --git a/acceptance/daemon/child-detaches/output.txt b/acceptance/daemon/child-detaches/output.txt index 30e9c5331..1e14c4514 100644 --- a/acceptance/daemon/child-detaches/output.txt +++ b/acceptance/daemon/child-detaches/output.txt @@ -1,2 +1,2 @@ -waiting for child process to finish -Process has ended +[script] waiting for child process to end... +[script][wait_pid] process has ended diff --git a/acceptance/daemon/child-detaches/script b/acceptance/daemon/child-detaches/script index 0e83daade..cd123b66b 100644 --- a/acceptance/daemon/child-detaches/script +++ b/acceptance/daemon/child-detaches/script @@ -1,8 +1,9 @@ export DATABRICKS_CLI_SELFTEST_CHILD_OUTPUT_FILE="out.parentchild.txt" -$CLI selftest parent &> out.parentchild.txt +$CLI selftest parent > "out.parentchild.txt" -echo "waiting for child process to finish" +echo "[script] waiting for child process to end..." +echo -n "[script]" wait_pid $(cat ./child.pid) rm ./child.pid diff --git a/acceptance/daemon/child-discards-output/output.txt b/acceptance/daemon/child-discards-output/output.txt index 5fe488ade..3e92a5cb3 100644 --- a/acceptance/daemon/child-discards-output/output.txt +++ b/acceptance/daemon/child-discards-output/output.txt @@ -1,9 +1,6 @@ -Parent process has started -Started the child process -Provided input: Hello from the other side -Released the child process -Parent process is exiting - -wait for child process to finish -Process has ended -the child process should not have written any output. +[parent] started child +[parent] input sent to child: Hello from the other side +[parent] exiting +[script] waiting for child process to end... +[script][wait_pid] process has ended +[script] child process should not have written any output. diff --git a/acceptance/daemon/child-discards-output/script b/acceptance/daemon/child-discards-output/script index 94ef19c70..c7833422f 100644 --- a/acceptance/daemon/child-discards-output/script +++ b/acceptance/daemon/child-discards-output/script @@ -1,9 +1,9 @@ $CLI selftest parent -echo " -wait for child process to finish" +echo "[script] waiting for child process to end..." +echo -n "[script]" wait_pid $(cat ./child.pid) -echo "the child process should not have written any output." +echo "[script] child process should not have written any output." rm ./child.pid diff --git a/cmd/selftest/child.go b/cmd/selftest/child.go index b1584970b..129042dec 100644 --- a/cmd/selftest/child.go +++ b/cmd/selftest/child.go @@ -1,14 +1,12 @@ package selftest import ( - "errors" "fmt" "io" "os" - "strconv" + "os/exec" "github.com/databricks/cli/libs/daemon" - "github.com/databricks/cli/libs/process" "github.com/spf13/cobra" ) @@ -17,27 +15,20 @@ func newChildCommand() *cobra.Command { return &cobra.Command{ Use: "child", RunE: func(cmd *cobra.Command, args []string) error { - parentPid, err := strconv.Atoi(os.Getenv(daemon.DatabricksCliParentPid)) + waitCmd := exec.Command("bash", "-euo", "pipefail", "wait_pid", os.Getenv(daemon.DatabricksCliParentPid)) + b, err := waitCmd.Output() if err != nil { - return fmt.Errorf("failed to parse parent PID: %w", err) - } - - err = process.Wait(parentPid) - if err != nil && !errors.As(err, &process.ErrProcessNotFound{}) { return fmt.Errorf("failed to wait for parent process: %w", err) } - - fmt.Println("\n====================") - fmt.Println("\nAll output from this point on is from the child process") - fmt.Println("Parent process has exited") + fmt.Print("[child]" + string(b)) + fmt.Println("[child] Parent process has exited") in, err := io.ReadAll(os.Stdin) if err != nil { return fmt.Errorf("failed to read from stdin: %w", err) } - fmt.Println("Received input from parent process:") - fmt.Println(string(in)) + fmt.Println("[child] input from parent: " + string(in)) return nil }, } diff --git a/cmd/selftest/parent.go b/cmd/selftest/parent.go index 7198f3d91..77d1cfe64 100644 --- a/cmd/selftest/parent.go +++ b/cmd/selftest/parent.go @@ -14,8 +14,6 @@ func newParentCommand() *cobra.Command { return &cobra.Command{ Use: "parent", RunE: func(cmd *cobra.Command, args []string) error { - fmt.Println("Parent process has started") - d := daemon.Daemon{ Env: os.Environ(), Args: []string{"selftest", "child"}, @@ -27,21 +25,19 @@ func newParentCommand() *cobra.Command { if err != nil { return fmt.Errorf("failed to start child process: %w", err) } - fmt.Println("Started the child process") + fmt.Println("[parent] started child") err = d.WriteInput([]byte("Hello from the other side\n")) if err != nil { return fmt.Errorf("failed to write to child process: %w", err) } - fmt.Println("Provided input: Hello from the other side") + fmt.Println("[parent] input sent to child: Hello from the other side") err = d.Release() if err != nil { return fmt.Errorf("failed to release child process: %w", err) } - fmt.Println("Released the child process") - - fmt.Println("Parent process is exiting") + fmt.Println("[parent] exiting") return nil }, } diff --git a/libs/process/wait.go b/libs/process/wait.go deleted file mode 100644 index 251ef1db6..000000000 --- a/libs/process/wait.go +++ /dev/null @@ -1,15 +0,0 @@ -package process - -import "fmt" - -type ErrProcessNotFound struct { - Pid int -} - -func (e ErrProcessNotFound) Error() string { - return fmt.Sprintf("process with pid %d does not exist", e.Pid) -} - -func Wait(pid int) error { - return waitForPid(pid) -} diff --git a/libs/process/wait_unix.go b/libs/process/wait_unix.go deleted file mode 100644 index 03f548f97..000000000 --- a/libs/process/wait_unix.go +++ /dev/null @@ -1,36 +0,0 @@ -//go:build linux || darwin - -package process - -import ( - "errors" - "os" - "syscall" - "time" -) - -func waitForPid(pid int) error { - p, err := os.FindProcess(pid) - if err != nil { - return err - } - - // Initial existence check. - if err := p.Signal(syscall.Signal(0)); err != nil { - if errors.Is(err, os.ErrProcessDone) { - return ErrProcessNotFound{Pid: pid} - } - return err - } - - // Polling loop until process exits - for { - if err := p.Signal(syscall.Signal(0)); err != nil { - if errors.Is(err, os.ErrProcessDone) { - return nil - } - return err - } - time.Sleep(100 * time.Millisecond) - } -} diff --git a/libs/process/wait_unix_test.go b/libs/process/wait_unix_test.go deleted file mode 100644 index 680e8b5c3..000000000 --- a/libs/process/wait_unix_test.go +++ /dev/null @@ -1,71 +0,0 @@ -package process - -import ( - "os" - "os/exec" - "path/filepath" - "runtime" - "strconv" - "testing" - "time" - - "github.com/databricks/cli/internal/testutil" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestWaitUnix(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("Skipping test on Windows") - } - - t.Parallel() - tmpDir := t.TempDir() - - pidFile := filepath.Join(tmpDir, "child.pid") - outputFile := filepath.Join(tmpDir, "output.txt") - - // For this test, we cannot start the background process in this test itself - // and instead have to use parent.sh as an intermediary. - // - // This is because in Unix if we start the background process in this test itself, - // the background process will be a child of this test process and thus would - // need to be reaped by this test process (using the Wait function / syscall). - // Otherwise waitForPid will forever wait for the background process to finish. - // - // If we rely on an intermediate script to start the background process, the - // background process is reasigned to the init process (PID 1) once the parent - // exits and thus we can successfully wait for it in this test using waitForPid function. - cmd := exec.Command("./testdata/parent.sh", pidFile, outputFile) - err := cmd.Start() - require.NoError(t, err) - - // Wait 5 seconds for the parent bash script to write the child's PID to the file. - var childPid int - require.Eventually(t, func() bool { - b, err := os.ReadFile(pidFile) - if err != nil { - return false - } - - childPid, err = strconv.Atoi(string(b)) - require.NoError(t, err) - return true - }, 2*time.Second, 100*time.Millisecond) - - // The output file should not exist yet since the background process should - // still be running. - assert.NoFileExists(t, outputFile) - - // Wait for the background process to finish. - err = waitForPid(childPid) - assert.NoError(t, err) - - // The output file should exist now since the background process has finished. - testutil.AssertFileContents(t, outputFile, "abc\n") - - // Since the background process has finished, waiting for it again should - // return an error. - err = waitForPid(childPid) - assert.Regexp(t, "process with pid .* does not exist", err.Error()) -} diff --git a/libs/process/wait_windows.go b/libs/process/wait_windows.go deleted file mode 100644 index c7a8d9ac7..000000000 --- a/libs/process/wait_windows.go +++ /dev/null @@ -1,44 +0,0 @@ -//go:build windows - -package process - -import ( - "errors" - "fmt" - "time" - - "golang.org/x/sys/windows" -) - -func waitForPid(pid int) error { - handle, err := windows.OpenProcess( - windows.SYNCHRONIZE|windows.PROCESS_QUERY_INFORMATION, - false, - uint32(pid), - ) - if errors.Is(err, windows.ERROR_INVALID_PARAMETER) { - return ErrProcessNotFound{Pid: pid} - } - if err != nil { - return fmt.Errorf("OpenProcess failed: %v", err) - } - defer windows.CloseHandle(handle) - - // Wait forever for the process to exit. Wait for 5 minutes max. - ret, err := windows.WaitForSingleObject(handle, uint32(5*time.Minute.Milliseconds())) - if err != nil { - return fmt.Errorf("Wait failed: %v", err) - } - - switch ret { - case windows.WAIT_OBJECT_0: - return nil // Process exited - case 0x00000102: - // Standard library does not have have a constant defined for this - // so we use the hex value directly. This is the WAIT_TIMEOUT value. - // ref: https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobject#return-value - return fmt.Errorf("process wait timed out") - default: - return fmt.Errorf("unexpected process wait return value: %d", ret) - } -} From b130787318b353acab4253db4f570c3e60804e2e Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 13:16:35 +0100 Subject: [PATCH 14/28] - --- cmd/selftest/child.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cmd/selftest/child.go b/cmd/selftest/child.go index 129042dec..b792575e2 100644 --- a/cmd/selftest/child.go +++ b/cmd/selftest/child.go @@ -10,11 +10,18 @@ import ( "github.com/spf13/cobra" ) -// TODO: Manually test that indeed latency is not added. func newChildCommand() *cobra.Command { return &cobra.Command{ Use: "child", RunE: func(cmd *cobra.Command, args []string) error { + // wait_pid lives in acceptance/bin. We expect this command to only be called + // from acceptance tests. + // + // Note: The golang stdlib only provides a way to wait on processes + // that are children of the current process. While it's possible to + // rely on os native syscalls to wait on arbitrary processes, it's hard + // to get right and unit test. So I opted to just rely on the wait_pid + // script here. waitCmd := exec.Command("bash", "-euo", "pipefail", "wait_pid", os.Getenv(daemon.DatabricksCliParentPid)) b, err := waitCmd.Output() if err != nil { From 50308f25a1fe698aa7a79122cc3661ac25aca114 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 13:17:14 +0100 Subject: [PATCH 15/28] - --- acceptance/bundle/debug/out.stderr.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/bundle/debug/out.stderr.txt b/acceptance/bundle/debug/out.stderr.txt index 031d58082..e5867e008 100644 --- a/acceptance/bundle/debug/out.stderr.txt +++ b/acceptance/bundle/debug/out.stderr.txt @@ -72,7 +72,7 @@ 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=metadata.AnnotatePipelines 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Using Terraform from DATABRICKS_TF_EXEC_PATH at [TERRAFORM] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize -10:07:59 Debug: DATABRICKS_TF_PROVIDER_VERSION as 1.62.0 does not match the current version 1.65.1, ignoring DATABRICKS_TF_CLI_CONFIG_FILE pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize +10:07:59 Debug: Using Terraform CLI config from DATABRICKS_TF_CLI_CONFIG_FILE at [DATABRICKS_TF_CLI_CONFIG_FILE] pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Environment variables for Terraform: ...redacted... pid=12345 mutator=initialize mutator=seq mutator=terraform.Initialize 10:07:59 Debug: Apply pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit 10:07:59 Debug: No script defined for postinit, skipping pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit From f6a02237cf9d9528c57e92308621f7ed4f9e2952 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 14:13:01 +0100 Subject: [PATCH 16/28] extend timeout to 1 minute --- acceptance/bin/wait_pid | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/bin/wait_pid b/acceptance/bin/wait_pid index 6e28c144c..dfd0e1d56 100755 --- a/acceptance/bin/wait_pid +++ b/acceptance/bin/wait_pid @@ -5,7 +5,7 @@ # shell. This script is more general purpose. wait_pid() { local pid=$1 - local max_attempts=100 # 100 * 0.1 seconds = 10 seconds + local max_attempts=600 # 600 * 0.1 seconds = 1 Minute local attempt=0 local sleep_time=0.1 From 6f63e14cb74b8fca6350e3f541496e72936ee008 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 14:26:56 +0100 Subject: [PATCH 17/28] - --- cmd/selftest/child.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/selftest/child.go b/cmd/selftest/child.go index b792575e2..3a880f45e 100644 --- a/cmd/selftest/child.go +++ b/cmd/selftest/child.go @@ -20,7 +20,7 @@ func newChildCommand() *cobra.Command { // Note: The golang stdlib only provides a way to wait on processes // that are children of the current process. While it's possible to // rely on os native syscalls to wait on arbitrary processes, it's hard - // to get right and unit test. So I opted to just rely on the wait_pid + // to get right and test. So I opted to just rely on the wait_pid // script here. waitCmd := exec.Command("bash", "-euo", "pipefail", "wait_pid", os.Getenv(daemon.DatabricksCliParentPid)) b, err := waitCmd.Output() From 11af4ba964ef0bacd417aeaafd94a3634a84bc7b Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 14:32:16 +0100 Subject: [PATCH 18/28] delay closing output log file --- libs/daemon/daemon.go | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/libs/daemon/daemon.go b/libs/daemon/daemon.go index 89b66e952..99ec4d97e 100644 --- a/libs/daemon/daemon.go +++ b/libs/daemon/daemon.go @@ -26,8 +26,9 @@ type Daemon struct { // Log file to write the child process's output to. LogFile string - cmd *exec.Cmd - stdin io.WriteCloser + outFile *os.File + cmd *exec.Cmd + stdin io.WriteCloser } func (d *Daemon) Start() error { @@ -43,7 +44,9 @@ func (d *Daemon) Start() error { d.cmd = exec.Command(executable, d.Args...) - // Set environment variable so that the child process know's it's parent's PID. + // Set environment variable so that the child process knows it's parent's PID. + // In unix systems orphaned processes are automatically re-parented to init (pid 1) + // so we cannot rely on os.Getppid() to get the original parent's pid. d.Env = append(d.Env, fmt.Sprintf("%s=%d", DatabricksCliParentPid, os.Getpid())) d.cmd.Env = d.Env @@ -55,14 +58,13 @@ func (d *Daemon) Start() error { // If a log file is provided, redirect stdout and stderr to the log file. if d.LogFile != "" { - f, err := os.OpenFile(d.LogFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) + d.outFile, err = os.OpenFile(d.LogFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) if err != nil { return fmt.Errorf("failed to open log file: %w", err) } - defer f.Close() - d.cmd.Stdout = f - d.cmd.Stderr = f + d.cmd.Stdout = d.outFile + d.cmd.Stderr = d.outFile } d.stdin, err = d.cmd.StdinPipe() @@ -98,6 +100,13 @@ func (d *Daemon) Release() error { } } + if d.outFile != nil { + err := d.outFile.Close() + if err != nil { + return fmt.Errorf("failed to close log file: %w", err) + } + } + if d.cmd == nil { return nil } From 75f3008b66411ef17c57c3d0f7ee2f69822da87b Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 14:33:41 +0100 Subject: [PATCH 19/28] - --- libs/daemon/daemon.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libs/daemon/daemon.go b/libs/daemon/daemon.go index 99ec4d97e..19a293086 100644 --- a/libs/daemon/daemon.go +++ b/libs/daemon/daemon.go @@ -111,8 +111,7 @@ func (d *Daemon) Release() error { return nil } - // The docs recommend calling Release if Wait is not called. This does not - // seem to be necessary since the functionality works without it. However, we - // we add it just to be safe. + // The docs for [os.Process.Release] recommend calling Release if Wait is not called. + // It's probably not necessary but we call it just to be safe. return d.cmd.Process.Release() } From e649fc056b0709d486529433f4e92204b50b4a74 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 14:34:13 +0100 Subject: [PATCH 20/28] - --- libs/process/testdata/parent.sh | 6 ------ 1 file changed, 6 deletions(-) delete mode 100755 libs/process/testdata/parent.sh diff --git a/libs/process/testdata/parent.sh b/libs/process/testdata/parent.sh deleted file mode 100755 index be80c0ee4..000000000 --- a/libs/process/testdata/parent.sh +++ /dev/null @@ -1,6 +0,0 @@ -#!/bin/bash -# Launch background process. -(sleep 5; echo "abc" > $2) & - -# Save PID of the background process to the file specified by the first argument. -echo -n $! > $1 From f6bd82d68f36c5bc6330362d062040656ea93752 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 14:49:24 +0100 Subject: [PATCH 21/28] add readme --- acceptance/daemon/child-detaches/script | 12 ++++++++++++ acceptance/daemon/child-discards-output/script | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/acceptance/daemon/child-detaches/script b/acceptance/daemon/child-detaches/script index cd123b66b..e5aecac55 100644 --- a/acceptance/daemon/child-detaches/script +++ b/acceptance/daemon/child-detaches/script @@ -7,3 +7,15 @@ echo -n "[script]" wait_pid $(cat ./child.pid) rm ./child.pid + +# README: +# This test validates two features of libs/daemon: +# 1. The parent process does not wait for the child process to end. +# 2. The child process correctly receives input from the parent process. +# +# Property (1) is validated because the child process waits for it's parent process +# to end before it writes to stdout. +# +# Property (2) is directly validated by the output of the child process. +# +# See the code for "selftest parent" and "selftest child" commands for more details. diff --git a/acceptance/daemon/child-discards-output/script b/acceptance/daemon/child-discards-output/script index c7833422f..bef0fa43d 100644 --- a/acceptance/daemon/child-discards-output/script +++ b/acceptance/daemon/child-discards-output/script @@ -7,3 +7,7 @@ wait_pid $(cat ./child.pid) echo "[script] child process should not have written any output." rm ./child.pid + +# README: +# THis test validates that stdout / stderr from the child process are not leaked +# to the parent process's output. From 8eb454da1486970a2cbbce6a3df8bd7dbdab4777 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 14:50:02 +0100 Subject: [PATCH 22/28] - --- acceptance/bin/wait_pid | 1 - 1 file changed, 1 deletion(-) diff --git a/acceptance/bin/wait_pid b/acceptance/bin/wait_pid index dfd0e1d56..6546182aa 100755 --- a/acceptance/bin/wait_pid +++ b/acceptance/bin/wait_pid @@ -1,6 +1,5 @@ #!/bin/bash - # wait in bash only works for processes that are direct children to the calling # shell. This script is more general purpose. wait_pid() { From 608f0ad94f92237e5b989a70fdacadb7f74f8564 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 14:51:03 +0100 Subject: [PATCH 23/28] - --- acceptance/bin/wait_pid | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/bin/wait_pid b/acceptance/bin/wait_pid index 6546182aa..093943f3d 100755 --- a/acceptance/bin/wait_pid +++ b/acceptance/bin/wait_pid @@ -27,7 +27,7 @@ wait_pid() { attempt=$((attempt + 1)) done - echo "Timeout: Process $pid did not end within 10 seconds" + echo "Timeout: Process $pid did not end within 1 minute" return 1 } From c59b11f7c11c679502273a077ec051989c0760dd Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 14:52:25 +0100 Subject: [PATCH 24/28] - --- acceptance/daemon/child-detaches/script | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/daemon/child-detaches/script b/acceptance/daemon/child-detaches/script index e5aecac55..32534c6f5 100644 --- a/acceptance/daemon/child-detaches/script +++ b/acceptance/daemon/child-detaches/script @@ -13,7 +13,7 @@ rm ./child.pid # 1. The parent process does not wait for the child process to end. # 2. The child process correctly receives input from the parent process. # -# Property (1) is validated because the child process waits for it's parent process +# Property (1) is validated because the child process waits for its parent process # to end before it writes to stdout. # # Property (2) is directly validated by the output of the child process. From dfd26e37ae7d313786b61ca0a1717e2d6705f4b6 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 14:59:29 +0100 Subject: [PATCH 25/28] - --- cmd/selftest/child.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/selftest/child.go b/cmd/selftest/child.go index 3a880f45e..c28fef11e 100644 --- a/cmd/selftest/child.go +++ b/cmd/selftest/child.go @@ -20,8 +20,7 @@ func newChildCommand() *cobra.Command { // Note: The golang stdlib only provides a way to wait on processes // that are children of the current process. While it's possible to // rely on os native syscalls to wait on arbitrary processes, it's hard - // to get right and test. So I opted to just rely on the wait_pid - // script here. + // to get right and test. waitCmd := exec.Command("bash", "-euo", "pipefail", "wait_pid", os.Getenv(daemon.DatabricksCliParentPid)) b, err := waitCmd.Output() if err != nil { From f504343774aaaf6a73b6b8ef93268b98d72fb85f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 14:59:54 +0100 Subject: [PATCH 26/28] - --- cmd/selftest/child.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/selftest/child.go b/cmd/selftest/child.go index c28fef11e..55c696e16 100644 --- a/cmd/selftest/child.go +++ b/cmd/selftest/child.go @@ -17,8 +17,8 @@ func newChildCommand() *cobra.Command { // wait_pid lives in acceptance/bin. We expect this command to only be called // from acceptance tests. // - // Note: The golang stdlib only provides a way to wait on processes - // that are children of the current process. While it's possible to + // Note: The golang standard library only provides a way to wait on + // processes that are children of the current process. While it's possible to // rely on os native syscalls to wait on arbitrary processes, it's hard // to get right and test. waitCmd := exec.Command("bash", "-euo", "pipefail", "wait_pid", os.Getenv(daemon.DatabricksCliParentPid)) From 86428dab79e047be71285272d99820f972faee21 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 15:02:37 +0100 Subject: [PATCH 27/28] - --- libs/daemon/daemon.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/libs/daemon/daemon.go b/libs/daemon/daemon.go index 19a293086..59bad5367 100644 --- a/libs/daemon/daemon.go +++ b/libs/daemon/daemon.go @@ -26,7 +26,7 @@ type Daemon struct { // Log file to write the child process's output to. LogFile string - outFile *os.File + logFile *os.File cmd *exec.Cmd stdin io.WriteCloser } @@ -58,13 +58,13 @@ func (d *Daemon) Start() error { // If a log file is provided, redirect stdout and stderr to the log file. if d.LogFile != "" { - d.outFile, err = os.OpenFile(d.LogFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) + d.logFile, err = os.OpenFile(d.LogFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) if err != nil { return fmt.Errorf("failed to open log file: %w", err) } - d.cmd.Stdout = d.outFile - d.cmd.Stderr = d.outFile + d.cmd.Stdout = d.logFile + d.cmd.Stderr = d.logFile } d.stdin, err = d.cmd.StdinPipe() @@ -100,8 +100,8 @@ func (d *Daemon) Release() error { } } - if d.outFile != nil { - err := d.outFile.Close() + if d.logFile != nil { + err := d.logFile.Close() if err != nil { return fmt.Errorf("failed to close log file: %w", err) } From 12f1fbdecee4f64679bfbea96a2c50313c61be1b Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 18 Feb 2025 15:02:56 +0100 Subject: [PATCH 28/28] - --- libs/daemon/daemon.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/daemon/daemon.go b/libs/daemon/daemon.go index 59bad5367..f074d057f 100644 --- a/libs/daemon/daemon.go +++ b/libs/daemon/daemon.go @@ -44,7 +44,7 @@ func (d *Daemon) Start() error { d.cmd = exec.Command(executable, d.Args...) - // Set environment variable so that the child process knows it's parent's PID. + // Set environment variable so that the child process knows its parent's PID. // In unix systems orphaned processes are automatically re-parented to init (pid 1) // so we cannot rely on os.Getppid() to get the original parent's pid. d.Env = append(d.Env, fmt.Sprintf("%s=%d", DatabricksCliParentPid, os.Getpid()))