Introduce BootstrapContext

Introduce BootstrapContext

Environ.Bootstrap now takes an additional
parameter, a *BootstrapContext. This is a
structure that permits access to the
caller's context. It currently comprises
stdio handles and a signal handler.

Fixes #1237736

https://code.launchpad.net/~axwalk/juju-core/lp1237736-bootstrap-context/+merge/197999

(do not edit description out of merge proposal)

[axw, 2013-12-06 07:33:28]

Please take a look.

[fwereade, 2013-12-11 08:36:46]

Looks generally clean, but I'd like rog's input on the interrupt handling.

https://codereview.appspot.com/38240043/diff/1/cloudinit/sshinit/configure.go
File cloudinit/sshinit/configure.go (right):

https://codereview.appspot.com/38240043/diff/1/cloudinit/sshinit/configure.go...
cloudinit/sshinit/configure.go:26: Config *cloudinit.Config
to follow on from the discussion earlier, should this maybe actually be a
*instance.Config? Not necessarily actionable, just trying to have a discussion.

https://codereview.appspot.com/38240043/diff/1/cloudinit/sshinit/configure.go...
cloudinit/sshinit/configure.go:28: // Stdin is required to solicit sudo prompts,
s/solicit/respond to/?

https://codereview.appspot.com/38240043/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/38240043/diff/1/environs/bootstrap/bootstrap.g...
environs/bootstrap/bootstrap.go:22: func Bootstrap(ctx
*environs.BootstrapContext, environ environs.Environ, cons constraints.Value)
error {
Random thought. Might it be good to make this func a method on BootstrapContext?

https://codereview.appspot.com/38240043/diff/1/environs/bootstrapcontext_test.go
File environs/bootstrapcontext_test.go (right):

https://codereview.appspot.com/38240043/diff/1/environs/bootstrapcontext_test...
environs/bootstrapcontext_test.go:71: }
I seem to recall rog wanted to do things differently here. Rog?

[axw, 2013-12-11 09:37:13]

Please take a look.

https://codereview.appspot.com/38240043/diff/1/cloudinit/sshinit/configure.go
File cloudinit/sshinit/configure.go (right):

https://codereview.appspot.com/38240043/diff/1/cloudinit/sshinit/configure.go...
cloudinit/sshinit/configure.go:26: Config *cloudinit.Config
On 2013/12/11 08:36:46, fwereade wrote:
> to follow on from the discussion earlier, should this maybe actually be a
> *instance.Config? Not necessarily actionable, just trying to have a
discussion.

I agree that environs/cloudinit should not be called what it is, but I'm not
sure what the right to call it is. I'm not super keen on putting it in instance,
as the configuration brings a lot of things together that aren't relevant at the
instance.Instance level after provisioning.

https://codereview.appspot.com/38240043/diff/1/cloudinit/sshinit/configure.go...
cloudinit/sshinit/configure.go:28: // Stdin is required to solicit sudo prompts,
On 2013/12/11 08:36:46, fwereade wrote:
> s/solicit/respond to/?

Done.

https://codereview.appspot.com/38240043/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/38240043/diff/1/environs/bootstrap/bootstrap.g...
environs/bootstrap/bootstrap.go:22: func Bootstrap(ctx
*environs.BootstrapContext, environ environs.Environ, cons constraints.Value)
error {
On 2013/12/11 08:36:46, fwereade wrote:
> Random thought. Might it be good to make this func a method on
BootstrapContext?

I'd rather not, it changes the meaning of what the BootstrapContext is.

My intention is just to capture information (stdio handles) about the context in
which Environ.Bootstrap is being called, and provide a way of manipulating that
context (i.e. installing signal handlers).

[rog, 2013-12-11 18:53:23]

Looks reasonable in general, but some concerns outlined below.

https://codereview.appspot.com/38240043/diff/20001/environs/bootstrapcontext.go
File environs/bootstrapcontext.go (right):

https://codereview.appspot.com/38240043/diff/20001/environs/bootstrapcontext....
environs/bootstrapcontext.go:49: func (ctx *BootstrapContext)
SetInterruptHandler(f func()) (old func()) {
I'm not keen on this approach, as it doesn't work well in a concurrent
situation. It's quite possible that we might have several concurrent goroutines
all wanting to know when they've been interrupted. This scheme means each one
may trample on another's handler.

The underlying os/signal mechanism works fine in that situation. Why not use
something more like that here?

e.g. 
type BootstrapContext struct {
        Stdin io.Reader
        Stdout io.Writer
	Stderr io.Writer
	InterruptNotify func(sig chan<- os.Signal)
	StopInterruptNotify func(chan<- os.Signal)
}


func DefaultBootstrapContext() *BootstrapContext {
	return &BootstrapContext{
		Stdin: os.Stdin,
		Stdout: os.Stdout,
		Stderr: os.Stderr,
		InterruptNotify: func(c chan<- os.Signal) {
			signal.Notify(c, os.Interrupt)
		},
		StopInterruptNotify: signal.Stop,
	}
}

One thought - perhaps it might be nice to specify it as an interface rather
than a concrete type; then cmd.Context could be made to satisfy it.
Something like this, perhaps?

type Stdio struct {
        Stdin io.Reader
        Stdout io.Writer
	Stderr io.Writer
}

type BootstrapContext interface {
    Stdio() Stdio
    InterruptNotify(sig chan<- os.Signal)
    StopInterruptNotify(chan<- os.Signal)
}

https://codereview.appspot.com/38240043/diff/20001/environs/manual/provisione...
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/38240043/diff/20001/environs/manual/provisione...
environs/manual/provisioner.go:50: // and must be a terminal (except in tests)
I know this is how it already is, but this doesn't make me feel happy.
Somehow it feels like the wrong level of abstraction, and feels like it makes
things hard to script.

This is just a handwave - I can't currently think of concrete suggestions for
anything better, sorry.

https://codereview.appspot.com/38240043/diff/20001/environs/sshstorage/storag...
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/38240043/diff/20001/environs/sshstorage/storag...
environs/sshstorage/storage.go:68: // TmpDir is the remote temporary directory
for storage.
Why do we need this? Can't we use a directory inside StorageDir?

[axw, 2013-12-19 08:44:33]

Please take a look.

https://codereview.appspot.com/38240043/diff/20001/environs/bootstrapcontext.go
File environs/bootstrapcontext.go (right):

https://codereview.appspot.com/38240043/diff/20001/environs/bootstrapcontext....
environs/bootstrapcontext.go:49: func (ctx *BootstrapContext)
SetInterruptHandler(f func()) (old func()) {
On 2013/12/11 18:53:24, rog wrote:
> I'm not keen on this approach, as it doesn't work well in a concurrent
> situation. It's quite possible that we might have several concurrent
goroutines
> all wanting to know when they've been interrupted. This scheme means each one
> may trample on another's handler.
> 
> The underlying os/signal mechanism works fine in that situation. Why not use
> something more like that here?
> 
> e.g. 
> type BootstrapContext struct {
>         Stdin io.Reader
>         Stdout io.Writer
> 	Stderr io.Writer
> 	InterruptNotify func(sig chan<- os.Signal)
> 	StopInterruptNotify func(chan<- os.Signal)
> }
> 
> 
> func DefaultBootstrapContext() *BootstrapContext {
> 	return &BootstrapContext{
> 		Stdin: os.Stdin,
> 		Stdout: os.Stdout,
> 		Stderr: os.Stderr,
> 		InterruptNotify: func(c chan<- os.Signal) {
> 			signal.Notify(c, os.Interrupt)
> 		},
> 		StopInterruptNotify: signal.Stop,
> 	}
> }
> 
> One thought - perhaps it might be nice to specify it as an interface rather
> than a concrete type; then cmd.Context could be made to satisfy it.
> Something like this, perhaps?
> 
> type Stdio struct {
>         Stdin io.Reader
>         Stdout io.Writer
> 	Stderr io.Writer
> }
> 
> type BootstrapContext interface {
>     Stdio() Stdio
>     InterruptNotify(sig chan<- os.Signal)
>     StopInterruptNotify(chan<- os.Signal)
> }
> 

I went with something very similar: Stdio(), Stdout(), and Stderr() instead of
Stdio(). Returning a struct makes for awkward package dependencies. This does
mean having a wrapper struct, as changing all the fields of cmd.Context to
methods is painful.

I've rewritten SetInterruptHandler, made it internal to
provider/common/bootstrap.go, and based it on the
InterruptNotify/StopInterruptNotify methods of BootstrapContext.

https://codereview.appspot.com/38240043/diff/20001/environs/manual/provisione...
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/38240043/diff/20001/environs/manual/provisione...
environs/manual/provisioner.go:50: // and must be a terminal (except in tests)
On 2013/12/11 18:53:24, rog wrote:
> I know this is how it already is, but this doesn't make me feel happy.
> Somehow it feels like the wrong level of abstraction, and feels like it makes
> things hard to script.
> 
> This is just a handwave - I can't currently think of concrete suggestions for
> anything better, sorry.

I don't think it's necessarily that bad; perhaps my wording threw you. It should
read "must be a terminal (except in tests, or if the remote user has
passwordless sudo enabled)". If passwordless sudo is enabled, then you can
script it.

https://codereview.appspot.com/38240043/diff/20001/environs/sshstorage/storag...
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/38240043/diff/20001/environs/sshstorage/storag...
environs/sshstorage/storage.go:68: // TmpDir is the remote temporary directory
for storage.
On 2013/12/11 18:53:24, rog wrote:
> Why do we need this? Can't we use a directory inside StorageDir?

I don't think it's a good idea to allow those temporary files to show up in the
storage results, nor to allow the client to manipulate them.

The reason why we can't specifically block out those temporary files is that
sshstorage overlaps exactly with the manual provider's "localstorage". So, even
if sshstorage ignored the temporary directory, localstorage wouldn't know about
it (or we'd have to tightly couple the two, which seems even more wrong.)

[axw, 2013-12-19 09:14:53]

On 2013/12/19 08:44:33, axw wrote:
> Please take a look.
> 
>
https://codereview.appspot.com/38240043/diff/20001/environs/bootstrapcontext.go
> File environs/bootstrapcontext.go (right):
> 
>
https://codereview.appspot.com/38240043/diff/20001/environs/bootstrapcontext....
> environs/bootstrapcontext.go:49: func (ctx *BootstrapContext)
> SetInterruptHandler(f func()) (old func()) {
> On 2013/12/11 18:53:24, rog wrote:
> > I'm not keen on this approach, as it doesn't work well in a concurrent
> > situation. It's quite possible that we might have several concurrent
> goroutines
> > all wanting to know when they've been interrupted. This scheme means each
one
> > may trample on another's handler.
> > 
> > The underlying os/signal mechanism works fine in that situation. Why not use
> > something more like that here?
> > 
> > e.g. 
> > type BootstrapContext struct {
> >         Stdin io.Reader
> >         Stdout io.Writer
> > 	Stderr io.Writer
> > 	InterruptNotify func(sig chan<- os.Signal)
> > 	StopInterruptNotify func(chan<- os.Signal)
> > }
> > 
> > 
> > func DefaultBootstrapContext() *BootstrapContext {
> > 	return &BootstrapContext{
> > 		Stdin: os.Stdin,
> > 		Stdout: os.Stdout,
> > 		Stderr: os.Stderr,
> > 		InterruptNotify: func(c chan<- os.Signal) {
> > 			signal.Notify(c, os.Interrupt)
> > 		},
> > 		StopInterruptNotify: signal.Stop,
> > 	}
> > }
> > 
> > One thought - perhaps it might be nice to specify it as an interface rather
> > than a concrete type; then cmd.Context could be made to satisfy it.
> > Something like this, perhaps?
> > 
> > type Stdio struct {
> >         Stdin io.Reader
> >         Stdout io.Writer
> > 	Stderr io.Writer
> > }
> > 
> > type BootstrapContext interface {
> >     Stdio() Stdio
> >     InterruptNotify(sig chan<- os.Signal)
> >     StopInterruptNotify(chan<- os.Signal)
> > }
> > 
> 
> I went with something very similar: Stdio(), Stdout(), and Stderr() instead of
> Stdio(). Returning a struct makes for awkward package dependencies. This does
> mean having a wrapper struct, as changing all the fields of cmd.Context to
> methods is painful.
> 
> I've rewritten SetInterruptHandler, made it internal to
> provider/common/bootstrap.go, and based it on the
> InterruptNotify/StopInterruptNotify methods of BootstrapContext.

Actually, in the end I got rid of that, got rid of tomb, and just used the
channel directly. Much simpler now, I think.

>
https://codereview.appspot.com/38240043/diff/20001/environs/manual/provisione...
> File environs/manual/provisioner.go (right):
> 
>
https://codereview.appspot.com/38240043/diff/20001/environs/manual/provisione...
> environs/manual/provisioner.go:50: // and must be a terminal (except in tests)
> On 2013/12/11 18:53:24, rog wrote:
> > I know this is how it already is, but this doesn't make me feel happy.
> > Somehow it feels like the wrong level of abstraction, and feels like it
makes
> > things hard to script.
> > 
> > This is just a handwave - I can't currently think of concrete suggestions
for
> > anything better, sorry.
> 
> I don't think it's necessarily that bad; perhaps my wording threw you. It
should
> read "must be a terminal (except in tests, or if the remote user has
> passwordless sudo enabled)". If passwordless sudo is enabled, then you can
> script it.
> 
>
https://codereview.appspot.com/38240043/diff/20001/environs/sshstorage/storag...
> File environs/sshstorage/storage.go (right):
> 
>
https://codereview.appspot.com/38240043/diff/20001/environs/sshstorage/storag...
> environs/sshstorage/storage.go:68: // TmpDir is the remote temporary directory
> for storage.
> On 2013/12/11 18:53:24, rog wrote:
> > Why do we need this? Can't we use a directory inside StorageDir?
> 
> I don't think it's a good idea to allow those temporary files to show up in
the
> storage results, nor to allow the client to manipulate them.
> 
> The reason why we can't specifically block out those temporary files is that
> sshstorage overlaps exactly with the manual provider's "localstorage". So,
even
> if sshstorage ignored the temporary directory, localstorage wouldn't know
about
> it (or we'd have to tightly couple the two, which seems even more wrong.)

[axw, 2013-12-19 09:29:25]

Please take a look.

[rog, 2013-12-19 11:29:57]

LGTM, thanks, with a few thoughts below.

Also, with respect to this:
> I don't think it's necessarily that bad; perhaps my wording threw you. It
should
> read "must be a terminal (except in tests, or if the remote user has
> passwordless sudo enabled)". If passwordless sudo is enabled, then you can
> script it.

I think the feeling I'm wary of these changes is that it
feels like a gradual creeping of command-line-specific
interaction into some of the core logic. I would like it
if our core logic was independent of that (I wish we didn't
have to shell out to ssh at all FWIW).

For example, previously it would have been possible
to call Bootstrap inside a web service, but with the new dependency
on stdio, that's not really viable any more.

https://codereview.appspot.com/38240043/diff/60001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/38240043/diff/60001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:81: type bootstrapContext struct {
nice

https://codereview.appspot.com/38240043/diff/60001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/38240043/diff/60001/environs/interface.go#newc...
environs/interface.go:200: // Environ.Bootstrap, providing it a means of
obtaining
s/it // ?

https://codereview.appspot.com/38240043/diff/60001/environs/testing/bootstrap.go
File environs/testing/bootstrap.go (right):

https://codereview.appspot.com/38240043/diff/60001/environs/testing/bootstrap...
environs/testing/bootstrap.go:50: func NewBootstrapContext(c *gc.C)
BootstrapContext {
Wouldn't it be better if this returned environs.BootstrapContext ?
(the local type could be unexported)

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

https://codereview.appspot.com/38240043/diff/60001/provider/common/bootstrap....
provider/common/bootstrap.go:91: for {
for _ = range ch {
     fmt.Fprintln(ctx.Stderr(), "Cleaning up failed bootstrap")
}
return
?

I wonder if we should allow the user to abort if they interrupt more than 3
times...

https://codereview.appspot.com/38240043/diff/60001/provider/common/bootstrap....
provider/common/bootstrap.go:154: // the terminal, which will be the ssh
subprocess at this
For the record, I don't believe this is actually true.
The interrupt gets sent to all processes in the foreground
process group of the terminal.

https://codereview.appspot.com/38240043/diff/60001/provider/common/bootstrap....
provider/common/bootstrap.go:372: case <-interrupted:
Much more direct - I like.

[axw, 2013-12-20 02:09:04]

On 2013/12/19 11:29:57, rog wrote:
> LGTM, thanks, with a few thoughts below.
> 
> Also, with respect to this:
> > I don't think it's necessarily that bad; perhaps my wording threw you. It
> should
> > read "must be a terminal (except in tests, or if the remote user has
> > passwordless sudo enabled)". If passwordless sudo is enabled, then you can
> > script it.
> 
> I think the feeling I'm wary of these changes is that it
> feels like a gradual creeping of command-line-specific
> interaction into some of the core logic. I would like it
> if our core logic was independent of that (I wish we didn't
> have to shell out to ssh at all FWIW).

That's fair, although I do think bootstrap is somewhat removed from the "core
logic". Your point about web services is a good one.

> For example, previously it would have been possible
> to call Bootstrap inside a web service, but with the new dependency
> on stdio, that's not really viable any more.

As mentioned on IRC, I don't think that's true. We push authorized_keys to the
cloud, and passwordless sudo is enabled by default; there's no need for any
interactivity. The exception is manual provisioning *iff* a prompt is required
(it may not be), which has never been supported outside of a non-interactive
terminal session.

We can do it all now with $SSHPASS, but that's not really ideal due to its
process-global nature. We could change that to add some programmable prompts, or
use PTYs directly to do the same, but it's starting to get quite complicated. Or
we could do it with go.crypto/ssh, which is something I hope to introduce in the
not too distant future.

> https://codereview.appspot.com/38240043/diff/60001/cmd/juju/bootstrap.go
> File cmd/juju/bootstrap.go (right):
> 
>
https://codereview.appspot.com/38240043/diff/60001/cmd/juju/bootstrap.go#newc...
> cmd/juju/bootstrap.go:81: type bootstrapContext struct {
> nice
> 
> https://codereview.appspot.com/38240043/diff/60001/environs/interface.go
> File environs/interface.go (right):
> 
>
https://codereview.appspot.com/38240043/diff/60001/environs/interface.go#newc...
> environs/interface.go:200: // Environ.Bootstrap, providing it a means of
> obtaining
> s/it // ?
> 
>
https://codereview.appspot.com/38240043/diff/60001/environs/testing/bootstrap.go
> File environs/testing/bootstrap.go (right):
> 
>
https://codereview.appspot.com/38240043/diff/60001/environs/testing/bootstrap...
> environs/testing/bootstrap.go:50: func NewBootstrapContext(c *gc.C)
> BootstrapContext {
> Wouldn't it be better if this returned environs.BootstrapContext ?
> (the local type could be unexported)
> 
>
https://codereview.appspot.com/38240043/diff/60001/provider/common/bootstrap.go
> File provider/common/bootstrap.go (right):
> 
>
https://codereview.appspot.com/38240043/diff/60001/provider/common/bootstrap....
> provider/common/bootstrap.go:91: for {
> for _ = range ch {
>      fmt.Fprintln(ctx.Stderr(), "Cleaning up failed bootstrap")
> }
> return
> ?
> 
> I wonder if we should allow the user to abort if they interrupt more than 3
> times...
> 
>
https://codereview.appspot.com/38240043/diff/60001/provider/common/bootstrap....
> provider/common/bootstrap.go:154: // the terminal, which will be the ssh
> subprocess at this
> For the record, I don't believe this is actually true.
> The interrupt gets sent to all processes in the foreground
> process group of the terminal.
> 
>
https://codereview.appspot.com/38240043/diff/60001/provider/common/bootstrap....
> provider/common/bootstrap.go:372: case <-interrupted:
> Much more direct - I like.

[axw, 2013-12-20 02:42:08]

Please take a look.

https://codereview.appspot.com/38240043/diff/60001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/38240043/diff/60001/environs/interface.go#newc...
environs/interface.go:200: // Environ.Bootstrap, providing it a means of
obtaining
On 2013/12/19 11:29:57, rog wrote:
> s/it // ?

I meant provided to Environ.Bootstrap. It makes sense either way to me; I'm not
fussed, I'll change it.

https://codereview.appspot.com/38240043/diff/60001/environs/testing/bootstrap.go
File environs/testing/bootstrap.go (right):

https://codereview.appspot.com/38240043/diff/60001/environs/testing/bootstrap...
environs/testing/bootstrap.go:50: func NewBootstrapContext(c *gc.C)
BootstrapContext {
On 2013/12/19 11:29:57, rog wrote:
> Wouldn't it be better if this returned environs.BootstrapContext ?
> (the local type could be unexported)

I had planned to do that, but some tests want to change the stdio objects.
Alternatively, NewBootstrapContext could just take a *cmd.Context. I'll do that
before landing.

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

https://codereview.appspot.com/38240043/diff/60001/provider/common/bootstrap....
provider/common/bootstrap.go:91: for {
On 2013/12/19 11:29:57, rog wrote:
> for _ = range ch {
>      fmt.Fprintln(ctx.Stderr(), "Cleaning up failed bootstrap")
> }
> return
> ?

Wow, that was dumb. Thanks.

> I wonder if we should allow the user to abort if they interrupt more than 3
> times...

Sounds reasonable, but I'd like to stick with what we've got for now and tweak
it based on feedback after it's been used a bit.

https://codereview.appspot.com/38240043/diff/60001/provider/common/bootstrap....
provider/common/bootstrap.go:154: // the terminal, which will be the ssh
subprocess at this
On 2013/12/19 11:29:57, rog wrote:
> For the record, I don't believe this is actually true.
> The interrupt gets sent to all processes in the foreground
> process group of the terminal.

My statement wasn't meant imply that it was exclusive of all other processes in
the group. That's why the signal remains blocked; so that when it gets delivered
to the child and this one, this one ignores it and simply waits on the child's
death.

If I'm still wrong, I'd be happy for some schooling :)