Merge lp:~dimitern/juju-core/320-lp-1286213-juju-run-socket-remove into lp:~go-bot/juju-core/trunk

[Dimiter Naydenov]

uniter: Fixes lp:1286213 - juju run.socket

Two changes:
- NewRunListener() in uniter accepts only socketPath,
not netType and address (we only use unix sockets now).
For Windows, we'll need more refactoring anyway.
- uniter removes the socket on startup, so that "address
already in use" error does not happen.

https://codereview.appspot.com/72550043/

R=

uniter: Fixes lp:1286213 - juju run.socket

Two changes:
- NewRunListener() in uniter accepts only socketPath,
not netType and address (we only use unix sockets now).
For Windows, we'll need more refactoring anyway.
- uniter removes the socket on startup, so that "address
already in use" error does not happen.

https://codereview.appspot.com/72550043/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+209985_code.launchpad.net,

Message:
Please take a look.

Description:
uniter: Fixes lp:1286213 - juju run.socket

Two changes:
- NewRunListener() in uniter accepts only socketPath,
not netType and address (we only use unix sockets now).
For Windows, we'll need more refactoring anyway.
- uniter removes the socket on startup, so that "address
already in use" error does not happen.

https://code.launchpad.net/~dimitern/juju-core/320-lp-1286213-juju-run-socket-remove/+merge/209985

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/72550043/

Affected files (+25, -19 lines):
   A [revision details]
   M cmd/jujud/run.go
   M cmd/jujud/run_test.go
   M worker/uniter/runlistener.go
   M worker/uniter/runlistener_test.go
   M worker/uniter/uniter.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140307170716-pzxvvv3g5ad9llkx
+New revision:
<email address hidden>

Index: cmd/jujud/run.go
=== modified file 'cmd/jujud/run.go'
--- cmd/jujud/run.go 2014-01-29 21:19:55 +0000
+++ cmd/jujud/run.go 2014-03-07 18:13:56 +0000
@@ -119,7 +119,7 @@

   socketPath := filepath.Join(unitDir, uniter.RunListenerFile)
   // make sure the socket exists
- client, err := rpc.Dial(uniter.RunListenerNetType, socketPath)
+ client, err := rpc.Dial("unix", socketPath)
   if err != nil {
    return nil, err
   }

Index: cmd/jujud/run_test.go
=== modified file 'cmd/jujud/run_test.go'
--- cmd/jujud/run_test.go 2014-03-05 19:41:34 +0000
+++ cmd/jujud/run_test.go 2014-03-07 18:13:56 +0000
@@ -181,7 +181,7 @@
   c.Assert(err, gc.IsNil)

   socketPath := filepath.Join(testAgentDir, uniter.RunListenerFile)
- listener, err := uniter.NewRunListener(&mockRunner{c}, "unix", socketPath)
+ listener, err := uniter.NewRunListener(&mockRunner{c}, socketPath)
   c.Assert(err, gc.IsNil)
   c.Assert(listener, gc.NotNil)
   s.AddCleanup(func(*gc.C) {

Index: worker/uniter/runlistener.go
=== modified file 'worker/uniter/runlistener.go'
--- worker/uniter/runlistener.go 2014-01-14 03:31:11 +0000
+++ worker/uniter/runlistener.go 2014-03-07 18:13:56 +0000
@@ -9,6 +9,7 @@
  import (
   "net"
   "net/rpc"
+ "os"
   "sync"

   "launchpad.net/juju-core/utils/exec"
@@ -49,18 +50,22 @@
   return err
  }

-// NewRunListener returns a new RunListener that is listening on the
network
-// type and address passed in. If a valid RunListener is returned, is has
the
-// go routine running, and should be closed by the creator when they are
done
-// with it.
-func NewRunListener(runner CommandRunner, netType, localAddr string)
(*RunListener, error) {
+// NewRunListener returns a new RunListener that is listening on given
+// unix socket path passed in. If a valid RunListener is returned, is
+// has the go routine running, and should be closed by the creator
+// when they are done with it.
+func NewRunListener(runner CommandRunner, socketPath string)
(*RunListener, error) {
   server := rpc.NewServer()
   if err := server.Register(&JujuRunServer{ru...

[Dimiter Naydenov (dimitern)]

"LGTM", says William.

https://codereview.appspot.com/72550043/

[Go Bot (go-bot)]

The attempt to merge lp:~dimitern/juju-core/320-lp-1286213-juju-run-socket-remove into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.012s
ok launchpad.net/juju-core/agent 1.016s
ok launchpad.net/juju-core/agent/mongo 0.522s
ok launchpad.net/juju-core/agent/tools 0.223s
ok launchpad.net/juju-core/bzr 5.102s
ok launchpad.net/juju-core/cert 2.740s
ok launchpad.net/juju-core/charm 0.451s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.030s
ok launchpad.net/juju-core/cloudinit/sshinit 0.834s
ok launchpad.net/juju-core/cmd 0.161s
ok launchpad.net/juju-core/cmd/charm-admin 0.739s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 182.823s
ok launchpad.net/juju-core/cmd/jujud 63.324s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 8.398s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.168s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.019s
ok launchpad.net/juju-core/container 0.049s
ok launchpad.net/juju-core/container/factory 0.047s
ok launchpad.net/juju-core/container/kvm 0.183s
ok launchpad.net/juju-core/container/kvm/mock 0.054s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.221s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.238s
ok launchpad.net/juju-core/environs 2.531s
ok launchpad.net/juju-core/environs/bootstrap 2.900s
ok launchpad.net/juju-core/environs/cloudinit 0.421s
ok launchpad.net/juju-core/environs/config 2.395s
ok launchpad.net/juju-core/environs/configstore 0.027s
ok launchpad.net/juju-core/environs/filestorage 0.026s
ok launchpad.net/juju-core/environs/httpstorage 0.675s
ok launchpad.net/juju-core/environs/imagemetadata 0.395s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.044s
ok launchpad.net/juju-core/environs/jujutest 0.192s
ok launchpad.net/juju-core/environs/manual 15.255s
ok launchpad.net/juju-core/environs/simplestreams 0.284s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.900s
ok launchpad.net/juju-core/environs/storage 0.775s
ok launchpad.net/juju-core/environs/sync 23.467s
ok launchpad.net/juju-core/environs/testing 0.144s
ok launchpad.net/juju-core/environs/tools 4.616s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.019s
ok launchpad.net/juju-core/instance 0.016s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 18.685s
ok launchpad.net/juju-core/...

=== modified file 'cmd/jujud/run.go'
--- cmd/jujud/run.go	2014-01-29 21:19:55 +0000
+++ cmd/jujud/run.go	2014-03-07 18:30:36 +0000
@@ -119,7 +119,7 @@
 
 	socketPath := filepath.Join(unitDir, uniter.RunListenerFile)
 	// make sure the socket exists
-	client, err := rpc.Dial(uniter.RunListenerNetType, socketPath)
+	client, err := rpc.Dial("unix", socketPath)
 	if err != nil {
 		return nil, err
 	}

=== modified file 'cmd/jujud/run_test.go'
--- cmd/jujud/run_test.go	2014-03-05 19:41:34 +0000
+++ cmd/jujud/run_test.go	2014-03-07 18:30:36 +0000
@@ -181,7 +181,7 @@
 	c.Assert(err, gc.IsNil)
 
 	socketPath := filepath.Join(testAgentDir, uniter.RunListenerFile)
-	listener, err := uniter.NewRunListener(&mockRunner{c}, "unix", socketPath)
+	listener, err := uniter.NewRunListener(&mockRunner{c}, socketPath)
 	c.Assert(err, gc.IsNil)
 	c.Assert(listener, gc.NotNil)
 	s.AddCleanup(func(*gc.C) {

=== modified file 'worker/uniter/runlistener.go'
--- worker/uniter/runlistener.go	2014-01-14 03:31:11 +0000
+++ worker/uniter/runlistener.go	2014-03-07 18:30:36 +0000
@@ -9,6 +9,7 @@
 import (
 	"net"
 	"net/rpc"
+	"os"
 	"sync"
 
 	"launchpad.net/juju-core/utils/exec"
@@ -49,18 +50,22 @@
 	return err
 }
 
-// NewRunListener returns a new RunListener that is listening on the network
-// type and address passed in. If a valid RunListener is returned, is has the
-// go routine running, and should be closed by the creator when they are done
-// with it.
-func NewRunListener(runner CommandRunner, netType, localAddr string) (*RunListener, error) {
+// NewRunListener returns a new RunListener that is listening on given
+// unix socket path passed in. If a valid RunListener is returned, is
+// has the go routine running, and should be closed by the creator
+// when they are done with it.
+func NewRunListener(runner CommandRunner, socketPath string) (*RunListener, error) {
 	server := rpc.NewServer()
 	if err := server.Register(&JujuRunServer{runner}); err != nil {
 		return nil, err
 	}
-	listener, err := net.Listen(netType, localAddr)
+	// In case the unix socket is present, delete it.
+	if err := os.Remove(socketPath); err != nil {
+		logger.Tracef("ignoring error on removing %q: %v", socketPath, err)
+	}
+	listener, err := net.Listen("unix", socketPath)
 	if err != nil {
-		logger.Errorf("failed to listen on %s %s: %v", netType, localAddr, err)
+		logger.Errorf("failed to listen on unix:%s: %v", socketPath, err)
 		return nil, err
 	}
 	runListener := &RunListener{

=== modified file 'worker/uniter/runlistener_test.go'
--- worker/uniter/runlistener_test.go	2014-01-14 03:31:11 +0000
+++ worker/uniter/runlistener_test.go	2014-03-07 18:30:36 +0000
@@ -24,7 +24,7 @@
 // Mirror the params to uniter.NewRunListener, but add cleanup to close it.
 func (s *ListenerSuite) NewRunListener(c *gc.C) *uniter.RunListener {
 	s.socketPath = filepath.Join(c.MkDir(), "test.listener")
-	listener, err := uniter.NewRunListener(&mockRunner{c}, "unix", s.socketPath)
+	listener, err := uniter.NewRunListener(&mockRunner{c}, s.socketPath)
 	c.Assert(err, gc.IsNil)
 	c.Assert(listener, gc.NotNil)
 	s.AddCleanup(func(*gc.C) {
@@ -33,13 +33,13 @@
 	return listener
 }
 
-func (s *ListenerSuite) TestNewRunListenerSecondFails(c *gc.C) {
+func (s *ListenerSuite) TestNewRunListenerOnExistingSocketRemovesItAndSucceeds(c *gc.C) {
 	s.NewRunListener(c)
 
-	listener, err := uniter.NewRunListener(&mockRunner{}, "unix", s.socketPath)
-
-	c.Assert(listener, gc.IsNil)
-	c.Assert(err, gc.ErrorMatches, ".* address already in use")
+	listener, err := uniter.NewRunListener(&mockRunner{}, s.socketPath)
+	c.Assert(err, gc.IsNil)
+	c.Assert(listener, gc.NotNil)
+	listener.Close()
 }
 
 func (s *ListenerSuite) TestClientCall(c *gc.C) {

=== modified file 'worker/uniter/uniter.go'
--- worker/uniter/uniter.go	2014-03-07 17:32:21 +0000
+++ worker/uniter/uniter.go	2014-03-07 18:30:36 +0000
@@ -41,8 +41,7 @@
 	// These work fine for linux, but should we need to work with windows
 	// workloads in the future, we'll need to move these into a file that is
 	// compiled conditionally for different targets and use tcp (most likely).
-	RunListenerNetType = "unix"
-	RunListenerFile    = "run.socket"
+	RunListenerFile = "run.socket"
 )
 
 // A UniterExecutionObserver gets the appropriate methods called when a hook
@@ -193,8 +192,8 @@
 	u.envName = env.Name()
 
 	runListenerSocketPath := filepath.Join(u.baseDir, RunListenerFile)
-	logger.Debugf("starting juju-run listener on %s:%s", RunListenerNetType, runListenerSocketPath)
-	u.runListener, err = NewRunListener(u, RunListenerNetType, runListenerSocketPath)
+	logger.Debugf("starting juju-run listener on unix:%s", runListenerSocketPath)
+	u.runListener, err = NewRunListener(u, runListenerSocketPath)
 	if err != nil {
 		return err
 	}