jujud: add primordial provisioning agent

jujud: add primordial provisioning agent



https://code.launchpad.net/~dave-cheney/juju/go-provisioning-agent/+merge/105912

Requires: https://code.launchpad.net/~dave-cheney/juju/go-environ-schema/+merge/105769

(do not edit description out of merge proposal)

[fwereade, 2012-05-18 09:08:25]

LGTM apart from NewProviderService; I'm not sure whether it should take a State,
or just the watcher, but I don't think it wants the Info at all.

https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provider_service.go
File cmd/jujud/provider_service.go (right):

https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provider_service.g...
cmd/jujud/provider_service.go:29: func NewProviderService(watcher
*state.ConfigWatcher, state *state.Info) ProviderService {
What's state for?

https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provider_service.g...
cmd/jujud/provider_service.go:81: kind, ok := config.Get("kind")
Shouldn't this be "type" according to the config file? Or are we going with
different formats?

https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provider_service.g...
cmd/jujud/provider_service.go:90: select {
I find this very pleasing.

https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:34: os.Exit(1)
I'd prefer not to jump across layers like this... can't we just return the error
from Run? We'll exit 1 anyway soon enough...

https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:43: envwatcher := state.WatchEnvrionConfig()
s/WatchEnvrionConfig/WatchEnvironConfig/

...and presumably in some other places too ;).

[dave_cheney.net, 2012-05-18 09:16:20]

The requirement to pass in the state.Info is a wart, currently
environ.StartInstance needs it. I'll put it in my backlog to figure out how to
remove that requirement. 

On 18/05/2012, at 19:08, fwereade@gmail.com wrote:

> LGTM apart from NewProviderService; I'm not sure whether it should take
> a State, or just the watcher, but I don't think it wants the Info at
> all.
> 
> 
>
https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provider_service.go
> File cmd/jujud/provider_service.go (right):
> 
>
https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provider_service.g...
> cmd/jujud/provider_service.go:29: func NewProviderService(watcher
> *state.ConfigWatcher, state *state.Info) ProviderService {
> What's state for?
> 
>
https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provider_service.g...
> cmd/jujud/provider_service.go:81: kind, ok := config.Get("kind")
> Shouldn't this be "type" according to the config file? Or are we going
> with different formats?
> 
>
https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provider_service.g...
> cmd/jujud/provider_service.go:90: select {
> I find this very pleasing.
> 
> https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provisioning.go
> File cmd/jujud/provisioning.go (right):
> 
>
https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provisioning.go#ne...
> cmd/jujud/provisioning.go:34: os.Exit(1)
> I'd prefer not to jump across layers like this... can't we just return
> the error from Run? We'll exit 1 anyway soon enough...
> 
>
https://codereview.appspot.com/6203083/diff/6001/cmd/jujud/provisioning.go#ne...
> cmd/jujud/provisioning.go:43: envwatcher := state.WatchEnvrionConfig()
> s/WatchEnvrionConfig/WatchEnvironConfig/
> 
> ...and presumably in some other places too ;).
> 
> https://codereview.appspot.com/6203083/

[rog, 2012-05-18 13:42:12]

as discussed on IRC this morning, i think there might be a better approach. we
will see!

[TheMue, 2012-05-21 07:23:28]

I'm missing some helpful comments to get a better understanding of what happens
here and why.

https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go
File cmd/jujud/machines.go (right):

https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go#newco...
cmd/jujud/machines.go:13: mu  sync.Mutex       // protects mm
mutex or mux is better than mu.

https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go#newco...
cmd/jujud/machines.go:14: mm  map[int]*machine // maps from state machine id to
*machine worker
machineMap or just machines will make it easier to read the code later.

https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go#newco...
cmd/jujud/machines.go:30: m.mu.Unlock()
defer m.mu.Unlock() after Lock() unlocks even if createMachine() panics.

https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go#newco...
cmd/jujud/machines.go:45: // must be called with m.mu locked, see StartMachine
// createMachine returns a new machine with the given id. It has
// to be called with m.mutex locked due to the registration of
// the machine in m.machines.

https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go#newco...
cmd/jujud/machines.go:67: type machine struct {
Missing comments, also for many methods.

[dave_cheney.net, 2012-05-21 07:26:47]

Thanks frank, this is still an WIP, i'll let you know when it's ready
for review.

On Mon, May 21, 2012 at 5:23 PM,  <themue@gmail.com> wrote:
> I'm missing some helpful comments to get a better understanding of what
> happens here and why.
>
>
> https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go
> File cmd/jujud/machines.go (right):
>
>
https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go#newco...
> cmd/jujud/machines.go:13: mu  sync.Mutex       // protects mm
> mutex or mux is better than mu.
>
>
https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go#newco...
> cmd/jujud/machines.go:14: mm  map[int]*machine // maps from state
> machine id to *machine worker
> machineMap or just machines will make it easier to read the code later.
>
>
https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go#newco...
> cmd/jujud/machines.go:30: m.mu.Unlock()
> defer m.mu.Unlock() after Lock() unlocks even if createMachine() panics.
>
>
https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go#newco...
> cmd/jujud/machines.go:45: // must be called with m.mu locked, see
> StartMachine
> // createMachine returns a new machine with the given id. It has
> // to be called with m.mutex locked due to the registration of
> // the machine in m.machines.
>
>
https://codereview.appspot.com/6203083/diff/12001/cmd/jujud/machines.go#newco...
> cmd/jujud/machines.go:67: type machine struct {
> Missing comments, also for many methods.
>
> https://codereview.appspot.com/6203083/