environs/ec2: use Go client on server.

environs/ec2: use Go client on server.



https://code.launchpad.net/~rogpeppe/juju/go-ec2-use-go-client/+merge/107246

(do not edit description out of merge proposal)

[rog, 2012-05-24 17:21:50]

Please take a look.

[niemeyer, 2012-05-25 02:29:56]

Great stuff!

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:34: // executables.
s/gives/is/
s/executables.//

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:94: addScripts(c,
This may be commented out for the moment, next to the TODO start machine agent
comment. These variables aren't going anywhere, which looks weird.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:106: " --env-type ec2",
Two env-types?

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit_test.go
File environs/ec2/cloudinit_test.go (left):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:209: var policyTests = []struct {
Nice to see all of that going.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit_test.go
File environs/ec2/cloudinit_test.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:61: t.checkScripts(c, "JUJU_MACHINE_ID=[0-9]+")
These tests are pure smoke and mirrors. :-( The configuration is completely
broken at the moment yet they pass. I guess they don't hurt, but I do wonder
about where the cost/benefit line lies.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/ec2.go#newcode220
environs/ec2/ec2.go:220: log.Printf("looking for tools for version %v; instance
spec %#v", version.Current, spec)
This should either be polished or turned into Debugf, as it doesn't feel like an
understandable or visually appealing message to have in a normal log.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/ec2.go#newcode223
environs/ec2/ec2.go:223: return nil, fmt.Errorf("cannot find tools for spec %#v:
%v", spec, err)
Same idea. This message will actually be seen by people somewhat often when
trying juju with other providers, and they'll be puzzled with what this means.
This can be something like:

("ec2: cannot find juju tools that would work on the specified instance: %v",
err)

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/image_test.go
File environs/ec2/image_test.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/image_test.go#n...
environs/ec2/image_test.go:17: var oldDefaultConstraint =
defaultInstanceConstraint
New variable isn't used in this file?

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/live_test.go
File environs/ec2/live_test.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:82: // This can happen if SetUpSuite fails.
s/!=/==/; Has it ever happened? :-)

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:113: s3 := amzs3.New(aws.Auth{}, ec2.Regions["test"])
It'd be nicer to preserve the package name as we're used to seeing it, and use
s3inst for this single instance variable.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:140: // Calculate the tools path once only so that we
don't
This looks like a lot of data shuffling. One has to stare these types and
functions and closures for a while to understand what is being done here. It
*looks* like we could easily do that the simple way by just listing and getting
the content from the storage.

I'd actually put the real tools as well. We don't gain much by not using that
data. There will only be a single copy in memory anyway.

Something along the lines of:

if first time:
    err := environs.PutTools(s)
    ... list /tools/juju-*
    ... get content and cache content and path
else:
    ... put content at path

[rog, 2012-05-25 07:28:47]

Please take a look.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:34: // executables.
On 2012/05/25 02:29:56, niemeyer wrote:
> s/gives/is/
> s/executables.//

Done.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:94: addScripts(c,
On 2012/05/25 02:29:56, niemeyer wrote:
> This may be commented out for the moment, next to the TODO start machine agent
> comment. These variables aren't going anywhere, which looks weird.

i agree it's weird, and i started out by removing it, but it's useful for the
tests to have it here, as it's the only  way i can tell in the tests that the
correct zookeeper address is being passed to non-bootstrap instances.

if i put in a comment, can we keep it here?

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:106: " --env-type ec2",
On 2012/05/25 02:29:56, niemeyer wrote:
> Two env-types?

Done.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit_test.go
File environs/ec2/cloudinit_test.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:61: t.checkScripts(c, "JUJU_MACHINE_ID=[0-9]+")
On 2012/05/25 02:29:56, niemeyer wrote:
> These tests are pure smoke and mirrors. :-( The configuration is completely
> broken at the moment yet they pass. I guess they don't hurt, but I do wonder
> about where the cost/benefit line lies. 

i agree, but i haven't been able to think of anything better. the aim is to test
the logic path within cloudinit.go while leaving wiggle room in the actual
scripts, which obviously we can't test locally.

have you got a better suggestion?

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/ec2.go#newcode220
environs/ec2/ec2.go:220: log.Printf("looking for tools for version %v; instance
spec %#v", version.Current, spec)
On 2012/05/25 02:29:56, niemeyer wrote:
> This should either be polished or turned into Debugf, as it doesn't feel like
an
> understandable or visually appealing message to have in a normal log.

Done.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/ec2.go#newcode223
environs/ec2/ec2.go:223: return nil, fmt.Errorf("cannot find tools for spec %#v:
%v", spec, err)
On 2012/05/25 02:29:56, niemeyer wrote:
> Same idea. This message will actually be seen by people somewhat often when
> trying juju with other providers, and they'll be puzzled with what this means.
> This can be something like:
> 
> ("ec2: cannot find juju tools that would work on the specified instance: %v",
> err)

Done.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/image_test.go
File environs/ec2/image_test.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/image_test.go#n...
environs/ec2/image_test.go:17: var oldDefaultConstraint =
defaultInstanceConstraint
On 2012/05/25 02:29:56, niemeyer wrote:
> New variable isn't used in this file?

Done.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/live_test.go
File environs/ec2/live_test.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:82: // This can happen if SetUpSuite fails.
On 2012/05/25 02:29:56, niemeyer wrote:
> s/!=/==/; Has it ever happened? :-)

not since i wrote that condition, obviously!
good catch.
it did happen before (and i took a while to find out why) hence the if.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:113: s3 := amzs3.New(aws.Auth{}, ec2.Regions["test"])
On 2012/05/25 02:29:56, niemeyer wrote:
> It'd be nicer to preserve the package name as we're used to seeing it, and use
> s3inst for this single instance variable.

Done.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:140: // Calculate the tools path once only so that we
don't
On 2012/05/25 02:29:56, niemeyer wrote:
> This looks like a lot of data shuffling. One has to stare these types and
> functions and closures for a while to understand what is being done here. It
> *looks* like we could easily do that the simple way by just listing and
getting
> the content from the storage.
> 
> I'd actually put the real tools as well. We don't gain much by not using that
> data. There will only be a single copy in memory anyway.

actually, this function is used in the live tests too, and i'd prefer to avoid
uploading *another* 17MB of data to the cloud, for no reason.

the other possibility is to export from environs a way of getting the tools path
for the current version. i'd be happy with that too.

[rog, 2012-05-25 17:12:03]

Please take a look.

[niemeyer, 2012-05-25 17:21:54]

LGTM, with the following sorted.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:94: addScripts(c,
On 2012/05/25 07:28:47, rog wrote:
> if i put in a comment, can we keep it here?

Sure, you can also keep them uncommented, but please at least pull up the "//
TODO start machine agent" comment here, since this is what these variables are
here for.

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit_test.go
File environs/ec2/cloudinit_test.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:61: t.checkScripts(c, "JUJU_MACHINE_ID=[0-9]+")
On 2012/05/25 07:28:47, rog wrote:
> have you got a better suggestion?

No, I'm fine with moving on like that for now. My suggestion if that turns out
to be too much of a burden will probably be to just drop it.

https://codereview.appspot.com/6245048/diff/3015/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/6245048/diff/3015/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:122: path :=
environs.ToolsPathForVersion(version.Current, environs.CurrentSeries,
environs.CurrentArch)
s/ForVersion//? There's no other ToolsPath, and it's just not for the Version
(version+arch+...).

https://codereview.appspot.com/6245048/diff/3015/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6245048/diff/3015/environs/tools.go#newcode35
environs/tools.go:35: // store the juju tools with the given version, OS and
architecture.
// ToolsPath returns the path that is used to store and
// retrieve the juju tools in a Storage.

https://codereview.appspot.com/6245048/diff/3015/environs/tools.go#newcode36
environs/tools.go:36: // It intended for testing purposes only.
Please drop the last line. If it's part of the API, it is indeed part of the
API.

[rog, 2012-05-28 08:24:36]

*** Submitted:

environs/ec2: use Go client on server.

R=niemeyer
CC=
https://codereview.appspot.com/6245048

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (right):

https://codereview.appspot.com/6245048/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:94: addScripts(c,
On 2012/05/25 17:21:54, niemeyer wrote:
> On 2012/05/25 07:28:47, rog wrote:
> > if i put in a comment, can we keep it here?
> 
> Sure, you can also keep them uncommented, but please at least pull up the "//
> TODO start machine agent" comment here, since this is what these variables are
> here for.

Done.

https://codereview.appspot.com/6245048/diff/3015/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/6245048/diff/3015/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:122: path :=
environs.ToolsPathForVersion(version.Current, environs.CurrentSeries,
environs.CurrentArch)
On 2012/05/25 17:21:54, niemeyer wrote:
> s/ForVersion//? There's no other ToolsPath, and it's just not for the Version
> (version+arch+...).

Done.

https://codereview.appspot.com/6245048/diff/3015/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6245048/diff/3015/environs/tools.go#newcode35
environs/tools.go:35: // store the juju tools with the given version, OS and
architecture.
On 2012/05/25 17:21:54, niemeyer wrote:
> // ToolsPath returns the path that is used to store and
> // retrieve the juju tools in a Storage.

Done.

https://codereview.appspot.com/6245048/diff/3015/environs/tools.go#newcode36
environs/tools.go:36: // It intended for testing purposes only.
On 2012/05/25 17:21:54, niemeyer wrote:
> Please drop the last line. If it's part of the API, it is indeed part of the
> API.

Done.