Merge lp:~axwalk/juju-core/lp1212916-hook-unix-socket-namespace into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

uniter: Use abstract namespace for unix sockets

Also, don't attempt to remove the socket after
closing the server. It's not necessary to do this
anyway; the Close method on the socket already
does an Unlink if not using the abstract namespace
(hence tests don't need changing).

Abstract namespaces are a Linux extension, so this
will break if we ever support Unixes. I've purposely
not checked the OS, so that it'll be obviously broken
and we can revisit this issue.

Fixes bug #1212916

https://codereview.appspot.com/12742045/

uniter: Use abstract namespace for unix sockets

Also, don't attempt to remove the socket after
closing the server. It's not necessary to do this
anyway; the Close method on the socket already
does an Unlink if not using the abstract namespace
(hence tests don't need changing).

Abstract namespaces are a Linux extension, so this
will break if we ever support Unixes. I've purposely
not checked the OS, so that it'll be obviously broken
and we can revisit this issue.

Fixes bug #1212916

https://codereview.appspot.com/12742045/

[Andrew Wilkins (axwalk)]

Reviewers: mp+181191_code.launchpad.net,

Message:
Please take a look.

Description:
uniter: Use abstract namespace for unix sockets

Also, don't attempt to remove the socket after
closing the server. It's not necessary to do this
anyway; the Close method on the socket already
does an Unlink if not using the abstract namespace
(hence tests don't need changing).

Abstract namespaces are a Linux extension, so this
will break if we ever support Unixes. I've purposely
not checked the OS, so that it'll be obviously broken
and we can revisit this issue.

Fixes bug #1212916

https://code.launchpad.net/~axwalk/juju-core/lp1212916-hook-unix-socket-namespace/+merge/181191

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M worker/uniter/jujuc/server.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-20130820022554-t4r3stekgq56x0ih
+New revision: <email address hidden>

Index: worker/uniter/uniter.go
=== modified file 'worker/uniter/uniter.go'
--- worker/uniter/uniter.go 2013-08-06 01:42:16 +0000
+++ worker/uniter/uniter.go 2013-08-21 03:27:47 +0000
@@ -338,6 +338,8 @@
    return jujuc.NewCommand(hctx, cmdName)
   }
   socketPath := filepath.Join(u.baseDir, "agent.socket")
+ // Use abstract namespace so we don't get stale socket files.
+ socketPath = "@" + socketPath
   srv, err := jujuc.NewServer(getCmd, socketPath)
   if err != nil {
    return err

Index: worker/uniter/jujuc/server.go
=== modified file 'worker/uniter/jujuc/server.go'
--- worker/uniter/jujuc/server.go 2013-05-02 15:55:42 +0000
+++ worker/uniter/jujuc/server.go 2013-08-21 03:27:47 +0000
@@ -9,16 +9,19 @@
  import (
   "bytes"
   "fmt"
- "launchpad.net/juju-core/cmd"
- "launchpad.net/juju-core/log"
   "net"
   "net/rpc"
- "os"
   "path/filepath"
   "sort"
   "sync"
+
+ "launchpad.net/loggo"
+
+ "launchpad.net/juju-core/cmd"
  )

+var logger = loggo.GetLogger("worker.uniter.jujuc")
+
  // newCommands maps Command names to initializers.
  var newCommands = map[string]func(Context) cmd.Command{
   "close-port": NewClosePortCommand,
@@ -102,8 +105,8 @@
   }
   j.mu.Lock()
   defer j.mu.Unlock()
- log.Infof("worker/uniter/jujuc: running hook tool %q %q",
req.CommandName, req.Args)
- log.Debugf("worker/uniter/jujuc: hook context id %q; dir %q",
req.ContextId, req.Dir)
+ logger.Infof("running hook tool %q %q", req.CommandName, req.Args)
+ logger.Debugf("hook context id %q; dir %q", req.ContextId, req.Dir)
   resp.Code = cmd.Main(c, ctx, req.Args)
   resp.Stdout = stdout.Bytes()
   resp.Stderr = stderr.Bytes()
@@ -176,6 +179,5 @@
  func (s *Server) Close() {
   close(s.closing)
   s.listener.Close()
- os.Remove(s.socketPath)
   <-s.closed
  }

[Roger Peppe (rogpeppe)]

On 2013/08/21 03:59:21, dfc wrote:
> On 2013/08/21 03:46:31, axw wrote:
> > Please take a look.

> LGTM. Thanks for fixing this, I'm surprised none of the tests needed
to change.

LGTM

https://codereview.appspot.com/12742045/

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-08-21 7:46, Andrew Wilkins wrote:
> uniter: Use abstract namespace for unix sockets
>
> Also, don't attempt to remove the socket after closing the server.
> It's not necessary to do this anyway; the Close method on the
> socket already does an Unlink if not using the abstract namespace
> (hence tests don't need changing).

This comment confuses me. You say "lets switch to the abstract
namespace" and then "we don't need to delete if we aren't using the
abstract namespace". Doesn't that mean we *do* need to delete?

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlIUl/oACgkQJdeBCYSNAAN6QgCgg0sq6bYo3PbTSRlxYdAAnpGh
zdUAnRtfwP5D/DmqLBaUaCpAp1mDXDXv
=TKs0
-----END PGP SIGNATURE-----

[Andrew Wilkins (axwalk)]

> > Also, don't attempt to remove the socket after closing the server.
> > It's not necessary to do this anyway; the Close method on the
> > socket already does an Unlink if not using the abstract namespace
> > (hence tests don't need changing).
>
> This comment confuses me. You say "lets switch to the abstract
> namespace" and then "we don't need to delete if we aren't using the
> abstract namespace". Doesn't that mean we *do* need to delete?

That paragraph refers to the removal of the os.Remove from jujuc.Server.Close. While we've moved to the abstract namespace in the normal runtime code, some tests call jujuc.NewServer directly with filesystem paths. What I was trying to get at was that the os.Remove was never necessary (unless the Go standard library changed behaviour?), which explains why the tests don't need to change. Let me know if I'm still not being clear.

=== modified file 'worker/uniter/jujuc/server.go'
--- worker/uniter/jujuc/server.go	2013-05-02 15:55:42 +0000
+++ worker/uniter/jujuc/server.go	2013-08-21 03:45:36 +0000
@@ -9,16 +9,19 @@
 import (
 	"bytes"
 	"fmt"
-	"launchpad.net/juju-core/cmd"
-	"launchpad.net/juju-core/log"
 	"net"
 	"net/rpc"
-	"os"
 	"path/filepath"
 	"sort"
 	"sync"
+
+	"launchpad.net/loggo"
+
+	"launchpad.net/juju-core/cmd"
 )
 
+var logger = loggo.GetLogger("worker.uniter.jujuc")
+
 // newCommands maps Command names to initializers.
 var newCommands = map[string]func(Context) cmd.Command{
 	"close-port":    NewClosePortCommand,
@@ -102,8 +105,8 @@
 	}
 	j.mu.Lock()
 	defer j.mu.Unlock()
-	log.Infof("worker/uniter/jujuc: running hook tool %q %q", req.CommandName, req.Args)
-	log.Debugf("worker/uniter/jujuc: hook context id %q; dir %q", req.ContextId, req.Dir)
+	logger.Infof("running hook tool %q %q", req.CommandName, req.Args)
+	logger.Debugf("hook context id %q; dir %q", req.ContextId, req.Dir)
 	resp.Code = cmd.Main(c, ctx, req.Args)
 	resp.Stdout = stdout.Bytes()
 	resp.Stderr = stderr.Bytes()
@@ -176,6 +179,5 @@
 func (s *Server) Close() {
 	close(s.closing)
 	s.listener.Close()
-	os.Remove(s.socketPath)
 	<-s.closed
 }

=== modified file 'worker/uniter/uniter.go'
--- worker/uniter/uniter.go	2013-08-06 01:42:16 +0000
+++ worker/uniter/uniter.go	2013-08-21 03:45:36 +0000
@@ -338,6 +338,8 @@
 		return jujuc.NewCommand(hctx, cmdName)
 	}
 	socketPath := filepath.Join(u.baseDir, "agent.socket")
+	// Use abstract namespace so we don't get stale socket files.
+	socketPath = "@" + socketPath
 	srv, err := jujuc.NewServer(getCmd, socketPath)
 	if err != nil {
 		return err