Merge lp:~axwalk/juju-core/manual-bootstrap into lp:~go-bot/juju-core/trunk

Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 1850
Proposed branch: lp:~axwalk/juju-core/manual-bootstrap
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 1104 lines (+616/-155)
16 files modified
cmd/juju/bootstrap.go (+19/-0)
cmd/jujud/machine.go (+1/-1)
environs/interface.go (+11/-0)
environs/manual/agent.go (+28/-40)
environs/manual/bootstrap.go (+135/-0)
environs/manual/bootstrap_test.go (+164/-0)
environs/manual/detection.go (+14/-3)
environs/manual/detection_test.go (+7/-63)
environs/manual/fakessh_test.go (+112/-0)
environs/manual/provisioner.go (+17/-10)
environs/manual/provisioner_test.go (+28/-23)
environs/manual/tools.go (+24/-0)
testing/testbase/patch.go (+12/-0)
testing/testbase/patch_test.go (+9/-0)
worker/localstorage/config.go (+14/-0)
worker/localstorage/worker.go (+21/-15)
To merge this branch: bzr merge lp:~axwalk/juju-core/manual-bootstrap
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+184714@code.launchpad.net

Commit message

Implement manual bootstrapping

environs/bootstrap:
  - Added BootstrapStorage interface. If an Environ
    implements this interface, its BootstrapStorage
    will be used by cmd/juju bootstrap.

cmd/juju:
  - Updated bootstrap subcommand to use
    environs/bootstrap.BootstrapStorage,
    as described above.

provider/local/storage:
  - Added LocalStorageConfig interface, which an
    Environ may implement to describe the configuration
    of a "localstorage".

environs/filestorage:
  - Added NewFileStorage (i.e. implement StorageWrite)
    for testing in environs/manual.

environs/manual:
  - Refactored existing code to support bootstrap machines.
  - Added "Bootstrap" function to manually bootstrap an
    existing machine.

To manually bootstrap, you must provide an environment that
implements the new LocalStorageConfig and BootstrapStorage
interfaces. In other words, you can (currently) only manually
bootstrap a machine which is expected to manage the storage
for its environment. This may be relaxed later if needed.

Next up is the null provider, which uses this new Bootstrap
function. I need to address one last issue with it first:
the null provider mustn't narrow the tools to default-series.

https://codereview.appspot.com/13635044/

Description of the change

Implement manual bootstrapping

environs/bootstrap:
  - Added BootstrapStorage interface. If an Environ
    implements this interface, its BootstrapStorage
    will be used by cmd/juju bootstrap.

cmd/juju:
  - Updated bootstrap subcommand to use
    environs/bootstrap.BootstrapStorage,
    as described above.

provider/local/storage:
  - Added LocalStorageConfig interface, which an
    Environ may implement to describe the configuration
    of a "localstorage".

environs/filestorage:
  - Added NewFileStorage (i.e. implement StorageWrite)
    for testing in environs/manual.

environs/manual:
  - Refactored existing code to support bootstrap machines.
  - Added "Bootstrap" function to manually bootstrap an
    existing machine.

To manually bootstrap, you must provide an environment that
implements the new LocalStorageConfig and BootstrapStorage
interfaces. In other words, you can (currently) only manually
bootstrap a machine which is expected to manage the storage
for its environment. This may be relaxed later if needed.

Next up is the null provider, which uses this new Bootstrap
function. I need to address one last issue with it first:
the null provider mustn't narrow the tools to default-series.

https://codereview.appspot.com/13635044/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :

Reviewers: mp+184714_code.launchpad.net,

Message:
Please take a look.

Description:
Implement manual bootstrapping

environs/bootstrap:
   - Added BootstrapStorage interface. If an Environ
     implements this interface, its BootstrapStorage
     will be used by cmd/juju bootstrap.

cmd/juju:
   - Updated bootstrap subcommand to use
     environs/bootstrap.BootstrapStorage,
     as described above.

provider/local/storage:
   - Added LocalStorageConfig interface, which an
     Environ may implement to describe the configuration
     of a "localstorage".

environs/filestorage:
   - Added NewFileStorage (i.e. implement StorageWrite)
     for testing in environs/manual.

environs/manual:
   - Refactored existing code to support bootstrap machines.
   - Added "Bootstrap" function to manually bootstrap an
     existing machine.

To manually bootstrap, you must provide an environment that
implements the new LocalStorageConfig and BootstrapStorage
interfaces. In other words, you can (currently) only manually
bootstrap a machine which is expected to manage the storage
for its environment. This may be relaxed later if needed.

Next up is the null provider, which uses this new Bootstrap
function. I need to address one last issue with it first:
the null provider mustn't narrow the tools to default-series.

https://code.launchpad.net/~axwalk/juju-core/manual-bootstrap/+merge/184714

(do not edit description out of merge proposal)

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

Affected files (+525, -53 lines):
   A [revision details]
   M cmd/juju/bootstrap.go
   M environs/bootstrap/bootstrap.go
   M environs/filestorage/filestorage.go
   M environs/manual/agent.go
   A environs/manual/bootstrap.go
   A environs/manual/bootstrap_test.go
   M environs/manual/provisioner.go
   M environs/manual/provisioner_test.go
   A environs/manual/tools.go
   A provider/local/storage/config.go
   M provider/local/storage/worker.go

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Revision history for this message
William Reade (fwereade) wrote :

Looking very nice; just a few questions, and a suggestion that you
consider moving the local storage worker into the top-level worker
package, rather than hiding it away under local.

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

https://codereview.appspot.com/13635044/diff/4001/cmd/juju/bootstrap.go#newcode71
cmd/juju/bootstrap.go:71: if bs, ok :=
environ.(bootstrap.BootstrapStorage); ok {
Would that maybe be ".(BootstrapStorager)"? Elsewhere, we're
experimenting with things like:

type HasBootstrapStorage interface {
     BootstrapStorage() (Storage, error)
}

...which IMO leads at least to more-readable casts. YMMV.

https://codereview.appspot.com/13635044/diff/4001/environs/filestorage/filestorage.go
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/filestorage/filestorage.go#newcode114
environs/filestorage/filestorage.go:114: _, err = io.CopyN(file, r,
length)
As suggested in the previous review, please copy the whole lot somewhere
out of the way and move atomically into place on success; in this case I
think we've definitely vulnerable to errors with longer-term
consequences.

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

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.go#newcode143
environs/manual/bootstrap.go:143: }
This is really nice; can we produce some output while we're doing so? I
guess it'll have to be straight to the log for now, but a few INFO
messages would be helpful.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.go#newcode146
environs/manual/bootstrap.go:146: // get the *state.Machine?
Don't think so...

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go
File environs/manual/bootstrap_test.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go#newcode153
environs/manual/bootstrap_test.go:153: c.Assert(Bootstrap(args),
gc.ErrorMatches, "Environ argument is nil")
FWIW, I'm kinda ok with panics for unexpected nils.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go#newcode172
environs/manual/bootstrap_test.go:172: args.MachineId = "1"
Eyebrow slightly raised here... I'm not sure that's actually valid. Do
we initialize the machine id counter in state such that all subsequent
numbers are higher?

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go#newcode204
environs/manual/bootstrap_test.go:204: defer sshresponse(c,
checkProvisionedScript, "", 0)()
This code block is looking awfully familiar now. Can we tidy it up a
little?

https://codereview.appspot.com/13635044/diff/4001/environs/manual/tools.go
File environs/manual/tools.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/tools.go#newcode23
environs/manual/tools.go:23: arches := possibleTools.Arches()
Why would we be getting multiple arches out of FindInstanceTools? Aren't
we passing one in?

https://codereview.appspot.com/13635044/

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (4.3 KiB)

Thank you for the review. I have moved provider/local/storage to
worker/localstorage.

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

https://codereview.appspot.com/13635044/diff/4001/cmd/juju/bootstrap.go#newcode71
cmd/juju/bootstrap.go:71: if bs, ok :=
environ.(bootstrap.BootstrapStorage); ok {
On 2013/09/11 13:51:21, fwereade wrote:
> Would that maybe be ".(BootstrapStorager)"?

Yes, that's a bit better, thanks.

> Elsewhere, we're experimenting with
> things like:

> type HasBootstrapStorage interface {
> BootstrapStorage() (Storage, error)
> }

> ...which IMO leads at least to more-readable casts. YMMV.

I'm not a big fan of the "HasX" style. IMO, the name should represent
the behaviour or role, not something an implementer has.

https://codereview.appspot.com/13635044/diff/4001/environs/filestorage/filestorage.go
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/filestorage/filestorage.go#newcode114
environs/filestorage/filestorage.go:114: _, err = io.CopyN(file, r,
length)
On 2013/09/11 13:51:21, fwereade wrote:
> As suggested in the previous review, please copy the whole lot
somewhere out of
> the way and move atomically into place on success; in this case I
think we've
> definitely vulnerable to errors with longer-term consequences.

Done. It's a bit simpler here :)

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

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.go#newcode143
environs/manual/bootstrap.go:143: }
On 2013/09/11 13:51:21, fwereade wrote:
> This is really nice; can we produce some output while we're doing so?
I guess
> it'll have to be straight to the log for now, but a few INFO messages
would be
> helpful.

Sure. I've added some logger.Infofs to checkProvisioned,
detectSeriesAndHardwareCharacteristics and provisionMachineAgent. I've
purposely not added Errorfs, as error results will be logged upstream.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.go#newcode146
environs/manual/bootstrap.go:146: // get the *state.Machine?
On 2013/09/11 13:51:21, fwereade wrote:
> Don't think so...

Oops! Nor do I. I had that in there from early on, and forgot to take it
out :)

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go
File environs/manual/bootstrap_test.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go#newcode153
environs/manual/bootstrap_test.go:153: c.Assert(Bootstrap(args),
gc.ErrorMatches, "Environ argument is nil")
On 2013/09/11 13:51:21, fwereade wrote:
> FWIW, I'm kinda ok with panics for unexpected nils.

Noted.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go#newcode172
environs/manual/bootstrap_test.go:172: args.MachineId = "1"
On 2013/09/11 13:51:21, fwereade wrote:
> Eyebrow slightly raised here... I'm not sure that's actually valid. Do
we
> initialize the machine id counter in state such that all subsequent
numbers are
> higher?

The reason for t...

Read more...

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Revision history for this message
William Reade (fwereade) wrote :
Download full text (4.6 KiB)

This is great, thanks. Modulo the /tmp-filesystem issue, this LGTM, on
the understanding that the local storage is not yet fit for purpose and
needs some concept of authentication [0] before we can actually let
people use this feature.

Let me know if I've completely misunderstood the overlap between
SSHStorage and LocalStorage, which is not a blocker for this CL but must
be resolved in a followup (if it's how I understood it to be).

[0] or to not be exposed, which we *might* be able to get away with if
we always used SSH storage from outside..?

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

https://codereview.appspot.com/13635044/diff/4001/cmd/juju/bootstrap.go#newcode71
cmd/juju/bootstrap.go:71: if bs, ok :=
environ.(bootstrap.BootstrapStorage); ok {
On 2013/09/12 07:59:00, axw1 wrote:
> On 2013/09/11 13:51:21, fwereade wrote:
> > Would that maybe be ".(BootstrapStorager)"?

> Yes, that's a bit better, thanks.

> > Elsewhere, we're experimenting with
> > things like:
> >
> > type HasBootstrapStorage interface {
> > BootstrapStorage() (Storage, error)
> > }
> >
> > ...which IMO leads at least to more-readable casts. YMMV.

> I'm not a big fan of the "HasX" style. IMO, the name should represent
the
> behaviour or role, not something an implementer has.

Your mileage may vary :). I'm not really sold on either way tbh.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go
File environs/manual/bootstrap_test.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go#newcode172
environs/manual/bootstrap_test.go:172: args.MachineId = "1"
On 2013/09/12 07:59:00, axw1 wrote:
> On 2013/09/11 13:51:21, fwereade wrote:
> > Eyebrow slightly raised here... I'm not sure that's actually valid.
Do we
> > initialize the machine id counter in state such that all subsequent
numbers
> are
> > higher?

> The reason for this test is that Environ.Bootstrap is provided with a
machine ID
> to use. manual.Bootstrap is invoked via the null provider's Bootstrap
method.
> Thus, the test exists so as not to make assumptions about being on
machine "0".

> Whether or not the counter is incremented... I don't think that can be
its
> concern, if it's been provided with the ID.

Yeah, this isn't your problem. I think it still is a problem that we can
allow bootstrap with arbitrary ids, but that other bits of the system
assign special meaning to "0" (it somehow seems that every time we
eliminate a special case for 0 we end up with a new one somewhere else).

https://codereview.appspot.com/13635044/diff/14001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13635044/diff/14001/cmd/jujud/machine.go#newcode30
cmd/jujud/machine.go:30: localstorage
"launchpad.net/juju-core/worker/localstorage"
drop the explicit name?

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go#newcode45
environs/filestorage/filestorage.go:45: func NewFileStorage(...

Read more...

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (3.2 KiB)

Haven't pushed changes yet, need to sort out the storage issues. Here's
my answers anyway.

https://codereview.appspot.com/13635044/diff/14001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13635044/diff/14001/cmd/jujud/machine.go#newcode30
cmd/jujud/machine.go:30: localstorage
"launchpad.net/juju-core/worker/localstorage"
On 2013/09/13 09:24:59, fwereade wrote:
> drop the explicit name?

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go#newcode45
environs/filestorage/filestorage.go:45: func NewFileStorage(path string)
(environs.Storage, error) {
On 2013/09/13 09:24:59, fwereade wrote:
> High-level question while I think of it: does the SSHStorage on
bootstrap get
> reused but backed with a FileStorage? I sort of imagine so, in which
case, cool
> ...but is it possible for there to be overlap? ie should we be sharing
the
> flocking everywhere?

> or am I just fantasizing?

filestorage is only used for testing at the moment.

environs/localstorage takes over from environs/sshstorage, reusing the
storage directory. However, there is no overlap in use;
environs/sshstorage is only used to write the tools and bootstrap state
file. *If* there's a failure bootstrapping, the bootstrap storage may
also be used to remove the state file.

Since there's no overlap, there's no need to share the locking. I did
just realise that I totally broke the manual bootstrap code though, when
I changed sshstorage's directory structure. I'll have to revisit that
before merging.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go#newcode110
environs/filestorage/filestorage.go:110: file, err :=
ioutil.TempFile("", "juju-filestorage-"+name)
On 2013/09/13 09:24:59, fwereade wrote:
> Same different-filesystems problem.

This is only for testing, but I will fix it anyway. I think localstorage
ought to be renamed httpstorage, and have the backend contain one of
these.

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

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode28
environs/manual/bootstrap.go:28: bootstrap.BootstrapStorager
On 2013/09/13 09:24:59, fwereade wrote:
> OK, I think my problem here is maybe that it's not *just* about
bootstrap
> storage, right? It's the same storage we always use, but this is the
path to it
> that's accessible from a client machine. Yes?

> So, at least in the short term, we *will* need SSHStorage and
LocalStorage to
> coexist.

They're pointing to the same location, but they're never used
concurrently.

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker.go
File worker/localstorage/worker.go (left):

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker.go#oldcode1
worker/localstorage/worker.go:1: package storage
On 2013/09/13 09:24:59, fwereade wrote:
> ah, I see. Can we call this localstorage please?

Sorry, fixed.

https://co...

Read more...

Revision history for this message
Roger Peppe (rogpeppe) wrote :
Download full text (7.5 KiB)

Looks good. Some minor comments and suggestions below.

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

https://codereview.appspot.com/13635044/diff/14001/cmd/juju/bootstrap.go#newcode76
cmd/juju/bootstrap.go:76: environ = &bootstrapStorageEnviron{environ,
bootstrapStorage}
nice

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstrap.go#newcode24
environs/bootstrap/bootstrap.go:24: // BootstrapStorager is an interface
that returns a environs.Storage that may
I think I'd define this inside environs/interface.go

This package doesn't define or use it, and that file
is a useful reference for people implementing providers.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go#newcode45
environs/filestorage/filestorage.go:45: func NewFileStorage(path string)
(environs.Storage, error) {
Doc comment please.

Also, wouldn't it make more sense to define NewFileStorageReader in
terms of NewFileStorage? That way you wouldn't need the dynamic type
conversion, as Storage can be assigned to StorageReader.

For future consideration, I think filestorage.New and
filestorage.NewReader would be more idiomatic names than
filestorage.NewFileStorage and filestorage.NewFileStorageReader.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go#newcode41
environs/manual/agent.go:41: machineenv map[string]string
s/machineenv/machineEnv/

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

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode42
environs/manual/bootstrap.go:42: var errHostEmpty = errors.New("Host
argument is empty")
Errors don't generally start with a capital letter.

Also, if it's only used once, I wouldn't bother defining a variable for
it.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode43
environs/manual/bootstrap.go:43: var errEnvironNil = errors.New("Environ
argument is nil")
ditto

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode51
environs/manual/bootstrap.go:51: // host will manually bootstrapped.
s/will/will be/ ?

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode65
environs/manual/bootstrap.go:65: err = fmt.Errorf("error checking if
provisioned: %v", err)
"failed to check provisioned status: %v" ?

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode78
environs/manual/bootstrap.go:78: defer func() {
Why not defer this after the call to SaveState and thereby lose the need
for the savedState variable?

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootst...

Read more...

Revision history for this message
Tim Penhey (thumper) wrote :
Download full text (5.0 KiB)

Still some changes needed here I think

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

https://codereview.appspot.com/13635044/diff/14001/cmd/juju/bootstrap.go#newcode76
cmd/juju/bootstrap.go:76: environ = &bootstrapStorageEnviron{environ,
bootstrapStorage}
If all we are caring about is the storage, why are we changing the
environ?

Does it not make sense to change the things that depend on the storage
to just depend on the storage and not the entire environment, then you
don't need to mock out the Storage method.

Obviously here the important thing is that we want to use a particular
storage for bootstrap, so I'd expect to be assigning the
bootstrapStorage to a storage variable, not mocking out the method with
a different structure.

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstrap.go#newcode24
environs/bootstrap/bootstrap.go:24: // BootstrapStorager is an interface
that returns a environs.Storage that may
On 2013/09/14 07:28:03, rog wrote:
> I think I'd define this inside environs/interface.go

> This package doesn't define or use it, and that file
> is a useful reference for people implementing providers.

Agreed. It is something that is used by the environment, so it makes
more sense to live there.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go#newcode110
environs/filestorage/filestorage.go:110: file, err :=
ioutil.TempFile("", "juju-filestorage-"+name)
On 2013/09/13 09:41:26, axw1 wrote:
> On 2013/09/13 09:24:59, fwereade wrote:
> > Same different-filesystems problem.

> This is only for testing, but I will fix it anyway. I think
localstorage ought
> to be renamed httpstorage, and have the backend contain one of these.

That sounds like a good idea IMO

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go
File environs/manual/agent.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go#newcode82
environs/manual/agent.go:82: err := environs.FinishMachineConfig(mcfg,
args.envcfg, constraints.Value{})
I fear that using names like "envcfg" means that we have more and more
assumed knowledge encoded into our variable names. Here I like some
parts of the Zen of Python "Explicit is better than implicit", yes it is
python, but I think the principle holds.

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

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode28
environs/manual/bootstrap.go:28: bootstrap.BootstrapStorager
Arg, here I know that I'm going to clash on names with Rog, but IMO
   BootstrapStorager
is a terrible name.
   HasBootstrapStorage
makes a lot more sense to me. Firstly you can look at the name and know
what is going on, and we aren't inventing words to go by the weird...

Read more...

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (14.4 KiB)

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

https://codereview.appspot.com/13635044/diff/14001/cmd/juju/bootstrap.go#newcode76
cmd/juju/bootstrap.go:76: environ = &bootstrapStorageEnviron{environ,
bootstrapStorage}
On 2013/09/16 03:02:08, thumper wrote:
> If all we are caring about is the storage, why are we changing the
environ?

> Does it not make sense to change the things that depend on the storage
to just
> depend on the storage and not the entire environment, then you don't
need to
> mock out the Storage method.

The storage must be passed down to environs/bootstrap Bootstrap, and
then to environs/tools FindTools. Possibly more places. Those things
also want an Environ (in the case of FindTools, it's hidden in an
interface conversion for the legacy fallback codepath).

Bootstrap storage must be used for *everything* until the environment is
bootstrapped. Leaving a method for acquiring broken/unusable storage
seems like a mistake to me.

> Obviously here the important thing is that we want to use a particular
storage
> for bootstrap, so I'd expect to be assigning the bootstrapStorage to a
storage
> variable, not mocking out the method with a different structure.

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstrap.go#newcode24
environs/bootstrap/bootstrap.go:24: // BootstrapStorager is an interface
that returns a environs.Storage that may
On 2013/09/14 07:28:03, rog wrote:
> I think I'd define this inside environs/interface.go

> This package doesn't define or use it, and that file
> is a useful reference for people implementing providers.

Fair point, done.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go#newcode45
environs/filestorage/filestorage.go:45: func NewFileStorage(path string)
(environs.Storage, error) {
On 2013/09/14 07:28:03, rog wrote:
> Doc comment please.

Done. Seems someone else came along and wrote a writer constructor in
the mean time. It lacks a doc comment, so I'll add to that :)

> Also, wouldn't it make more sense to define NewFileStorageReader in
terms of
> NewFileStorage? That way you wouldn't need the dynamic type
conversion, as
> Storage can be assigned to StorageReader.

That would simplify the code here. My rationale for doing it this way is
that it limits exposure.

IMO, you shouldn't be able to convert a StorageReader to a Storage,
unless you originally created it with NewStorage and converted it to
StorageReader.

> For future consideration, I think filestorage.New and
filestorage.NewReader
> would be more idiomatic names than filestorage.NewFileStorage and
> filestorage.NewFileStorageReader.

I do agree, but sadly it seems there's no consensus.

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go#newcode110
environs/filestorage/filestorage.go:110: file, err :=
ioutil.TempFile...

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Revision history for this message
Roger Peppe (rogpeppe) wrote :

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

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode28
environs/manual/bootstrap.go:28: bootstrap.BootstrapStorager
On 2013/09/16 04:56:48, axw1 wrote:
> On 2013/09/16 03:02:08, thumper wrote:
> > Arg, here I know that I'm going to clash on names with Rog, but IMO
> > BootstrapStorager
> > is a terrible name.
> > HasBootstrapStorage
> > makes a lot more sense to me. Firstly you can look at the name and
know what
> is
> > going on, and we aren't inventing words to go by the weird idea that
> interfaces
> > should end with "er".
> >
> > bootstrapStorage, ok := sometype.(HasBootstrapStorage)
> >
> > reads ok to me (FSVO ok given the syntax of the type assertion).

> Copy and paste of my reply to William: I'm not a big fan of the "HasX"
style.
> IMO, the name should represent the behaviour or role, not something an
> implementer has.

+lots.

I'm not keen on "BootstrapStorager" as a name either,
but I think it's important that interface names are nouns
not predicates - an interface name should represent what the
object *is*, not what it *can be*.

https://codereview.appspot.com/13635044/

Revision history for this message
Tim Penhey (thumper) wrote :

On 2013/09/16 16:51:34, rog wrote:

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go
> File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode28
> environs/manual/bootstrap.go:28: bootstrap.BootstrapStorager
> On 2013/09/16 04:56:48, axw1 wrote:
> > On 2013/09/16 03:02:08, thumper wrote:
> > > Arg, here I know that I'm going to clash on names with Rog, but
IMO
> > > BootstrapStorager
> > > is a terrible name.
> > > HasBootstrapStorage
> > > makes a lot more sense to me. Firstly you can look at the name
and know
> what
> > is
> > > going on, and we aren't inventing words to go by the weird idea
that
> > interfaces
> > > should end with "er".
> > >
> > > bootstrapStorage, ok := sometype.(HasBootstrapStorage)
> > >
> > > reads ok to me (FSVO ok given the syntax of the type assertion).
> >
> > Copy and paste of my reply to William: I'm not a big fan of the
"HasX" style.
> > IMO, the name should represent the behaviour or role, not something
an
> > implementer has.

> +lots.

> I'm not keen on "BootstrapStorager" as a name either,
> but I think it's important that interface names are nouns
> not predicates - an interface name should represent what the
> object *is*, not what it *can be*.

Why not then just call the interface "BootstrapStorage".

https://codereview.appspot.com/13635044/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

On 16 September 2013 22:01, <email address hidden> wrote:
> Why not then just call the interface "BootstrapStorage".

That would be appropriate for the storage itself, but this
interface represents an object that can be asked *for* the storage,
which doesn't seem quite the same thing to me.

Revision history for this message
William Reade (fwereade) wrote :

If the only remaining contentious point is (Has)BootstrapStorage(r),
then I call dealer's choice and LGTM whatever he picks; I don't think
any of the names are great, but I also don't think any of them are so
bad as to unacceptably reduce the quality of the codebase ;).

If there are other problems, carry on as usual, ofc, but let's not block
this on naming arguments.

https://codereview.appspot.com/13635044/

Revision history for this message
Go Bot (go-bot) wrote :

The attempt to merge lp:~axwalk/juju-core/manual-bootstrap into lp:juju-core failed. Below is the output from the failed tests.

environs/manual/bootstrap_test.go

# launchpad.net/juju-core/cmd/juju
cmd/juju/bootstrap.go:237: undefined: environs.Storage
cmd/juju/bootstrap.go:240: undefined: environs.Storage

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'cmd/juju/bootstrap.go'
--- cmd/juju/bootstrap.go 2013-09-20 00:52:13 +0000
+++ cmd/juju/bootstrap.go 2013-09-20 05:05:14 +0000
@@ -17,6 +17,7 @@
17 "launchpad.net/juju-core/environs"17 "launchpad.net/juju-core/environs"
18 "launchpad.net/juju-core/environs/bootstrap"18 "launchpad.net/juju-core/environs/bootstrap"
19 "launchpad.net/juju-core/environs/config"19 "launchpad.net/juju-core/environs/config"
20 "launchpad.net/juju-core/environs/storage"
20 "launchpad.net/juju-core/environs/sync"21 "launchpad.net/juju-core/environs/sync"
21 envtools "launchpad.net/juju-core/environs/tools"22 envtools "launchpad.net/juju-core/environs/tools"
22 "launchpad.net/juju-core/errors"23 "launchpad.net/juju-core/errors"
@@ -68,6 +69,15 @@
68 if err != nil {69 if err != nil {
69 return err70 return err
70 }71 }
72 // If the environment has a special bootstrap Storage, use it wherever
73 // we'd otherwise use environ.Storage.
74 if bs, ok := environ.(environs.BootstrapStorager); ok {
75 bootstrapStorage, err := bs.BootstrapStorage()
76 if err != nil {
77 return fmt.Errorf("failed to acquire bootstrap storage: %v", err)
78 }
79 environ = &bootstrapStorageEnviron{environ, bootstrapStorage}
80 }
71 // TODO: if in verbose mode, write out to Stdout if a new cert was created.81 // TODO: if in verbose mode, write out to Stdout if a new cert was created.
72 _, err = environs.EnsureCertificate(environ, environs.WriteCertAndKey)82 _, err = environs.EnsureCertificate(environ, environs.WriteCertAndKey)
73 if err != nil {83 if err != nil {
@@ -222,3 +232,12 @@
222 }232 }
223 return unique.Values()233 return unique.Values()
224}234}
235
236type bootstrapStorageEnviron struct {
237 environs.Environ
238 bootstrapStorage storage.Storage
239}
240
241func (b *bootstrapStorageEnviron) Storage() storage.Storage {
242 return b.bootstrapStorage
243}
225244
=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go 2013-09-18 06:27:52 +0000
+++ cmd/jujud/machine.go 2013-09-20 05:05:14 +0000
@@ -20,7 +20,6 @@
20 "launchpad.net/juju-core/log"20 "launchpad.net/juju-core/log"
21 "launchpad.net/juju-core/names"21 "launchpad.net/juju-core/names"
22 "launchpad.net/juju-core/provider"22 "launchpad.net/juju-core/provider"
23 localstorage "launchpad.net/juju-core/provider/local/storage"
24 "launchpad.net/juju-core/state"23 "launchpad.net/juju-core/state"
25 "launchpad.net/juju-core/state/api/params"24 "launchpad.net/juju-core/state/api/params"
26 "launchpad.net/juju-core/state/apiserver"25 "launchpad.net/juju-core/state/apiserver"
@@ -29,6 +28,7 @@
29 "launchpad.net/juju-core/worker/cleaner"28 "launchpad.net/juju-core/worker/cleaner"
30 "launchpad.net/juju-core/worker/deployer"29 "launchpad.net/juju-core/worker/deployer"
31 "launchpad.net/juju-core/worker/firewaller"30 "launchpad.net/juju-core/worker/firewaller"
31 "launchpad.net/juju-core/worker/localstorage"
32 "launchpad.net/juju-core/worker/logger"32 "launchpad.net/juju-core/worker/logger"
33 "launchpad.net/juju-core/worker/machiner"33 "launchpad.net/juju-core/worker/machiner"
34 "launchpad.net/juju-core/worker/minunitsworker"34 "launchpad.net/juju-core/worker/minunitsworker"
3535
=== modified file 'environs/interface.go'
--- environs/interface.go 2013-09-18 07:13:09 +0000
+++ environs/interface.go 2013-09-20 05:05:14 +0000
@@ -68,6 +68,17 @@
68 PublicStorage() storage.StorageReader68 PublicStorage() storage.StorageReader
69}69}
7070
71// BootstrapStorager is an interface that returns a environs.Storage that may
72// be used before the bootstrap machine agent has been provisioned.
73//
74// This is useful for environments where the storage is managed by the machine
75// agent once bootstrapped.
76type BootstrapStorager interface {
77 // BootstrapStorager returns an environs.Storage that may be used while
78 // bootstrapping a machine.
79 BootstrapStorage() (storage.Storage, error)
80}
81
71// ConfigGetter implements access to an environments configuration.82// ConfigGetter implements access to an environments configuration.
72type ConfigGetter interface {83type ConfigGetter interface {
73 // Config returns the configuration data with which the Environ was created.84 // Config returns the configuration data with which the Environ was created.
7485
=== modified file 'environs/manual/agent.go'
--- environs/manual/agent.go 2013-09-13 08:39:36 +0000
+++ environs/manual/agent.go 2013-09-20 05:05:14 +0000
@@ -14,7 +14,7 @@
14 "launchpad.net/juju-core/constraints"14 "launchpad.net/juju-core/constraints"
15 "launchpad.net/juju-core/environs"15 "launchpad.net/juju-core/environs"
16 "launchpad.net/juju-core/environs/cloudinit"16 "launchpad.net/juju-core/environs/cloudinit"
17 envtools "launchpad.net/juju-core/environs/tools"17 "launchpad.net/juju-core/environs/config"
18 "launchpad.net/juju-core/provider"18 "launchpad.net/juju-core/provider"
19 "launchpad.net/juju-core/state"19 "launchpad.net/juju-core/state"
20 "launchpad.net/juju-core/state/api"20 "launchpad.net/juju-core/state/api"
@@ -23,21 +23,27 @@
23)23)
2424
25type provisionMachineAgentArgs struct {25type provisionMachineAgentArgs struct {
26 host string26 host string
27 dataDir string27 dataDir string
28 env environs.Environ28 bootstrap bool
29 machine *state.Machine29 environConfig *config.Config
30 nonce string30 machineId string
31 stateInfo *state.Info31 nonce string
32 apiInfo *api.Info32 stateFileURL string
33 series string33 stateInfo *state.Info
34 arch string34 apiInfo *api.Info
35 tools *tools.Tools35 tools *tools.Tools
36
37 // agentEnv is an optional map of
38 // arbitrary key/value pairs to pass
39 // into the machine agent.
40 agentEnv map[string]string
36}41}
3742
38// provisionMachineAgent connects to a machine over SSH,43// provisionMachineAgent connects to a machine over SSH,
39// copies across the tools, and installs a machine agent.44// copies across the tools, and installs a machine agent.
40func provisionMachineAgent(args provisionMachineAgentArgs) error {45func provisionMachineAgent(args provisionMachineAgentArgs) error {
46 logger.Infof("Provisioning machine agent on %s", args.host)
41 script, err := provisionMachineAgentScript(args)47 script, err := provisionMachineAgentScript(args)
42 if err != nil {48 if err != nil {
43 return err49 return err
@@ -58,28 +64,27 @@
58// provisionMachineAgentScript generates the script necessary64// provisionMachineAgentScript generates the script necessary
59// to install a machine agent on the specified host.65// to install a machine agent on the specified host.
60func provisionMachineAgentScript(args provisionMachineAgentArgs) (string, error) {66func provisionMachineAgentScript(args provisionMachineAgentArgs) (string, error) {
61 tools := args.tools
62 if tools == nil {
63 var err error
64 tools, err = findMachineAgentTools(args.env, args.series, args.arch)
65 if err != nil {
66 return "", err
67 }
68 }
69
70 // We generate a cloud-init config, which we'll then pull the runcmds67 // We generate a cloud-init config, which we'll then pull the runcmds
71 // and prerequisite packages out of. Rather than generating a cloud-config,68 // and prerequisite packages out of. Rather than generating a cloud-config,
72 // we'll just generate a shell script.69 // we'll just generate a shell script.
73 mcfg := environs.NewMachineConfig(args.machine.Id(), args.nonce, args.stateInfo, args.apiInfo)70 var mcfg *cloudinit.MachineConfig
71 if args.bootstrap {
72 mcfg = environs.NewBootstrapMachineConfig(args.machineId, args.nonce)
73 } else {
74 mcfg = environs.NewMachineConfig(args.machineId, args.nonce, args.stateInfo, args.apiInfo)
75 }
74 if args.dataDir != "" {76 if args.dataDir != "" {
75 mcfg.DataDir = args.dataDir77 mcfg.DataDir = args.dataDir
76 }78 }
77 mcfg.Tools = tools79 mcfg.Tools = args.tools
78 err := environs.FinishMachineConfig(mcfg, args.env.Config(), constraints.Value{})80 err := environs.FinishMachineConfig(mcfg, args.environConfig, constraints.Value{})
79 if err != nil {81 if err != nil {
80 return "", err82 return "", err
81 }83 }
82 mcfg.AgentEnvironment[agent.ProviderType] = provider.Null84 mcfg.AgentEnvironment[agent.ProviderType] = provider.Null
85 for k, v := range args.agentEnv {
86 mcfg.AgentEnvironment[k] = v
87 }
83 cloudcfg := corecloudinit.New()88 cloudcfg := corecloudinit.New()
84 if cloudcfg, err = cloudinit.Configure(mcfg, cloudcfg); err != nil {89 if cloudcfg, err = cloudinit.Configure(mcfg, cloudcfg); err != nil {
85 return "", err90 return "", err
@@ -116,20 +121,3 @@
116 script = append(head, tail...)121 script = append(head, tail...)
117 return strings.Join(script, "\n"), nil122 return strings.Join(script, "\n"), nil
118}123}
119
120func findMachineAgentTools(env environs.Environ, series, arch string) (*tools.Tools, error) {
121 agentVersion, ok := env.Config().AgentVersion()
122 if !ok {
123 return nil, fmt.Errorf("no agent version set in environment configuration")
124 }
125 possibleTools, err := envtools.FindInstanceTools(env, agentVersion, series, &arch)
126 if err != nil {
127 return nil, err
128 }
129 arches := possibleTools.Arches()
130 possibleTools, err = possibleTools.Match(tools.Filter{Arch: arch})
131 if err != nil {
132 return nil, fmt.Errorf("chosen architecture %v not present in %v", arch, arches)
133 }
134 return possibleTools[0], nil
135}
136124
=== added file 'environs/manual/bootstrap.go'
--- environs/manual/bootstrap.go 1970-01-01 00:00:00 +0000
+++ environs/manual/bootstrap.go 2013-09-20 05:05:14 +0000
@@ -0,0 +1,135 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package manual
5
6import (
7 "errors"
8 "fmt"
9
10 "launchpad.net/juju-core/agent"
11 "launchpad.net/juju-core/environs"
12 envtools "launchpad.net/juju-core/environs/tools"
13 "launchpad.net/juju-core/instance"
14 "launchpad.net/juju-core/names"
15 "launchpad.net/juju-core/provider"
16 "launchpad.net/juju-core/state"
17 "launchpad.net/juju-core/tools"
18 "launchpad.net/juju-core/worker/localstorage"
19)
20
21const BootstrapInstanceId = instance.Id(manualInstancePrefix)
22
23// LocalStorageEnviron is an Environ where the bootstrap node
24// manages its own local storage.
25type LocalStorageEnviron interface {
26 environs.Environ
27 environs.BootstrapStorager
28 localstorage.LocalStorageConfig
29}
30
31type BootstrapArgs struct {
32 Host string
33 Environ LocalStorageEnviron
34 MachineId string
35 PossibleTools tools.List
36}
37
38// TODO(axw) make this configurable?
39const dataDir = "/var/lib/juju"
40
41func errMachineIdInvalid(machineId string) error {
42 return fmt.Errorf("%q is not a valid machine ID", machineId)
43}
44
45// NewManualBootstrapEnviron wraps a LocalStorageEnviron with another which
46// overrides the Bootstrap method; when Bootstrap is invoked, the specified
47// host will be manually bootstrapped.
48func Bootstrap(args BootstrapArgs) (err error) {
49 if args.Host == "" {
50 return errors.New("host argument is empty")
51 }
52 if args.Environ == nil {
53 return errors.New("environ argument is nil")
54 }
55 if !names.IsMachine(args.MachineId) {
56 return errMachineIdInvalid(args.MachineId)
57 }
58
59 provisioned, err := checkProvisioned(args.Host)
60 if err != nil {
61 return fmt.Errorf("failed to check provisioned status: %v", err)
62 }
63 if provisioned {
64 return ErrProvisioned
65 }
66
67 bootstrapStorage, err := args.Environ.BootstrapStorage()
68 if err != nil {
69 return err
70 }
71
72 hc, series, err := detectSeriesAndHardwareCharacteristics(args.Host)
73 if err != nil {
74 return fmt.Errorf("error detecting hardware characteristics: %v", err)
75 }
76
77 // Filter tools based on detected series/arch.
78 logger.Infof("Filtering possible tools: %v", args.PossibleTools)
79 possibleTools, err := args.PossibleTools.Match(tools.Filter{
80 Arch: *hc.Arch,
81 Series: series,
82 })
83 if err != nil {
84 return err
85 }
86
87 // Store the state file. If provisioning fails, we'll remove the file.
88 logger.Infof("Saving bootstrap state file to bootstrap storage")
89 err = provider.SaveState(
90 bootstrapStorage,
91 &provider.BootstrapState{
92 StateInstances: []instance.Id{BootstrapInstanceId},
93 Characteristics: []instance.HardwareCharacteristics{hc},
94 },
95 )
96 if err != nil {
97 return err
98 }
99 defer func() {
100 if err != nil {
101 logger.Errorf("bootstrapping failed, removing state file: %v", err)
102 bootstrapStorage.Remove(provider.StateFile)
103 }
104 }()
105
106 // Get a file:// scheme tools URL for the tools, which will have been
107 // copied to the remote machine's storage directory.
108 tools := *possibleTools[0]
109 storageDir := args.Environ.StorageDir()
110 toolsStorageName := envtools.StorageName(tools.Version)
111 tools.URL = fmt.Sprintf("file://%s/%s", storageDir, toolsStorageName)
112
113 // Add the local storage configuration.
114 agentEnv := map[string]string{
115 agent.StorageAddr: args.Environ.StorageAddr(),
116 agent.StorageDir: storageDir,
117 agent.SharedStorageAddr: args.Environ.SharedStorageAddr(),
118 agent.SharedStorageDir: args.Environ.SharedStorageDir(),
119 }
120
121 // Finally, provision the machine agent.
122 stateFileURL := fmt.Sprintf("file://%s/%s", storageDir, provider.StateFile)
123 err = provisionMachineAgent(provisionMachineAgentArgs{
124 host: args.Host,
125 dataDir: dataDir,
126 environConfig: args.Environ.Config(),
127 stateFileURL: stateFileURL,
128 machineId: args.MachineId,
129 bootstrap: true,
130 nonce: state.BootstrapNonce,
131 tools: &tools,
132 agentEnv: agentEnv,
133 })
134 return err
135}
0136
=== added file 'environs/manual/bootstrap_test.go'
--- environs/manual/bootstrap_test.go 1970-01-01 00:00:00 +0000
+++ environs/manual/bootstrap_test.go 2013-09-20 05:05:14 +0000
@@ -0,0 +1,164 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package manual
5
6import (
7 "os"
8
9 gc "launchpad.net/gocheck"
10
11 "launchpad.net/juju-core/environs"
12 "launchpad.net/juju-core/environs/filestorage"
13 "launchpad.net/juju-core/environs/storage"
14 "launchpad.net/juju-core/environs/tools"
15 coreerrors "launchpad.net/juju-core/errors"
16 "launchpad.net/juju-core/instance"
17 "launchpad.net/juju-core/juju/testing"
18 "launchpad.net/juju-core/provider"
19 jc "launchpad.net/juju-core/testing/checkers"
20)
21
22type bootstrapSuite struct {
23 testing.JujuConnSuite
24 env *localStorageEnviron
25}
26
27var _ = gc.Suite(&bootstrapSuite{})
28
29type localStorageEnviron struct {
30 environs.Environ
31 storage storage.Storage
32 storageAddr string
33 storageDir string
34 sharedStorageAddr string
35 sharedStorageDir string
36}
37
38func (e *localStorageEnviron) BootstrapStorage() (storage.Storage, error) {
39 return e.storage, nil
40}
41
42func (e *localStorageEnviron) StorageAddr() string {
43 return e.storageAddr
44}
45
46func (e *localStorageEnviron) StorageDir() string {
47 return e.storageDir
48}
49
50func (e *localStorageEnviron) SharedStorageAddr() string {
51 return e.sharedStorageAddr
52}
53
54func (e *localStorageEnviron) SharedStorageDir() string {
55 return e.sharedStorageDir
56}
57
58func (s *bootstrapSuite) SetUpTest(c *gc.C) {
59 s.JujuConnSuite.SetUpTest(c)
60 s.env = &localStorageEnviron{
61 Environ: s.Conn.Environ,
62 storageDir: c.MkDir(),
63 }
64 storage, err := filestorage.NewFileStorageWriter(s.env.storageDir, filestorage.UseDefaultTmpDir)
65 c.Assert(err, gc.IsNil)
66 s.env.storage = storage
67}
68
69func (s *bootstrapSuite) getArgs(c *gc.C) BootstrapArgs {
70 hostname, err := os.Hostname()
71 c.Assert(err, gc.IsNil)
72 toolsList, err := tools.FindBootstrapTools(s.Conn.Environ, tools.BootstrapToolsParams{})
73 c.Assert(err, gc.IsNil)
74 return BootstrapArgs{
75 Host: hostname,
76 Environ: s.env,
77 MachineId: "0",
78 PossibleTools: toolsList,
79 }
80}
81
82func (s *bootstrapSuite) TestBootstrap(c *gc.C) {
83 args := s.getArgs(c)
84 args.Host = "ubuntu@" + args.Host
85
86 defer fakeSSH{series: s.Conn.Environ.Config().DefaultSeries()}.install(c).Restore()
87 err := Bootstrap(args)
88 c.Assert(err, gc.IsNil)
89
90 bootstrapState, err := provider.LoadState(s.env.storage)
91 c.Assert(err, gc.IsNil)
92 c.Assert(
93 bootstrapState.StateInstances,
94 gc.DeepEquals,
95 []instance.Id{BootstrapInstanceId},
96 )
97
98 // Do it all again; this should work, despite the fact that
99 // there's a bootstrap state file. Existence for that is
100 // checked in general bootstrap code (environs/bootstrap).
101 defer fakeSSH{series: s.Conn.Environ.Config().DefaultSeries()}.install(c).Restore()
102 err = Bootstrap(args)
103 c.Assert(err, gc.IsNil)
104
105 // We *do* check that the machine has no juju* upstart jobs, though.
106 defer installFakeSSH(c, "", "/etc/init/jujud-machine-0.conf", 0).Restore()
107 err = Bootstrap(args)
108 c.Assert(err, gc.Equals, ErrProvisioned)
109}
110
111func (s *bootstrapSuite) TestBootstrapScriptFailure(c *gc.C) {
112 args := s.getArgs(c)
113 args.Host = "ubuntu@" + args.Host
114 series := s.Conn.Environ.Config().DefaultSeries()
115 defer fakeSSH{series: series, provisionAgentExitCode: 1}.install(c).Restore()
116 err := Bootstrap(args)
117 c.Assert(err, gc.NotNil)
118
119 // Since the script failed, the state file should have been
120 // removed from storage.
121 _, err = provider.LoadState(s.env.storage)
122 c.Assert(err, jc.Satisfies, coreerrors.IsNotBootstrapped)
123}
124
125func (s *bootstrapSuite) TestBootstrapEmptyHost(c *gc.C) {
126 args := s.getArgs(c)
127 args.Host = ""
128 c.Assert(Bootstrap(args), gc.ErrorMatches, "host argument is empty")
129}
130
131func (s *bootstrapSuite) TestBootstrapNilEnviron(c *gc.C) {
132 args := s.getArgs(c)
133 args.Environ = nil
134 c.Assert(Bootstrap(args), gc.ErrorMatches, "environ argument is nil")
135}
136
137func (s *bootstrapSuite) TestBootstrapInvalidMachineId(c *gc.C) {
138 args := s.getArgs(c)
139 args.MachineId = ""
140 c.Assert(Bootstrap(args), gc.ErrorMatches, `"" is not a valid machine ID`)
141 args.MachineId = "bahhumbug"
142 c.Assert(Bootstrap(args), gc.ErrorMatches, `"bahhumbug" is not a valid machine ID`)
143}
144
145func (s *bootstrapSuite) TestBootstrapAlternativeMachineId(c *gc.C) {
146 args := s.getArgs(c)
147 args.MachineId = "1"
148 defer fakeSSH{series: s.Conn.Environ.Config().DefaultSeries()}.install(c).Restore()
149 c.Assert(Bootstrap(args), gc.IsNil)
150}
151
152func (s *bootstrapSuite) TestBootstrapNoMatchingTools(c *gc.C) {
153 // Empty tools list.
154 args := s.getArgs(c)
155 args.PossibleTools = nil
156 series := s.Conn.Environ.Config().DefaultSeries()
157 defer fakeSSH{series: series, skipProvisionAgent: true}.install(c).Restore()
158 c.Assert(Bootstrap(args), gc.ErrorMatches, "no matching tools available")
159
160 // Non-empty list, but none that match the series/arch.
161 defer fakeSSH{series: "edgy", skipProvisionAgent: true}.install(c).Restore()
162 args = s.getArgs(c)
163 c.Assert(Bootstrap(args), gc.ErrorMatches, "no matching tools available")
164}
0165
=== modified file 'environs/manual/detection.go'
--- environs/manual/detection.go 2013-09-12 02:04:05 +0000
+++ environs/manual/detection.go 2013-09-20 05:05:14 +0000
@@ -11,6 +11,7 @@
11 "strings"11 "strings"
1212
13 "launchpad.net/juju-core/instance"13 "launchpad.net/juju-core/instance"
14 "launchpad.net/juju-core/utils"
14)15)
1516
16// checkProvisionedScript is the script to run on the remote machine17// checkProvisionedScript is the script to run on the remote machine
@@ -23,9 +24,9 @@
23// checkProvisioned checks if any juju upstart jobs already24// checkProvisioned checks if any juju upstart jobs already
24// exist on the host machine.25// exist on the host machine.
25func checkProvisioned(sshHost string) (bool, error) {26func checkProvisioned(sshHost string) (bool, error) {
26 cmd := sshCommand(sshHost, "bash")27 logger.Infof("Checking if %s is already provisioned", sshHost)
28 cmd := sshCommand(sshHost, fmt.Sprintf("bash -c %s", utils.ShQuote(checkProvisionedScript)))
27 var stdout, stderr bytes.Buffer29 var stdout, stderr bytes.Buffer
28 cmd.Stdin = bytes.NewBufferString(checkProvisionedScript)
29 cmd.Stdout = &stdout30 cmd.Stdout = &stdout
30 cmd.Stderr = &stderr31 cmd.Stderr = &stderr
31 if err := cmd.Run(); err != nil {32 if err := cmd.Run(); err != nil {
@@ -34,13 +35,21 @@
34 }35 }
35 return false, err36 return false, err
36 }37 }
37 return len(strings.TrimSpace(stdout.String())) > 0, nil38 output := strings.TrimSpace(stdout.String())
39 provisioned := len(output) > 0
40 if provisioned {
41 logger.Infof("%s is already provisioned [%q]", sshHost, output)
42 } else {
43 logger.Infof("%s is not provisioned", sshHost)
44 }
45 return provisioned, nil
38}46}
3947
40// detectSeriesHardwareCharacteristics detects the OS series48// detectSeriesHardwareCharacteristics detects the OS series
41// and hardware characteristics of the remote machine by49// and hardware characteristics of the remote machine by
42// connecting to the machine and executing a bash script.50// connecting to the machine and executing a bash script.
43func detectSeriesAndHardwareCharacteristics(sshHost string) (hc instance.HardwareCharacteristics, series string, err error) {51func detectSeriesAndHardwareCharacteristics(sshHost string) (hc instance.HardwareCharacteristics, series string, err error) {
52 logger.Infof("Detecting series and characteristics on %s", sshHost)
44 cmd := sshCommand(sshHost, "bash")53 cmd := sshCommand(sshHost, "bash")
45 cmd.Stdin = bytes.NewBufferString(detectionScript)54 cmd.Stdin = bytes.NewBufferString(detectionScript)
46 out, err := cmd.CombinedOutput()55 out, err := cmd.CombinedOutput()
@@ -101,6 +110,7 @@
101 }110 }
102111
103 // TODO(axw) calculate CpuPower. What algorithm do we use?112 // TODO(axw) calculate CpuPower. What algorithm do we use?
113 logger.Infof("series: %s, characteristics: %s", series, hc)
104 return hc, series, nil114 return hc, series, nil
105}115}
106116
@@ -116,6 +126,7 @@
116}126}
117127
118const detectionScript = `#!/bin/bash128const detectionScript = `#!/bin/bash
129set -e
119lsb_release -cs130lsb_release -cs
120uname -m131uname -m
121grep MemTotal /proc/meminfo132grep MemTotal /proc/meminfo
122133
=== modified file 'environs/manual/detection_test.go'
--- environs/manual/detection_test.go 2013-09-20 02:53:59 +0000
+++ environs/manual/detection_test.go 2013-09-20 05:05:14 +0000
@@ -4,10 +4,6 @@
4package manual4package manual
55
6import (6import (
7 "fmt"
8 "io/ioutil"
9 "os"
10 "path/filepath"
11 "strings"7 "strings"
128
13 gc "launchpad.net/gocheck"9 gc "launchpad.net/gocheck"
@@ -22,58 +18,6 @@
2218
23var _ = gc.Suite(&detectionSuite{})19var _ = gc.Suite(&detectionSuite{})
2420
25// sshscript should only print the result on the first execution,
26// to handle the case where it's called multiple times. On
27// subsequent executions, it should find the next 'ssh' in $PATH
28// and exec that.
29var sshscript = `#!/bin/bash
30if [ ! -e "$0.run" ]; then
31 touch "$0.run"
32 diff "$0.expected-input" -
33 exitcode=$?
34 if [ $exitcode -ne 0 ]; then
35 echo "ERROR: did not match expected input" >&2
36 exit $exitcode
37 fi
38 # stdout
39 %s
40 # stderr
41 %s
42 exit %d
43else
44 export PATH=${PATH#*:}
45 exec ssh $*
46fi`
47
48// sshresponse creates a fake "ssh" command in a new $PATH,
49// updates $PATH, and returns a function to reset $PATH to
50// its original value when called.
51//
52// output may be:
53// - nil (no output)
54// - a string (stdout)
55// - a slice of strings, of length two (stdout, stderr)
56func sshresponse(c *gc.C, input string, output interface{}, rc int) func() {
57 fakebin := c.MkDir()
58 ssh := filepath.Join(fakebin, "ssh")
59 sshexpectedinput := ssh + ".expected-input"
60 var stdout, stderr string
61 switch output := output.(type) {
62 case nil:
63 case string:
64 stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output)
65 case []string:
66 stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output[0])
67 stderr = fmt.Sprintf("cat>&2<<EOF\n%s\nEOF", output[1])
68 }
69 script := fmt.Sprintf(sshscript, stdout, stderr, rc)
70 err := ioutil.WriteFile(ssh, []byte(script), 0777)
71 c.Assert(err, gc.IsNil)
72 err = ioutil.WriteFile(sshexpectedinput, []byte(input), 0644)
73 c.Assert(err, gc.IsNil)
74 return testbase.PatchEnvironment("PATH", fakebin+":"+os.Getenv("PATH"))
75}
76
77func (s *detectionSuite) TestDetectSeries(c *gc.C) {21func (s *detectionSuite) TestDetectSeries(c *gc.C) {
78 response := strings.Join([]string{22 response := strings.Join([]string{
79 "edgy",23 "edgy",
@@ -81,14 +25,14 @@
81 "MemTotal: 4096 kB",25 "MemTotal: 4096 kB",
82 "processor: 0",26 "processor: 0",
83 }, "\n")27 }, "\n")
84 defer sshresponse(c, detectionScript, response, 0)()28 defer installFakeSSH(c, detectionScript, response, 0)()
85 _, series, err := detectSeriesAndHardwareCharacteristics("whatever")29 _, series, err := detectSeriesAndHardwareCharacteristics("whatever")
86 c.Assert(err, gc.IsNil)30 c.Assert(err, gc.IsNil)
87 c.Assert(series, gc.Equals, "edgy")31 c.Assert(series, gc.Equals, "edgy")
88}32}
8933
90func (s *detectionSuite) TestDetectionError(c *gc.C) {34func (s *detectionSuite) TestDetectionError(c *gc.C) {
91 defer sshresponse(c, detectionScript, "oh noes", 33)()35 defer installFakeSSH(c, detectionScript, "oh noes", 33)()
92 _, _, err := detectSeriesAndHardwareCharacteristics("whatever")36 _, _, err := detectSeriesAndHardwareCharacteristics("whatever")
93 c.Assert(err, gc.ErrorMatches, "exit status 33 \\(oh noes\\)")37 c.Assert(err, gc.ErrorMatches, "exit status 33 \\(oh noes\\)")
94}38}
@@ -148,7 +92,7 @@
148 for i, test := range tests {92 for i, test := range tests {
149 c.Logf("test %d: %s", i, test.summary)93 c.Logf("test %d: %s", i, test.summary)
150 scriptResponse := strings.Join(test.scriptResponse, "\n")94 scriptResponse := strings.Join(test.scriptResponse, "\n")
151 defer sshresponse(c, detectionScript, scriptResponse, 0)()95 defer installFakeSSH(c, detectionScript, scriptResponse, 0)()
152 hc, _, err := detectSeriesAndHardwareCharacteristics("hostname")96 hc, _, err := detectSeriesAndHardwareCharacteristics("hostname")
153 c.Assert(err, gc.IsNil)97 c.Assert(err, gc.IsNil)
154 c.Assert(hc.String(), gc.Equals, test.expectedHc)98 c.Assert(hc.String(), gc.Equals, test.expectedHc)
@@ -156,25 +100,25 @@
156}100}
157101
158func (s *detectionSuite) TestCheckProvisioned(c *gc.C) {102func (s *detectionSuite) TestCheckProvisioned(c *gc.C) {
159 defer sshresponse(c, checkProvisionedScript, "", 0)()103 defer installFakeSSH(c, "", "", 0)()
160 provisioned, err := checkProvisioned("example.com")104 provisioned, err := checkProvisioned("example.com")
161 c.Assert(err, gc.IsNil)105 c.Assert(err, gc.IsNil)
162 c.Assert(provisioned, jc.IsFalse)106 c.Assert(provisioned, jc.IsFalse)
163107
164 defer sshresponse(c, checkProvisionedScript, "non-empty", 0)()108 defer installFakeSSH(c, "", "non-empty", 0)()
165 provisioned, err = checkProvisioned("example.com")109 provisioned, err = checkProvisioned("example.com")
166 c.Assert(err, gc.IsNil)110 c.Assert(err, gc.IsNil)
167 c.Assert(provisioned, jc.IsTrue)111 c.Assert(provisioned, jc.IsTrue)
168112
169 // stderr should not affect result.113 // stderr should not affect result.
170 defer sshresponse(c, checkProvisionedScript, []string{"", "non-empty-stderr"}, 0)()114 defer installFakeSSH(c, "", []string{"", "non-empty-stderr"}, 0)()
171 provisioned, err = checkProvisioned("example.com")115 provisioned, err = checkProvisioned("example.com")
172 c.Assert(err, gc.IsNil)116 c.Assert(err, gc.IsNil)
173 c.Assert(provisioned, jc.IsFalse)117 c.Assert(provisioned, jc.IsFalse)
174118
175 // if the script fails for whatever reason, then checkProvisioned119 // if the script fails for whatever reason, then checkProvisioned
176 // will return an error. stderr will be included in the error message.120 // will return an error. stderr will be included in the error message.
177 defer sshresponse(c, checkProvisionedScript, []string{"non-empty-stdout", "non-empty-stderr"}, 255)()121 defer installFakeSSH(c, "", []string{"non-empty-stdout", "non-empty-stderr"}, 255)()
178 _, err = checkProvisioned("example.com")122 _, err = checkProvisioned("example.com")
179 c.Assert(err, gc.ErrorMatches, "exit status 255 \\(non-empty-stderr\\)")123 c.Assert(err, gc.ErrorMatches, "exit status 255 \\(non-empty-stderr\\)")
180}124}
181125
=== added file 'environs/manual/fakessh_test.go'
--- environs/manual/fakessh_test.go 1970-01-01 00:00:00 +0000
+++ environs/manual/fakessh_test.go 2013-09-20 05:05:14 +0000
@@ -0,0 +1,112 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package manual
5
6import (
7 "fmt"
8 "io/ioutil"
9 "os"
10 "path/filepath"
11 "strings"
12
13 gc "launchpad.net/gocheck"
14
15 "launchpad.net/juju-core/testing/testbase"
16)
17
18// sshscript should only print the result on the first execution,
19// to handle the case where it's called multiple times. On
20// subsequent executions, it should find the next 'ssh' in $PATH
21// and exec that.
22var sshscript = `#!/bin/bash
23if [ ! -e "$0.run" ]; then
24 touch "$0.run"
25 diff "$0.expected-input" -
26 exitcode=$?
27 if [ $exitcode -ne 0 ]; then
28 echo "ERROR: did not match expected input" >&2
29 exit $exitcode
30 fi
31 # stdout
32 %s
33 # stderr
34 %s
35 exit %d
36else
37 export PATH=${PATH#*:}
38 exec ssh $*
39fi`
40
41// installFakeSSH creates a fake "ssh" command in a new $PATH,
42// updates $PATH, and returns a function to reset $PATH to
43// its original value when called.
44//
45// output may be:
46// - nil (no output)
47// - a string (stdout)
48// - a slice of strings, of length two (stdout, stderr)
49func installFakeSSH(c *gc.C, input string, output interface{}, rc int) testbase.Restorer {
50 fakebin := c.MkDir()
51 ssh := filepath.Join(fakebin, "ssh")
52 sshexpectedinput := ssh + ".expected-input"
53 var stdout, stderr string
54 switch output := output.(type) {
55 case nil:
56 case string:
57 stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output)
58 case []string:
59 stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output[0])
60 stderr = fmt.Sprintf("cat>&2<<EOF\n%s\nEOF", output[1])
61 }
62 script := fmt.Sprintf(sshscript, stdout, stderr, rc)
63 err := ioutil.WriteFile(ssh, []byte(script), 0777)
64 c.Assert(err, gc.IsNil)
65 err = ioutil.WriteFile(sshexpectedinput, []byte(input), 0644)
66 c.Assert(err, gc.IsNil)
67 return testbase.PatchEnvironment("PATH", fakebin+":"+os.Getenv("PATH"))
68}
69
70// fakeSSH wraps the invocation of installFakeSSH based on the parameters.
71type fakeSSH struct {
72 series string
73 arch string
74
75 // exit code for the machine agent provisioning script.
76 provisionAgentExitCode int
77
78 // there are conditions other than error in the above
79 // that might cause provisioning to not go ahead, such
80 // as tools being missing.
81 skipProvisionAgent bool
82}
83
84// install installs fake SSH commands, which will respond to
85// manual provisioning/bootstrapping commands with the specified
86// output and exit codes.
87func (r fakeSSH) install(c *gc.C) testbase.Restorer {
88 series := r.series
89 if series == "" {
90 series = "precise"
91 }
92 arch := r.arch
93 if arch == "" {
94 arch = "amd64"
95 }
96 detectionoutput := strings.Join([]string{
97 series,
98 arch,
99 "MemTotal: 4096 kB",
100 "processor: 0",
101 }, "\n")
102 var restore testbase.Restorer
103 add := func(input string, output interface{}, rc int) {
104 restore = restore.Add(installFakeSSH(c, input, output, rc))
105 }
106 if !r.skipProvisionAgent {
107 add("", nil, r.provisionAgentExitCode)
108 }
109 add(detectionScript, detectionoutput, 0)
110 add("", nil, 0) // checkProvisioned
111 return restore
112}
0113
=== modified file 'environs/manual/provisioner.go'
--- environs/manual/provisioner.go 2013-09-18 04:43:04 +0000
+++ environs/manual/provisioner.go 2013-09-20 05:05:14 +0000
@@ -104,6 +104,14 @@
104 return nil, err104 return nil, err
105 }105 }
106106
107 tools := args.Tools
108 if tools == nil {
109 tools, err = findInstanceTools(env, series, *hc.Arch)
110 if err != nil {
111 return nil, err
112 }
113 }
114
107 // Generate a unique nonce for the machine.115 // Generate a unique nonce for the machine.
108 uuid, err := utils.NewUUID()116 uuid, err := utils.NewUUID()
109 if err != nil {117 if err != nil {
@@ -137,16 +145,15 @@
137145
138 // Finally, provision the machine agent.146 // Finally, provision the machine agent.
139 err = provisionMachineAgent(provisionMachineAgentArgs{147 err = provisionMachineAgent(provisionMachineAgentArgs{
140 host: args.Host,148 host: args.Host,
141 dataDir: args.DataDir,149 dataDir: args.DataDir,
142 env: env,150 environConfig: env.Config(),
143 machine: m,151 machineId: m.Id(),
144 nonce: nonce,152 bootstrap: false,
145 stateInfo: stateInfo,153 nonce: nonce,
146 apiInfo: apiInfo,154 stateInfo: stateInfo,
147 series: series,155 apiInfo: apiInfo,
148 arch: *hc.Arch,156 tools: tools,
149 tools: args.Tools,
150 })157 })
151 if err != nil {158 if err != nil {
152 return m, err159 return m, err
153160
=== modified file 'environs/manual/provisioner_test.go'
--- environs/manual/provisioner_test.go 2013-09-20 00:33:38 +0000
+++ environs/manual/provisioner_test.go 2013-09-20 05:05:14 +0000
@@ -6,13 +6,15 @@
6import (6import (
7 "fmt"7 "fmt"
8 "os"8 "os"
9 "strings"
109
11 gc "launchpad.net/gocheck"10 gc "launchpad.net/gocheck"
1211
13 "launchpad.net/juju-core/environs/tools"12 "launchpad.net/juju-core/environs/storage"
13 envtesting "launchpad.net/juju-core/environs/testing"
14 "launchpad.net/juju-core/instance"14 "launchpad.net/juju-core/instance"
15 "launchpad.net/juju-core/juju/testing"15 "launchpad.net/juju-core/juju/testing"
16 jc "launchpad.net/juju-core/testing/checkers"
17 "launchpad.net/juju-core/version"
16)18)
1719
18type provisionerSuite struct {20type provisionerSuite struct {
@@ -31,32 +33,35 @@
31}33}
3234
33func (s *provisionerSuite) TestProvisionMachine(c *gc.C) {35func (s *provisionerSuite) TestProvisionMachine(c *gc.C) {
34 // Prepare a mock ssh response for the detection phase.36 const series = "precise"
35 detectionoutput := strings.Join([]string{37 const arch = "amd64"
36 "edgy",
37 "armv4",
38 "MemTotal: 4096 kB",
39 "processor: 0",
40 }, "\n")
4138
42 args := s.getArgs(c)39 args := s.getArgs(c)
43 hostname := args.Host40 hostname := args.Host
44 args.Host = "ubuntu@" + args.Host41 args.Host = "ubuntu@" + args.Host
4542
46 defer sshresponse(c, detectionScript, detectionoutput, 0)()43 envtesting.RemoveTools(c, s.Conn.Environ.Storage())
47 defer sshresponse(c, checkProvisionedScript, "", 0)()44 envtesting.RemoveTools(c, s.Conn.Environ.PublicStorage().(storage.Storage))
45 defer fakeSSH{
46 series: series, arch: arch, skipProvisionAgent: true,
47 }.install(c).Restore()
48 m, err := ProvisionMachine(args)48 m, err := ProvisionMachine(args)
49 c.Assert(err, gc.ErrorMatches, "no matching tools available")49 c.Assert(err, gc.ErrorMatches, "no tools available")
50 c.Assert(m, gc.IsNil)50 c.Assert(m, gc.IsNil)
5151
52 toolsList, err := tools.FindBootstrapTools(s.Conn.Environ, tools.BootstrapToolsParams{})52 cfg := s.Conn.Environ.Config()
53 c.Assert(err, gc.IsNil)53 number, ok := cfg.AgentVersion()
54 args.Tools = toolsList[0]54 c.Assert(ok, jc.IsTrue)
55 binVersion := version.Binary{number, series, arch}
56 envtesting.UploadFakeToolsVersion(c, s.Conn.Environ.Storage(), binVersion)
5557
56 for _, errorCode := range []int{255, 0} {58 for i, errorCode := range []int{255, 0} {
57 defer sshresponse(c, "", "", errorCode)() // executing script59 c.Logf("test %d: code %d", i, errorCode)
58 defer sshresponse(c, detectionScript, detectionoutput, 0)()60 defer fakeSSH{
59 defer sshresponse(c, checkProvisionedScript, "", 0)()61 series: series,
62 arch: arch,
63 provisionAgentExitCode: errorCode,
64 }.install(c).Restore()
60 m, err = ProvisionMachine(args)65 m, err = ProvisionMachine(args)
61 if errorCode != 0 {66 if errorCode != 0 {
62 c.Assert(err, gc.ErrorMatches, fmt.Sprintf("exit status %d", errorCode))67 c.Assert(err, gc.ErrorMatches, fmt.Sprintf("exit status %d", errorCode))
@@ -64,9 +69,9 @@
64 } else {69 } else {
65 c.Assert(err, gc.IsNil)70 c.Assert(err, gc.IsNil)
66 c.Assert(m, gc.NotNil)71 c.Assert(m, gc.NotNil)
67 // machine ID will be 2, not 1. Even though we failed and the72 // machine ID will be incremented. Even though we failed and the
68 // machine is removed, the ID is not reused.73 // machine is removed, the ID is not reused.
69 c.Assert(m.Id(), gc.Equals, "2")74 c.Assert(m.Id(), gc.Equals, fmt.Sprint(i))
70 instanceId, err := m.InstanceId()75 instanceId, err := m.InstanceId()
71 c.Assert(err, gc.IsNil)76 c.Assert(err, gc.IsNil)
72 c.Assert(instanceId, gc.Equals, instance.Id("manual:"+hostname))77 c.Assert(instanceId, gc.Equals, instance.Id("manual:"+hostname))
@@ -75,10 +80,10 @@
7580
76 // Attempting to provision a machine twice should fail. We effect81 // Attempting to provision a machine twice should fail. We effect
77 // this by checking for existing juju upstart configurations.82 // this by checking for existing juju upstart configurations.
78 defer sshresponse(c, checkProvisionedScript, "/etc/init/jujud-machine-0.conf", 0)()83 defer installFakeSSH(c, "", "/etc/init/jujud-machine-0.conf", 0)()
79 _, err = ProvisionMachine(args)84 _, err = ProvisionMachine(args)
80 c.Assert(err, gc.Equals, ErrProvisioned)85 c.Assert(err, gc.Equals, ErrProvisioned)
81 defer sshresponse(c, checkProvisionedScript, "/etc/init/jujud-machine-0.conf", 255)()86 defer installFakeSSH(c, "", "/etc/init/jujud-machine-0.conf", 255)()
82 _, err = ProvisionMachine(args)87 _, err = ProvisionMachine(args)
83 c.Assert(err, gc.ErrorMatches, "error checking if provisioned: exit status 255")88 c.Assert(err, gc.ErrorMatches, "error checking if provisioned: exit status 255")
84}89}
8590
=== added file 'environs/manual/tools.go'
--- environs/manual/tools.go 1970-01-01 00:00:00 +0000
+++ environs/manual/tools.go 2013-09-20 05:05:14 +0000
@@ -0,0 +1,24 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package manual
5
6import (
7 "fmt"
8
9 "launchpad.net/juju-core/environs"
10 envtools "launchpad.net/juju-core/environs/tools"
11 "launchpad.net/juju-core/tools"
12)
13
14func findInstanceTools(env environs.Environ, series, arch string) (*tools.Tools, error) {
15 agentVersion, ok := env.Config().AgentVersion()
16 if !ok {
17 return nil, fmt.Errorf("no agent version set in environment configuration")
18 }
19 possibleTools, err := envtools.FindInstanceTools(env, agentVersion, series, &arch)
20 if err != nil {
21 return nil, err
22 }
23 return possibleTools[0], nil
24}
025
=== modified file 'testing/testbase/patch.go'
--- testing/testbase/patch.go 2013-09-20 02:10:46 +0000
+++ testing/testbase/patch.go 2013-09-20 05:05:14 +0000
@@ -12,6 +12,18 @@
12// to restore some previous state.12// to restore some previous state.
13type Restorer func()13type Restorer func()
1414
15// Add returns a Restorer that restores first f1
16// and then f. It is valid to call this on a nil
17// Restorer.
18func (f Restorer) Add(f1 Restorer) Restorer {
19 return func() {
20 f1.Restore()
21 if f != nil {
22 f.Restore()
23 }
24 }
25}
26
15// Restore restores some previous state.27// Restore restores some previous state.
16func (r Restorer) Restore() {28func (r Restorer) Restore() {
17 r()29 r()
1830
=== modified file 'testing/testbase/patch_test.go'
--- testing/testbase/patch_test.go 2013-09-20 02:10:46 +0000
+++ testing/testbase/patch_test.go 2013-09-20 05:05:14 +0000
@@ -74,3 +74,12 @@
74 c.Check(os.Getenv(envName), gc.Equals, "initial")74 c.Check(os.Getenv(envName), gc.Equals, "initial")
75 os.Setenv(envName, oldValue)75 os.Setenv(envName, oldValue)
76}76}
77
78func (*PatchEnvironmentSuite) TestRestorerAdd(c *gc.C) {
79 var order []string
80 first := testbase.Restorer(func() { order = append(order, "first") })
81 second := testbase.Restorer(func() { order = append(order, "second") })
82 restore := first.Add(second)
83 restore()
84 c.Assert(order, gc.DeepEquals, []string{"second", "first"})
85}
7786
=== renamed directory 'provider/local/storage' => 'worker/localstorage'
=== added file 'worker/localstorage/config.go'
--- worker/localstorage/config.go 1970-01-01 00:00:00 +0000
+++ worker/localstorage/config.go 2013-09-20 05:05:14 +0000
@@ -0,0 +1,14 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package localstorage
5
6// LocalStorageConfig is an interface that, if implemented, may be used
7// to configure a machine agent for use with the localstorage worker in
8// this package.
9type LocalStorageConfig interface {
10 StorageDir() string
11 StorageAddr() string
12 SharedStorageDir() string
13 SharedStorageAddr() string
14}
015
=== modified file 'worker/localstorage/worker.go'
--- provider/local/storage/worker.go 2013-09-19 03:14:56 +0000
+++ worker/localstorage/worker.go 2013-09-20 05:05:14 +0000
@@ -1,4 +1,7 @@
1package storage1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package localstorage
25
3import (6import (
4 "launchpad.net/loggo"7 "launchpad.net/loggo"
@@ -10,7 +13,7 @@
10 "launchpad.net/juju-core/worker"13 "launchpad.net/juju-core/worker"
11)14)
1215
13var logger = loggo.GetLogger("juju.local.storage")16var logger = loggo.GetLogger("juju.worker.localstorage")
1417
15type storageWorker struct {18type storageWorker struct {
16 config agent.Config19 config agent.Config
@@ -55,19 +58,22 @@
5558
56 sharedStorageDir := s.config.Value(agent.SharedStorageDir)59 sharedStorageDir := s.config.Value(agent.SharedStorageDir)
57 sharedStorageAddr := s.config.Value(agent.SharedStorageAddr)60 sharedStorageAddr := s.config.Value(agent.SharedStorageAddr)
58 logger.Infof("serving %s on %s", sharedStorageDir, sharedStorageAddr)61 if sharedStorageAddr != "" && sharedStorageDir != "" {
5962 logger.Infof("serving %s on %s", sharedStorageDir, sharedStorageAddr)
60 sharedStorage, err := filestorage.NewFileStorageWriter(sharedStorageDir, filestorage.UseDefaultTmpDir)63 sharedStorage, err := filestorage.NewFileStorageWriter(sharedStorageDir, filestorage.UseDefaultTmpDir)
61 if err != nil {64 if err != nil {
62 logger.Errorf("error with local storage: %v", err)65 logger.Errorf("error with local storage: %v", err)
63 return err66 return err
64 }67 }
65 sharedStorageListener, err := httpstorage.Serve(sharedStorageAddr, sharedStorage)68 sharedStorageListener, err := httpstorage.Serve(sharedStorageAddr, sharedStorage)
66 if err != nil {69 if err != nil {
67 logger.Errorf("error with local storage: %v", err)70 logger.Errorf("error with local storage: %v", err)
68 return err71 return err
69 }72 }
70 defer sharedStorageListener.Close()73 defer sharedStorageListener.Close()
74 } else {
75 logger.Infof("no shared storage: dir=%q addr=%q", sharedStorageDir, sharedStorageAddr)
76 }
7177
72 logger.Infof("storage routines started, awaiting death")78 logger.Infof("storage routines started, awaiting death")
7379

Subscribers

People subscribed via source and target branches

to status/vote changes: