Merge lp://staging/~fwereade/juju-core/environs-tools-final-replacement into lp://staging/~juju/juju-core/trunk

Reviewers: mp+158830_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: sync-tools fixes

sync-tools now requires the --dev flag before copying development
versions,
and no longer copies into a public bucket when the contents of that
bucket
are not available to the target environment.

This branch removes the last of the fuzzy multi-bucket tools code.

https://code.launchpad.net/~fwereade/juju-core/environs-tools-final-replacement/+merge/158830

Requires:
https://code.launchpad.net/~fwereade/juju-core/environs-tools-provisioning-integration/+merge/158805

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8748046/

Affected files:
   A [revision details]
   M cmd/juju/synctools.go
   M cmd/juju/synctools_test.go
   M environs/tools.go
   M environs/tools_test.go

LGTM with the "private tools present" error fixed and a couple more
minor suggestions.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150
cmd/juju/synctools.go:150: return fmt.Errorf("private tools present:
public tools would be ignored")
i think this should be just a warning, if anything.
just because we have private tools present in the current environment
doesn't mean we don't want to make tools available to others in the
public bucket.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161
cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches,
tools.ErrNoTools:
still pained by this.

https://codereview.appspot.com/8748046/diff/1/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/8748046/diff/1/environs/tools.go#newcode102
environs/tools.go:102: log.Infof("environs: filtering tools by series:
%s", series)
these feel like they should be Debugf calls to me (and probably not even
that)

if you think the log messages are worth it, i'd probably just do:

log.Debugf("environs: filtering tools by %+v", filter) just before the
Match.

https://codereview.appspot.com/8748046/diff/1/environs/tools.go#newcode108
environs/tools.go:108: log.Infof("environs: filtering tools by
architecture: %s", *cons.Arch)
as above.

https://codereview.appspot.com/8748046/diff/1/environs/tools.go#newcode124
environs/tools.go:124: log.Infof("environs: finding exact version %s",
vers)
Debugf, i think, if anything.

https://codereview.appspot.com/8748046/

LGTM with a few trivials.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go
File cmd/juju/synctools_test.go (right):

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go#newcode116
cmd/juju/synctools_test.go:116: "1.0.0-precise-amd64",
"1.0.0-quantal-amd64", "1.0.0-quantal-i386")
please, put these on separate lines (or at least one line per series).

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go#newcode145
cmd/juju/synctools_test.go:145: "1.0.0-precise-amd64",
"1.0.0-quantal-amd64", "1.0.0-quantal-i386")
ditto

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go#newcode160
cmd/juju/synctools_test.go:160: "1.0.0-precise-amd64",
"1.0.0-quantal-amd64",
ditto - make them consistent

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go#newcode176
cmd/juju/synctools_test.go:176: "1.0.0-precise-amd64",
"1.0.0-quantal-amd64", "1.0.0-quantal-i386")
...and here

https://codereview.appspot.com/8748046/diff/1/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/8748046/diff/1/environs/tools.go#newcode102
environs/tools.go:102: log.Infof("environs: filtering tools by series:
%s", series)
On 2013/04/15 13:48:21, rog wrote:
> these feel like they should be Debugf calls to me (and probably not
even that)

> if you think the log messages are worth it, i'd probably just do:

> log.Debugf("environs: filtering tools by %+v", filter) just before the
Match.

I think it's nice as an Infof, at least it's consistent with the func
above. If anything, I'll merge the two messages in one: "environs:
filtering tools by version %q and series %q"

https://codereview.appspot.com/8748046/

Please take a look.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150
cmd/juju/synctools.go:150: return fmt.Errorf("private tools present:
public tools would be ignored")
On 2013/04/15 13:48:21, rog wrote:
> i think this should be just a warning, if anything.
> just because we have private tools present in the current environment
doesn't
> mean we don't want to make tools available to others in the public
bucket.

Honestly, I question the value of --public regardless. If I were setting
it up I'd have a shared-tools environ that I never actually used other
than to sync tools into, and use its control-bucket as my other
environs' public-bucket.

Given the current fallback rules, a public sync-tools into an env with
private tools will fail to perform the purpose of sync-tools -- make
them *available* to the target env -- and so I think it's actually
better to fail before performing a ineffectual copy of X MiB via one's
local machine.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161
cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches,
tools.ErrNoTools:
On 2013/04/15 13:48:21, rog wrote:
> still pained by this.

Each non-test file that uses them together also needs to differentiate
between the two situations. I don't think it's unreasonable to have
distinct errors.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go
File cmd/juju/synctools_test.go (right):

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go#newcode116
cmd/juju/synctools_test.go:116: "1.0.0-precise-amd64",
"1.0.0-quantal-amd64", "1.0.0-quantal-i386")
On 2013/04/15 15:00:54, dimitern wrote:
> please, put these on separate lines (or at least one line per series).

heh, they're like this because I was trying to maintain consistency.
Done.

https://codereview.appspot.com/8748046/

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150
cmd/juju/synctools.go:150: return fmt.Errorf("private tools present:
public tools would be ignored")
On 2013/04/16 08:43:40, fwereade wrote:
> On 2013/04/15 13:48:21, rog wrote:
> > i think this should be just a warning, if anything.
> > just because we have private tools present in the current
environment doesn't
> > mean we don't want to make tools available to others in the public
bucket.

> Honestly, I question the value of --public regardless. If I were
setting it up
> I'd have a shared-tools environ that I never actually used other than
to sync
> tools into, and use its control-bucket as my other environs'
public-bucket.

> Given the current fallback rules, a public sync-tools into an env with
private
> tools will fail to perform the purpose of sync-tools -- make them
*available* to
> the target env -- and so I think it's actually better to fail before
performing
> a ineffectual copy of X MiB via one's local machine.

i think if we're asking it to do something, it should darn well do it.
the user may well know best.

a warning would be fine, i think.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161
cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches,
tools.ErrNoTools:
On 2013/04/16 08:43:40, fwereade wrote:
> On 2013/04/15 13:48:21, rog wrote:
> > still pained by this.

> Each non-test file that uses them together also needs to differentiate
between
> the two situations.

the code above demonstrates that statement isn't actually true.

taking a quick look through all the places that use the tools.Err*
values, i don't see any where the errors are particularly useful, and
sometimes they make things more obscure.

to try and check that i wasn't on crack, i rustled up a branch which did
what i'm suggesting (lose the special errors and the major-version
argument to ListTools). overall (not counting the tests, which i didn't
touch) it's 27 less lines of code and sync-tools will now work with all
tools, not just the same major version.

in particular, putting the major-version matching inside Match seems
like the Right Thing to do.

lp:~rogpeppe/juju-core/fwereade-less-errors

https://codereview.appspot.com/8748046/

It continues... is there some use case I have failed to properly take
into account?

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150
cmd/juju/synctools.go:150: return fmt.Errorf("private tools present:
public tools would be ignored")
On 2013/04/16 10:01:00, rog wrote:
> On 2013/04/16 08:43:40, fwereade wrote:
> > On 2013/04/15 13:48:21, rog wrote:
> > > i think this should be just a warning, if anything.
> > > just because we have private tools present in the current
environment
> doesn't
> > > mean we don't want to make tools available to others in the public
bucket.
> >
> > Honestly, I question the value of --public regardless. If I were
setting it up
> > I'd have a shared-tools environ that I never actually used other
than to sync
> > tools into, and use its control-bucket as my other environs'
public-bucket.
> >
> > Given the current fallback rules, a public sync-tools into an env
with private
> > tools will fail to perform the purpose of sync-tools -- make them
*available*
> to
> > the target env -- and so I think it's actually better to fail before
> performing
> > a ineffectual copy of X MiB via one's local machine.

> i think if we're asking it to do something, it should darn well do it.
> the user may well know best.

> a warning would be fine, i think.

A sync-tools into a public bucket being used by a running environ with
tools *will not work* from the POV of the user. They ask for the latest
tools, and juju does a bunch of work, and they don't get the latest
tools.

If they want fresh tools to run on a real environ, they can
destroy-environment, sync-tools --public, bootstrap; and if they're
using the environment purely as a frontend to a public tools bucket,
they'll never have private tools messing everything up.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161
cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches,
tools.ErrNoTools:
On 2013/04/16 10:01:00, rog wrote:
> On 2013/04/16 08:43:40, fwereade wrote:
> > On 2013/04/15 13:48:21, rog wrote:
> > > still pained by this.
> >
> > Each non-test file that uses them together also needs to
differentiate between
> > the two situations.

> the code above demonstrates that statement isn't actually true.

...it's the code above *that* that demonstrates that it in fact just as
I stated it.

     case tools.ErrNoTools:
     case nil, tools.ErrNoMatches:

> taking a quick look through all the places that use the tools.Err*
values, i
> don't see any where the errors are particularly useful, and sometimes
they make
> things more obscure.

AIUI, what we agreed in atlanta was that private tools would render
public ones inaccessible. I don't think that adding an exception to that
rule makes anything clearer... I feel twitchy enough about the magic
source env and hardcoded use of its public-storage, but I managed to
resist the temptation to add a --source env-name feature (which would
default to the magic env, and in all cases would cause sync-tools to
draw from only the *available* tools in the source env).

I haven't yet been...

i'm not going to argue these points any more. i have registered my
objections, and i don't want to block progress. if you still think it's
appropriate, please go ahead.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150
cmd/juju/synctools.go:150: return fmt.Errorf("private tools present:
public tools would be ignored")
On 2013/04/16 10:58:58, fwereade wrote:
> On 2013/04/16 10:01:00, rog wrote:
> > On 2013/04/16 08:43:40, fwereade wrote:
> > > On 2013/04/15 13:48:21, rog wrote:
> > > > i think this should be just a warning, if anything.
> > > > just because we have private tools present in the current
environment
> > doesn't
> > > > mean we don't want to make tools available to others in the
public bucket.
> > >
> > > Honestly, I question the value of --public regardless. If I were
setting it
> up
> > > I'd have a shared-tools environ that I never actually used other
than to
> sync
> > > tools into, and use its control-bucket as my other environs'
public-bucket.
> > >
> > > Given the current fallback rules, a public sync-tools into an env
with
> private
> > > tools will fail to perform the purpose of sync-tools -- make them
> *available*
> > to
> > > the target env -- and so I think it's actually better to fail
before
> > performing
> > > a ineffectual copy of X MiB via one's local machine.
> >
> > i think if we're asking it to do something, it should darn well do
it.
> > the user may well know best.
> >
> > a warning would be fine, i think.

> A sync-tools into a public bucket being used by a running environ with
tools
> *will not work* from the POV of the user. They ask for the latest
tools, and
> juju does a bunch of work, and they don't get the latest tools.

If they specify --public, they're not asking for the latest tools.
They're asking them to be made available to the "public".

I can easily think of a situation where I'd want to use synctools
with an existing environment bootstrapped with --upload-tools;
in fact, that's quite possibly the way I'd *normally* use them
(juju bootstrap --upload-tools; check it works; juju sync-tools
--public)

The worst possible down side of letting the sync carry on is that it
spends
a while copying tools (and you'd get a warning). I can't see that as a
significant problem.

Aborting with an error seems to me to be patronising behaviour.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161
cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches,
tools.ErrNoTools:
On 2013/04/16 10:58:58, fwereade wrote:
> On 2013/04/16 10:01:00, rog wrote:
> > On 2013/04/16 08:43:40, fwereade wrote:
> > > On 2013/04/15 13:48:21, rog wrote:
> > > > still pained by this.
> > >
> > > Each non-test file that uses them together also needs to
differentiate
> between
> > > the two situations.
> >
> > the code above demonstrates that statement isn't actually true.

> ...it's the code above *that* that demonstrates that it in fact just
as I stated
> it.

> case tools.ErrNoTools:
> case nil, tools.ErrNoMatches:

> > taking a quick look through all the places th...

*** Submitted:

cmd/juju: sync-tools fixes

sync-tools now requires the --dev flag before copying development
versions,
and no longer copies into a public bucket when the contents of that
bucket
are not available to the target environment.

This branch removes the last of the fuzzy multi-bucket tools code.

R=rog, dimitern
CC=
https://codereview.appspot.com/8748046

https://codereview.appspot.com/8748046/

I probably shouldn't keep banging on about this, but some of it's
actually really important.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150
cmd/juju/synctools.go:150: return fmt.Errorf("private tools present:
public tools would be ignored")
On 2013/04/16 11:54:24, rog wrote:
> If they specify --public, they're not asking for the latest tools.
> They're asking them to be made available to the "public".

sync-tools is an environment-level command, which is intended to make
tools available to that environment. The --public flag causes it to use
the public-bucket in doing so.

> I can easily think of a situation where I'd want to use synctools
> with an existing environment bootstrapped with --upload-tools;
> in fact, that's quite possibly the way I'd *normally* use them
> (juju bootstrap --upload-tools; check it works; juju sync-tools
--public)

Please explain what purpose this serves.

> The worst possible down side of letting the sync carry on is that it
spends
> a while copying tools (and you'd get a warning). I can't see that as a
> significant problem.

The problem is that, from the perspective of the user who wants to use
newer tools, the command has *failed* despite reporting success.

> Aborting with an error seems to me to be patronising behaviour.

The user has set up their environment such that it cannot use public
tools. Wasting the user's time and bandwidth to do something that cannot
possibly work and then throwing up our hands and saying "but we warned
you" doesn't seem ideal. TBH, I think that has a pretty clear "I told
you so but you just wouldn't listen" subtext that's pretty patronising
itself.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161
cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches,
tools.ErrNoTools:
On 2013/04/16 11:54:24, rog wrote:
> On 2013/04/16 10:58:58, fwereade wrote:
> > AIUI, what we agreed in atlanta was that private tools would render
public
> ones
> > inaccessible. I don't think that adding an exception to that rule
makes
> anything
> > clearer...

> i don't think i'm making an exception to that rule.
> if anything i think it's making *less* exceptions.
> the user-observable behaviour with my suggested changes
> should be exactly the same as your branch is now
> (with the exception that --all actually means "all")

You are introducing a fuzzy and unnamed concept that maps to a group of
environments that happen to share some arbitrary convenient config
setting, and you are conflating those with actual environments. I hope I
do not need to reiterate that the difference between "working" and "not
working", as described above, is in fact user-observable.

> that's leaning hard on your particular definition of "available",
meaning
> "available to this particular client". i'd prefer a more general
definition:
> "available to some client".

I mean "available to the target environment". If we follow your
definition, we don't need sync-tools at all, because any syncable tool
is inherently accessible by *some* environment. But I don't see ho...

On 17 April 2013 10:26, William Reade <email address hidden> wrote:
> I probably shouldn't keep banging on about this, but some of it's
> actually really important.

Agreed. I hope this discussion is actually helping us come to
a shared understanding of these issues.

> On 2013/04/16 11:54:24, rog wrote:
>> If they specify --public, they're not asking for the latest tools.
>> They're asking them to be made available to the "public".
>
> sync-tools is an environment-level command, which is intended to make
> tools available to that environment. The --public flag causes it to use
> the public-bucket in doing so.

The public bucket is shared between many environments
(that's kind of the whole point of it).

Just because my current environment is using tools from
its private bucket doesn't mean that I don't want to make
tools available in the public bucket where other environments
that use it can then have access to them. Those other
environments might potentially want to bootstrap with a different major version
too.

>> I can easily think of a situation where I'd want to use synctools
>> with an existing environment bootstrapped with --upload-tools;
>> in fact, that's quite possibly the way I'd *normally* use them
>> (juju bootstrap --upload-tools; check it works; juju sync-tools
> --public)
>
> Please explain what purpose this serves.
>
>> The worst possible down side of letting the sync carry on is that it
> spends
>> a while copying tools (and you'd get a warning). I can't see that as a
>> significant problem.
>
> The problem is that, from the perspective of the user who wants to use
> newer tools, the command has *failed* despite reporting success.

That is true. But that perspective is not that only valid one, I believe.

>> Aborting with an error seems to me to be patronising behaviour.
>
> The user has set up their environment such that it cannot use public
> tools.

As above, you are assuming that the only reason for uploading
to the public bucket is so that *this* environment can use them.

>> i don't think i'm making an exception to that rule.
>> if anything i think it's making *less* exceptions.
>> the user-observable behaviour with my suggested changes
>> should be exactly the same as your branch is now
>> (with the exception that --all actually means "all")
>
> You are introducing a fuzzy and unnamed concept that maps to a group of
> environments that happen to share some arbitrary convenient config
> setting, and you are conflating those with actual environments.

I'm not sure I understand how what you're saying there relates to
what I suggested.

> I hope I
> do not need to reiterate that the difference between "working" and "not
> working", as described above, is in fact user-observable.

The issues above (returning an error when uploading to the public
bucket if there's stuff in the private bucket) are orthogonal to
the List suggestions, aren't they?

>> that's leaning hard on your particular definition of "available",
> meaning
>> "available to this particular client". i'd prefer a more general
> definition:
>> "available to some client".
>
> I mean "available to the target environment". If we follow your
> definition, ...