Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1059)

Issue 9937046: First part of the provisioner refactoring.

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 9 months ago by thumper
Modified:
10 years, 9 months ago
Reviewers:
mp+167684, fwereade, wallyworld
Visibility:
Public.

Description

First part of the provisioner refactoring. This branch breaks up the current provisioner and defines some interfaces that we'll be using for containers. The broker interface is what we have starting, stopping instances, and listing instances and related machines. An environment broker is written that defers the actual calls to the environ. The provisioner now creates a provisioning task for the environment provider using a machine watcher and the environment broker. The common provisioning methods are now in provisioner_task.go. I refactored some of the methods as we were duplicating a lot of calls, like get all instances, and then getting instances for individual ids. Also, looking up specific machines, and also getting all machines. Now we just do the "all" gets, and keep a map around for the duration of the checks. https://code.launchpad.net/~thumper/juju-core/provisioner-step-1/+merge/167684 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 32

Patch Set 2 : First part of the provisioner refactoring. #

Total comments: 22

Patch Set 3 : First part of the provisioner refactoring. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+485 lines, -305 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
A worker/provisioner/broker.go View 1 1 chunk +27 lines, -0 lines 0 comments Download
A worker/provisioner/environ_broker.go View 1 1 chunk +21 lines, -0 lines 0 comments Download
M worker/provisioner/provisioner.go View 1 2 4 chunks +23 lines, -281 lines 0 comments Download
A worker/provisioner/provisioner_task.go View 1 2 1 chunk +372 lines, -0 lines 0 comments Download
M worker/provisioner/provisioner_test.go View 1 2 8 chunks +40 lines, -24 lines 0 comments Download

Messages

Total messages: 10
thumper
Please take a look.
10 years, 9 months ago (2013-06-06 03:37:21 UTC) #1
wallyworld
I think you have essentially copied a bunch of methods from provisioner to provisioner_task, so ...
10 years, 9 months ago (2013-06-06 04:12:43 UTC) #2
mue
Still interested in how the provisioning of containers on possibly each machine and there without ...
10 years, 9 months ago (2013-06-06 07:30:30 UTC) #3
fwereade
Only serious issue is the AllMachines stuff that I don't think we need. I'll try ...
10 years, 9 months ago (2013-06-06 14:50:54 UTC) #4
rog
https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go File worker/provisioner/broker.go (right): https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode23 worker/provisioner/broker.go:23: StopInstances([]environs.Instance) error On 2013/06/06 04:12:43, wallyworld wrote: > I ...
10 years, 9 months ago (2013-06-06 16:07:35 UTC) #5
thumper
https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go File worker/provisioner/broker.go (right): https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#newcode20 worker/provisioner/broker.go:20: StartInstance(machineId, machineNonce string, series string, cons constraints.Value, info *state.Info, ...
10 years, 9 months ago (2013-06-06 22:14:52 UTC) #6
thumper
Please take a look.
10 years, 9 months ago (2013-06-07 03:44:19 UTC) #7
thumper
On 2013/06/07 03:44:19, thumper wrote: > Please take a look. FYI - bootstrapped EC2 with ...
10 years, 9 months ago (2013-06-07 04:04:57 UTC) #8
fwereade
Huuuge pile of comments; the majority are not conveniently addressable this CL, but I would ...
10 years, 9 months ago (2013-06-07 22:40:45 UTC) #9
thumper
10 years, 9 months ago (2013-06-10 03:48:25 UTC) #10
*** Submitted:

First part of the provisioner refactoring.

This branch breaks up the current provisioner and defines some interfaces that
we'll be using for containers.

The broker interface is what we have starting, stopping instances, and listing
instances and related machines.

An environment broker is written that defers the actual calls to the environ.

The provisioner now creates a provisioning task for the environment provider
using a machine watcher and the environment broker.  The common provisioning
methods are now in provisioner_task.go.

I refactored some of the methods as we were duplicating a lot of calls, like
get all instances, and then getting instances for individual ids.  Also,
looking up specific machines, and also getting all machines.  Now we just do
the "all" gets, and keep a map around for the duration of the checks.

R=wallyworld, mue, fwereade, rog
CC=
https://codereview.appspot.com/9937046

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
worker/provisioner/provisioner.go:26: environmentProvisioner ProvisionerTask
On 2013/06/07 22:40:45, fwereade wrote:
> Only used in one method, no need to put it on the type.

Done.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
worker/provisioner/provisioner.go:73: stateInfo, apiInfo, err :=
p.environ.StateInfo()
On 2013/06/07 22:40:45, fwereade wrote:
> Hmm. This should probably come from state instead (one day the API). It has
> Addresses() and APIAddresses() methods, and a CACert() method required by
both,
> and you can build perfectly good *Infos from scratch inside the task so long
as
> they have access to that information. 
> 
> And, it should definitely not be done just once in the lifetime of the
> provisioner: the set of addresses is theoretically subject to change and I'd
> prefer to get "fresh" data at least once for every group of machines we start.
> But for now it's moot wrt observable behaviour, so leave them out of this CL.

Should have a follow up to do this.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
worker/provisioner/provisioner.go:91: defer p.environmentProvisioner.Stop()
On 2013/06/07 22:40:45, fwereade wrote:
> `defer watcher.Stop(p.environmentProvisioner, &p.tomb)` to pass shutdown
errors
> into this type's tomb.

I had a look at this.  I think we should move the method.  I inferred by the
name that it expects a watcher, but it doesn't, just a Stopper.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
worker/provisioner/provisioner.go:97: case cfg, ok :=
<-environWatcher.Changes():
On 2013/06/07 22:40:45, fwereade wrote:
> Soon (ie before we write an API that poos this whole thing over the wire every
> time) I'd like to make this an EntityWatcher, and to get EnvironConfig
directly
> and explicitly in response to the empty notifications. Not this CL though.

poos this whole thing? Sounds meesy.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
worker/provisioner/provisioner.go:104: case <-p.environmentProvisioner.Dying():
On 2013/06/07 22:40:45, fwereade wrote:
> `return watcher.MustErr(p.environmentProvisioner)` would be conventional, I
> think; and it'd be good to put it just after the <-p.tomb.Dying case -- it's
> usual to see the simple just-shut-stuff-down cases handled early, for ease of
> sanity verification in one place, and for the meaty selects to come last.
> 
> FWIW, I'd actually support a log statement inside watcher.MustErr, so we get
it
> tracked in every similar case -- even with less sophistication -- instead of
> just here.

Hesitant to do this here, as right now, watcher.MustErr panics with "watcher" in
the name, which is wrong in this case.  Moving case up though.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
worker/provisioner/provisioner_task.go:34: Machine(id string) (*state.Machine,
error)
On 2013/06/07 22:40:45, fwereade wrote:
> I'm not convinced this delivers value, really, but I reserve judgment for now.

Well there aren't extra tests around this yet.  Perhaps we could just pass
*state.State in, but I'd prefer to defined which methods the task uses.  It
makes for a cleaner interface.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
worker/provisioner/provisioner_task.go:51: apiInfo:        apiInfo,
On 2013/06/07 22:40:45, fwereade wrote:
> Yeah, these shouldn't be passed all the way through; you *could* just pass an
> explicit *State in place of machineGetter :).

Perhaps clean this up in the follow-up.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
worker/provisioner/provisioner_task.go:273: delete(task.machines, machine.Id())
On 2013/06/07 22:40:45, fwereade wrote:
> Why here? Seems like a pretty bizarre side-effect to leave unremarked...
> shouldn't we explicitly do that at machine.Remove() time instead, really?

Yes it does make more sense to do that after the machine.Remove().  Moved.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
worker/provisioner/provisioner_task.go:290: }
On 2013/06/07 22:40:45, fwereade wrote:
> We should delete from the .instances cache here, I think, lest it grow without
> bound.

.instances gets reset every time we get some changes.  Instead of adding and
removing from it, and also hitting it to get all instances when finding the
unknown instances, I just hit All once each time and refresh the cache.

https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision...
worker/provisioner/provisioner_task.go:294: func (task *provisionerTask)
startMachines(machines []*state.Machine) error {
On 2013/06/07 22:40:45, fwereade wrote:
> This might actually be a good spot to construct up-to-date *state.Info and
> *api.Info template objects, and pass them down for use by setupAuthentication.
> New values every time there's a new burst of machines provisioned, but no need
> to grab fresh data for every single machine.
> 
> Getting an API method that allows us to *watch* for these changes and not have
> to grab blindly would be really great, but is not a task for today.

I agree, but not right now.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b