|
|
DescriptionFirst 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. #
MessagesTotal messages: 10
Please take a look.
Sign in to reply to this message.
I think you have essentially copied a bunch of methods from provisioner to provisioner_task, so mainly a mechanical change. LGTM 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#ne... worker/provisioner/broker.go:23: StopInstances([]environs.Instance) error I think the signature here was done to match the existing Environs StopInstances but I'd rather see StopInstances([]string) ie pass in the ids od the instances to stop. There's no need to pass in more info than is necessary and callers typically should be designed to pass in just the ids. Both ec2 and openstack just extract the ids anyway and mass can be tweaked as needed. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_brok... File worker/provisioner/environ_broker.go (right): https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_brok... worker/provisioner/environ_broker.go:11: func newEnvironmentBroker(environ environs.Environ, state *state.State) Broker { newEnvironBroker I think is more idiomatic https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.go File worker/provisioner/provisioner.go (right): https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.... worker/provisioner/provisioner.go:84: p.environmentProvider = newProvisionerTask( environBroker perhaps, expecially if newEnvironmentBroker is renamed
Sign in to reply to this message.
Still interested in how the provisioning of containers on possibly each machine and there without an environBroker will look like. Or is this provisioner only intended as environment provisioner? 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#ne... worker/provisioner/broker.go:23: StopInstances([]environs.Instance) error On 2013/06/06 04:12:43, wallyworld wrote: > I think the signature here was done to match the existing Environs StopInstances > but I'd rather see StopInstances([]string) ie pass in the ids od the instances > to stop. There's no need to pass in more info than is necessary and callers > typically should be designed to pass in just the ids. Both ec2 and openstack > just extract the ids anyway and mass can be tweaked as needed. +1
Sign in to reply to this message.
Only serious issue is the AllMachines stuff that I don't think we need. I'll try to swing by tonight for a chat; if not, I'm fine calling progress-not-perfection and LGTM, but I'd be happiest if we could resolve it first. 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#ne... worker/provisioner/broker.go:20: StartInstance(machineId, machineNonce string, series string, cons constraints.Value, info *state.Info, apiInfo *api.Info) (environs.Instance, error) maybe that'll be an instance.Instance one day... but not today, certainly. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#ne... worker/provisioner/broker.go:23: StopInstances([]environs.Instance) error On 2013/06/06 07:30:30, mue wrote: > On 2013/06/06 04:12:43, wallyworld wrote: > > I think the signature here was done to match the existing Environs > StopInstances > > but I'd rather see StopInstances([]string) ie pass in the ids od the instances > > to stop. There's no need to pass in more info than is necessary and callers > > typically should be designed to pass in just the ids. Both ec2 and openstack > > just extract the ids anyway and mass can be tweaked as needed. > > +1 +0.5, but I'd like to keep this CL focused. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#ne... worker/provisioner/broker.go:29: AllMachines() ([]*state.Machine, error) Not quite sure about this. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_brok... File worker/provisioner/environ_broker.go (right): https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_brok... worker/provisioner/environ_broker.go:17: *state.State I think we can keep state out of here. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.go File worker/provisioner/provisioner.go (right): https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.... worker/provisioner/provisioner.go:91: // we should perhaps watch in the loop? yeah, select on its Dying()? https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.... worker/provisioner/provisioner.go:98: p.environmentProvider.Wait() defer a Stop just after creation, surely? https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... File worker/provisioner/provisioner_task.go (right): https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... worker/provisioner/provisioner_task.go:74: // Call processMachines to stop any unknown instances before watching machines. I *think* this whole block is redundant. The first event out of the watcher will contain all relevant machine ids, and if we pass that through to populateMachineMaps we can just use that... right? https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... worker/provisioner/provisioner_task.go:125: // instance for the same machine ID. I think this comment is wrong, but I'm still in driveby mode. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... worker/provisioner/provisioner_task.go:147: machines, err := task.broker.AllMachines() Yeah, there's no call for AllMachines() at all. We just need to pass the list of changed machine ids down from the watcher, get st.Machine() for any that not are already known (inefficient, susceptible to bulk-op smartness soon, justifiable on grounds of simplicity today IMO), skip NotFounds... and we're done. Right? https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... worker/provisioner/provisioner_task.go:189: status, _, err := machine.Status() If we fix the SetStatus(error) on provisioning failure to SetInstanceError, or something, we'll want to change this to check InstanceError as well. Not for this CL ofc. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... worker/provisioner/provisioner_task.go:236: // instance is already dead, and we don't need to stop it. Hmm. It might mean it exists but EC2 can't be bothered to tell us about it. Just a thought, not sure of the consequences. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... worker/provisioner/provisioner_task.go:295: // error; just keep going with the other machines. One day, this should be SetInstanceError. Not this CL.
Sign in to reply to this message.
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#ne... worker/provisioner/broker.go:23: StopInstances([]environs.Instance) error On 2013/06/06 04:12:43, wallyworld wrote: > I think the signature here was done to match the existing Environs StopInstances > but I'd rather see StopInstances([]string) ie pass in the ids od the instances > to stop. There's no need to pass in more info than is necessary and callers > typically should be designed to pass in just the ids. Both ec2 and openstack > just extract the ids anyway and mass can be tweaked as needed. the reason the StopInstances call was designed the way it was is so you cannot stop an instance that is not part of the current environment (you have to call Environ.Instances to get the instance first, which can make sure that the instance id corresponds to one in the environment). perhaps it's not a problem to relax that restriction, but i think it's worth pointing out.
Sign in to reply to this message.
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#ne... worker/provisioner/broker.go:20: StartInstance(machineId, machineNonce string, series string, cons constraints.Value, info *state.Info, apiInfo *api.Info) (environs.Instance, error) On 2013/06/06 14:50:55, fwereade wrote: > maybe that'll be an instance.Instance one day... but not today, certainly. Perhaps *state.Machine? instance.Instance is for started instances isn't it? https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#ne... worker/provisioner/broker.go:23: StopInstances([]environs.Instance) error On 2013/06/06 16:07:35, rog wrote: > On 2013/06/06 04:12:43, wallyworld wrote: > > I think the signature here was done to match the existing Environs > StopInstances > > but I'd rather see StopInstances([]string) ie pass in the ids od the instances > > to stop. There's no need to pass in more info than is necessary and callers > > typically should be designed to pass in just the ids. Both ec2 and openstack > > just extract the ids anyway and mass can be tweaked as needed. > > the reason the StopInstances call was designed the way it was is so you > cannot stop an instance that is not part of the current environment > (you have to call Environ.Instances to get the instance first, which can make > sure that the instance id corresponds to one in the environment). > > perhaps it's not a problem to relax that restriction, but i think it's worth > pointing out. Perhaps []state.InstanceId? this is a string type under the covers, but better matches intent. The provisioner has done the "lookup the instances" for the machine ids it gets, so we know, at least from the provisioner side, that all of the values being passed through should be valid. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#ne... worker/provisioner/broker.go:29: AllMachines() ([]*state.Machine, error) On 2013/06/06 14:50:55, fwereade wrote: > Not quite sure about this. I thought quite a bit about this, and ended up putting it here. The provisioner task uses it when trying to identify the unknown instances. It made sense to me that the broker would supply this. Otherwise it is a global call to State, and we certainly don't care about "AllMachines" when dealing with a container on a machine 4. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_brok... File worker/provisioner/environ_broker.go (right): https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_brok... worker/provisioner/environ_broker.go:11: func newEnvironmentBroker(environ environs.Environ, state *state.State) Broker { On 2013/06/06 04:12:43, wallyworld wrote: > newEnvironBroker I think is more idiomatic probably https://codereview.appspot.com/9937046/diff/1/worker/provisioner/environ_brok... worker/provisioner/environ_broker.go:17: *state.State On 2013/06/06 14:50:55, fwereade wrote: > I think we can keep state out of here. It is used only for the AllMachines call, which I commented on the earlier comment. Since the environBroker just provides the Broker interface, no-one calls anything else on state. Yay the magic of struct composition. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.go File worker/provisioner/provisioner.go (right): https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.... worker/provisioner/provisioner.go:84: p.environmentProvider = newProvisionerTask( On 2013/06/06 04:12:43, wallyworld wrote: > environBroker perhaps, expecially if newEnvironmentBroker is renamed Agh, should be called environmentProvisioner. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.... worker/provisioner/provisioner.go:91: // we should perhaps watch in the loop? On 2013/06/06 14:50:55, fwereade wrote: > yeah, select on its Dying()? That probably makes sense. Get the error out and pass it back? https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner.... worker/provisioner/provisioner.go:98: p.environmentProvider.Wait() On 2013/06/06 14:50:55, fwereade wrote: > defer a Stop just after creation, surely? I wasn't sure, that is why I asked :) https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... File worker/provisioner/provisioner_task.go (right): https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... worker/provisioner/provisioner_task.go:74: // Call processMachines to stop any unknown instances before watching machines. On 2013/06/06 14:50:55, fwereade wrote: > I *think* this whole block is redundant. The first event out of the watcher will > contain all relevant machine ids, and if we pass that through to > populateMachineMaps we can just use that... right? I have no idea. This was just copied from the other file. If however, as you say, the watcher has changes straight away, then yes, this block is entirely redundant. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... worker/provisioner/provisioner_task.go:125: // instance for the same machine ID. On 2013/06/06 14:50:55, fwereade wrote: > I think this comment is wrong, but I'm still in driveby mode. Again, just copied from the other file without too much thought. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... worker/provisioner/provisioner_task.go:147: machines, err := task.broker.AllMachines() On 2013/06/06 14:50:55, fwereade wrote: > Yeah, there's no call for AllMachines() at all. We just need to pass the list of > changed machine ids down from the watcher, get st.Machine() for any that not are > already known (inefficient, susceptible to bulk-op smartness soon, justifiable > on grounds of simplicity today IMO), skip NotFounds... and we're done. Right? The call for AllMachines used to be in the method for finding unknown instances. How, if we don't get all appropriate machines, are we supposed to get the machines for the unknown instance ids? I didn't realise that the first change set from the watcher returned all related ids. Perhaps we should have a method to get state.Machines given a slice of instance ids? https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... worker/provisioner/provisioner_task.go:189: status, _, err := machine.Status() On 2013/06/06 14:50:55, fwereade wrote: > If we fix the SetStatus(error) on provisioning failure to SetInstanceError, or > something, we'll want to change this to check InstanceError as well. Not for > this CL ofc. ok
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
On 2013/06/07 03:44:19, thumper wrote: > Please take a look. FYI - bootstrapped EC2 with this code, after the status below, I destroyed the environment and was able to see all the instances shutting down from the AWS console. $ juju status machines: "0": agent-state: started agent-version: 1.11.1.1 dns-name: ec2-23-20-239-165.compute-1.amazonaws.com instance-id: i-83b05aec series: precise "1": agent-state: started agent-version: 1.11.1.1 dns-name: ec2-54-235-31-114.compute-1.amazonaws.com instance-id: i-55f5b63a series: precise "2": agent-state: started agent-version: 1.11.1.1 dns-name: ec2-54-242-136-39.compute-1.amazonaws.com instance-id: i-d9f4b7b6 series: precise services: mysql: charm: cs:precise/mysql-23 exposed: false relations: cluster: - mysql units: mysql/0: agent-state: installed agent-version: 1.11.1.1 machine: "2" public-address: ec2-54-242-136-39.compute-1.amazonaws.com wordpress: charm: cs:precise/wordpress-15 exposed: false relations: loadbalancer: - wordpress units: wordpress/0: agent-state: pending agent-version: 1.11.1.1 machine: "1" public-address: ec2-54-235-31-114.compute-1.amazonaws.com
Sign in to reply to this message.
Huuuge pile of comments; the majority are not conveniently addressable this CL, but I would appreciate consideration of those that are. I'm happy to LGTM on the understanding that you will give them due consideration before merging. 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#ne... worker/provisioner/broker.go:20: StartInstance(machineId, machineNonce string, series string, cons constraints.Value, info *state.Info, apiInfo *api.Info) (environs.Instance, error) On 2013/06/06 22:14:53, thumper wrote: > On 2013/06/06 14:50:55, fwereade wrote: > > maybe that'll be an instance.Instance one day... but not today, certainly. > > Perhaps *state.Machine? instance.Instance is for started instances isn't it? There's a firm distinction between machines and instances: a machine exists purely in state, and an instance is completely independent of state. What I'm getting at is that Instance is going to stop being an environs-specific concept, and should probably move to its own package at some point. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/broker.go#ne... worker/provisioner/broker.go:23: StopInstances([]environs.Instance) error On 2013/06/06 22:14:53, thumper wrote: > On 2013/06/06 16:07:35, rog wrote: > > On 2013/06/06 04:12:43, wallyworld wrote: > > > I think the signature here was done to match the existing Environs > > StopInstances > > > but I'd rather see StopInstances([]string) ie pass in the ids od the > instances > > > to stop. There's no need to pass in more info than is necessary and callers > > > typically should be designed to pass in just the ids. Both ec2 and openstack > > > just extract the ids anyway and mass can be tweaked as needed. > > > > the reason the StopInstances call was designed the way it was is so you > > cannot stop an instance that is not part of the current environment > > (you have to call Environ.Instances to get the instance first, which can make > > sure that the instance id corresponds to one in the environment). > > > > perhaps it's not a problem to relax that restriction, but i think it's worth > > pointing out. > > Perhaps []state.InstanceId? this is a string type under the covers, but better > matches intent. > > The provisioner has done the "lookup the instances" for the machine ids it gets, > so we know, at least from the provisioner side, that all of the values being > passed through should be valid. I'd be ok with InstanceId fwiw. https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... File worker/provisioner/provisioner_task.go (right): https://codereview.appspot.com/9937046/diff/1/worker/provisioner/provisioner_... worker/provisioner/provisioner_task.go:147: machines, err := task.broker.AllMachines() On 2013/06/06 22:14:53, thumper wrote: > On 2013/06/06 14:50:55, fwereade wrote: > > Yeah, there's no call for AllMachines() at all. We just need to pass the list > of > > changed machine ids down from the watcher, get st.Machine() for any that not > are > > already known (inefficient, susceptible to bulk-op smartness soon, justifiable > > on grounds of simplicity today IMO), skip NotFounds... and we're done. Right? > > The call for AllMachines used to be in the method for finding unknown instances. > How, if we don't get all appropriate machines, are we supposed to get the > machines for the unknown instance ids? > > I didn't realise that the first change set from the watcher returned all related > ids. Perhaps we should have a method to get state.Machines given a slice of > instance ids? +100. I think there's a comment from niemeyer to that effect from way back somewhere in this code. 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 Only used in one method, no need to put it on the type. https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision... worker/provisioner/provisioner.go:73: stateInfo, apiInfo, err := p.environ.StateInfo() 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. https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision... worker/provisioner/provisioner.go:91: defer p.environmentProvisioner.Stop() `defer watcher.Stop(p.environmentProvisioner, &p.tomb)` to pass shutdown errors into this type's tomb. https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision... worker/provisioner/provisioner.go:97: case cfg, ok := <-environWatcher.Changes(): 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. https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision... worker/provisioner/provisioner.go:104: case <-p.environmentProvisioner.Dying(): `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. 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) I'm not convinced this delivers value, really, but I reserve judgment for now. https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision... worker/provisioner/provisioner_task.go:51: apiInfo: apiInfo, Yeah, these shouldn't be passed all the way through; you *could* just pass an explicit *State in place of machineGetter :). https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision... worker/provisioner/provisioner_task.go:273: delete(task.machines, machine.Id()) Why here? Seems like a pretty bizarre side-effect to leave unremarked... shouldn't we explicitly do that at machine.Remove() time instead, really? https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision... worker/provisioner/provisioner_task.go:290: } We should delete from the .instances cache here, I think, lest it grow without bound. https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision... worker/provisioner/provisioner_task.go:294: func (task *provisionerTask) startMachines(machines []*state.Machine) error { 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. https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision... worker/provisioner/provisioner_task.go:307: // state.Info as the PA. Getting fresh addresses from state at appropriate junctures, and using them in setupAuthentication, will render this comment redundant. https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision... File worker/provisioner/provisioner_test.go (right): https://codereview.appspot.com/9937046/diff/8002/worker/provisioner/provision... worker/provisioner/provisioner_test.go:171: for !instanceIds.IsEmpty() { Nice.
Sign in to reply to this message.
*** 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.
|