Merge lp:~frankban/juju-core/bug-1166224-service-deploy-panic into lp:~juju/juju-core/trunk

[Francesco Banconi]

Fix ServiceDeploy panic when called via the API.

Fix the panic that happens when calling ServiceDeploy via the juju-core API.

Add a global charm.CacheDir variable, in order to avoid the call to
config.JujuHomePath inside the CharmStore.Get method.

charm.CacheDir is originally empty, and is then set by the machineAgent.Run
method.

Pre-implementation call with Roger.

https://codereview.appspot.com/8566043/

[Francesco Banconi (frankban)]

Reviewers: mp+157836_code.launchpad.net,

Message:
Please take a look.

Description:
Fix ServiceDeploy panic when called via the API.

Fix the panic that happens when calling ServiceDeploy via the juju-core
API.

Add a global charm.CacheDir variable, in order to avoid the call to
config.JujuHomePath inside the CharmStore.Get method.

charm.CacheDir is originally empty, and is then set by the
machineAgent.Run
method.

Pre-implementation call with Roger.

https://code.launchpad.net/~frankban/juju-core/bug-1166224-service-deploy-panic/+merge/157836

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M charm/repo.go
   M charm/repo_test.go
   M cmd/jujud/machine.go
   M cmd/jujud/machine_test.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: <email address hidden>
+New revision:
<email address hidden>

Index: charm/repo.go
=== modified file 'charm/repo.go'
--- charm/repo.go 2013-04-03 23:29:07 +0000
+++ charm/repo.go 2013-04-08 16:36:35 +0000
@@ -7,7 +7,6 @@
   "fmt"
   "io"
   "io/ioutil"
- "launchpad.net/juju-core/environs/config"
   "launchpad.net/juju-core/log"
   "net/http"
   "net/url"
@@ -16,6 +15,9 @@
   "strings"
  )

+// CacheDir stores the charm cache directory path.
+var CacheDir string
+
  // InfoResponse is sent by the charm store in response to charm-info
requests.
  type InfoResponse struct {
   Revision int `json:"revision"` // Zero is valid. Can't omitempty.
@@ -211,8 +213,11 @@

  // Get returns the charm referenced by curl.
  func (s *CharmStore) Get(curl *URL) (Charm, error) {
- cachePath := config.JujuHomePath("cache")
- if err := os.MkdirAll(cachePath, 0755); err != nil {
+ // MachineAgent.Run should have already set the CacheDir.
+ if CacheDir == "" {
+ panic("the charm cache directory path is empty")
+ }
+ if err := os.MkdirAll(CacheDir, 0755); err != nil {
    return nil, err
   }
   rev, digest, err := s.revision(curl)
@@ -224,14 +229,14 @@
   } else if curl.Revision != rev {
    return nil, fmt.Errorf("charm: store returned charm with wrong revision
for %q", curl.String())
   }
- path := filepath.Join(cachePath, Quote(curl.String())+".charm")
+ path := filepath.Join(CacheDir, Quote(curl.String())+".charm")
   if verify(path, digest) != nil {
    resp, err := http.Get(s.baseURL + "/charm/" +
url.QueryEscape(curl.Path()))
    if err != nil {
     return nil, err
    }
    defer resp.Body.Close()
- f, err := ioutil.TempFile(cachePath, "charm-download")
+ f, err := ioutil.TempFile(CacheDir, "charm-download")
    if err != nil {
     return nil, err
    }

Index: charm/repo_test.go
=== modified file 'charm/repo_test.go'
--- charm/repo_test.go 2013-04-03 23:29:07 +0000
+++ charm/repo_test.go 2013-04-09 09:51:19 +0000
@@ -8,7 +8,6 @@
   "io/ioutil"
   . "launchpad.net/gocheck"
   "launchpad.net/juju-core/charm"
- "launchpad.net/juju-core/environs/config"
   "launchpad.net/juju-core/log"
 ...

[Roger Peppe (rogpeppe)]

not lgtm for the moment - the CharmDir is not set when the juju command
is run.

please could you move the checkJujuHome function from cmd/juju/main.go
into the juju package, rename it to InitJujuHome, and add a line to set
charm.CharmDir appropriately.

https://codereview.appspot.com/8566043/diff/1/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode214
charm/repo.go:214: // Get returns the charm referenced by curl.
// CacheDir must have been set, otherwise Get will panic.

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode216
charm/repo.go:216: // MachineAgent.Run should have already set the
CacheDir.
// The cache location must have been previously set.
(it might have been set directly too, or via juju.SetJujuHome)

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode218
charm/repo.go:218: panic("the charm cache directory path is empty")
s/the //

just convention.

https://codereview.appspot.com/8566043/

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/8566043/diff/1/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode214
charm/repo.go:214: // Get returns the charm referenced by curl.
On 2013/04/09 11:24:13, rog wrote:
> // CacheDir must have been set, otherwise Get will panic.

Done.

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode216
charm/repo.go:216: // MachineAgent.Run should have already set the
CacheDir.
On 2013/04/09 11:24:13, rog wrote:
> // The cache location must have been previously set.
> (it might have been set directly too, or via juju.SetJujuHome)

Done.

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode218
charm/repo.go:218: panic("the charm cache directory path is empty")
On 2013/04/09 11:24:13, rog wrote:
> s/the //

> just convention.

Done.

https://codereview.appspot.com/8566043/

[Roger Peppe (rogpeppe)]

LGTM with the below points addressed.
thanks!

https://codereview.appspot.com/8566043/diff/7001/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/8566043/diff/7001/cmd/juju/main.go#newcode30
cmd/juju/main.go:30: fmt.Fprintf(os.Stderr, "command failed:
"+err.Error()+"\n")
fmt.Fprintf(os.Stderr, "error: %s\n", err)

to be consistent with cmd.Main

in general passing a non-constant argument to Fprintf-like functions is
not a good idea regardless.

https://codereview.appspot.com/8566043/diff/7001/cmd/juju/main.go#newcode31
cmd/juju/main.go:31: os.Exit(1)
i know it was like this before, but i think this should probably be
os.Exit(2).

https://codereview.appspot.com/8566043/diff/7001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8566043/diff/7001/juju/conn.go#newcode413
juju/conn.go:413: // It also sets up the charm cache directory path in
charm.CacheDir.
// InitJujuHome initializes the charm and environs/config
// packages to use default paths based on the
// $JUJU_HOME or $HOME environment variables.
// This function should be called before calling NewConn or
// Conn.Deploy.

?

https://codereview.appspot.com/8566043/

[Francesco Banconi (frankban)]

*** Submitted:

Fix ServiceDeploy panic when called via the API.

Fix the panic that happens when calling ServiceDeploy via the juju-core
API.

Add a global charm.CacheDir variable, in order to avoid the call to
config.JujuHomePath inside the CharmStore.Get method.

charm.CacheDir is originally empty, and is then set by the
machineAgent.Run
method.

Pre-implementation call with Roger.

R=rog, TheMue
CC=
https://codereview.appspot.com/8566043

https://codereview.appspot.com/8566043/diff/7001/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/8566043/diff/7001/cmd/juju/main.go#newcode30
cmd/juju/main.go:30: fmt.Fprintf(os.Stderr, "command failed:
"+err.Error()+"\n")
On 2013/04/09 16:22:59, rog wrote:
> fmt.Fprintf(os.Stderr, "error: %s\n", err)

> to be consistent with cmd.Main

> in general passing a non-constant argument to Fprintf-like functions
is not a
> good idea regardless.

Done.

https://codereview.appspot.com/8566043/diff/7001/cmd/juju/main.go#newcode31
cmd/juju/main.go:31: os.Exit(1)
On 2013/04/09 16:22:59, rog wrote:
> i know it was like this before, but i think this should probably be
os.Exit(2).

Done.

https://codereview.appspot.com/8566043/diff/7001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8566043/diff/7001/juju/conn.go#newcode413
juju/conn.go:413: // It also sets up the charm cache directory path in
charm.CacheDir.
On 2013/04/09 16:22:59, rog wrote:
> // InitJujuHome initializes the charm and environs/config
> // packages to use default paths based on the
> // $JUJU_HOME or $HOME environment variables.
> // This function should be called before calling NewConn or
> // Conn.Deploy.

> ?

Done.

https://codereview.appspot.com/8566043/

=== modified file 'charm/repo.go'
--- charm/repo.go	2013-04-09 14:37:05 +0000
+++ charm/repo.go	2013-04-09 16:40:27 +0000
@@ -7,7 +7,6 @@
 	"fmt"
 	"io"
 	"io/ioutil"
-	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/log"
 	"net/http"
 	"net/url"
@@ -16,6 +15,9 @@
 	"strings"
 )
 
+// CacheDir stores the charm cache directory path.
+var CacheDir string
+
 // InfoResponse is sent by the charm store in response to charm-info requests.
 type InfoResponse struct {
 	Revision int      `json:"revision"` // Zero is valid. Can't omitempty.
@@ -213,9 +215,13 @@
 }
 
 // Get returns the charm referenced by curl.
+// CacheDir must have been set, otherwise Get will panic.
 func (s *CharmStore) Get(curl *URL) (Charm, error) {
-	cachePath := config.JujuHomePath("cache")
-	if err := os.MkdirAll(cachePath, 0755); err != nil {
+	// The cache location must have been previously set.
+	if CacheDir == "" {
+		panic("charm cache directory path is empty")
+	}
+	if err := os.MkdirAll(CacheDir, 0755); err != nil {
 		return nil, err
 	}
 	rev, digest, err := s.revision(curl)
@@ -227,14 +233,14 @@
 	} else if curl.Revision != rev {
 		return nil, fmt.Errorf("charm: store returned charm with wrong revision for %q", curl.String())
 	}
-	path := filepath.Join(cachePath, Quote(curl.String())+".charm")
+	path := filepath.Join(CacheDir, Quote(curl.String())+".charm")
 	if verify(path, digest) != nil {
 		resp, err := http.Get(s.BaseURL + "/charm/" + url.QueryEscape(curl.Path()))
 		if err != nil {
 			return nil, err
 		}
 		defer resp.Body.Close()
-		f, err := ioutil.TempFile(cachePath, "charm-download")
+		f, err := ioutil.TempFile(CacheDir, "charm-download")
 		if err != nil {
 			return nil, err
 		}

=== modified file 'charm/repo_test.go'
--- charm/repo_test.go	2013-04-09 14:37:05 +0000
+++ charm/repo_test.go	2013-04-09 16:40:27 +0000
@@ -8,7 +8,6 @@
 	"io/ioutil"
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/charm"
-	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/testing"
 	"net"
@@ -150,23 +149,24 @@
 type StoreSuite struct {
 	server      *MockStore
 	store       *charm.CharmStore
-	oldJujuHome string
+	oldCacheDir string
 }
 
 var _ = Suite(&StoreSuite{})
 
 func (s *StoreSuite) SetUpSuite(c *C) {
 	s.server = NewMockStore(c)
+	s.oldCacheDir = charm.CacheDir
 }
 
 func (s *StoreSuite) SetUpTest(c *C) {
-	s.oldJujuHome = config.SetJujuHome(c.MkDir())
+	charm.CacheDir = c.MkDir()
 	s.store = charm.NewStore("http://127.0.0.1:4444")
 	s.server.downloads = nil
 }
 
 func (s *StoreSuite) TearDownSuite(c *C) {
-	config.SetJujuHome(s.oldJujuHome)
+	charm.CacheDir = s.oldCacheDir
 	s.server.lis.Close()
 }
 
@@ -245,12 +245,12 @@
 }
 
 func (s *StoreSuite) TestGetBadCache(c *C) {
-	c.Assert(os.Mkdir(config.JujuHomePath("cache"), 0777), IsNil)
+	c.Assert(os.Mkdir(filepath.Join(charm.CacheDir, "cache"), 0777), IsNil)
 	base := "cs:series/good"
 	curl := charm.MustParseURL(base)
 	revCurl := charm.MustParseURL(base + "-23")
 	name := charm.Quote(revCurl.String()) + ".charm"
-	err := ioutil.WriteFile(config.JujuHomePath("cache", name), nil, 0666)
+	err := ioutil.WriteFile(filepath.Join(charm.CacheDir, "cache", name), nil, 0666)
 	c.Assert(err, IsNil)
 	ch, err := s.store.Get(curl)
 	c.Assert(err, IsNil)

=== modified file 'cmd/juju/main.go'
--- cmd/juju/main.go	2013-04-05 18:51:03 +0000
+++ cmd/juju/main.go	2013-04-09 16:40:27 +0000
@@ -3,9 +3,8 @@
 import (
 	"fmt"
 	"launchpad.net/juju-core/cmd"
-	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/juju"
 	"os"
-	"path/filepath"
 )
 
 // When we import an environment provider implementation
@@ -23,27 +22,14 @@
 https://juju.ubuntu.com/
 `
 
-// checkJujuHome retrieves $JUJU_HOME or $HOME to set the juju home.
-// In case both variables aren't set the command will exit with an
-// error.
-func checkJujuHome() {
-	jujuHome := os.Getenv("JUJU_HOME")
-	if jujuHome == "" {
-		home := os.Getenv("HOME")
-		if home == "" {
-			fmt.Fprintf(os.Stderr, "command failed: cannot determine juju home, neither $JUJU_HOME nor $HOME are set")
-			os.Exit(1)
-		}
-		jujuHome = filepath.Join(home, ".juju")
-	}
-	config.SetJujuHome(jujuHome)
-}
-
 // Main registers subcommands for the juju executable, and hands over control
 // to the cmd package. This function is not redundant with main, because it
 // provides an entry point for testing with arbitrary command line arguments.
 func Main(args []string) {
-	checkJujuHome()
+	if err := juju.InitJujuHome(); err != nil {
+		fmt.Fprintf(os.Stderr, "error: %s\n", err)
+		os.Exit(2)
+	}
 	juju := cmd.NewSuperCommand(cmd.SuperCommandParams{
 		Name: "juju",
 		Doc:  jujuDoc,

=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go	2013-04-05 13:47:17 +0000
+++ cmd/jujud/machine.go	2013-04-09 16:40:27 +0000
@@ -3,6 +3,7 @@
 import (
 	"fmt"
 	"launchpad.net/gnuflag"
+	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/environs/agent"
 	"launchpad.net/juju-core/log"
@@ -13,6 +14,7 @@
 	"launchpad.net/juju-core/worker/machiner"
 	"launchpad.net/juju-core/worker/provisioner"
 	"launchpad.net/tomb"
+	"path/filepath"
 	"time"
 )
 
@@ -58,6 +60,7 @@
 	if err := a.Conf.read(state.MachineTag(a.MachineId)); err != nil {
 		return err
 	}
+	charm.CacheDir = filepath.Join(a.Conf.DataDir, "charmcache")
 	defer log.Noticef("cmd/jujud: machine agent exiting")
 	defer a.tomb.Done()
 

=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2013-04-09 09:32:06 +0000
+++ cmd/jujud/machine_test.go	2013-04-09 16:40:27 +0000
@@ -3,6 +3,7 @@
 import (
 	"fmt"
 	. "launchpad.net/gocheck"
+	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/environs/agent"
 	"launchpad.net/juju-core/environs/dummy"
@@ -10,16 +11,26 @@
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/watcher"
 	"launchpad.net/juju-core/testing"
+	"path/filepath"
 	"reflect"
 	"time"
 )
 
 type MachineSuite struct {
 	agentSuite
+	oldCacheDir string
 }
 
 var _ = Suite(&MachineSuite{})
 
+func (s *MachineSuite) SetUpSuite(c *C) {
+	s.oldCacheDir = charm.CacheDir
+}
+
+func (s *MachineSuite) TearDownSuite(c *C) {
+	charm.CacheDir = s.oldCacheDir
+}
+
 // primeAgent adds a new Machine to run the given jobs, and sets up the
 // machine agent's directory.  It returns the new machine, the
 // agent's configuration and the tools currently running.
@@ -72,7 +83,7 @@
 }
 
 func (s *MachineSuite) TestRunStop(c *C) {
-	m, _, _ := s.primeAgent(c, state.JobHostUnits)
+	m, ac, _ := s.primeAgent(c, state.JobHostUnits)
 	a := s.newAgent(c, m)
 	done := make(chan error)
 	go func() {
@@ -81,6 +92,7 @@
 	err := a.Stop()
 	c.Assert(err, IsNil)
 	c.Assert(<-done, IsNil)
+	c.Assert(charm.CacheDir, Equals, filepath.Join(ac.DataDir, "charmcache"))
 }
 
 func (s *MachineSuite) TestWithDeadMachine(c *C) {

=== modified file 'juju/conn.go'
--- juju/conn.go	2013-04-08 15:27:15 +0000
+++ juju/conn.go	2013-04-09 16:40:27 +0000
@@ -11,12 +11,14 @@
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/trivial"
 	"net/url"
 	"os"
+	"path/filepath"
 	"time"
 )
 
@@ -405,3 +407,20 @@
 	}
 	return validated
 }
+
+// InitJujuHome initializes the charm and environs/config packages to use
+// default paths based on the $JUJU_HOME or $HOME environment variables.
+// This function should be called before calling NewConn or Conn.Deploy.
+func InitJujuHome() error {
+	jujuHome := os.Getenv("JUJU_HOME")
+	if jujuHome == "" {
+		home := os.Getenv("HOME")
+		if home == "" {
+			return errors.New("cannot determine juju home, neither $JUJU_HOME nor $HOME are set")
+		}
+		jujuHome = filepath.Join(home, ".juju")
+	}
+	config.SetJujuHome(jujuHome)
+	charm.CacheDir = filepath.Join(jujuHome, "charmcache")
+	return nil
+}

=== modified file 'juju/conn_test.go'
--- juju/conn_test.go	2013-04-02 04:52:41 +0000
+++ juju/conn_test.go	2013-04-09 16:40:27 +0000
@@ -6,6 +6,7 @@
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/dummy"
 	"launchpad.net/juju-core/juju"
 	"launchpad.net/juju-core/juju/testing"
@@ -474,3 +475,50 @@
 		c.Assert(mcons, DeepEquals, cons)
 	}
 }
+
+type InitJujuHomeSuite struct {
+	originalHome     string
+	originalJujuHome string
+}
+
+var _ = Suite(&InitJujuHomeSuite{})
+
+func (s *InitJujuHomeSuite) SetUpTest(c *C) {
+	s.originalHome = os.Getenv("HOME")
+	s.originalJujuHome = os.Getenv("JUJU_HOME")
+}
+
+func (s *InitJujuHomeSuite) TearDownTest(c *C) {
+	os.Setenv("HOME", s.originalHome)
+	os.Setenv("JUJU_HOME", s.originalJujuHome)
+}
+
+func (s *InitJujuHomeSuite) TestJujuHome(c *C) {
+	os.Setenv("JUJU_HOME", "/my/juju/home")
+	err := juju.InitJujuHome()
+	c.Assert(err, IsNil)
+	c.Assert(config.JujuHome(), Equals, "/my/juju/home")
+}
+
+func (s *InitJujuHomeSuite) TestHome(c *C) {
+	os.Setenv("JUJU_HOME", "")
+	os.Setenv("HOME", "/my/home/")
+	err := juju.InitJujuHome()
+	c.Assert(err, IsNil)
+	c.Assert(config.JujuHome(), Equals, "/my/home/.juju")
+}
+
+func (s *InitJujuHomeSuite) TestError(c *C) {
+	os.Setenv("JUJU_HOME", "")
+	os.Setenv("HOME", "")
+	err := juju.InitJujuHome()
+	c.Assert(err, ErrorMatches, "cannot determine juju home.*")
+}
+
+func (s *InitJujuHomeSuite) TestCacheDir(c *C) {
+	os.Setenv("JUJU_HOME", "/foo/bar")
+	c.Assert(charm.CacheDir, Equals, "")
+	err := juju.InitJujuHome()
+	c.Assert(err, IsNil)
+	c.Assert(charm.CacheDir, Equals, "/foo/bar/charmcache")
+}