Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clean up remote.Cmd api #17609

Merged
merged 2 commits into from
Mar 23, 2018
Merged

clean up remote.Cmd api #17609

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3fbdee0
clean up remote.Cmd api
jbardin Mar 16, 2018
ad8642e
have remote.ExitError format errors and status
jbardin Mar 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 10 additions & 20 deletions builtin/provisioners/chef/resource_provisioner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
Expand Down
29 changes: 10 additions & 19 deletions builtin/provisioners/habitat/resource_provisioner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
Expand All @@ -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
Expand Down
10 changes: 4 additions & 6 deletions builtin/provisioners/remote-exec/resource_provisioner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(""))
Expand Down
101 changes: 42 additions & 59 deletions builtin/provisioners/salt-masterless/resource_provisioner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -162,46 +160,35 @@ 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,
Stderr: errW,
}

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
}
}
Expand Down Expand Up @@ -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)),
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
30 changes: 18 additions & 12 deletions communicator/remote/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package remote

import (
"fmt"
"io"
"sync"
)
Expand Down Expand Up @@ -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 {
// Wait waits for the remote command to complete.
// 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()

return c.err
}
if c.err != nil {
return c.err
}

// ExitStatus returns the exit status of the remote command
func (c *Cmd) ExitStatus() int {
c.Lock()
defer c.Unlock()
if c.exitStatus != 0 {
return ExitError(c.exitStatus)
}

return c.exitStatus
return nil
}

// Wait waits for the remote command to complete.
func (c *Cmd) Wait() {
<-c.exitCh
type ExitError int

func (e ExitError) Error() string {
return fmt.Sprintf("exit status: %d", e)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the most common thing to do with this error is to re-package it in a more verbose message that includes the command name. Should we just make the error type include that itself already so that this error has a useful error message by default? (We could still recognize it and replace it with a more specific error in cases where there's something more interesting to say, of course.)

It looks like we can get to the command as c.Command at the point where this error is generated, so should have everything we need to do it, at the expense of it needing to be a struct rather than just an integer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice idea. I was trying to keep it minimal, since users (though not in any cases here) often just want the integer exit code, and this matches the output of *exec.ExitError. Though since nothing in the provisioners as actually checking the code, maybe we wrap it up with the command string and get rid of all the fmt.Errorfs.

}
6 changes: 3 additions & 3 deletions communicator/ssh/communicator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 2 additions & 5 deletions communicator/ssh/communicator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down