provider/null: define simplestreams tools source

provider/null: define simplestreams tools source

This is somewhat involved, as there's some fixes
required (and one drive-by in sshstorage). The
change to use GetToolsSources is simple enough,
and matches what the local provider does.

The Environ override struct in cmd/juju/bootstrap.go
had to so that the optional Environ interfaces were
still implemented. Overriding like that loses the
additional interfaces.

To do the above, environs.BootstrapStorager has been
changed to have a method that tells the environ to
use bootstrap storage, rather than returning a
bootstrap storage. The null provider has changed to
match, of course, as well as cmd/juju.

https://code.launchpad.net/~axwalk/juju-core/null-provider-customsources/+merge/187978

(do not edit description out of merge proposal)

[axw, 2013-09-27 05:32:20]

Please take a look.

[fwereade, 2013-09-27 09:47:33]

Good direction, but a few notes:

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go
File provider/null/environ.go (left):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#oldco...
provider/null/environ.go:43: }
It's quite nice to do "var _" type asserts by the definition of the type, given
that we seem to be growing optional interfaces at a reasonable rate. Compiled
documentation, if you like, that stops you having to read all the methods.

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#newco...
provider/null/environ.go:126: func (e *nullEnviron)
EnableBootstrapStorage(enable bool) (wasEnabled bool, err error) {
We should be locking here, shouldn't we? Even if it makes no practical
difference today, environs should be goroutine-safe.

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#newco...
provider/null/environ.go:153:
storage.NewStorageSimpleStreamsDataSource(e.Storage(), storage.BaseToolsPath),
This is starting to look awfully familiar. No action obviously right today, but
it feels like a bit of a smell.

https://codereview.appspot.com/14011046/diff/1/provider/null/environ_test.go
File provider/null/environ_test.go (right):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ_test.go#...
provider/null/environ_test.go:96: }
I don't see any tests for EnableBootstrapStorage.

[rog, 2013-09-27 10:35:25]

I never did like the optional Environ interfaces, and now I have another reason
- they break encapsulation.
My preferred solution here would be to remove all those interfaces so we have
One True Environ Interface, and leave the code as it was before.

Perhaps there's another way of solving this issue without adding the fairly icky
EnableBootstrapStorage. Could we make the provider use its bootstrap storage
automatically if it detects that it has not been bootstrapped? Then we could
lose all the externally visible BootstrapStorage stuff, perhaps.

Looks reasonable in general; some comments and suggestions below (the sshstorage
ones should be addressed in a separate CL).

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

https://codereview.appspot.com/14011046/diff/1/environs/manual/bootstrap.go#n...
environs/manual/bootstrap.go:67: bootstrapStorage := args.Environ.Storage()
move this down to just before it's used, perhaps?

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
environs/sshstorage/storage.go:263: if _, err := io.ReadFull(r, buf); err != nil
{
I'm not very happy about this. Why are we reading in
the entire input when we can stream it?

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
environs/sshstorage/storage.go:267: tmpdir := utils.ShQuote(s.tmpdir)
Given that we're using these paths unquoted in many places,
I'd like to see a sanity check that they don't contain
any dubious characters.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
environs/sshstorage/storage.go:272: "export TMPDIR=%s && TMPFILE=`mktemp` &&
((%s && mv $TMPFILE %s) || rm -f $TMPFILE)",
What's TMPDIR used for?

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#newco...
provider/null/environ.go:126: func (e *nullEnviron)
EnableBootstrapStorage(enable bool) (wasEnabled bool, err error) {
On 2013/09/27 09:47:33, fwereade wrote:
> We should be locking here, shouldn't we? Even if it makes no practical
> difference today, environs should be goroutine-safe.

+1

But also, why even have the option to disable bootstrap storage?
Will we ever need to do that? This method is used in one quite
specialised way - I don't see that we need to clutter its interface
too much. I'd lose the enable arg and the wasEnabled return value.

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#newco...
provider/null/environ.go:133: return false, errors.New("bootstrap storage
already disabled")
Can't we just make this method idempotent?

[axw1, 2013-09-29 16:05:25]

Thanks for the reviews.

rog: agreed, it would be *much* nicer if this could be determined automatically.
I don't have a solution to that, though.

I'll send another CL to fix some sshstorage things.

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

https://codereview.appspot.com/14011046/diff/1/environs/manual/bootstrap.go#n...
environs/manual/bootstrap.go:67: bootstrapStorage := args.Environ.Storage()
On 2013/09/27 10:35:25, rog wrote:
> move this down to just before it's used, perhaps?

Done.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
environs/sshstorage/storage.go:263: if _, err := io.ReadFull(r, buf); err != nil
{
On 2013/09/27 10:35:25, rog wrote:
> I'm not very happy about this. Why are we reading in
> the entire input when we can stream it?

Because we can't? There are multiple input streams to a long-lived bash process,
where the data input is interleaved with commands. If there's a better way to do
it, I'm happy to make changes.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
environs/sshstorage/storage.go:267: tmpdir := utils.ShQuote(s.tmpdir)
On 2013/09/27 10:35:25, rog wrote:
> Given that we're using these paths unquoted in many places,
> I'd like to see a sanity check that they don't contain
> any dubious characters.

Where are we using them unquoted? I found one occurrence of s.remotepath after
adding a test (good call), but that doesn't equate to "in many places". AFAICS,
all other cases are ShQuoted.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
environs/sshstorage/storage.go:272: "export TMPDIR=%s && TMPFILE=`mktemp` &&
((%s && mv $TMPFILE %s) || rm -f $TMPFILE)",
On 2013/09/27 10:35:25, rog wrote:
> What's TMPDIR used for?

Just to influence mktemp. It was previously set elsewhere, and I just failed to
notice the obvious simplification. I'll update this to call `mktemp
--tmpdir=...`

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go
File provider/null/environ.go (left):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#oldco...
provider/null/environ.go:43: }
On 2013/09/27 09:47:33, fwereade wrote:
> It's quite nice to do "var _" type asserts by the definition of the type,
given
> that we seem to be growing optional interfaces at a reasonable rate. Compiled
> documentation, if you like, that stops you having to read all the methods.

Yeah, I was aware of the convention. As an experiment, I put the assertion in a
test, as it feels wrong to impose a package dependency just for the assertion.
Binary size won't change, but compilation speed will take a hit.

But, point taken about documentation. I've added in here, and dropped the
assertion test. A short lived experiment ;)

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#newco...
provider/null/environ.go:126: func (e *nullEnviron)
EnableBootstrapStorage(enable bool) (wasEnabled bool, err error) {
On 2013/09/27 10:35:25, rog wrote:
> On 2013/09/27 09:47:33, fwereade wrote:
> > We should be locking here, shouldn't we? Even if it makes no practical
> > difference today, environs should be goroutine-safe.
> 
> +1
> 
> But also, why even have the option to disable bootstrap storage?
> Will we ever need to do that? This method is used in one quite
> specialised way - I don't see that we need to clutter its interface
> too much. I'd lose the enable arg and the wasEnabled return value.

Done.

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#newco...
provider/null/environ.go:133: return false, errors.New("bootstrap storage
already disabled")
On 2013/09/27 10:35:25, rog wrote:
> Can't we just make this method idempotent?

Done.

https://codereview.appspot.com/14011046/diff/1/provider/null/environ.go#newco...
provider/null/environ.go:153:
storage.NewStorageSimpleStreamsDataSource(e.Storage(), storage.BaseToolsPath),
On 2013/09/27 09:47:33, fwereade wrote:
> This is starting to look awfully familiar. No action obviously right today,
but
> it feels like a bit of a smell.

Agreed.

https://codereview.appspot.com/14011046/diff/1/provider/null/environ_test.go
File provider/null/environ_test.go (right):

https://codereview.appspot.com/14011046/diff/1/provider/null/environ_test.go#...
provider/null/environ_test.go:96: }
On 2013/09/27 09:47:33, fwereade wrote:
> I don't see any tests for EnableBootstrapStorage.

Added one.

[axw, 2013-09-29 16:27:46]

Please take a look.

[rog, 2013-09-30 13:31:08]

> rog: agreed, it would be *much* nicer if this could be determined
automatically.
> I don't have a solution to that, though.

One possibility might be to make the null provider storage fall back to
bootstrap storage if the regular storage is not available. We could make the
null provider Prepare ssh to the bootstrap host and start a stub server (in some
portable language, so we don't need to push tools across) that simply returns an
error, say "awaiting bootstrap" to any request; then at bootstrap time we'd
replace that by the real storage server.

This would all be more simple, of course, if we stipulated that the null
provider always bootstrapped using the current machine for the bootstrap
machine. I imagine that's off the cards though.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
environs/sshstorage/storage.go:263: if _, err := io.ReadFull(r, buf); err != nil
{
On 2013/09/29 16:05:25, axw1 wrote:
> On 2013/09/27 10:35:25, rog wrote:
> > I'm not very happy about this. Why are we reading in
> > the entire input when we can stream it?
> 
> Because we can't? There are multiple input streams to a long-lived bash
process,
> where the data input is interleaved with commands. If there's a better way to
do
> it, I'm happy to make changes.

I'm probably missing something, but it seems to me that
the buffer is passed to s.run, then encoded to base64
(line 145), the result of which is used for its length (line 148),
and then written with Write (line 157).

It seems to me that you could pass in the reader and length directly,
create a base64.Encoder use base64.EncodedLen and io.Copy to
the encoder to stream the input.

The down side is if there happen to be less bytes than
advertised in the stream, your remote script will get out of sync.
You could use a more robust input reader than
"head -q -c 999". How about using a here document instead?
"base64 -d << '@EOF' | (%s)"

Then you could ignore the expected length and stream
the input, relying on the shell to find the end of it.

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
environs/sshstorage/storage.go:267: tmpdir := utils.ShQuote(s.tmpdir)
On 2013/09/29 16:05:25, axw1 wrote:
> On 2013/09/27 10:35:25, rog wrote:
> > Given that we're using these paths unquoted in many places,
> > I'd like to see a sanity check that they don't contain
> > any dubious characters.
> 
> Where are we using them unquoted? I found one occurrence of s.remotepath after
> adding a test (good call), but that doesn't equate to "in many places".
AFAICS,
> all other cases are ShQuoted.

I'm not sure what I was thinking when I wrote that comment :)

https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
environs/sshstorage/storage.go:272: "export TMPDIR=%s && TMPFILE=`mktemp` &&
((%s && mv $TMPFILE %s) || rm -f $TMPFILE)",
On 2013/09/29 16:05:25, axw1 wrote:
> On 2013/09/27 10:35:25, rog wrote:
> > What's TMPDIR used for?
> 
> Just to influence mktemp. It was previously set elsewhere, and I just failed
to
> notice the obvious simplification. I'll update this to call `mktemp
> --tmpdir=...`

Ah, I didn't realise mktemp recognised $TMPDIR.
For the record, I'd generally prefer:

TMPFILE=`TMPDIR=%s mktemp`

to keep the scope limited - it's then more
obvious that the variable is set specifically
for that command.

[axw1, 2013-10-01 00:35:25]

On 2013/09/30 13:31:08, rog wrote:
> > rog: agreed, it would be *much* nicer if this could be determined
> automatically.
> > I don't have a solution to that, though.
> 
> One possibility might be to make the null provider storage fall back to
> bootstrap storage if the regular storage is not available. We could make the
> null provider Prepare ssh to the bootstrap host and start a stub server (in
some
> portable language, so we don't need to push tools across) that simply returns
an
> error, say "awaiting bootstrap" to any request; then at bootstrap time we'd
> replace that by the real storage server.

I'll have to have a think about that some. I don't really like the idea of
requiring additional VMs/interpreters/etc. to support this, as we may want to
expand to other target OSs. Anyway, can we deal with this outside of this CL? I
can open a tech-debt bug to investigate it after 1.16.0 is out.

> This would all be more simple, of course, if we stipulated that the null
> provider always bootstrapped using the current machine for the bootstrap
> machine. I imagine that's off the cards though.

I think so. Having to connect to the bootstrap machine is (if only a little)
more onerous than necessary.

> https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go
> File environs/sshstorage/storage.go (right):
> 
>
https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
> environs/sshstorage/storage.go:263: if _, err := io.ReadFull(r, buf); err !=
nil
> {
> On 2013/09/29 16:05:25, axw1 wrote:
> > On 2013/09/27 10:35:25, rog wrote:
> > > I'm not very happy about this. Why are we reading in
> > > the entire input when we can stream it?
> > 
> > Because we can't? There are multiple input streams to a long-lived bash
> process,
> > where the data input is interleaved with commands. If there's a better way
to
> do
> > it, I'm happy to make changes.
> 
> I'm probably missing something, but it seems to me that
> the buffer is passed to s.run, then encoded to base64
> (line 145), the result of which is used for its length (line 148),
> and then written with Write (line 157).

Now we're even; I don't know what *I* was thinking when I wrote what I did ;)
Yes, I think you're right and we could just Write with a chained reader. I will
handle this in an update to https://codereview.appspot.com/14102043/

> It seems to me that you could pass in the reader and length directly,
> create a base64.Encoder use base64.EncodedLen and io.Copy to
> the encoder to stream the input.
> 
> The down side is if there happen to be less bytes than
> advertised in the stream, your remote script will get out of sync.
> You could use a more robust input reader than
> "head -q -c 999". How about using a here document instead?
> "base64 -d << '@EOF' | (%s)"
> 
> Then you could ignore the expected length and stream
> the input, relying on the shell to find the end of it.

This is pretty similar to how I did it originally, though the input was in a
here-doc on the command line ;)  I'm not familiar with that @EOF syntax - will
have to read up.

>
https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
> environs/sshstorage/storage.go:267: tmpdir := utils.ShQuote(s.tmpdir)
> On 2013/09/29 16:05:25, axw1 wrote:
> > On 2013/09/27 10:35:25, rog wrote:
> > > Given that we're using these paths unquoted in many places,
> > > I'd like to see a sanity check that they don't contain
> > > any dubious characters.
> > 
> > Where are we using them unquoted? I found one occurrence of s.remotepath
after
> > adding a test (good call), but that doesn't equate to "in many places".
> AFAICS,
> > all other cases are ShQuoted.
> 
> I'm not sure what I was thinking when I wrote that comment :)
> 
>
https://codereview.appspot.com/14011046/diff/1/environs/sshstorage/storage.go...
> environs/sshstorage/storage.go:272: "export TMPDIR=%s && TMPFILE=`mktemp` &&
> ((%s && mv $TMPFILE %s) || rm -f $TMPFILE)",
> On 2013/09/29 16:05:25, axw1 wrote:
> > On 2013/09/27 10:35:25, rog wrote:
> > > What's TMPDIR used for?
> > 
> > Just to influence mktemp. It was previously set elsewhere, and I just failed
> to
> > notice the obvious simplification. I'll update this to call `mktemp
> > --tmpdir=...`
> 
> Ah, I didn't realise mktemp recognised $TMPDIR.
> For the record, I'd generally prefer:
> 
> TMPFILE=`TMPDIR=%s mktemp`
> 
> to keep the scope limited - it's then more
> obvious that the variable is set specifically
> for that command.

Fair enough, though I think in this particular case `mktemp --tmpdir=%s` is even
more obvious.

[thumper, 2013-10-02 04:26:49]

LGTM with one comment on base test suite.

https://codereview.appspot.com/14011046/diff/11001/provider/null/environ_test.go
File provider/null/environ_test.go (right):

https://codereview.appspot.com/14011046/diff/11001/provider/null/environ_test...
provider/null/environ_test.go:24: testbase.CleanupSuite
I'd recommend using the LoggingSuite just because it catches the logging that
may happen anywhere, and puts it in the test log, so it is visible in the test
failures.

[axw1, 2013-10-02 05:06:17]

Thanks for the review.

https://codereview.appspot.com/14011046/diff/11001/provider/null/environ_test.go
File provider/null/environ_test.go (right):

https://codereview.appspot.com/14011046/diff/11001/provider/null/environ_test...
provider/null/environ_test.go:24: testbase.CleanupSuite
On 2013/10/02 04:26:49, thumper wrote:
> I'd recommend using the LoggingSuite just because it catches the logging that
> may happen anywhere, and puts it in the test log, so it is visible in the test
> failures.

Done.