Merge lp:~frankban/juju-core/bug-1166224-service-deploy-panic into lp:~juju/juju-core/trunk
- bug-1166224-service-deploy-panic
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 1132 | ||||
Proposed branch: | lp:~frankban/juju-core/bug-1166224-service-deploy-panic | ||||
Merge into: | lp:~juju/juju-core/trunk | ||||
Diff against target: |
343 lines (+105/-31) 7 files modified
charm/repo.go (+11/-5) charm/repo_test.go (+6/-6) cmd/juju/main.go (+5/-19) cmd/jujud/machine.go (+3/-0) cmd/jujud/machine_test.go (+13/-1) juju/conn.go (+19/-0) juju/conn_test.go (+48/-0) |
||||
To merge this branch: | bzr merge lp:~frankban/juju-core/bug-1166224-service-deploy-panic | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email: mp+157836@code.launchpad.net |
Commit message
Description of the change
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.
Francesco Banconi (frankban) wrote : | # |
Roger Peppe (rogpeppe) wrote : | # |
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:/
File charm/repo.go (right):
https:/
charm/repo.go:214: // Get returns the charm referenced by curl.
// CacheDir must have been set, otherwise Get will panic.
https:/
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:/
charm/repo.go:218: panic("the charm cache directory path is empty")
s/the //
just convention.
Francesco Banconi (frankban) wrote : | # |
Please take a look.
https:/
File charm/repo.go (right):
https:/
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:/
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:/
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.
Roger Peppe (rogpeppe) wrote : | # |
LGTM with the below points addressed.
thanks!
https:/
File cmd/juju/main.go (right):
https:/
cmd/juju/
"+err.Error()+"\n")
fmt.Fprintf(
to be consistent with cmd.Main
in general passing a non-constant argument to Fprintf-like functions is
not a good idea regardless.
https:/
cmd/juju/
i know it was like this before, but i think this should probably be
os.Exit(2).
https:/
File juju/conn.go (right):
https:/
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.
?
Francesco Banconi (frankban) wrote : | # |
*** 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:/
https:/
File cmd/juju/main.go (right):
https:/
cmd/juju/
"+err.Error()+"\n")
On 2013/04/09 16:22:59, rog wrote:
> fmt.Fprintf(
> 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:/
cmd/juju/
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:/
File juju/conn.go (right):
https:/
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.
Preview Diff
1 | === modified file 'charm/repo.go' |
2 | --- charm/repo.go 2013-04-09 14:37:05 +0000 |
3 | +++ charm/repo.go 2013-04-09 16:40:27 +0000 |
4 | @@ -7,7 +7,6 @@ |
5 | "fmt" |
6 | "io" |
7 | "io/ioutil" |
8 | - "launchpad.net/juju-core/environs/config" |
9 | "launchpad.net/juju-core/log" |
10 | "net/http" |
11 | "net/url" |
12 | @@ -16,6 +15,9 @@ |
13 | "strings" |
14 | ) |
15 | |
16 | +// CacheDir stores the charm cache directory path. |
17 | +var CacheDir string |
18 | + |
19 | // InfoResponse is sent by the charm store in response to charm-info requests. |
20 | type InfoResponse struct { |
21 | Revision int `json:"revision"` // Zero is valid. Can't omitempty. |
22 | @@ -213,9 +215,13 @@ |
23 | } |
24 | |
25 | // Get returns the charm referenced by curl. |
26 | +// CacheDir must have been set, otherwise Get will panic. |
27 | func (s *CharmStore) Get(curl *URL) (Charm, error) { |
28 | - cachePath := config.JujuHomePath("cache") |
29 | - if err := os.MkdirAll(cachePath, 0755); err != nil { |
30 | + // The cache location must have been previously set. |
31 | + if CacheDir == "" { |
32 | + panic("charm cache directory path is empty") |
33 | + } |
34 | + if err := os.MkdirAll(CacheDir, 0755); err != nil { |
35 | return nil, err |
36 | } |
37 | rev, digest, err := s.revision(curl) |
38 | @@ -227,14 +233,14 @@ |
39 | } else if curl.Revision != rev { |
40 | return nil, fmt.Errorf("charm: store returned charm with wrong revision for %q", curl.String()) |
41 | } |
42 | - path := filepath.Join(cachePath, Quote(curl.String())+".charm") |
43 | + path := filepath.Join(CacheDir, Quote(curl.String())+".charm") |
44 | if verify(path, digest) != nil { |
45 | resp, err := http.Get(s.BaseURL + "/charm/" + url.QueryEscape(curl.Path())) |
46 | if err != nil { |
47 | return nil, err |
48 | } |
49 | defer resp.Body.Close() |
50 | - f, err := ioutil.TempFile(cachePath, "charm-download") |
51 | + f, err := ioutil.TempFile(CacheDir, "charm-download") |
52 | if err != nil { |
53 | return nil, err |
54 | } |
55 | |
56 | === modified file 'charm/repo_test.go' |
57 | --- charm/repo_test.go 2013-04-09 14:37:05 +0000 |
58 | +++ charm/repo_test.go 2013-04-09 16:40:27 +0000 |
59 | @@ -8,7 +8,6 @@ |
60 | "io/ioutil" |
61 | . "launchpad.net/gocheck" |
62 | "launchpad.net/juju-core/charm" |
63 | - "launchpad.net/juju-core/environs/config" |
64 | "launchpad.net/juju-core/log" |
65 | "launchpad.net/juju-core/testing" |
66 | "net" |
67 | @@ -150,23 +149,24 @@ |
68 | type StoreSuite struct { |
69 | server *MockStore |
70 | store *charm.CharmStore |
71 | - oldJujuHome string |
72 | + oldCacheDir string |
73 | } |
74 | |
75 | var _ = Suite(&StoreSuite{}) |
76 | |
77 | func (s *StoreSuite) SetUpSuite(c *C) { |
78 | s.server = NewMockStore(c) |
79 | + s.oldCacheDir = charm.CacheDir |
80 | } |
81 | |
82 | func (s *StoreSuite) SetUpTest(c *C) { |
83 | - s.oldJujuHome = config.SetJujuHome(c.MkDir()) |
84 | + charm.CacheDir = c.MkDir() |
85 | s.store = charm.NewStore("http://127.0.0.1:4444") |
86 | s.server.downloads = nil |
87 | } |
88 | |
89 | func (s *StoreSuite) TearDownSuite(c *C) { |
90 | - config.SetJujuHome(s.oldJujuHome) |
91 | + charm.CacheDir = s.oldCacheDir |
92 | s.server.lis.Close() |
93 | } |
94 | |
95 | @@ -245,12 +245,12 @@ |
96 | } |
97 | |
98 | func (s *StoreSuite) TestGetBadCache(c *C) { |
99 | - c.Assert(os.Mkdir(config.JujuHomePath("cache"), 0777), IsNil) |
100 | + c.Assert(os.Mkdir(filepath.Join(charm.CacheDir, "cache"), 0777), IsNil) |
101 | base := "cs:series/good" |
102 | curl := charm.MustParseURL(base) |
103 | revCurl := charm.MustParseURL(base + "-23") |
104 | name := charm.Quote(revCurl.String()) + ".charm" |
105 | - err := ioutil.WriteFile(config.JujuHomePath("cache", name), nil, 0666) |
106 | + err := ioutil.WriteFile(filepath.Join(charm.CacheDir, "cache", name), nil, 0666) |
107 | c.Assert(err, IsNil) |
108 | ch, err := s.store.Get(curl) |
109 | c.Assert(err, IsNil) |
110 | |
111 | === modified file 'cmd/juju/main.go' |
112 | --- cmd/juju/main.go 2013-04-05 18:51:03 +0000 |
113 | +++ cmd/juju/main.go 2013-04-09 16:40:27 +0000 |
114 | @@ -3,9 +3,8 @@ |
115 | import ( |
116 | "fmt" |
117 | "launchpad.net/juju-core/cmd" |
118 | - "launchpad.net/juju-core/environs/config" |
119 | + "launchpad.net/juju-core/juju" |
120 | "os" |
121 | - "path/filepath" |
122 | ) |
123 | |
124 | // When we import an environment provider implementation |
125 | @@ -23,27 +22,14 @@ |
126 | https://juju.ubuntu.com/ |
127 | ` |
128 | |
129 | -// checkJujuHome retrieves $JUJU_HOME or $HOME to set the juju home. |
130 | -// In case both variables aren't set the command will exit with an |
131 | -// error. |
132 | -func checkJujuHome() { |
133 | - jujuHome := os.Getenv("JUJU_HOME") |
134 | - if jujuHome == "" { |
135 | - home := os.Getenv("HOME") |
136 | - if home == "" { |
137 | - fmt.Fprintf(os.Stderr, "command failed: cannot determine juju home, neither $JUJU_HOME nor $HOME are set") |
138 | - os.Exit(1) |
139 | - } |
140 | - jujuHome = filepath.Join(home, ".juju") |
141 | - } |
142 | - config.SetJujuHome(jujuHome) |
143 | -} |
144 | - |
145 | // Main registers subcommands for the juju executable, and hands over control |
146 | // to the cmd package. This function is not redundant with main, because it |
147 | // provides an entry point for testing with arbitrary command line arguments. |
148 | func Main(args []string) { |
149 | - checkJujuHome() |
150 | + if err := juju.InitJujuHome(); err != nil { |
151 | + fmt.Fprintf(os.Stderr, "error: %s\n", err) |
152 | + os.Exit(2) |
153 | + } |
154 | juju := cmd.NewSuperCommand(cmd.SuperCommandParams{ |
155 | Name: "juju", |
156 | Doc: jujuDoc, |
157 | |
158 | === modified file 'cmd/jujud/machine.go' |
159 | --- cmd/jujud/machine.go 2013-04-05 13:47:17 +0000 |
160 | +++ cmd/jujud/machine.go 2013-04-09 16:40:27 +0000 |
161 | @@ -3,6 +3,7 @@ |
162 | import ( |
163 | "fmt" |
164 | "launchpad.net/gnuflag" |
165 | + "launchpad.net/juju-core/charm" |
166 | "launchpad.net/juju-core/cmd" |
167 | "launchpad.net/juju-core/environs/agent" |
168 | "launchpad.net/juju-core/log" |
169 | @@ -13,6 +14,7 @@ |
170 | "launchpad.net/juju-core/worker/machiner" |
171 | "launchpad.net/juju-core/worker/provisioner" |
172 | "launchpad.net/tomb" |
173 | + "path/filepath" |
174 | "time" |
175 | ) |
176 | |
177 | @@ -58,6 +60,7 @@ |
178 | if err := a.Conf.read(state.MachineTag(a.MachineId)); err != nil { |
179 | return err |
180 | } |
181 | + charm.CacheDir = filepath.Join(a.Conf.DataDir, "charmcache") |
182 | defer log.Noticef("cmd/jujud: machine agent exiting") |
183 | defer a.tomb.Done() |
184 | |
185 | |
186 | === modified file 'cmd/jujud/machine_test.go' |
187 | --- cmd/jujud/machine_test.go 2013-04-09 09:32:06 +0000 |
188 | +++ cmd/jujud/machine_test.go 2013-04-09 16:40:27 +0000 |
189 | @@ -3,6 +3,7 @@ |
190 | import ( |
191 | "fmt" |
192 | . "launchpad.net/gocheck" |
193 | + "launchpad.net/juju-core/charm" |
194 | "launchpad.net/juju-core/cmd" |
195 | "launchpad.net/juju-core/environs/agent" |
196 | "launchpad.net/juju-core/environs/dummy" |
197 | @@ -10,16 +11,26 @@ |
198 | "launchpad.net/juju-core/state/api" |
199 | "launchpad.net/juju-core/state/watcher" |
200 | "launchpad.net/juju-core/testing" |
201 | + "path/filepath" |
202 | "reflect" |
203 | "time" |
204 | ) |
205 | |
206 | type MachineSuite struct { |
207 | agentSuite |
208 | + oldCacheDir string |
209 | } |
210 | |
211 | var _ = Suite(&MachineSuite{}) |
212 | |
213 | +func (s *MachineSuite) SetUpSuite(c *C) { |
214 | + s.oldCacheDir = charm.CacheDir |
215 | +} |
216 | + |
217 | +func (s *MachineSuite) TearDownSuite(c *C) { |
218 | + charm.CacheDir = s.oldCacheDir |
219 | +} |
220 | + |
221 | // primeAgent adds a new Machine to run the given jobs, and sets up the |
222 | // machine agent's directory. It returns the new machine, the |
223 | // agent's configuration and the tools currently running. |
224 | @@ -72,7 +83,7 @@ |
225 | } |
226 | |
227 | func (s *MachineSuite) TestRunStop(c *C) { |
228 | - m, _, _ := s.primeAgent(c, state.JobHostUnits) |
229 | + m, ac, _ := s.primeAgent(c, state.JobHostUnits) |
230 | a := s.newAgent(c, m) |
231 | done := make(chan error) |
232 | go func() { |
233 | @@ -81,6 +92,7 @@ |
234 | err := a.Stop() |
235 | c.Assert(err, IsNil) |
236 | c.Assert(<-done, IsNil) |
237 | + c.Assert(charm.CacheDir, Equals, filepath.Join(ac.DataDir, "charmcache")) |
238 | } |
239 | |
240 | func (s *MachineSuite) TestWithDeadMachine(c *C) { |
241 | |
242 | === modified file 'juju/conn.go' |
243 | --- juju/conn.go 2013-04-08 15:27:15 +0000 |
244 | +++ juju/conn.go 2013-04-09 16:40:27 +0000 |
245 | @@ -11,12 +11,14 @@ |
246 | "launchpad.net/juju-core/charm" |
247 | "launchpad.net/juju-core/constraints" |
248 | "launchpad.net/juju-core/environs" |
249 | + "launchpad.net/juju-core/environs/config" |
250 | "launchpad.net/juju-core/log" |
251 | "launchpad.net/juju-core/state" |
252 | "launchpad.net/juju-core/state/api/params" |
253 | "launchpad.net/juju-core/trivial" |
254 | "net/url" |
255 | "os" |
256 | + "path/filepath" |
257 | "time" |
258 | ) |
259 | |
260 | @@ -405,3 +407,20 @@ |
261 | } |
262 | return validated |
263 | } |
264 | + |
265 | +// InitJujuHome initializes the charm and environs/config packages to use |
266 | +// default paths based on the $JUJU_HOME or $HOME environment variables. |
267 | +// This function should be called before calling NewConn or Conn.Deploy. |
268 | +func InitJujuHome() error { |
269 | + jujuHome := os.Getenv("JUJU_HOME") |
270 | + if jujuHome == "" { |
271 | + home := os.Getenv("HOME") |
272 | + if home == "" { |
273 | + return errors.New("cannot determine juju home, neither $JUJU_HOME nor $HOME are set") |
274 | + } |
275 | + jujuHome = filepath.Join(home, ".juju") |
276 | + } |
277 | + config.SetJujuHome(jujuHome) |
278 | + charm.CacheDir = filepath.Join(jujuHome, "charmcache") |
279 | + return nil |
280 | +} |
281 | |
282 | === modified file 'juju/conn_test.go' |
283 | --- juju/conn_test.go 2013-04-02 04:52:41 +0000 |
284 | +++ juju/conn_test.go 2013-04-09 16:40:27 +0000 |
285 | @@ -6,6 +6,7 @@ |
286 | "launchpad.net/juju-core/charm" |
287 | "launchpad.net/juju-core/constraints" |
288 | "launchpad.net/juju-core/environs" |
289 | + "launchpad.net/juju-core/environs/config" |
290 | "launchpad.net/juju-core/environs/dummy" |
291 | "launchpad.net/juju-core/juju" |
292 | "launchpad.net/juju-core/juju/testing" |
293 | @@ -474,3 +475,50 @@ |
294 | c.Assert(mcons, DeepEquals, cons) |
295 | } |
296 | } |
297 | + |
298 | +type InitJujuHomeSuite struct { |
299 | + originalHome string |
300 | + originalJujuHome string |
301 | +} |
302 | + |
303 | +var _ = Suite(&InitJujuHomeSuite{}) |
304 | + |
305 | +func (s *InitJujuHomeSuite) SetUpTest(c *C) { |
306 | + s.originalHome = os.Getenv("HOME") |
307 | + s.originalJujuHome = os.Getenv("JUJU_HOME") |
308 | +} |
309 | + |
310 | +func (s *InitJujuHomeSuite) TearDownTest(c *C) { |
311 | + os.Setenv("HOME", s.originalHome) |
312 | + os.Setenv("JUJU_HOME", s.originalJujuHome) |
313 | +} |
314 | + |
315 | +func (s *InitJujuHomeSuite) TestJujuHome(c *C) { |
316 | + os.Setenv("JUJU_HOME", "/my/juju/home") |
317 | + err := juju.InitJujuHome() |
318 | + c.Assert(err, IsNil) |
319 | + c.Assert(config.JujuHome(), Equals, "/my/juju/home") |
320 | +} |
321 | + |
322 | +func (s *InitJujuHomeSuite) TestHome(c *C) { |
323 | + os.Setenv("JUJU_HOME", "") |
324 | + os.Setenv("HOME", "/my/home/") |
325 | + err := juju.InitJujuHome() |
326 | + c.Assert(err, IsNil) |
327 | + c.Assert(config.JujuHome(), Equals, "/my/home/.juju") |
328 | +} |
329 | + |
330 | +func (s *InitJujuHomeSuite) TestError(c *C) { |
331 | + os.Setenv("JUJU_HOME", "") |
332 | + os.Setenv("HOME", "") |
333 | + err := juju.InitJujuHome() |
334 | + c.Assert(err, ErrorMatches, "cannot determine juju home.*") |
335 | +} |
336 | + |
337 | +func (s *InitJujuHomeSuite) TestCacheDir(c *C) { |
338 | + os.Setenv("JUJU_HOME", "/foo/bar") |
339 | + c.Assert(charm.CacheDir, Equals, "") |
340 | + err := juju.InitJujuHome() |
341 | + c.Assert(err, IsNil) |
342 | + c.Assert(charm.CacheDir, Equals, "/foo/bar/charmcache") |
343 | +} |
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: machine. go machine_ test.go
A [revision details]
M charm/repo.go
M charm/repo_test.go
M cmd/jujud/
M cmd/jujud/
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 net/juju- core/environs/ config" net/juju- core/log"
=== 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.
"launchpad.
"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. JujuHomePath( "cache" ) cachePath, 0755); err != nil { CacheDir, 0755); err != nil { Join(cachePath, Quote(curl. String( ))+".charm" ) Join(CacheDir, Quote(curl. String( ))+".charm" ) (curl.Path( ))) TempFile( cachePath, "charm-download") TempFile( CacheDir, "charm-download")
func (s *CharmStore) Get(curl *URL) (Charm, error) {
- cachePath := config.
- if err := os.MkdirAll(
+ // MachineAgent.Run should have already set the CacheDir.
+ if CacheDir == "" {
+ panic("the charm cache directory path is empty")
+ }
+ if err := os.MkdirAll(
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.
+ path := filepath.
if verify(path, digest) != nil {
resp, err := http.Get(s.baseURL + "/charm/" +
url.QueryEscape
if err != nil {
return nil, err
}
defer resp.Body.Close()
- f, err := ioutil.
+ f, err := ioutil.
if err != nil {
return nil, err
}
Index: charm/repo_test.go repo_test. go' net/gocheck" net/juju- core/charm" net/juju- core/environs/ config" net/juju- core/log"
=== modified file 'charm/
--- 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.
"launchpad.
- "launchpad.
"launchpad.
...