From 3fbdee07774a1ca547d72f2bf300e8c35e22bae3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 16 Mar 2018 10:54:53 -0400 Subject: [PATCH 1/2] clean up remote.Cmd api Combine the ExitStatus and Err values from remote.Cmd into an error returned by Wait, better matching the behavior of the os/exec package. Non-zero exit codes are returned from Wait as a remote.ExitError. Communicator related errors are returned directly. Clean up all the error handling in the provisioners using a communicator. Also remove the extra copyOutput synchronization that was copied from package to package. --- .../provisioners/chef/resource_provisioner.go | 30 ++---- .../habitat/resource_provisioner.go | 29 ++--- .../remote-exec/resource_provisioner.go | 10 +- .../salt-masterless/resource_provisioner.go | 101 ++++++++---------- communicator/remote/command.go | 40 ++++--- communicator/ssh/communicator.go | 6 +- communicator/ssh/communicator_test.go | 7 +- 7 files changed, 94 insertions(+), 129 deletions(-) diff --git a/builtin/provisioners/chef/resource_provisioner.go b/builtin/provisioners/chef/resource_provisioner.go index ea44e9fd0..19f226eca 100644 --- a/builtin/provisioners/chef/resource_provisioner.go +++ b/builtin/provisioners/chef/resource_provisioner.go @@ -680,17 +680,10 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi outR, outW := io.Pipe() errR, errW := io.Pipe() - outDoneCh := make(chan struct{}) - errDoneCh := make(chan struct{}) - go p.copyOutput(o, outR, outDoneCh) - go p.copyOutput(o, errR, errDoneCh) - go func() { - // Wait for output to clean up - outW.Close() - errW.Close() - <-outDoneCh - <-errDoneCh - }() + go p.copyOutput(o, outR) + go p.copyOutput(o, errR) + defer outW.Close() + defer errW.Close() cmd := &remote.Cmd{ Command: command, @@ -703,20 +696,17 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi return fmt.Errorf("Error executing command %q: %v", cmd.Command, err) } - cmd.Wait() - if cmd.Err() != nil { - return cmd.Err() - } - - if cmd.ExitStatus() != 0 { - return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus()) + if err := cmd.Wait(); err != nil { + if rc, ok := err.(remote.ExitError); ok { + return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, rc) + } + return err } return nil } -func (p *provisioner) copyOutput(o terraform.UIOutput, r io.Reader, doneCh chan<- struct{}) { - defer close(doneCh) +func (p *provisioner) copyOutput(o terraform.UIOutput, r io.Reader) { lr := linereader.New(r) for line := range lr.Ch { o.Output(line) diff --git a/builtin/provisioners/habitat/resource_provisioner.go b/builtin/provisioners/habitat/resource_provisioner.go index ada06a88c..d4067bd86 100644 --- a/builtin/provisioners/habitat/resource_provisioner.go +++ b/builtin/provisioners/habitat/resource_provisioner.go @@ -729,8 +729,7 @@ func (p *provisioner) uploadUserTOML(o terraform.UIOutput, comm communicator.Com } -func (p *provisioner) copyOutput(o terraform.UIOutput, r io.Reader, doneCh chan<- struct{}) { - defer close(doneCh) +func (p *provisioner) copyOutput(o terraform.UIOutput, r io.Reader) { lr := linereader.New(r) for line := range lr.Ch { o.Output(line) @@ -741,16 +740,10 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi outR, outW := io.Pipe() errR, errW := io.Pipe() - outDoneCh := make(chan struct{}) - errDoneCh := make(chan struct{}) - go p.copyOutput(o, outR, outDoneCh) - go p.copyOutput(o, errR, errDoneCh) - defer func() { - outW.Close() - errW.Close() - <-outDoneCh - <-errDoneCh - }() + go p.copyOutput(o, outR) + go p.copyOutput(o, errR) + defer outW.Close() + defer errW.Close() cmd := &remote.Cmd{ Command: command, @@ -762,13 +755,11 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi return fmt.Errorf("Error executing command %q: %v", cmd.Command, err) } - cmd.Wait() - if cmd.Err() != nil { - return cmd.Err() - } - - if cmd.ExitStatus() != 0 { - return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus()) + if err := cmd.Wait(); err != nil { + if rc, ok := err.(remote.ExitError); ok { + return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, rc) + } + return err } return nil diff --git a/builtin/provisioners/remote-exec/resource_provisioner.go b/builtin/provisioners/remote-exec/resource_provisioner.go index 8c3d671b9..a1662bc78 100644 --- a/builtin/provisioners/remote-exec/resource_provisioner.go +++ b/builtin/provisioners/remote-exec/resource_provisioner.go @@ -196,16 +196,14 @@ func runScripts( if err := comm.Start(cmd); err != nil { return fmt.Errorf("Error starting script: %v", err) } - cmd.Wait() - if err := cmd.Err(); err != nil { + if err := cmd.Wait(); err != nil { + if rc, ok := err.(remote.ExitError); ok { + return fmt.Errorf("Script exited with non-zero exit status: %d", rc) + } return fmt.Errorf("Remote command exited with error: %s", err) } - if cmd.ExitStatus() != 0 { - err = fmt.Errorf("Script exited with non-zero exit status: %d", cmd.ExitStatus()) - } - // Upload a blank follow up file in the same path to prevent residual // script contents from remaining on remote machine empty := bytes.NewReader([]byte("")) diff --git a/builtin/provisioners/salt-masterless/resource_provisioner.go b/builtin/provisioners/salt-masterless/resource_provisioner.go index dd19d419e..2dc59a912 100644 --- a/builtin/provisioners/salt-masterless/resource_provisioner.go +++ b/builtin/provisioners/salt-masterless/resource_provisioner.go @@ -114,8 +114,6 @@ func Provisioner() terraform.ResourceProvisioner { // Apply executes the file provisioner func applyFn(ctx context.Context) error { // Decode the raw config for this provisioner - var err error - o := ctx.Value(schema.ProvOutputKey).(terraform.UIOutput) d := ctx.Value(schema.ProvConfigDataKey).(*schema.ResourceData) connState := ctx.Value(schema.ProvRawStateKey).(*terraform.InstanceState) @@ -162,21 +160,20 @@ func applyFn(ctx context.Context) error { err = fmt.Errorf("Unable to download Salt: %s", err) } - if err == nil { - cmd.Wait() - if cmd.Err() != nil { - err = cmd.Err() - } else if cmd.ExitStatus() != 0 { - err = fmt.Errorf("Curl exited with non-zero exit status: %d", cmd.ExitStatus()) + if err := cmd.Wait(); err != nil { + if rc, ok := err.(remote.ExitError); ok { + return fmt.Errorf("Curl exited with non-zero exit status: %d", rc) } + return err } outR, outW := io.Pipe() errR, errW := io.Pipe() - outDoneCh := make(chan struct{}) - errDoneCh := make(chan struct{}) - go copyOutput(o, outR, outDoneCh) - go copyOutput(o, errR, errDoneCh) + go copyOutput(o, outR) + go copyOutput(o, errR) + defer outW.Close() + defer errW.Close() + cmd = &remote.Cmd{ Command: fmt.Sprintf("%s /tmp/install_salt.sh %s", p.sudo("sh"), p.BootstrapArgs), Stdout: outW, @@ -184,24 +181,14 @@ func applyFn(ctx context.Context) error { } o.Output(fmt.Sprintf("Installing Salt with command %s", cmd.Command)) - if err = comm.Start(cmd); err != nil { - err = fmt.Errorf("Unable to install Salt: %s", err) + if err := comm.Start(cmd); err != nil { + return fmt.Errorf("Unable to install Salt: %s", err) } - if err == nil { - cmd.Wait() - if cmd.Err() != nil { - err = cmd.Err() - } else if cmd.ExitStatus() != 0 { - err = fmt.Errorf("install_salt.sh exited with non-zero exit status: %d", cmd.ExitStatus()) + if err := cmd.Wait(); err != nil { + if rc, ok := err.(remote.ExitError); ok { + return fmt.Errorf("install_salt.sh exited with non-zero exit status: %d", rc) } - } - // Wait for output to clean up - outW.Close() - errW.Close() - <-outDoneCh - <-errDoneCh - if err != nil { return err } } @@ -270,11 +257,11 @@ func applyFn(ctx context.Context) error { outR, outW := io.Pipe() errR, errW := io.Pipe() - outDoneCh := make(chan struct{}) - errDoneCh := make(chan struct{}) + go copyOutput(o, outR) + go copyOutput(o, errR) + defer outW.Close() + defer errW.Close() - go copyOutput(o, outR, outDoneCh) - go copyOutput(o, errR, errDoneCh) o.Output(fmt.Sprintf("Running: salt-call --local %s", p.CmdArgs)) cmd := &remote.Cmd{ Command: p.sudo(fmt.Sprintf("salt-call --local %s", p.CmdArgs)), @@ -285,21 +272,13 @@ func applyFn(ctx context.Context) error { err = fmt.Errorf("Error executing salt-call: %s", err) } - if err == nil { - cmd.Wait() - if cmd.Err() != nil { - err = cmd.Err() - } else if cmd.ExitStatus() != 0 { - err = fmt.Errorf("Script exited with non-zero exit status: %d", cmd.ExitStatus()) + if err := cmd.Wait(); err != nil { + if rc, ok := err.(remote.ExitError); ok { + return fmt.Errorf("Script exited with non-zero exit status: %d", rc) } + return err } - // Wait for output to clean up - outW.Close() - errW.Close() - <-outDoneCh - <-errDoneCh - - return err + return nil } // Prepends sudo to supplied command if config says to @@ -360,12 +339,13 @@ func (p *provisioner) moveFile(o terraform.UIOutput, comm communicator.Communica if err := comm.Start(cmd); err != nil { return fmt.Errorf("Unable to move %s to %s: %s", src, dst, err) } - cmd.Wait() - if cmd.ExitStatus() != 0 { - return fmt.Errorf("Unable to move %s to %s: exit status: %d", src, dst, cmd.ExitStatus()) + if err := cmd.Wait(); err != nil { + if rc, ok := err.(remote.ExitError); ok { + return fmt.Errorf("Unable to move %s to %s: exit status: %d", src, dst, rc) + } + return err } - - return cmd.Err() + return nil } func (p *provisioner) createDir(o terraform.UIOutput, comm communicator.Communicator, dir string) error { @@ -377,11 +357,13 @@ func (p *provisioner) createDir(o terraform.UIOutput, comm communicator.Communic return err } - cmd.Wait() - if cmd.ExitStatus() != 0 { - return fmt.Errorf("Non-zero exit status.") + if err := cmd.Wait(); err != nil { + if _, ok := err.(remote.ExitError); ok { + return fmt.Errorf("Non-zero exit status.") + } + return err } - return cmd.Err() + return nil } func (p *provisioner) removeDir(o terraform.UIOutput, comm communicator.Communicator, dir string) error { @@ -392,11 +374,13 @@ func (p *provisioner) removeDir(o terraform.UIOutput, comm communicator.Communic if err := comm.Start(cmd); err != nil { return err } - cmd.Wait() - if cmd.ExitStatus() != 0 { - return fmt.Errorf("Non-zero exit status.") + if err := cmd.Wait(); err != nil { + if _, ok := err.(remote.ExitError); ok { + return fmt.Errorf("Non-zero exit status.") + } + return err } - return cmd.Err() + return nil } func (p *provisioner) uploadDir(o terraform.UIOutput, comm communicator.Communicator, dst, src string, ignore []string) error { @@ -549,8 +533,7 @@ func decodeConfig(d *schema.ResourceData) (*provisioner, error) { } func copyOutput( - o terraform.UIOutput, r io.Reader, doneCh chan<- struct{}) { - defer close(doneCh) + o terraform.UIOutput, r io.Reader) { lr := linereader.New(r) for line := range lr.Ch { o.Output(line) diff --git a/communicator/remote/command.go b/communicator/remote/command.go index 0804e70fa..ad69fdfa4 100644 --- a/communicator/remote/command.go +++ b/communicator/remote/command.go @@ -1,6 +1,7 @@ package remote import ( + "fmt" "io" "sync" ) @@ -59,23 +60,28 @@ func (c *Cmd) SetExitStatus(status int, err error) { close(c.exitCh) } -// Err returns any communicator related error. -func (c *Cmd) Err() error { - c.Lock() - defer c.Unlock() - - return c.err -} - -// ExitStatus returns the exit status of the remote command -func (c *Cmd) ExitStatus() int { - c.Lock() - defer c.Unlock() - - return c.exitStatus -} - // Wait waits for the remote command to complete. -func (c *Cmd) Wait() { +// Wait may return an error from the communicator, or an ExitError if the +// process exits with a non-zero exit status. +func (c *Cmd) Wait() error { <-c.exitCh + + c.Lock() + defer c.Unlock() + + if c.err != nil { + return c.err + } + + if c.exitStatus != 0 { + return ExitError(c.exitStatus) + } + + return nil +} + +type ExitError int + +func (e ExitError) Error() string { + return fmt.Sprintf("exit status: %d", e) } diff --git a/communicator/ssh/communicator.go b/communicator/ssh/communicator.go index 4945f5dd8..9d162f5aa 100644 --- a/communicator/ssh/communicator.go +++ b/communicator/ssh/communicator.go @@ -359,11 +359,11 @@ func (c *Communicator) UploadScript(path string, input io.Reader) error { "Error chmodding script file to 0777 in remote "+ "machine: %s", err) } - cmd.Wait() - if cmd.ExitStatus() != 0 { + + if err := cmd.Wait(); err != nil { return fmt.Errorf( "Error chmodding script file to 0777 in remote "+ - "machine %d: %s %s", cmd.ExitStatus(), stdout.String(), stderr.String()) + "machine %v: %s %s", err, stdout.String(), stderr.String()) } return nil diff --git a/communicator/ssh/communicator_test.go b/communicator/ssh/communicator_test.go index 4eb3799ce..546d6f88c 100644 --- a/communicator/ssh/communicator_test.go +++ b/communicator/ssh/communicator_test.go @@ -219,13 +219,10 @@ func TestLostConnection(t *testing.T) { c.Disconnect() }() - cmd.Wait() - if cmd.Err() == nil { + err = cmd.Wait() + if err == nil { t.Fatal("expected communicator error") } - if cmd.ExitStatus() != 0 { - t.Fatal("command should not have returned an exit status") - } } func TestHostKey(t *testing.T) { From ad8642e2c294822e4ce37bdf931a9b7078b466da Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 23 Mar 2018 11:36:57 -0400 Subject: [PATCH 2/2] have remote.ExitError format errors and status Since all use cases of ExitStatus are just putting it into fmt.Errorf, usually with the command string, have ExitStatus do that for the caller. --- .../provisioners/chef/resource_provisioner.go | 3 -- .../habitat/resource_provisioner.go | 3 -- .../remote-exec/resource_provisioner.go | 5 +--- .../salt-masterless/resource_provisioner.go | 18 ------------ communicator/remote/command.go | 29 ++++++++++++------- 5 files changed, 20 insertions(+), 38 deletions(-) diff --git a/builtin/provisioners/chef/resource_provisioner.go b/builtin/provisioners/chef/resource_provisioner.go index 19f226eca..ac47d2fdd 100644 --- a/builtin/provisioners/chef/resource_provisioner.go +++ b/builtin/provisioners/chef/resource_provisioner.go @@ -697,9 +697,6 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi } if err := cmd.Wait(); err != nil { - if rc, ok := err.(remote.ExitError); ok { - return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, rc) - } return err } diff --git a/builtin/provisioners/habitat/resource_provisioner.go b/builtin/provisioners/habitat/resource_provisioner.go index d4067bd86..a5ade0a97 100644 --- a/builtin/provisioners/habitat/resource_provisioner.go +++ b/builtin/provisioners/habitat/resource_provisioner.go @@ -756,9 +756,6 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi } if err := cmd.Wait(); err != nil { - if rc, ok := err.(remote.ExitError); ok { - return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, rc) - } return err } diff --git a/builtin/provisioners/remote-exec/resource_provisioner.go b/builtin/provisioners/remote-exec/resource_provisioner.go index a1662bc78..0f1f90117 100644 --- a/builtin/provisioners/remote-exec/resource_provisioner.go +++ b/builtin/provisioners/remote-exec/resource_provisioner.go @@ -198,10 +198,7 @@ func runScripts( } if err := cmd.Wait(); err != nil { - if rc, ok := err.(remote.ExitError); ok { - return fmt.Errorf("Script exited with non-zero exit status: %d", rc) - } - return fmt.Errorf("Remote command exited with error: %s", err) + return err } // Upload a blank follow up file in the same path to prevent residual diff --git a/builtin/provisioners/salt-masterless/resource_provisioner.go b/builtin/provisioners/salt-masterless/resource_provisioner.go index 2dc59a912..3f2f9b403 100644 --- a/builtin/provisioners/salt-masterless/resource_provisioner.go +++ b/builtin/provisioners/salt-masterless/resource_provisioner.go @@ -161,9 +161,6 @@ func applyFn(ctx context.Context) error { } if err := cmd.Wait(); err != nil { - if rc, ok := err.(remote.ExitError); ok { - return fmt.Errorf("Curl exited with non-zero exit status: %d", rc) - } return err } @@ -186,9 +183,6 @@ func applyFn(ctx context.Context) error { } if err := cmd.Wait(); err != nil { - if rc, ok := err.(remote.ExitError); ok { - return fmt.Errorf("install_salt.sh exited with non-zero exit status: %d", rc) - } return err } } @@ -273,9 +267,6 @@ func applyFn(ctx context.Context) error { } if err := cmd.Wait(); err != nil { - if rc, ok := err.(remote.ExitError); ok { - return fmt.Errorf("Script exited with non-zero exit status: %d", rc) - } return err } return nil @@ -340,9 +331,6 @@ func (p *provisioner) moveFile(o terraform.UIOutput, comm communicator.Communica return fmt.Errorf("Unable to move %s to %s: %s", src, dst, err) } if err := cmd.Wait(); err != nil { - if rc, ok := err.(remote.ExitError); ok { - return fmt.Errorf("Unable to move %s to %s: exit status: %d", src, dst, rc) - } return err } return nil @@ -358,9 +346,6 @@ func (p *provisioner) createDir(o terraform.UIOutput, comm communicator.Communic } if err := cmd.Wait(); err != nil { - if _, ok := err.(remote.ExitError); ok { - return fmt.Errorf("Non-zero exit status.") - } return err } return nil @@ -375,9 +360,6 @@ func (p *provisioner) removeDir(o terraform.UIOutput, comm communicator.Communic return err } if err := cmd.Wait(); err != nil { - if _, ok := err.(remote.ExitError); ok { - return fmt.Errorf("Non-zero exit status.") - } return err } return nil diff --git a/communicator/remote/command.go b/communicator/remote/command.go index ad69fdfa4..4c90368ec 100644 --- a/communicator/remote/command.go +++ b/communicator/remote/command.go @@ -69,19 +69,28 @@ func (c *Cmd) Wait() error { c.Lock() defer c.Unlock() - if c.err != nil { - return c.err - } - - if c.exitStatus != 0 { - return ExitError(c.exitStatus) + if c.err != nil || c.exitStatus != 0 { + return &ExitError{ + Command: c.Command, + ExitStatus: c.exitStatus, + Err: c.err, + } } return nil } -type ExitError int - -func (e ExitError) Error() string { - return fmt.Sprintf("exit status: %d", e) +// ExitError is returned by Wait to indicate and error executing the remote +// command, or a non-zero exit status. +type ExitError struct { + Command string + ExitStatus int + Err error +} + +func (e *ExitError) Error() string { + if e.Err != nil { + return fmt.Sprintf("error executing %q: %v", e.Command, e.Err) + } + return fmt.Sprintf("%q exit status: %d", e.Command, e.ExitStatus) }