From 84b8cdcf75cb1d27a35e44707351a2d23bc073cd Mon Sep 17 00:00:00 2001 From: Travis Nielsen Date: Tue, 17 Mar 2020 15:58:04 -0600 Subject: [PATCH] exec: simplify the exec package from unused methods and logging The methods and arguments to the exec methods are not all used anymore. This cleans up the methods to only what is necessary to improve the readability and maintainability. Signed-off-by: Travis Nielsen --- pkg/util/exec/exec.go | 85 +++++++++++++---------------- pkg/util/exec/test/mockexec.go | 9 --- pkg/util/exec/translate_exec.go | 7 --- tests/framework/utils/k8s_helper.go | 6 +- 4 files changed, 41 insertions(+), 66 deletions(-) diff --git a/pkg/util/exec/exec.go b/pkg/util/exec/exec.go index d99151b93..f64548a96 100644 --- a/pkg/util/exec/exec.go +++ b/pkg/util/exec/exec.go @@ -31,39 +31,26 @@ import ( ) type Executor interface { - StartExecuteCommand(debug bool, actionName string, command string, arg ...string) (*exec.Cmd, error) - ExecuteCommand(debug bool, actionName string, command string, arg ...string) error - ExecuteCommandWithOutput(debug bool, actionName string, command string, arg ...string) (string, error) - ExecuteCommandWithCombinedOutput(debug bool, actionName string, command string, arg ...string) (string, error) - ExecuteCommandWithOutputFile(debug bool, actionName, command, outfileArg string, arg ...string) (string, error) - ExecuteCommandWithOutputFileTimeout(debug bool, timeout time.Duration, actionName, command, outfileArg string, arg ...string) (string, error) - ExecuteCommandWithTimeout(debug bool, timeout time.Duration, actionName string, command string, arg ...string) (string, error) + ExecuteCommand(suppressLogOutput bool, actionName string, command string, arg ...string) error + ExecuteCommandWithOutput(suppressLogCommand bool, actionName string, command string, arg ...string) (string, error) + ExecuteCommandWithCombinedOutput(suppressLogOutput bool, actionName string, command string, arg ...string) (string, error) + ExecuteCommandWithOutputFile(suppressLogOutput bool, actionName, command, outfileArg string, arg ...string) (string, error) + ExecuteCommandWithOutputFileTimeout(suppressLogOutput bool, timeout time.Duration, actionName, command, outfileArg string, arg ...string) (string, error) + ExecuteCommandWithTimeout(suppressLogOutput bool, timeout time.Duration, actionName string, command string, arg ...string) (string, error) ExecuteStat(name string) (os.FileInfo, error) } type CommandExecutor struct { } -// Start a process and return immediately -func (*CommandExecutor) StartExecuteCommand(debug bool, actionName string, command string, arg ...string) (*exec.Cmd, error) { - cmd, stdout, stderr, err := startCommand(debug, command, arg...) - if err != nil { - return cmd, createCommandError(err, actionName) - } - - go logOutput(actionName, stdout, stderr) - - return cmd, nil -} - // Start a process and wait for its completion -func (*CommandExecutor) ExecuteCommand(debug bool, actionName string, command string, arg ...string) error { - cmd, stdout, stderr, err := startCommand(debug, command, arg...) +func (*CommandExecutor) ExecuteCommand(suppressLogOutput bool, actionName string, command string, arg ...string) error { + cmd, stdout, stderr, err := startCommand(suppressLogOutput, command, arg...) if err != nil { return createCommandError(err, actionName) } - logOutput(actionName, stdout, stderr) + logOutput(suppressLogOutput, actionName, stdout, stderr) if err := cmd.Wait(); err != nil { return createCommandError(err, actionName) @@ -73,8 +60,8 @@ func (*CommandExecutor) ExecuteCommand(debug bool, actionName string, command st } // ExecuteCommandWithTimeout starts a process and wait for its completion with timeout. -func (*CommandExecutor) ExecuteCommandWithTimeout(debug bool, timeout time.Duration, actionName string, command string, arg ...string) (string, error) { - logCommand(debug, command, arg...) +func (*CommandExecutor) ExecuteCommandWithTimeout(suppressLogOutput bool, timeout time.Duration, actionName string, command string, arg ...string) (string, error) { + logCommand(suppressLogOutput, command, arg...) cmd := exec.Command(command, arg...) var b bytes.Buffer @@ -90,11 +77,11 @@ func (*CommandExecutor) ExecuteCommandWithTimeout(debug bool, timeout time.Durat done <- cmd.Wait() }() - interrupSent := false + interruptSent := false for { select { case <-time.After(timeout): - if interrupSent { + if interruptSent { logger.Infof("Timeout waiting for process %s to return after interrupt signal was sent. Sending kill signal to the process", command) var e error if err := cmd.Process.Kill(); err != nil { @@ -111,12 +98,12 @@ func (*CommandExecutor) ExecuteCommandWithTimeout(debug bool, timeout time.Durat logger.Errorf("Failed to send interrupt signal to process %s: %+v", command, err) // kill signal will be sent next loop } - interrupSent = true + interruptSent = true case err := <-done: if err != nil { return strings.TrimSpace(b.String()), createCommandError(err, command) } - if interrupSent { + if interruptSent { e := fmt.Errorf("Timeout waiting for the command %s to return", command) return strings.TrimSpace(b.String()), createCommandError(e, command) } @@ -125,20 +112,20 @@ func (*CommandExecutor) ExecuteCommandWithTimeout(debug bool, timeout time.Durat } } -func (*CommandExecutor) ExecuteCommandWithOutput(debug bool, actionName string, command string, arg ...string) (string, error) { - logCommand(debug, command, arg...) +func (*CommandExecutor) ExecuteCommandWithOutput(suppressLogOutput bool, actionName string, command string, arg ...string) (string, error) { + logCommand(suppressLogOutput, command, arg...) cmd := exec.Command(command, arg...) return runCommandWithOutput(actionName, cmd, false) } -func (*CommandExecutor) ExecuteCommandWithCombinedOutput(debug bool, actionName string, command string, arg ...string) (string, error) { - logCommand(debug, command, arg...) +func (*CommandExecutor) ExecuteCommandWithCombinedOutput(suppressLogOutput bool, actionName string, command string, arg ...string) (string, error) { + logCommand(suppressLogOutput, command, arg...) cmd := exec.Command(command, arg...) return runCommandWithOutput(actionName, cmd, true) } // Same as ExecuteCommandWithOutputFile but with a timeout limit. -func (*CommandExecutor) ExecuteCommandWithOutputFileTimeout(debug bool, timeout time.Duration, actionName string, +func (*CommandExecutor) ExecuteCommandWithOutputFileTimeout(suppressLogOutput bool, timeout time.Duration, actionName string, command, outfileArg string, arg ...string) (string, error) { outFile, err := ioutil.TempFile("", "") @@ -149,7 +136,7 @@ func (*CommandExecutor) ExecuteCommandWithOutputFileTimeout(debug bool, timeout defer os.Remove(outFile.Name()) arg = append(arg, outfileArg, outFile.Name()) - logCommand(debug, command, arg...) + logCommand(suppressLogOutput, command, arg...) ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() @@ -160,7 +147,7 @@ func (*CommandExecutor) ExecuteCommandWithOutputFileTimeout(debug bool, timeout // if there was anything that went to stdout/stderr then log it, even before // we return an error if string(cmdOut) != "" { - logger.Info(string(cmdOut)) + writeToLog(suppressLogOutput, string(cmdOut)) } if ctx.Err() == context.DeadlineExceeded { @@ -175,7 +162,7 @@ func (*CommandExecutor) ExecuteCommandWithOutputFileTimeout(debug bool, timeout return string(fileOut), err } -func (*CommandExecutor) ExecuteCommandWithOutputFile(debug bool, actionName string, command, outfileArg string, arg ...string) (string, error) { +func (*CommandExecutor) ExecuteCommandWithOutputFile(suppressLogOutput bool, actionName string, command, outfileArg string, arg ...string) (string, error) { // create a temporary file to serve as the output file for the command to be run and ensure // it is cleaned up after this function is done @@ -189,12 +176,12 @@ func (*CommandExecutor) ExecuteCommandWithOutputFile(debug bool, actionName stri // append the output file argument to the list or args arg = append(arg, outfileArg, outFile.Name()) - logCommand(debug, command, arg...) + logCommand(suppressLogOutput, command, arg...) cmd := exec.Command(command, arg...) cmdOut, err := cmd.CombinedOutput() // if there was anything that went to stdout/stderr then log it, even before we return an error if string(cmdOut) != "" { - logger.Debug(string(cmdOut)) + writeToLog(suppressLogOutput, string(cmdOut)) } if err != nil { return string(cmdOut), err @@ -205,8 +192,8 @@ func (*CommandExecutor) ExecuteCommandWithOutputFile(debug bool, actionName stri return string(fileOut), err } -func startCommand(debug bool, command string, arg ...string) (*exec.Cmd, io.ReadCloser, io.ReadCloser, error) { - logCommand(debug, command, arg...) +func startCommand(suppressLogOutput bool, command string, arg ...string) (*exec.Cmd, io.ReadCloser, io.ReadCloser, error) { + logCommand(suppressLogOutput, command, arg...) cmd := exec.Command(command, arg...) stdout, err := cmd.StdoutPipe() @@ -228,16 +215,16 @@ func (*CommandExecutor) ExecuteStat(name string) (os.FileInfo, error) { } // read from reader line by line and write it to the log -func logFromReader(logger *capnslog.PackageLogger, reader io.ReadCloser) { +func logFromReader(suppressLogOutput bool, logger *capnslog.PackageLogger, reader io.ReadCloser) { in := bufio.NewScanner(reader) lastLine := "" for in.Scan() { lastLine = in.Text() - logger.Info(lastLine) + writeToLog(suppressLogOutput, lastLine) } } -func logOutput(name string, stdout, stderr io.ReadCloser) { +func logOutput(suppressLogOutput bool, name string, stdout, stderr io.ReadCloser) { if stdout == nil || stderr == nil { logger.Warningf("failed to collect stdout and stderr") return @@ -254,8 +241,8 @@ func logOutput(name string, stdout, stderr io.ReadCloser) { } } - go logFromReader(childLogger, stderr) - logFromReader(childLogger, stdout) + go logFromReader(suppressLogOutput, childLogger, stderr) + logFromReader(suppressLogOutput, childLogger, stdout) } func runCommandWithOutput(actionName string, cmd *exec.Cmd, combinedOutput bool) (string, error) { @@ -277,9 +264,13 @@ func runCommandWithOutput(actionName string, cmd *exec.Cmd, combinedOutput bool) return out, nil } -func logCommand(debug bool, command string, arg ...string) { +func logCommand(suppressLogOutput bool, command string, arg ...string) { msg := fmt.Sprintf("Running command: %s %s", command, strings.Join(arg, " ")) - if debug { + writeToLog(suppressLogOutput, msg) +} + +func writeToLog(suppressLogOutput bool, msg string) { + if suppressLogOutput { logger.Debug(msg) } else { logger.Info(msg) diff --git a/pkg/util/exec/test/mockexec.go b/pkg/util/exec/test/mockexec.go index 6ade83894..652b525a4 100644 --- a/pkg/util/exec/test/mockexec.go +++ b/pkg/util/exec/test/mockexec.go @@ -41,15 +41,6 @@ func (e *MockExecutor) ExecuteCommand(debug bool, actionName string, command str return nil } -func (e *MockExecutor) StartExecuteCommand(debug bool, actionName string, command string, arg ...string) (*exec.Cmd, error) { - if e.MockStartExecuteCommand != nil { - return e.MockStartExecuteCommand(debug, actionName, command, arg...) - } - - args := []string{command} - return &exec.Cmd{Args: append(args, arg...)}, nil -} - func (e *MockExecutor) ExecuteCommandWithOutput(debug bool, actionName string, command string, arg ...string) (string, error) { if e.MockExecuteCommandWithOutput != nil { return e.MockExecuteCommandWithOutput(debug, actionName, command, arg...) diff --git a/pkg/util/exec/translate_exec.go b/pkg/util/exec/translate_exec.go index 06ae9a09b..68450d367 100644 --- a/pkg/util/exec/translate_exec.go +++ b/pkg/util/exec/translate_exec.go @@ -18,7 +18,6 @@ package exec import ( "fmt" "os" - "os/exec" "time" ) @@ -34,12 +33,6 @@ type TranslateCommandExecutor struct { Translator func(debug bool, actionName string, command string, arg ...string) (string, []string) } -// StartExecuteCommand starts a process and return immediately -func (e *TranslateCommandExecutor) StartExecuteCommand(debug bool, actionName string, command string, arg ...string) (*exec.Cmd, error) { - transCommand, transArgs := e.Translator(debug, actionName, command, arg...) - return e.Executor.StartExecuteCommand(debug, actionName, transCommand, transArgs...) -} - // ExecuteCommand starts a process and wait for its completion func (e *TranslateCommandExecutor) ExecuteCommand(debug bool, actionName string, command string, arg ...string) error { transCommand, transArgs := e.Translator(debug, actionName, command, arg...) diff --git a/tests/framework/utils/k8s_helper.go b/tests/framework/utils/k8s_helper.go index c9f87bc60..ee1df1722 100644 --- a/tests/framework/utils/k8s_helper.go +++ b/tests/framework/utils/k8s_helper.go @@ -117,7 +117,7 @@ func (k8sh *K8sHelper) GetDockerImage(image string) error { if dockercmd == "" { dockercmd = "docker" } - return k8sh.executor.ExecuteCommand(false, "", dockercmd, "pull", image) + return k8sh.executor.ExecuteCommand(false, dockercmd, "pull", image) } // SetDeploymentVersion sets the container version on the deployment. It is assumed to be the rook/ceph image. @@ -162,9 +162,9 @@ func (k8sh *K8sHelper) KubectlWithStdin(stdin string, args ...string) (string, e } func getKubeConfig(executor exec.Executor) (*rest.Config, error) { - context, err := executor.ExecuteCommandWithOutput(false, "", "kubectl", "config", "view", "-o", "json") + context, err := executor.ExecuteCommandWithOutput(false, "kubectl", "config", "view", "-o", "json") if err != nil { - k8slogger.Errorf("Errors Encountered while executing kubectl command : %v", err) + k8slogger.Errorf("failed to execute kubectl command. %v", err) } // Parse the kubectl context to get the settings for client connections