environs/ec2: bootstrap

environs/ec2: bootstrap

This propsal is an incremental improvement towards ec2 bootstrapping.
I've taken the great work Roger did and incorporated Williams testing
changes. juju bootstrap is now able to create machine/0 with a provisioner 
running. The provisioner currently does not do anything as it has no secrets.

Once the problem of pushing the real secrets to ec2 is solved, changing the
HasProvisioner flag in live_test.go should allow this test to pass in the 
real -amazon environment.

https://code.launchpad.net/~dave-cheney/juju-core/go-environs-ec2-cloudinit-fixes/+merge/112274

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-07-05 01:27:17]

Please take a look.

[niemeyer, 2012-07-05 21:30:40]

We've talked about this on the channel yesterday.

[dave_cheney.net, 2012-07-06 06:28:45]

Hello,

I would like to repropose this as a WIP that allows us to have a functional juju
bootstrap. I've backed out all the local testing hacks in favor of TODO to be
resolved when juju/conn.Deploy lands.

[dave_cheney.net, 2012-07-06 06:29:26]

Please take a look.

[niemeyer, 2012-07-06 19:44:56]

Almost there.

https://codereview.appspot.com/6347044/diff/5003/environs/dummy/environs_test.go
File environs/dummy/environs_test.go (right):

https://codereview.appspot.com/6347044/diff/5003/environs/dummy/environs_test...
environs/dummy/environs_test.go:28: HasProvisioner: false,
Isn't CanOpenState enough?

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

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:119: svc := upstart.NewService("juju-provision-agent")
I suggest adopting a "juju-<jujud subcmd>" convention for our upstart scripts.

In this case, "juju-provisioning".

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:121: // we don't have to second-guess upstart's
quoting rules.
I don't understand the TODO goal. The string is pasted into the upstart script
as-is. We're not guessing quoting rules, I think?

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:129: // TODO Out?
Out seems bogus. We should really be relying on upstart's own handling of
logging rather than doing simplistic file-append as we are now. Please drop the
TODO and let's fix that within juju-core/upstart itself later.

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

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:64: t.checkScripts(c, "jujud provisioning")
Could this be slightly richer, checking that the proper zookeeper argument was
provided, for example?

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

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:57: HasProvisioner: false,
Please revert to original state.

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/suite_test.go
File environs/ec2/suite_test.go (right):

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/suite_test.go#n...
environs/ec2/suite_test.go:14: func TestEC2(t *stdtesting.T) {
logDebug := log.Debug
defer func() { log.Debug = logDebug }()

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/suite_test.go#n...
environs/ec2/suite_test.go:23: coretesting.ZkTestPackage(t)
It seems like multiple changes to the test harness could be reverted back to the
original state. Would you mind to do a self-review on the CL and see which of
these changes are really necessary for the tests being put in place in this CL?

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:94: func (t *LiveTests) testProvisioning(c *C, st
*state.State) {
This should be in a test by itself. Note that TestBootstrap uses BootstrapOnce.
It won't deploy a new environment if you call it again on another test.

That said, the test should be careful to clean up after itself. For example,
will the logic below call RemoveMachine if checkStartInstance fails?

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:109: var agentReaction =
environs.AttemptStrategy{
s/agentReaction/waitAgent/; agentReaction.Start reads poorly.

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:123: if instId != "" {
Argh. We have to fix InstanceId to never return "" with an error. Filed a bug:

https://bugs.launchpad.net/juju-core/+bug/1021893

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:136: // Wait for machine to get instance id.
Comment is wrong.

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:149: func checkMachineId(c *C, m *state.Machine,
inst environs.Instance) {
s/MachineId/InstanceId/; in the documentation too.

[niemeyer, 2012-07-06 19:45:54]

https://codereview.appspot.com/6347044/diff/5003/environs/dummy/environs_test.go
File environs/dummy/environs_test.go (right):

https://codereview.appspot.com/6347044/diff/5003/environs/dummy/environs_test...
environs/dummy/environs_test.go:28: HasProvisioner: false,
On 2012/07/06 19:44:56, niemeyer wrote:
> Isn't CanOpenState enough?

Sorry, please ignore that comment. HasProvisioner looks good after getting to
the end of the CL.

[dave_cheney.net, 2012-07-09 00:52:57]

Please take a look.

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

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:119: svc := upstart.NewService("juju-provision-agent")
On 2012/07/06 19:44:56, niemeyer wrote:
> I suggest adopting a "juju-<jujud subcmd>" convention for our upstart scripts.
> 
> In this case, "juju-provisioning".

Done.

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:121: // we don't have to second-guess upstart's
quoting rules.
On 2012/07/06 19:44:56, niemeyer wrote:
> I don't understand the TODO goal. The string is pasted into the upstart script
> as-is. We're not guessing quoting rules, I think?

This is Roger's todo, i'll leave it for him.

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:129: // TODO Out?
On 2012/07/06 19:44:56, niemeyer wrote:
> Out seems bogus. We should really be relying on upstart's own handling of
> logging rather than doing simplistic file-append as we are now. Please drop
the
> TODO and let's fix that within juju-core/upstart itself later.

Done.

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

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:64: t.checkScripts(c, "jujud provisioning")
On 2012/07/06 19:44:56, niemeyer wrote:
> Could this be slightly richer, checking that the proper zookeeper argument was
> provided, for example?

Done.

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

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:57: HasProvisioner: false,
On 2012/07/06 19:44:56, niemeyer wrote:
> Please revert to original state.

Done.

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/suite_test.go
File environs/ec2/suite_test.go (right):

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/suite_test.go#n...
environs/ec2/suite_test.go:14: func TestEC2(t *stdtesting.T) {
On 2012/07/06 19:44:56, niemeyer wrote:
> logDebug := log.Debug
> defer func() { log.Debug = logDebug }()

Done.

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/suite_test.go#n...
environs/ec2/suite_test.go:23: coretesting.ZkTestPackage(t)
On 2012/07/06 19:44:56, niemeyer wrote:
> It seems like multiple changes to the test harness could be reverted back to
the
> original state. Would you mind to do a self-review on the CL and see which of
> these changes are really necessary for the tests being put in place in this
CL?

Using ZkTestPackage isn't necessary if we do not try to mock a provisioning
agent. I have reverted this change.

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:94: func (t *LiveTests) testProvisioning(c *C, st
*state.State) {
On 2012/07/06 19:44:56, niemeyer wrote:
> This should be in a test by itself. Note that TestBootstrap uses
BootstrapOnce.
> It won't deploy a new environment if you call it again on another test.

Done.

> That said, the test should be careful to clean up after itself. For example,
> will the logic below call RemoveMachine if checkStartInstance fails?

No, the code uses assert so the test will abort. i have renamed the method to
make that clearer.

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:109: var agentReaction =
environs.AttemptStrategy{
On 2012/07/06 19:44:56, niemeyer wrote:
> s/agentReaction/waitAgent/; agentReaction.Start reads poorly.

Done.

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:123: if instId != "" {
This isn't actually testing that instId is "", it's using the presence of a
instId name as a signal to exit the loop. I have rewritten it to be clearer.

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:136: // Wait for machine to get instance id.
On 2012/07/06 19:44:56, niemeyer wrote:
> Comment is wrong.

Done.

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:149: func checkMachineId(c *C, m *state.Machine,
inst environs.Instance) {
On 2012/07/06 19:44:56, niemeyer wrote:
> s/MachineId/InstanceId/; in the documentation too.

Done.

[niemeyer, 2012-07-09 22:59:37]

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

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:121: // we don't have to second-guess upstart's
quoting rules.

<niemeyer> davecheney: The %q there is bogus.. that's probably what it is
referring to
<niemeyer> davecheney: I suggest using something simpler, such as "'%s'"
<niemeyer> davecheney: No address should contain single quotes in them, so this
should be fine

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:94: func (t *LiveTests) testProvisioning(c *C, st
*state.State) {
On 2012/07/09 00:52:58, dfc wrote:
> On 2012/07/06 19:44:56, niemeyer wrote:
> > This should be in a test by itself. Note that TestBootstrap uses
> BootstrapOnce.
> > It won't deploy a new environment if you call it again on another test.
> 
> Done.

Apparently not?

> > That said, the test should be careful to clean up after itself. For example,
> > will the logic below call RemoveMachine if checkStartInstance fails?
> 
> No, the code uses assert so the test will abort. i have renamed the method to
> make that clearer.

The point being made is that it should *not* leave the environment in a state
different than the one it found when it started, otherwise it will corrupt other
tests using BootstrapOnce.

https://codereview.appspot.com/6347044/diff/11/environs/ec2/suite_test.go
File environs/ec2/suite_test.go (right):

https://codereview.appspot.com/6347044/diff/11/environs/ec2/suite_test.go#new...
environs/ec2/suite_test.go:16: defer func() { log.Debug = false }()
This shouldn't be altering the global state unknowingly. The suggested approach
preserves it rather than forcing it.

[dave_cheney.net, 2012-07-10 00:13:19]

Please take a look.

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

https://codereview.appspot.com/6347044/diff/5003/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:121: // we don't have to second-guess upstart's
quoting rules.
On 2012/07/09 22:59:37, niemeyer wrote:
> 
> <niemeyer> davecheney: The %q there is bogus.. that's probably what it is
> referring to
> <niemeyer> davecheney: I suggest using something simpler, such as "'%s'"
> <niemeyer> davecheney: No address should contain single quotes in them, so
this
> should be fine

Done.

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:94: func (t *LiveTests) testProvisioning(c *C, st
*state.State) {
I think you reviewed an old version of this file, patchset #13, the latest did
not have this method.

https://codereview.appspot.com/6347044/diff/11/environs/ec2/suite_test.go
File environs/ec2/suite_test.go (right):

https://codereview.appspot.com/6347044/diff/11/environs/ec2/suite_test.go#new...
environs/ec2/suite_test.go:16: defer func() { log.Debug = false }()
I've just deleted it, it didn't add enough value.

[dave_cheney.net, 2012-07-10 03:19:57]

Thank you for your comments. I have split the test into two individual tests
now.

[dave_cheney.net, 2012-07-10 03:20:37]

Please take a look.

[niemeyer, 2012-07-10 03:43:41]

Thanks Dave. That's almost it. There's just some stuff that doesn't have to be
done again.

https://codereview.appspot.com/6347044/diff/21003/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/21003/environs/jujutest/livetests...
environs/jujutest/livetests.go:98: c.Assert(err, ErrorMatches, "environment is
already bootstrapped")
This has been tested above, so we don't have to test it again.

https://codereview.appspot.com/6347044/diff/21003/environs/jujutest/livetests...
environs/jujutest/livetests.go:103: c.Assert(info.Addrs, Not(HasLen), 0)
We also don't need the assertions besides (err, IsNil). This has been tested
above too.

https://codereview.appspot.com/6347044/diff/21003/environs/jujutest/livetests...
environs/jujutest/livetests.go:107: defer t.Destroy(c)
The goal of BootstrapOnce is to make an environment available to several tests
without having to reset it all the time. This environment is destroyed at the
end of the suite instead, so this shouldn't be done here. TestBootstrap above is
not ideal either, btw, but note how it ends with BootstrapOnce at the end.

[dave_cheney.net, 2012-07-10 07:03:58]

Please take a look.

https://codereview.appspot.com/6347044/diff/21003/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/21003/environs/jujutest/livetests...
environs/jujutest/livetests.go:98: c.Assert(err, ErrorMatches, "environment is
already bootstrapped")
On 2012/07/10 03:43:41, niemeyer wrote:
> This has been tested above, so we don't have to test it again.

Done.

https://codereview.appspot.com/6347044/diff/21003/environs/jujutest/livetests...
environs/jujutest/livetests.go:103: c.Assert(info.Addrs, Not(HasLen), 0)
On 2012/07/10 03:43:41, niemeyer wrote:
> We also don't need the assertions besides (err, IsNil). This has been tested
> above too.

Done.

https://codereview.appspot.com/6347044/diff/21003/environs/jujutest/livetests...
environs/jujutest/livetests.go:107: defer t.Destroy(c)
On 2012/07/10 03:43:41, niemeyer wrote:
> The goal of BootstrapOnce is to make an environment available to several tests
> without having to reset it all the time. This environment is destroyed at the
> end of the suite instead, so this shouldn't be done here. TestBootstrap above
is
> not ideal either, btw, but note how it ends with BootstrapOnce at the end.

I've benchmarked this on my machine and there is no difference in wall clock
speed doing the defer t.destroy(c) or not, both runs take 49 seconds. I would
like to leave this as is rather than trying to recover the c.Assert panic, or
doing all the checks manually.

[rog, 2012-07-10 08:07:56]

looking good. it's a pity we can't actually exercise the tests.

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:94: func (t *LiveTests) testProvisioning(c *C, st
*state.State) {
On 2012/07/06 19:44:56, niemeyer wrote:
> This should be in a test by itself. Note that TestBootstrap uses
BootstrapOnce.
> It won't deploy a new environment if you call it again on another test.

The reason I put this inside TestBootstrap was that TestBootstrap bootstraps
*and* destroys, so that unless TestBootstrapProvisioner runs before
TestBootstrap (which it won't because it's alphabetically after), we will wait
for a full bootstrap cycle twice, which takes ages and ages.

As a way to get around this, we could rename TestBootstrapProvisioner to
TestProvisioner and add a comment.
Alternatively we could change TestBootstrap so that it doesn't destroy and
re-bootstrap.

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

https://codereview.appspot.com/6347044/diff/7023/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:64: t.checkScripts(c, `jujud provisioning
--zookeeper-servers 'localhost:2181'`)
s/:2181'/`+zkPortSuffix+"'"/

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

https://codereview.appspot.com/6347044/diff/7023/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:58: HasProvisioner:   false, // TODO(dfc) waiting on
juju/conn.Depoy
s/Depoy/Deploy/

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

https://codereview.appspot.com/6347044/diff/7023/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:16: "launchpad.net/juju-core/state/testing"
minor thought: i'm not sure why juju-core/testing should be the one to be
unaliased here. perhaps both of them could be aliased: "statetesting" and
"coretesting"

https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
environs/jujutest/livetests.go:141: c.Assert(err, IsNil)
you could move this before the if statement and then lose the if statement.

https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
environs/jujutest/livetests.go:172: } else {
s/else {//

https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
environs/jujutest/livetests.go:174: }
d

https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
environs/jujutest/livetests.go:179: id, err := m.InstanceId()
i'm not sure why this is calling InstanceId again when we've been calling it in
the loop to get it. also, this logic won't work when inst is nil.

[dave_cheney.net, 2012-07-10 08:24:24]

Thanks for your review Rog. I'd say this one is polished to a fine sheen.

[dave_cheney.net, 2012-07-10 08:24:46]

Please take a look.

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

https://codereview.appspot.com/6347044/diff/7023/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:64: t.checkScripts(c, `jujud provisioning
--zookeeper-servers 'localhost:2181'`)
On 2012/07/10 08:07:56, rog wrote:
> s/:2181'/`+zkPortSuffix+"'"/

Done.

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

https://codereview.appspot.com/6347044/diff/7023/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:58: HasProvisioner:   false, // TODO(dfc) waiting on
juju/conn.Depoy
On 2012/07/10 08:07:56, rog wrote:
> s/Depoy/Deploy/

Done.

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

https://codereview.appspot.com/6347044/diff/7023/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:16: "launchpad.net/juju-core/state/testing"
On 2012/07/10 08:07:56, rog wrote:
> minor thought: i'm not sure why juju-core/testing should be the one to be
> unaliased here. perhaps both of them could be aliased: "statetesting" and
> "coretesting"

good point, not for this proposal tho.

https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
environs/jujutest/livetests.go:141: c.Assert(err, IsNil)
On 2012/07/10 08:07:56, rog wrote:
> you could move this before the if statement and then lose the if statement.

Done.

https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
environs/jujutest/livetests.go:172: } else {
On 2012/07/10 08:07:56, rog wrote:
> s/else {//

Done.

https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
environs/jujutest/livetests.go:174: }
On 2012/07/10 08:07:56, rog wrote:
> d

Done.

https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
environs/jujutest/livetests.go:179: id, err := m.InstanceId()
On 2012/07/10 08:07:56, rog wrote:
> i'm not sure why this is calling InstanceId again when we've been calling it
in
> the loop to get it. also, this logic won't work when inst is nil.

Done.

[rog, 2012-07-10 08:40:22]

LGTM

On 10 July 2012 09:24,  <dave@cheney.net> wrote:
> Please take a look.
>
>
>
>
https://codereview.appspot.com/6347044/diff/7023/environs/ec2/cloudinit_test.go
> File environs/ec2/cloudinit_test.go (right):
>
>
https://codereview.appspot.com/6347044/diff/7023/environs/ec2/cloudinit_test....
> environs/ec2/cloudinit_test.go:64: t.checkScripts(c, `jujud provisioning
> --zookeeper-servers 'localhost:2181'`)
> On 2012/07/10 08:07:56, rog wrote:
>>
>> s/:2181'/`+zkPortSuffix+"'"/
>
>
> Done.
>
>
> https://codereview.appspot.com/6347044/diff/7023/environs/ec2/live_test.go
> File environs/ec2/live_test.go (right):
>
>
https://codereview.appspot.com/6347044/diff/7023/environs/ec2/live_test.go#ne...
> environs/ec2/live_test.go:58: HasProvisioner:   false, // TODO(dfc)
> waiting on juju/conn.Depoy
> On 2012/07/10 08:07:56, rog wrote:
>>
>> s/Depoy/Deploy/
>
>
> Done.
>
>
> https://codereview.appspot.com/6347044/diff/7023/environs/ec2/local_test.go
> File environs/ec2/local_test.go (right):
>
>
https://codereview.appspot.com/6347044/diff/7023/environs/ec2/local_test.go#n...
> environs/ec2/local_test.go:16: "launchpad.net/juju-core/state/testing"
> On 2012/07/10 08:07:56, rog wrote:
>>
>> minor thought: i'm not sure why juju-core/testing should be the one to
>
> be
>>
>> unaliased here. perhaps both of them could be aliased: "statetesting"
>
> and
>>
>> "coretesting"
>
>
> good point, not for this proposal tho.
>
>
>
https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests.go
> File environs/jujutest/livetests.go (right):
>
>
https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
> environs/jujutest/livetests.go:141: c.Assert(err, IsNil)
> On 2012/07/10 08:07:56, rog wrote:
>>
>> you could move this before the if statement and then lose the if
>
> statement.
>
> Done.
>
>
>
https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
> environs/jujutest/livetests.go:172: } else {
> On 2012/07/10 08:07:56, rog wrote:
>>
>> s/else {//
>
>
> Done.
>
>
https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
> environs/jujutest/livetests.go:174: }
> On 2012/07/10 08:07:56, rog wrote:
>>
>> d
>
>
> Done.
>
>
>
https://codereview.appspot.com/6347044/diff/7023/environs/jujutest/livetests....
> environs/jujutest/livetests.go:179: id, err := m.InstanceId()
> On 2012/07/10 08:07:56, rog wrote:
>>
>> i'm not sure why this is calling InstanceId again when we've been
>
> calling it in
>>
>> the loop to get it. also, this logic won't work when inst is nil.
>
>
> Done.
>
> https://codereview.appspot.com/6347044/

[niemeyer, 2012-07-10 13:10:16]

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
environs/jujutest/livetests.go:94: func (t *LiveTests) testProvisioning(c *C, st
*state.State) {
On 2012/07/10 08:07:56, rog wrote:
> The reason I put this inside TestBootstrap was that TestBootstrap bootstraps
> *and* destroys, so that unless TestBootstrapProvisioner runs before
> TestBootstrap (which it won't because it's alphabetically after), we will wait
> for a full bootstrap cycle twice, which takes ages and ages.

The only purpose of BootstrapOnce is to not have that exact issue. If we go down
that road, every single test will end up inside a single function, which doesn't
sound quite great.

> As a way to get around this, we could rename TestBootstrapProvisioner to
> TestProvisioner and add a comment.
> Alternatively we could change TestBootstrap so that it doesn't destroy and
> re-bootstrap.

Yes, that's a better solution.

[niemeyer, 2012-07-10 13:13:46]

LGTM. Only a trivial before submitting it please:

https://codereview.appspot.com/6347044/diff/14005/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/14005/environs/jujutest/livetests...
environs/jujutest/livetests.go:100: // defer t.Destroy(c)
Please remove this with its comment. We're not testing Bootstrap in this test.
We're testing the provisioner deployment.

[rog, 2012-07-10 13:21:52]

On 10 July 2012 14:10,  <n13m3y3r@gmail.com> wrote:
>
>
https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests.go
> File environs/jujutest/livetests.go (right):
>
>
https://codereview.appspot.com/6347044/diff/5003/environs/jujutest/livetests....
> environs/jujutest/livetests.go:94: func (t *LiveTests)
> testProvisioning(c *C, st *state.State) {
> On 2012/07/10 08:07:56, rog wrote:
>>
>> The reason I put this inside TestBootstrap was that TestBootstrap
>
> bootstraps
>>
>> *and* destroys, so that unless TestBootstrapProvisioner runs before
>> TestBootstrap (which it won't because it's alphabetically after), we
>
> will wait
>>
>> for a full bootstrap cycle twice, which takes ages and ages.
>
>
> The only purpose of BootstrapOnce is to not have that exact issue. If we
> go down that road, every single test will end up inside a single
> function, which doesn't sound quite great.
>
>
>> As a way to get around this, we could rename TestBootstrapProvisioner
>
> to
>>
>> TestProvisioner and add a comment.
>> Alternatively we could change TestBootstrap so that it doesn't destroy
>
> and
>>
>> re-bootstrap.
>
>
> Yes, that's a better solution.

it seems a pity to lose the test for that functionality.
i believe that part of the test has failed before.

perhaps the test suite code that tears down the
once-bootstrapped environment could test that
re-bootstrap works, although that seems a bit odd too.

[gustavo.niemeyer, 2012-07-10 13:32:21]

On Tue, Jul 10, 2012 at 10:21 AM, roger peppe <rogpeppe@gmail.com> wrote:
> perhaps the test suite code that tears down the
> once-bootstrapped environment could test that
> re-bootstrap works, although that seems a bit odd too.

If we re-bootstrap, we have to pay for the price of re-bootstrapping,
and then the function ordering doesn't matter. I don't see how we
can't have it both ways.


gustavo @ http://niemeyer.net

[rog, 2012-07-10 15:26:01]

On 10 July 2012 14:31, Gustavo Niemeyer <gustavo.niemeyer@canonical.com> wrote:
> On Tue, Jul 10, 2012 at 10:21 AM, roger peppe <rogpeppe@gmail.com> wrote:
>> perhaps the test suite code that tears down the
>> once-bootstrapped environment could test that
>> re-bootstrap works, although that seems a bit odd too.
>
> If we re-bootstrap, we have to pay for the price of re-bootstrapping,
> and then the function ordering doesn't matter. I don't see how we
> can't have it both ways.

the call to Bootstrap doesn't take long. it's connecting to the state
that does, which the re-bootstrap test doesn't have to do.

[gustavo.niemeyer, 2012-07-10 15:29:27]

On Tue, Jul 10, 2012 at 12:26 PM, roger peppe <rogpeppe@gmail.com> wrote:
> the call to Bootstrap doesn't take long. it's connecting to the state
> that does, which the re-bootstrap test doesn't have to do.

We seem pretty out of sync here. Bootstrap bootstraps an environment.
If we're talking to Amazon, that takes as long as it takes to
provision a machine on Amazon, which is very far from fast.


gustavo @ http://niemeyer.net

[rog, 2012-07-10 16:05:42]

On 10 July 2012 16:29, Gustavo Niemeyer <gustavo.niemeyer@canonical.com> wrote:
> On Tue, Jul 10, 2012 at 12:26 PM, roger peppe <rogpeppe@gmail.com> wrote:
>> the call to Bootstrap doesn't take long. it's connecting to the state
>> that does, which the re-bootstrap test doesn't have to do.
>
> We seem pretty out of sync here. Bootstrap bootstraps an environment.
> If we're talking to Amazon, that takes as long as it takes to
> provision a machine on Amazon, which is very far from fast.

I just made some measurements. A call to RunInstances takes about 2
seconds for me.
A call to Bootstrap(true) takes about 1 minute (almost all of which is
time spent
uploading the binaries). A call to Bootstrap(true), then connecting to the
state
takes about 4 minutes.

One problem is both those tests are connecting to the state. Perhaps the
code in TestBootstrap that connects to the state should be deleted;
the provisioner test (and others) will test that anyway.

[gustavo.niemeyer, 2012-07-10 20:54:51]

On Tue, Jul 10, 2012 at 1:05 PM, roger peppe <rogpeppe@gmail.com> wrote:
> I just made some measurements. A call to RunInstances takes about 2
> seconds for me.
> A call to Bootstrap(true) takes about 1 minute (almost all of which is
> time spent uploading the binaries). A call to Bootstrap(true), then
> connecting to the state takes about 4 minutes.

Sure, because connecting to the state requires that the bootstrap
completes. The operation that takes time is bootstrapping.


gustavo @ http://niemeyer.net

[dave_cheney.net, 2012-07-10 21:59:31]

Thank you for your review.

Gustavo/Roger - i'll repropose patch set 12 (local ec2 PA tests) where we can
continue the discussion.

https://codereview.appspot.com/6347044/diff/14005/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6347044/diff/14005/environs/jujutest/livetests...
environs/jujutest/livetests.go:100: // defer t.Destroy(c)
On 2012/07/10 13:13:46, niemeyer wrote:
> Please remove this with its comment. We're not testing Bootstrap in this test.
> We're testing the provisioner deployment.

Done.

[dave_cheney.net, 2012-07-10 22:13:07]

*** Submitted:

environs/ec2: bootstrap

This propsal is an incremental improvement towards ec2 bootstrapping.
I've taken the great work Roger did and incorporated Williams testing
changes. juju bootstrap is now able to create machine/0 with a provisioner 
running. The provisioner currently does not do anything as it has no secrets.

Once the problem of pushing the real secrets to ec2 is solved, changing the
HasProvisioner flag in live_test.go should allow this test to pass in the 
real -amazon environment.

R=niemeyer, rog, gustavo.niemeyer
CC=
https://codereview.appspot.com/6347044