Issue 6294066: cmd/jujud: provisioner can now start and stop instances (Closed)
|
Created:
11 years, 10 months ago by dave Modified:
11 years, 10 months ago Reviewers:
mp+109753 Visibility:
Public. |
Descriptioncmd/jujud: provisioner can now start and stop instances
This branch adds the ability for the provisioning agent to
start and stop instances according to changes in state.
To be implemented in a later branch is the ability for the PA
to reconcile the set of instances running with the set of
instances known to the state.
https://code.launchpad.net/~dave-cheney/juju-core/go-cmd-provisioning-start-stop-instance/+merge/109753
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : cmd/jujud: provisioner can now start and stop instances #
Total comments: 36
Patch Set 3 : cmd/jujud: provisioner can now start and stop instances #Patch Set 4 : cmd/jujud: provisioner can now start and stop instances #Patch Set 5 : cmd/jujud: provisioner can now start and stop instances #
Total comments: 20
Patch Set 6 : cmd/jujud: provisioner can now start and stop instances #Patch Set 7 : cmd/jujud: provisioner can now start and stop instances #Patch Set 8 : cmd/jujud: provisioner can now start and stop instances #Patch Set 9 : cmd/jujud: provisioner can now start and stop instances #
Total comments: 4
Patch Set 10 : cmd/jujud: provisioner can now start and stop instances #Patch Set 11 : cmd/jujud: provisioner can now start and stop instances #
Total comments: 49
Patch Set 12 : cmd/jujud: provisioner can now start and stop instances #
Total comments: 6
Patch Set 13 : cmd/jujud: provisioner can now start and stop instances #Patch Set 14 : cmd/jujud: provisioner can now start and stop instances #
Total comments: 5
Patch Set 15 : cmd/jujud: provisioner can now start and stop instances #
MessagesTotal messages: 24
Please take a look.
Sign in to reply to this message.
this is looking good. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:56: providerIdToInstance map[string]environs.Instance i'm not sure that "providerId" is the right term here. it sounds like the id *of* a provider, which isn't right. we already have a term for what you're calling a provider id - it's an instance id. i think i'd call this field "instances". https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:57: machineIdToProviderId map[int]string i might call this "instanceIds", but i wonder. why not just store the environs.Instance here, rather than storing the id that needs looking up in the other map? then you could have: instancesById map[string] environs.Instance instancesByMachineId map[int] environs.Instance (I think those names might read a little more easily than instanceIdToInstance and machineIdToInstance but YMMV.) https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:153: // step 1. filter machines with provider ids and without // step 1. find all machines without instance ids. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:161: notrunning = append(notrunning, m) i'm not quite seeing why you build up the slice here rather than calling p.startMachine and eliminating the need for step 2. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:184: if len(stopping) > 0 { i'm not sure this is the right approach. the dummy environs does not say exactly what actions have occurred; it says what methods have been called, which is a different thing. i think the test should use the dummy op as a synchronisation point - when it has happened it should check the actual instances to see if they are as expected. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:190: func (p *Provisioner) startMachines(machines []*state.Machine) ([]*state.Machine, error) { it seems like this method could go. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:206: // state.Info as the PA. actually, i'm not sure that's true. the PA will be given a state.Info that refers to localhost, as when the cloudinit is created, we don't know the DNS address of the zk state. i have a feeling that we'll need to find out the DNS address of the local machine (possibly by interrogating the local machine) and use that to construct a new state.Info. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:212: // assign the provider id to the macine s/provider/instance/ https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:214: return fmt.Errorf("unable to store provider id for machine %v: %v", m, err) s/provider/instance/ https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:249: return nil, nil replace with: p.providerIdToInstance[id] = inst ? https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:268: func (p *Provisioner) findInstance(id string) (environs.Instance, error) { is this method really worth it? https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:40: // seed /environment to point to dummy // see the state's environment to point to dummy. (/environment may not be the way that state implements EnvironConfig). https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:168: // make the environment unpalitable to the config checker unpalatable https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:190: case <-time.After(1 * time.Second): 1 second seems like quite a long time, given we're always doing it. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:228: // make the environment unpalitable to the config checker unpalatable https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:239: // the PA should create it using the old environment s/environment/environment./ https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:253: // restarted in this test. tf. Methods like Fatalf and Assert should not be used. i'm not sure this is great. at various points below, if the check fails, the test will panic. why not just defer p.Stop anyway? there's no problem calling Stop twice on the same p. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:284: c.Errorf("provisioner started an instance: %v", o) s/started an instance/performed an unexpected action/ ? (you don't know for sure that the action is StartInstance) https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:310: // make the environment unpalitable to the config checker unpalatable https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:328: } this code is duplicated quite a few times. how about a helper function: // addMachine adds a machine to the start and checks // whether the provisioner correctly acts on it; // shouldProvision specifies whether the machine // should be provisioned. func (s *provisioningSuite) addMachine(c *C, op <-chan dummy.Operation, shouldProvision bool)
Sign in to reply to this message.
Thanks for your feedback Rog, lots of little things to fix. I'll take another pass tonight. On 12/06/2012, at 19:34, rogpeppe@gmail.com wrote: > this is looking good. > > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go > File cmd/jujud/provisioning.go (right): > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... > cmd/jujud/provisioning.go:56: providerIdToInstance > map[string]environs.Instance > i'm not sure that "providerId" is the right term here. it sounds like > the id *of* a provider, which isn't right. > > we already have a term for what you're calling a provider id - it's an > instance id. > > i think i'd call this field "instances". > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... > cmd/jujud/provisioning.go:57: machineIdToProviderId map[int]string > i might call this "instanceIds", but i wonder. why not just store the > environs.Instance here, rather than storing the id that needs looking up > in the other map? > > then you could have: > instancesById map[string] environs.Instance > instancesByMachineId map[int] environs.Instance > > (I think those names might read a little more easily than > instanceIdToInstance and machineIdToInstance but YMMV.) > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... > cmd/jujud/provisioning.go:153: // step 1. filter machines with provider > ids and without > // step 1. find all machines without instance ids. > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... > cmd/jujud/provisioning.go:161: notrunning = append(notrunning, m) > i'm not quite seeing why you build up the slice here rather than calling > p.startMachine and eliminating the need for step 2. > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... > cmd/jujud/provisioning.go:184: if len(stopping) > 0 { > i'm not sure this is the right approach. the dummy environs does not say > exactly what actions have occurred; it says what methods have been > called, which is a different thing. > > i think the test should use the dummy op as a synchronisation point - > when it has happened it should check > the actual instances to see if they are as expected. > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... > cmd/jujud/provisioning.go:190: func (p *Provisioner) > startMachines(machines []*state.Machine) ([]*state.Machine, error) { > it seems like this method could go. > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... > cmd/jujud/provisioning.go:206: // state.Info as the PA. > actually, i'm not sure that's true. the PA will be given a state.Info > that refers to localhost, as when the cloudinit is created, we don't > know the DNS address of the zk state. > i have a feeling that we'll need to find out the DNS address of the > local machine (possibly by interrogating the local machine) and use that > to construct a new state.Info. > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... > cmd/jujud/provisioning.go:212: // assign the provider id to the macine > s/provider/instance/ > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... > cmd/jujud/provisioning.go:214: return fmt.Errorf("unable to store > provider id for machine %v: %v", m, err) > s/provider/instance/ > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... > cmd/jujud/provisioning.go:249: return nil, nil > replace with: > p.providerIdToInstance[id] = inst > ? > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... > cmd/jujud/provisioning.go:268: func (p *Provisioner) findInstance(id > string) (environs.Instance, error) { > is this method really worth it? > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.go > File cmd/jujud/provisioning_test.go (right): > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... > cmd/jujud/provisioning_test.go:40: // seed /environment to point to > dummy > // see the state's environment to point to dummy. > > (/environment may not be the way that state implements > EnvironConfig). > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... > cmd/jujud/provisioning_test.go:168: // make the environment unpalitable > to the config checker > unpalatable > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... > cmd/jujud/provisioning_test.go:190: case <-time.After(1 * time.Second): > 1 second seems like quite a long time, given we're always doing it. > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... > cmd/jujud/provisioning_test.go:228: // make the environment unpalitable > to the config checker > unpalatable > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... > cmd/jujud/provisioning_test.go:239: // the PA should create it using the > old environment > s/environment/environment./ > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... > cmd/jujud/provisioning_test.go:253: // restarted in this test. tf. > Methods like Fatalf and Assert should not be used. > i'm not sure this is great. at various points below, if the check fails, > the test will panic. why not just defer p.Stop anyway? there's no > problem calling Stop twice on the same p. > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... > cmd/jujud/provisioning_test.go:284: c.Errorf("provisioner started an > instance: %v", o) > s/started an instance/performed an unexpected action/ > ? > (you don't know for sure that the action is StartInstance) > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... > cmd/jujud/provisioning_test.go:310: // make the environment unpalitable > to the config checker > unpalatable > > https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... > cmd/jujud/provisioning_test.go:328: } > this code is duplicated quite a few times. how about a helper function: > > // addMachine adds a machine to the start and checks > // whether the provisioner correctly acts on it; > // shouldProvision specifies whether the machine > // should be provisioned. > func (s *provisioningSuite) addMachine(c *C, op <-chan dummy.Operation, > shouldProvision bool) > > https://codereview.appspot.com/6294066/
Sign in to reply to this message.
Great feedback Rog, thanks. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:153: // step 1. filter machines with provider ids and without On 2012/06/12 09:34:02, rog wrote: > // step 1. find all machines without instance ids. Done. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:161: notrunning = append(notrunning, m) On 2012/06/12 09:34:02, rog wrote: > i'm not quite seeing why you build up the slice here rather than calling > p.startMachine and eliminating the need for step 2. At one point I was using the list of started machines, I guess i'm not anymore https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:184: if len(stopping) > 0 { On 2012/06/12 09:34:02, rog wrote: > i'm not sure this is the right approach. the dummy environs does not say exactly > what actions have occurred; it says what methods have been called, which is a > different thing. > > i think the test should use the dummy op as a synchronisation point - when it > has happened it should check > the actual instances to see if they are as expected. Dummy does not return that information, it probably should. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:190: func (p *Provisioner) startMachines(machines []*state.Machine) ([]*state.Machine, error) { On 2012/06/12 09:34:02, rog wrote: > it seems like this method could go. I'd like to keep it for the moment, I'd like to keep processMachines short. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:206: // state.Info as the PA. On 2012/06/12 09:34:02, rog wrote: > actually, i'm not sure that's true. the PA will be given a state.Info that > refers to localhost, as when the cloudinit is created, we don't know the DNS > address of the zk state. > i have a feeling that we'll need to find out the DNS address of the local > machine (possibly by interrogating the local machine) and use that to construct > a new state.Info. I agree, how can the PA discover the state.Info that should be used for newly instantiated machines ? https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:212: // assign the provider id to the macine On 2012/06/12 09:34:02, rog wrote: > s/provider/instance/ Done. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:212: // assign the provider id to the macine On 2012/06/12 09:34:02, rog wrote: > s/provider/instance/ Done. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:214: return fmt.Errorf("unable to store provider id for machine %v: %v", m, err) On 2012/06/12 09:34:02, rog wrote: > s/provider/instance/ Done. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:249: return nil, nil On 2012/06/12 09:34:02, rog wrote: > replace with: > p.providerIdToInstance[id] = inst > ? Done. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:268: func (p *Provisioner) findInstance(id string) (environs.Instance, error) { On 2012/06/12 09:34:02, rog wrote: > is this method really worth it? It used to be, but the code is a bit simpler (as we're still debating the environ.AllMachines addition)
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:40: // seed /environment to point to dummy On 2012/06/12 09:34:02, rog wrote: > // see the state's environment to point to dummy. > > (/environment may not be the way that state implements > EnvironConfig). Done. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:168: // make the environment unpalitable to the config checker On 2012/06/12 09:34:02, rog wrote: > unpalatable Done. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:190: case <-time.After(1 * time.Second): On 2012/06/12 09:34:02, rog wrote: > 1 second seems like quite a long time, given we're always doing it. Done. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:310: // make the environment unpalitable to the config checker On 2012/06/12 09:34:02, rog wrote: > unpalatable Done. https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:328: } On 2012/06/12 09:34:02, rog wrote: > this code is duplicated quite a few times. how about a helper function: > > // addMachine adds a machine to the start and checks > // whether the provisioner correctly acts on it; > // shouldProvision specifies whether the machine > // should be provisioned. > func (s *provisioningSuite) addMachine(c *C, op <-chan dummy.Operation, > shouldProvision bool) Nice, done
Sign in to reply to this message.
a few more comments... https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/2001/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:56: providerIdToInstance map[string]environs.Instance nice to see this go. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:56: // TODO(dfc) machineId should be a uint why do you think so? https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:57: machineIdToInstance map[int]environs.Instance s/machineIdToInstance/instances/ ? https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:152: // step 1. find machines without instance ids (tf. not running) "tf." ? the fact that non-running machines are those without instance ids is part of the implementation of findNotRunning, so not appropriate to mention here i think. how about this? // step 1. find which of the added machines have not // yet been allocated a running instance. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:158: // step 2. start all the notrunning machines // step 2. start the machines we found. ? https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:181: // findNotRunning filters machines without an InstanceId set, these are defined as not running. s/filters/finds/ ? "filters" doesn't imply whether it includes or excludes them. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning_test.go#... cmd/jujud/provisioning_test.go:56: // make the environment unpalitable to the config checker unpalatable :-) https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning_test.go#... cmd/jujud/provisioning_test.go:178: c.Assert(s.invalidateEnvironment(), IsNil) i'm not that keen on having side effects in assert statements. i think i'd write this: err := s.invalidateEnvironment() c.Assert(err, IsNil) YMMV.
Sign in to reply to this message.
I'll let you polish this with Roger until you're both happy or would like another pair of eyes (whatever happens first). Marking it as WIP for the moment.
Sign in to reply to this message.
Understood, thanks, Rogers feedback has been very useful. On 14/06/2012, at 6:04, n13m3y3r@gmail.com wrote: > I'll let you polish this with Roger until you're both happy or would > like another pair of eyes (whatever happens first). > > Marking it as WIP for the moment. > > https://codereview.appspot.com/6294066/
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:56: // TODO(dfc) machineId should be a uint On 2012/06/13 08:25:14, rog wrote: > why do you think so? Because state.AddMachine() starts at 0. Returning a negative machine number is an error, so we should let the type system do the enforcement. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:57: machineIdToInstance map[int]environs.Instance On 2012/06/13 08:25:14, rog wrote: > s/machineIdToInstance/instances/ ? It's a one to one mapping of machine id -> instance, I think the singular is correct. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:152: // step 1. find machines without instance ids (tf. not running) On 2012/06/13 08:25:14, rog wrote: > "tf." ? therefore > the fact that non-running machines are those without instance ids is part of the > implementation of findNotRunning, so not appropriate to mention here i think. > > how about this? > > // step 1. find which of the added machines have not > // yet been allocated a running instance. SGTM. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:158: // step 2. start all the notrunning machines On 2012/06/13 08:25:14, rog wrote: > // step 2. start the machines we found. > ? Done. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:181: // findNotRunning filters machines without an InstanceId set, these are defined as not running. On 2012/06/13 08:25:14, rog wrote: > s/filters/finds/ ? > "filters" doesn't imply whether it includes or excludes them. Done. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning_test.go#... cmd/jujud/provisioning_test.go:56: // make the environment unpalitable to the config checker On 2012/06/13 08:25:14, rog wrote: > unpalatable :-) Done. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning_test.go#... cmd/jujud/provisioning_test.go:178: c.Assert(s.invalidateEnvironment(), IsNil) On 2012/06/13 08:25:14, rog wrote: > i'm not that keen on having side effects in assert statements. i think i'd write > this: > > err := s.invalidateEnvironment() > c.Assert(err, IsNil) SGTM.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:56: // TODO(dfc) machineId should be a uint On 2012/06/14 00:32:03, dfc wrote: > On 2012/06/13 08:25:14, rog wrote: > > why do you think so? > > Because state.AddMachine() starts at 0. Returning a negative machine number is > an error, so we should let the type system do the enforcement. given that we never do arithmetic with machine ids, yeah, i could buy that. even better maybe might be to define a type: type MachineId uint somewhere, although that might seem over verbose. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:57: machineIdToInstance map[int]environs.Instance On 2012/06/14 00:32:03, dfc wrote: > On 2012/06/13 08:25:14, rog wrote: > > s/machineIdToInstance/instances/ ? > > It's a one to one mapping of machine id -> instance, I think the singular is > correct. it's also a collection of all the instances we currently know about. but i'm happy with machineIdToInstance too. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:152: // step 1. find machines without instance ids (tf. not running) On 2012/06/14 00:32:03, dfc wrote: > On 2012/06/13 08:25:14, rog wrote: > > "tf." ? > > therefore i haven't seen that abbreviation before. best not to use it, i think. or you could use "∴" :-) https://codereview.appspot.com/6294066/diff/10004/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/10004/cmd/jujud/provisioning.go#n... cmd/jujud/provisioning.go:56: // TODO(dfc) machineId should be a uint if we're going to have this comment, perhaps it should be on state.Machine.Id, from whence machine ids flow. https://codereview.appspot.com/6294066/diff/10004/cmd/jujud/provisioning.go#n... cmd/jujud/provisioning.go:254: // instancesForMachines returns a list of environs.Instance that represent the list of machines running // instancesForMachines returns the instance for // each machine that has been allocated an instance. ? (the type signature says the rest, i think).
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:56: // TODO(dfc) machineId should be a uint I'll remove the TODO and put it on my backlog. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:57: machineIdToInstance map[int]environs.Instance Thanks. This may change (or be removed) depending on how much use it gets, and the moment it's write through when new machines are created, but if it doesn't pull its weight i'll remove it. https://codereview.appspot.com/6294066/diff/5/cmd/jujud/provisioning.go#newco... cmd/jujud/provisioning.go:152: // step 1. find machines without instance ids (tf. not running) beleted. https://codereview.appspot.com/6294066/diff/10004/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/10004/cmd/jujud/provisioning.go#n... cmd/jujud/provisioning.go:56: // TODO(dfc) machineId should be a uint On 2012/06/14 07:30:51, rog wrote: > if we're going to have this comment, perhaps it should be on state.Machine.Id, > from whence machine ids flow. I've removed the TODO, i'll tackle that battle in another CL. https://codereview.appspot.com/6294066/diff/10004/cmd/jujud/provisioning.go#n... cmd/jujud/provisioning.go:254: // instancesForMachines returns a list of environs.Instance that represent the list of machines running On 2012/06/14 07:30:51, rog wrote: > // instancesForMachines returns the instance for > // each machine that has been allocated an instance. > > ? > (the type signature says the rest, i think). Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Great work, Dave. Have some comments, but they are pretty simple stuff. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:56: machineIdToInstance map[int]environs.Instance This is just a suggestion, but I'd call this as "instances", and maybe document with "// machine id => instance" here. The usage we make of it is all very clear (e.g. p.instances[m.Id()]) https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:153: notrunning, err := p.findNotRunning(changes.Added) s/Running/Started/ and s/running/started/ Besides being the term we actually use in the interface, I'd like to avoid "running" just because it could be slightly misleading as it's exactly the term used by EC2 for instances that have finished the allocation process. If a machine is "pending" in EC2, it's not yet "running", but we shouldn't be firing another instance here in that situation ("pending" means it's allocated and will get "running" soon). https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:173: // provider, environs like dummy do not consider this a noop. Interesting. I guess it should? Food for another CL, anyway. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:175: return p.environ.StopInstances(stopping) We need to delete the cached instance here, or the cache will never stop growing. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:191: log.Printf("machine %s already running as instance %q", m, id) s/running/started/ https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:203: log.Printf("starting machine %v", m) I suggest logging within startMachine rather than here, as we can add more details. See below. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:215: if err != nil { log.Printf("provisioner can't start machine %s: %v", m, err) https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:217: } log.Printf("provisioner started machine %s as instance %s", m, inst.Id()) https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:221: return fmt.Errorf("unable to store instance id for machine %v: %v", m, err) The state package will return an error message that looks like this with the work Frank is pushing. We can just return err here. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:229: // instanceForMachine returns the environs.Instance that represents this machines' running s/'s running instance// https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:239: if id == "" { Ugh. Please add a TODO here: // TODO InstanceId should return an error if the id isn't set. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:243: insts, err := p.environ.Instances([]string{id}) This is a remote call onto the provider, which means it has a relatively high cost. We're taking a list of instances, splitting them up, and then doing N calls with a list of len 1, when we can actually do a single call with list of N. The underlying Instances call was made that way precisely so we can do that. This is an optimization, though, and I'm happy to have that in a separate branch so you can integrate this now. Please add a TODO here if you decide to do this. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:55: func (s *ProvisioningSuite) invalidateEnvironment() error { Missing docs. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:65: func (s *ProvisioningSuite) fixEnvironment() error { Missing docs. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:79: // checkStartInstance checks that a machine has been started. Let's please use the "machine" and "instance" terms sensibly. They mean different things for the juju implementation, and thus shouldn't be used as synonyms. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:82: // provisioners. Please reindent comment. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:85: c.Check(o.Kind, Equals, dummy.OpStartInstance) There's no reason for this to be the first operation in the channel. The provisioner implementation is free to do whatever it pleases before that. It should loop waiting for this operation to show up. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:87: c.Errorf("ProvisioningAgent did not action AddMachine after 3 second") The message disagrees with the actual verification. I'd also omit the timing from the error. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:91: // checkStopInstance checks that a machine has been stopped. Ditto re. instance/machine. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:94: // provisioners. Please reindent comment. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:97: c.Check(o.Kind, Equals, dummy.OpStopInstances) Ditto for the looping. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:99: c.Errorf("ProvisioningAgent did not action RmoveMachine after 3 second") Ditto for the message. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:167: s.checkStartInstance(c, op) Should also test that the instance id for the started machine was properly set in the machine. Would also be nice to start at least another one with the same tests, to make sure the logic works for more than a single case, and then stop a single one of them, to make sure it also knows how to stop a *specific* machine. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:194: c.Errorf("provisioner started an instance") All of these verifications of <-op have to be fixed. They shouldn't assume they know what the operation is, nor that they know what other calls the provisioner implementation is using internally and in which order.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:56: machineIdToInstance map[int]environs.Instance On 2012/06/16 01:18:43, niemeyer wrote: > This is just a suggestion, but I'd call this as "instances", and maybe document > with "// machine id => instance" here. The usage we make of it is all very clear > (e.g. p.instances[m.Id()]) Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:153: notrunning, err := p.findNotRunning(changes.Added) On 2012/06/16 01:18:43, niemeyer wrote: > s/Running/Started/ and s/running/started/ > > Besides being the term we actually use in the interface, I'd like to avoid > "running" just because it could be slightly misleading as it's exactly the term > used by EC2 for instances that have finished the allocation process. If a > machine is "pending" in EC2, it's not yet "running", but we shouldn't be firing > another instance here in that situation ("pending" means it's allocated and will > get "running" soon). Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:173: // provider, environs like dummy do not consider this a noop. On 2012/06/16 01:18:43, niemeyer wrote: > Interesting. I guess it should? Food for another CL, anyway. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:175: return p.environ.StopInstances(stopping) On 2012/06/16 01:18:43, niemeyer wrote: > We need to delete the cached instance here, or the cache will never stop > growing. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:191: log.Printf("machine %s already running as instance %q", m, id) On 2012/06/16 01:18:43, niemeyer wrote: > s/running/started/ Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:203: log.Printf("starting machine %v", m) On 2012/06/16 01:18:43, niemeyer wrote: > I suggest logging within startMachine rather than here, as we can add more > details. See below. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:215: if err != nil { On 2012/06/16 01:18:43, niemeyer wrote: > log.Printf("provisioner can't start machine %s: %v", m, err) Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:221: return fmt.Errorf("unable to store instance id for machine %v: %v", m, err) On 2012/06/16 01:18:43, niemeyer wrote: > The state package will return an error message that looks like this with the > work Frank is pushing. We can just return err here. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:229: // instanceForMachine returns the environs.Instance that represents this machines' running On 2012/06/16 01:18:43, niemeyer wrote: > s/'s running instance// Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:239: if id == "" { On 2012/06/16 01:18:43, niemeyer wrote: > Ugh. Please add a TODO here: > > // TODO InstanceId should return an error if the id isn't set. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning.go#ne... cmd/jujud/provisioning.go:243: insts, err := p.environ.Instances([]string{id}) On 2012/06/16 01:18:43, niemeyer wrote: > This is a remote call onto the provider, which means it has a relatively high > cost. We're taking a list of instances, splitting them up, and then doing N > calls with a list of len 1, when we can actually do a single call with list of > N. The underlying Instances call was made that way precisely so we can do that. > > This is an optimization, though, and I'm happy to have that in a separate branch > so you can integrate this now. Please add a TODO here if you decide to do this. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:55: func (s *ProvisioningSuite) invalidateEnvironment() error { On 2012/06/16 01:18:43, niemeyer wrote: > Missing docs. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:65: func (s *ProvisioningSuite) fixEnvironment() error { On 2012/06/16 01:18:43, niemeyer wrote: > Missing docs. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:79: // checkStartInstance checks that a machine has been started. On 2012/06/16 01:18:43, niemeyer wrote: > Let's please use the "machine" and "instance" terms sensibly. They mean > different things for the juju implementation, and thus shouldn't be used as > synonyms. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:82: // provisioners. On 2012/06/16 01:18:43, niemeyer wrote: > Please reindent comment. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:85: c.Check(o.Kind, Equals, dummy.OpStartInstance) On 2012/06/16 01:18:43, niemeyer wrote: > There's no reason for this to be the first operation in the channel. The > provisioner implementation is free to do whatever it pleases before that. It > should loop waiting for this operation to show up. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:87: c.Errorf("ProvisioningAgent did not action AddMachine after 3 second") On 2012/06/16 01:18:43, niemeyer wrote: > The message disagrees with the actual verification. > > I'd also omit the timing from the error. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:91: // checkStopInstance checks that a machine has been stopped. On 2012/06/16 01:18:43, niemeyer wrote: > Ditto re. instance/machine. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:94: // provisioners. On 2012/06/16 01:18:43, niemeyer wrote: > Please reindent comment. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:97: c.Check(o.Kind, Equals, dummy.OpStopInstances) On 2012/06/16 01:18:43, niemeyer wrote: > Ditto for the looping. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:99: c.Errorf("ProvisioningAgent did not action RmoveMachine after 3 second") On 2012/06/16 01:18:43, niemeyer wrote: > Ditto for the message. Done. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:167: s.checkStartInstance(c, op) On 2012/06/16 01:18:43, niemeyer wrote: > Should also test that the instance id for the started machine was properly set > in the machine. Would also be nice to start at least another one with the same > tests, to make sure the logic works for more than a single case, and then stop a > single one of them, to make sure it also knows how to stop a *specific* machine. Dummy does not currently provide that information, only the name of the environment (state.name) that was invoked against. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:194: c.Errorf("provisioner started an instance") On 2012/06/16 01:18:43, niemeyer wrote: > All of these verifications of <-op have to be fixed. They shouldn't assume they > know what the operation is, nor that they know what other calls the provisioner > implementation is using internally and in which order. Yup, that was not precise enough. Done.
Sign in to reply to this message.
Almost there. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:167: s.checkStartInstance(c, op) On 2012/06/18 05:15:01, dfc wrote: > On 2012/06/16 01:18:43, niemeyer wrote: > > Should also test that the instance id for the started machine was properly set > > in the machine. Would also be nice to start at least another one with the same > > tests, to make sure the logic works for more than a single case, and then stop > a > > single one of them, to make sure it also knows how to stop a *specific* > machine. > > Dummy does not currently provide that information, only the name of the > environment (state.name) that was invoked against. I don't think I understand the issue. The machine in state must necessarily its instance id set. If it doesn't, our logic is broken, and this test should break too, right? https://codereview.appspot.com/6294066/diff/16001/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/16001/cmd/jujud/provisioning.go#n... cmd/jujud/provisioning.go:58: // instance.Id() => *state.Machine s/()// (so they're both equivalent) https://codereview.appspot.com/6294066/diff/16001/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/16001/cmd/jujud/provisioning_test... cmd/jujud/provisioning_test.go:89: case dummy.OpStartInstance: Isn't that still broken? It's ignoring the operation no matter what its kind is (return and // ignore do the same thing here). It has to loop around the switch waiting for the right operation to show up, and then blow up if it doesn't show up in the right amount of time. https://codereview.appspot.com/6294066/diff/16001/cmd/jujud/provisioning_test... cmd/jujud/provisioning_test.go:122: case dummy.OpStopInstances: Same issue.
Sign in to reply to this message.
Thank you for review, i'll address these points now. https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/8005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:167: s.checkStartInstance(c, op) > I don't think I understand the issue. The machine in state must necessarily its > instance id set. If it doesn't, our logic is broken, and this test should break > too, right? Yup, I'll add a check to verify that machine.InstanceId() isn't "" after each machine is started. At the moment that will have to be done via polling as you can't watch a machines config. I've added a TODO for that feature. https://codereview.appspot.com/6294066/diff/16001/cmd/jujud/provisioning.go File cmd/jujud/provisioning.go (right): https://codereview.appspot.com/6294066/diff/16001/cmd/jujud/provisioning.go#n... cmd/jujud/provisioning.go:58: // instance.Id() => *state.Machine On 2012/06/18 22:08:49, niemeyer wrote: > s/()// (so they're both equivalent) Done. https://codereview.appspot.com/6294066/diff/16001/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/16001/cmd/jujud/provisioning_test... cmd/jujud/provisioning_test.go:89: case dummy.OpStartInstance: On 2012/06/18 22:08:49, niemeyer wrote: > Isn't that still broken? It's ignoring the operation no matter what its kind is > (return and // ignore do the same thing here). It has to loop around the switch > waiting for the right operation to show up, and then blow up if it doesn't show > up in the right amount of time. Sorry, that was my fault, I had the correct logic in checkNotStartInstance https://codereview.appspot.com/6294066/diff/16001/cmd/jujud/provisioning_test... cmd/jujud/provisioning_test.go:122: case dummy.OpStopInstances: On 2012/06/18 22:08:49, niemeyer wrote: > Same issue. Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
A couple of ideas for your appraisal, but it LGTM in either case. Thanks for the ride. https://codereview.appspot.com/6294066/diff/4005/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/4005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:138: case <-time.After(3 * time.Second): Why the former one is 2, and this is 3? Perhaps even better.. these two functions change only the operation kind they're expecting, and the message that is shown. Maybe make both a single underlying function? https://codereview.appspot.com/6294066/diff/4005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:148: for a := veryShortAttempt.Start(); a.Next(); { Hah, nice use case. https://codereview.appspot.com/6294066/diff/4005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:164: for a := veryShortAttempt.Start(); a.Next(); { These two functions are also pretty much the same. Make them into one?
Sign in to reply to this message.
Thanks for sticking with me, will address your final comments and submit. Then it's on to shutting down unknown machines (which is a much smaller changeset) https://codereview.appspot.com/6294066/diff/4005/cmd/jujud/provisioning_test.go File cmd/jujud/provisioning_test.go (right): https://codereview.appspot.com/6294066/diff/4005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:138: case <-time.After(3 * time.Second): On 2012/06/19 01:08:46, niemeyer wrote: > Why the former one is 2, and this is 3? > > Perhaps even better.. these two functions change only the operation kind they're > expecting, and the message that is shown. Maybe make both a single underlying > function? I can probably make them even shorter. What actually matters is the call to time.Sleep() so there is a trip through the scheduler. Although relying on that behaviour is not a long term strategy. https://codereview.appspot.com/6294066/diff/4005/cmd/jujud/provisioning_test.... cmd/jujud/provisioning_test.go:164: for a := veryShortAttempt.Start(); a.Next(); { On 2012/06/19 01:08:46, niemeyer wrote: > These two functions are also pretty much the same. Make them into one? PTAL
Sign in to reply to this message.
*** Submitted: cmd/jujud: provisioner can now start and stop instances This branch adds the ability for the provisioning agent to start and stop instances according to changes in state. To be implemented in a later branch is the ability for the PA to reconcile the set of instances running with the set of instances known to the state. R=rog, niemeyer CC= https://codereview.appspot.com/6294066
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
