provider/common: attempt all addresses in waitSSH

provider/common: attempt all addresses in waitSSH

We now attempt all addresses during waitSSH, and
refresh the addresses periodically. Providers may
return private addresses; for some clouds this is
what we want to use, and for others it is not.

As private addresses are relative, one may be able
to connect to the private address and yet be
connecting to a machine other than the intended
one. For this reason, we emit the machine's nonce
during cloud-init to a file (/var/lib/juju/nonce.txt),
and verify it during waitSSH.

The nonce verification also serves to synchronise
cloud-init and sshinit. Since the file is written
as the last thing cloud-init does, we can safely
run sshinit once it has been verified.

Fixes #1258240
Fixes #1259942

https://code.launchpad.net/~axwalk/juju-core/lp1258240-bootstrap-refresh-dnsname-take2/+merge/198867

(do not edit description out of merge proposal)

[axw, 2013-12-13 04:13:23]

Please take a look.

[axw, 2013-12-13 04:25:05]

On 2013/12/13 04:13:23, axw wrote:
> Please take a look.

Hm, actually, I think this may break when a floating/public IP comes along that
we can't connect to from an internal network.

[axw, 2013-12-13 10:19:32]

On 2013/12/13 04:25:05, axw wrote:
> On 2013/12/13 04:13:23, axw wrote:
> > Please take a look.
> 
> Hm, actually, I think this may break when a floating/public IP comes along
that
> we can't connect to from an internal network.

I played around with a dynamic select (reflect.Select) allowing the loop to
receive results from old/ongoing attempts. I think combined with a check for
some file we write to disk in cloud-init this could be workable. I'll continue
on with it next week.

[jameinel, 2013-12-15 12:36:37]

https://codereview.appspot.com/40860048/diff/1/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/40860048/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:121: func (i *refreshingInstance) DNSName()
(string, error) {
Doing this trick of reloading the Instance so that we don't cache sounds like
something we would do if we didn't control the whole library.

Is there a reason we can't just change the behavior. Either add a method to
indicate we want the uncached DNSName or a flag or something else to Instance?
Adding a proxy instance just to create them on the fly seems like a strange
workaround to me.

[axw, 2013-12-16 07:30:32]

Please take a look.

https://codereview.appspot.com/40860048/diff/1/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/40860048/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:121: func (i *refreshingInstance) DNSName()
(string, error) {
On 2013/12/15 12:36:38, jameinel wrote:
> Doing this trick of reloading the Instance so that we don't cache sounds like
> something we would do if we didn't control the whole library.
> 
> Is there a reason we can't just change the behavior. Either add a method to
> indicate we want the uncached DNSName or a flag or something else to Instance?
> Adding a proxy instance just to create them on the fly seems like a strange
> workaround to me.

I will investigate this. In the mean time, I have updated the CL so that it:
 - works on HP Cloud
 - works on Canonistack (i.e. where you really do want to use the private IP)
 - verifies hosts by checking a file created by cloud-init contains the
machine's nonce

It works by attempting *all* addresses in parallel. I did away with the initial
port 22 check, and went straight to SSH; it's not slow relative to the rest of
the bootstrap process, and it's necessary anyway to verify the nonce.

[axw, 2013-12-16 07:41:53]

https://codereview.appspot.com/40860048/diff/20001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/40860048/diff/20001/provider/common/bootstrap....
provider/common/bootstrap.go:161: `, nonceFile,
utils.ShQuote(machineConfig.MachineNonce))
The bootstrap machine's nonce is static, so it's conceivable that the private
address points to another machine that has Juju on it and is also a bootstrap
node. Unlikely, perhaps, but conceivable.

If this CL is okay otherwise, I'll change Juju to use a UUID for the bootstrap
nonce too.

[axw, 2013-12-16 09:56:56]

Please take a look.

[axw, 2013-12-16 09:58:58]

On 2013/12/16 09:56:56, axw wrote:
> Please take a look.

I had intended to branch and prereq, but I forgot the vital branch step. Oh
well. If it's too much, let me know and I can try to fix it. Otherwise...
there's a Refresh method on instance.Instance now.

[axw, 2013-12-17 12:16:38]

Please take a look.

[axw, 2013-12-18 01:01:01]

Please take a look.

[axw, 2013-12-18 03:18:08]

Please take a look.

[axw, 2013-12-18 03:36:31]

Please take a look.

[rog, 2013-12-18 09:01:04]

Looks great in general, with some comments and suggestions below.

https://codereview.appspot.com/40860048/diff/60001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/40860048/diff/60001/provider/common/bootstrap....
provider/common/bootstrap.go:320: // wait for the tasks to complete. If not
s/not/no/

https://codereview.appspot.com/40860048/diff/120001/environs/cloudinit/cloudi...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/40860048/diff/120001/environs/cloudinit/cloudi...
environs/cloudinit/cloudinit.go:163: c.AddFile(path.Join(cfg.DataDir,
NonceFile), cfg.MachineNonce, 0644)
Is this really the last thing that happens in cloudinit?
What if there are some scripts in the cloudinit.Config?

I think this should probably be an added script.

https://codereview.appspot.com/40860048/diff/120001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/40860048/diff/120001/provider/common/bootstrap...
provider/common/bootstrap.go:237: time.Sleep(hc.checkDelay)
This should probably allow itself to be stopped when closed.j

https://codereview.appspot.com/40860048/diff/120001/provider/common/bootstrap...
provider/common/bootstrap.go:288: for _, addr := range addresses {
I was wondering if some of this code might look nicer in a separate type that
wraps the Try.

type parallelHostChecker struct {
    active make(map[instance.Address]chan struct{}
    *parallel.Try
}

func newParallelHostChecker(checkDelay, checkHostScript) *parallelHostChecker

func (g *parallelHostChecker) SetAddresses(addrs []instance.Address)
    
func (g *parallelHostChecker) Close() error {
    g.Try.Close()
    for _, ch := range active {
        close(ch)
    }
    return nil
}

https://codereview.appspot.com/40860048/diff/120001/provider/common/bootstrap...
File provider/common/bootstrap_test.go (right):

https://codereview.appspot.com/40860048/diff/120001/provider/common/bootstrap...
provider/common/bootstrap_test.go:364: buf := &bytes.Buffer{}
Might be worth running go test -race just to make sure that it's ok to use this,
assuming you haven't already.

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
provider/ec2/ec2.go:71: instanceMu sync.Mutex
just "mu" should be fine here.
conventionally it's situated above the fields that it guards.

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
provider/ec2/ec2.go:75: return inst.InstanceId
shouldn't this use the mutex?

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
provider/ec2/ec2.go:81: return instance.Id(inst.InstanceId)
ditto

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
provider/ec2/ec2.go:85: return inst.State.Name
ditto

https://codereview.appspot.com/40860048/diff/120001/provider/openstack/provid...
File provider/openstack/provider.go (right):

https://codereview.appspot.com/40860048/diff/120001/provider/openstack/provid...
provider/openstack/provider.go:286: serverDetailMu sync.Mutex
again, i'd put this at the end of the struct, named mu, followed by
serverDetail.

If we add more mutex-guarded fields, we'd probably reuse the mutex.

https://codereview.appspot.com/40860048/diff/120001/provider/openstack/provid...
provider/openstack/provider.go:321: func (inst *openstackInstance) idUnlocked()
instance.Id {
I'm not sure this pulls its weight.

How about just:

func idFromDetail(d *nova.ServerDetail) instance.Id {
    return instance.Id(inst.serverDetail.Id)
}
then Id can just become:

func (inst *openstackInstance) Id() instance.Id {
    return idFromDetail(inst.getServerDetail())
}

[axw, 2013-12-18 10:22:10]

Please take a look.

https://codereview.appspot.com/40860048/diff/120001/environs/cloudinit/cloudi...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/40860048/diff/120001/environs/cloudinit/cloudi...
environs/cloudinit/cloudinit.go:163: c.AddFile(path.Join(cfg.DataDir,
NonceFile), cfg.MachineNonce, 0644)
On 2013/12/18 09:01:05, rog wrote:
> Is this really the last thing that happens in cloudinit?
> What if there are some scripts in the cloudinit.Config?

It really is. ConfigureBasic is the only thing called to set up cloud-init for
the bootstrap node.

> I think this should probably be an added script.

Added where though? All of the providers call environs.ComposeUserData, and so
there's no nice place to do that.

I could move environs.ComposeUserData to provider/common and add the script in
that function, but I don't think we'll have gained anything.

https://codereview.appspot.com/40860048/diff/120001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/40860048/diff/120001/provider/common/bootstrap...
provider/common/bootstrap.go:237: time.Sleep(hc.checkDelay)
On 2013/12/18 09:01:05, rog wrote:
> This should probably allow itself to be stopped when closed.j

Done.

https://codereview.appspot.com/40860048/diff/120001/provider/common/bootstrap...
provider/common/bootstrap.go:288: for _, addr := range addresses {
On 2013/12/18 09:01:05, rog wrote:
> I was wondering if some of this code might look nicer in a separate type that
> wraps the Try.
> 
> type parallelHostChecker struct {
>     active make(map[instance.Address]chan struct{}
>     *parallel.Try
> }
> 
> func newParallelHostChecker(checkDelay, checkHostScript) *parallelHostChecker
> 
> func (g *parallelHostChecker) SetAddresses(addrs []instance.Address)
>     
> func (g *parallelHostChecker) Close() error {
>     g.Try.Close()
>     for _, ch := range active {
>         close(ch)
>     }
>     return nil
> }

Done.

https://codereview.appspot.com/40860048/diff/120001/provider/common/bootstrap...
File provider/common/bootstrap_test.go (right):

https://codereview.appspot.com/40860048/diff/120001/provider/common/bootstrap...
provider/common/bootstrap_test.go:364: buf := &bytes.Buffer{}
On 2013/12/18 09:01:05, rog wrote:
> Might be worth running go test -race just to make sure that it's ok to use
this,
> assuming you haven't already.

The printing to stderr is all done within one goroutine (the one that calls
WaitSSH), so there's no potential for race.

However, go test -race has shown up something else :)
In bootstrap.go, hostChecker.loop spawns a goroutine that may outlive loop (and
everything that calls that). That goroutine calls connectSSH, a function var
that is replaced in the test setup/teardown.

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
provider/ec2/ec2.go:71: instanceMu sync.Mutex
On 2013/12/18 09:01:05, rog wrote:
> just "mu" should be fine here.
> conventionally it's situated above the fields that it guards.

Done.

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
provider/ec2/ec2.go:75: return inst.InstanceId
On 2013/12/18 09:01:05, rog wrote:
> shouldn't this use the mutex?

No, the mutex only guards ec2Instance.Instance (i.e. the embedded
*ec2.Instance). I've put the mutex above that field and separated them from "e"
to make it more obvious.

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
provider/ec2/ec2.go:81: return instance.Id(inst.InstanceId)
On 2013/12/18 09:01:05, rog wrote:
> ditto

ditto

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
provider/ec2/ec2.go:85: return inst.State.Name
On 2013/12/18 09:01:05, rog wrote:
> ditto

ditto :)

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
provider/ec2/ec2.go:110: err := inst.Refresh()
Oops, there's a potential double lock on the mutex here. Fixed that.

https://codereview.appspot.com/40860048/diff/120001/provider/openstack/provid...
File provider/openstack/provider.go (right):

https://codereview.appspot.com/40860048/diff/120001/provider/openstack/provid...
provider/openstack/provider.go:286: serverDetailMu sync.Mutex
On 2013/12/18 09:01:05, rog wrote:
> again, i'd put this at the end of the struct, named mu, followed by
> serverDetail.
> 
> If we add more mutex-guarded fields, we'd probably reuse the mutex.

Done.

https://codereview.appspot.com/40860048/diff/120001/provider/openstack/provid...
provider/openstack/provider.go:321: func (inst *openstackInstance) idUnlocked()
instance.Id {
On 2013/12/18 09:01:05, rog wrote:
> I'm not sure this pulls its weight.
> 
> How about just:
> 
> func idFromDetail(d *nova.ServerDetail) instance.Id {
>     return instance.Id(inst.serverDetail.Id)
> }
> then Id can just become:
> 
> func (inst *openstackInstance) Id() instance.Id {
>     return idFromDetail(inst.getServerDetail())
> }

Good point. I went a step further and inlined the use if inst.serverDetail.Id in
Refresh, and then:

func (inst *openstackInstance) Id() instance.Id {
    return instance.Id(inst.getServerDetail().Id)
}

[axw, 2013-12-18 10:34:04]

Please take a look.

[rog, 2013-12-18 12:49:08]

LGTM, thanks, except I'd like to know a bit more about the AddFile vs AddRunCmd
issue.

https://codereview.appspot.com/40860048/diff/120001/environs/cloudinit/cloudi...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/40860048/diff/120001/environs/cloudinit/cloudi...
environs/cloudinit/cloudinit.go:163: c.AddFile(path.Join(cfg.DataDir,
NonceFile), cfg.MachineNonce, 0644)
On 2013/12/18 10:22:10, axw wrote:
> On 2013/12/18 09:01:05, rog wrote:
> > Is this really the last thing that happens in cloudinit?
> > What if there are some scripts in the cloudinit.Config?
> 
> It really is. ConfigureBasic is the only thing called to set up cloud-init for
> the bootstrap node.
> 
> > I think this should probably be an added script.
> 
> Added where though? All of the providers call environs.ComposeUserData, and so
> there's no nice place to do that.

I was thinking that instead of this AddFile we'd just
call c.AddRunCmd(scriptToCreateMachineNonceFile).

If we do things as above and one of the providers uses the additionalScripts
argument to ComposeUserData, won't we
write the nonce file before running any of those scripts?

But I'm probably misunderstanding the issues around this.

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
provider/ec2/ec2.go:75: return inst.InstanceId
On 2013/12/18 10:22:10, axw wrote:
> On 2013/12/18 09:01:05, rog wrote:
> > shouldn't this use the mutex?
> 
> No, the mutex only guards ec2Instance.Instance (i.e. the embedded
> *ec2.Instance). I've put the mutex above that field and separated them from
"e"
> to make it more obvious.

Ahem. Discussed online.

https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
provider/ec2/ec2.go:110: err := inst.Refresh()
On 2013/12/18 10:22:10, axw wrote:
> Oops, there's a potential double lock on the mutex here. Fixed that.

Since that evidently wasn't covered by the tests, perhaps we could have a test
that did cover that?

https://codereview.appspot.com/40860048/diff/120001/provider/openstack/provid...
File provider/openstack/provider.go (right):

https://codereview.appspot.com/40860048/diff/120001/provider/openstack/provid...
provider/openstack/provider.go:321: func (inst *openstackInstance) idUnlocked()
instance.Id {
On 2013/12/18 10:22:10, axw wrote:
> On 2013/12/18 09:01:05, rog wrote:
> > I'm not sure this pulls its weight.
> > 
> > How about just:
> > 
> > func idFromDetail(d *nova.ServerDetail) instance.Id {
> >     return instance.Id(inst.serverDetail.Id)
> > }
> > then Id can just become:
> > 
> > func (inst *openstackInstance) Id() instance.Id {
> >     return idFromDetail(inst.getServerDetail())
> > }
> 
> Good point. I went a step further and inlined the use if inst.serverDetail.Id
in
> Refresh, and then:
> 
> func (inst *openstackInstance) Id() instance.Id {
>     return instance.Id(inst.getServerDetail().Id)
> }

Cool - I almost suggested that.

[axw, 2013-12-18 14:16:20]

On 2013/12/18 12:49:08, rog wrote:
> LGTM, thanks, except I'd like to know a bit more about the AddFile vs
AddRunCmd
> issue.
> 
>
https://codereview.appspot.com/40860048/diff/120001/environs/cloudinit/cloudi...
> File environs/cloudinit/cloudinit.go (right):
> 
>
https://codereview.appspot.com/40860048/diff/120001/environs/cloudinit/cloudi...
> environs/cloudinit/cloudinit.go:163: c.AddFile(path.Join(cfg.DataDir,
> NonceFile), cfg.MachineNonce, 0644)
> On 2013/12/18 10:22:10, axw wrote:
> > On 2013/12/18 09:01:05, rog wrote:
> > > Is this really the last thing that happens in cloudinit?
> > > What if there are some scripts in the cloudinit.Config?
> > 
> > It really is. ConfigureBasic is the only thing called to set up cloud-init
for
> > the bootstrap node.
> > 
> > > I think this should probably be an added script.
> > 
> > Added where though? All of the providers call environs.ComposeUserData, and
so
> > there's no nice place to do that.
> 
> I was thinking that instead of this AddFile we'd just
> call c.AddRunCmd(scriptToCreateMachineNonceFile).
> 
> If we do things as above and one of the providers uses the additionalScripts
> argument to ComposeUserData, won't we
> write the nonce file before running any of those scripts?
> 
> But I'm probably misunderstanding the issues around this.

Ah, I see what you mean. Well, AddFile is actually a runcmd underneath, so it's
not a problem. The additionalScripts are added before ConfigureBasic is run, so
the AddFile runcmd comes last.

> https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go
> File provider/ec2/ec2.go (right):
> 
>
https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
> provider/ec2/ec2.go:75: return inst.InstanceId
> On 2013/12/18 10:22:10, axw wrote:
> > On 2013/12/18 09:01:05, rog wrote:
> > > shouldn't this use the mutex?
> > 
> > No, the mutex only guards ec2Instance.Instance (i.e. the embedded
> > *ec2.Instance). I've put the mutex above that field and separated them from
> "e"
> > to make it more obvious.
> 
> Ahem. Discussed online.
> 
>
https://codereview.appspot.com/40860048/diff/120001/provider/ec2/ec2.go#newco...
> provider/ec2/ec2.go:110: err := inst.Refresh()
> On 2013/12/18 10:22:10, axw wrote:
> > Oops, there's a potential double lock on the mutex here. Fixed that.
> 
> Since that evidently wasn't covered by the tests, perhaps we could have a test
> that did cover that?

Yes, I've just modified goamz/ec2/ec2test to set DNSName to "" the first time it
returns an instance. This showed up a bug. I'll repropose the fix here, then
propose the change to goamz later.

>
https://codereview.appspot.com/40860048/diff/120001/provider/openstack/provid...
> File provider/openstack/provider.go (right):
> 
>
https://codereview.appspot.com/40860048/diff/120001/provider/openstack/provid...
> provider/openstack/provider.go:321: func (inst *openstackInstance)
idUnlocked()
> instance.Id {
> On 2013/12/18 10:22:10, axw wrote:
> > On 2013/12/18 09:01:05, rog wrote:
> > > I'm not sure this pulls its weight.
> > > 
> > > How about just:
> > > 
> > > func idFromDetail(d *nova.ServerDetail) instance.Id {
> > >     return instance.Id(inst.serverDetail.Id)
> > > }
> > > then Id can just become:
> > > 
> > > func (inst *openstackInstance) Id() instance.Id {
> > >     return idFromDetail(inst.getServerDetail())
> > > }
> > 
> > Good point. I went a step further and inlined the use if
inst.serverDetail.Id
> in
> > Refresh, and then:
> > 
> > func (inst *openstackInstance) Id() instance.Id {
> >     return instance.Id(inst.getServerDetail().Id)
> > }
> 
> Cool - I almost suggested that.

[axw, 2013-12-18 14:19:34]

Please take a look.

[axw, 2013-12-18 15:19:16]

Please take a look.

https://codereview.appspot.com/40860048/diff/120001/environs/cloudinit/cloudi...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/40860048/diff/120001/environs/cloudinit/cloudi...
environs/cloudinit/cloudinit.go:163: c.AddFile(path.Join(cfg.DataDir,
NonceFile), cfg.MachineNonce, 0644)
On 2013/12/18 12:49:09, rog wrote:
> On 2013/12/18 10:22:10, axw wrote:
> > On 2013/12/18 09:01:05, rog wrote:
> > > Is this really the last thing that happens in cloudinit?
> > > What if there are some scripts in the cloudinit.Config?
> > 
> > It really is. ConfigureBasic is the only thing called to set up cloud-init
for
> > the bootstrap node.
> > 
> > > I think this should probably be an added script.
> > 
> > Added where though? All of the providers call environs.ComposeUserData, and
so
> > there's no nice place to do that.
> 
> I was thinking that instead of this AddFile we'd just
> call c.AddRunCmd(scriptToCreateMachineNonceFile).
> 
> If we do things as above and one of the providers uses the additionalScripts
> argument to ComposeUserData, won't we
> write the nonce file before running any of those scripts?
> 
> But I'm probably misunderstanding the issues around this.

As discussed on IRC, I've made this explicitly use AddScripts (which generates
multiple runcmds) in place of AddFile, in the event AddFile gets changed.