Issue 6404051: firewaller: added handling of machine units changes (Closed)
|
Created:
13 years, 1 month ago by TheMue Modified:
13 years ago Reviewers:
mp+115167 Visibility:
Public. |
Descriptionfirewaller: added handling of machine units changes
The second iteration of the firewaller adds the handling of
changes of the machine units. Here the adding of services is
missing and will be one of the next steps. The next step
itself will be the handling of unit ports changes.
https://code.launchpad.net/~themue/juju-core/go-worker-firewaller-machineunits/+merge/115167
Requires: https://code.launchpad.net/~themue/juju-core/go-worker-firewaller-machines/+merge/114938
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 13
Patch Set 2 : firewaller: added handling of machine units changes #Patch Set 3 : firewaller: added handling of machine units changes #Patch Set 4 : firewaller: added handling of machine units changes #Patch Set 5 : firewaller: added handling of machine units changes #Patch Set 6 : firewaller: added handling of machine units changes #Patch Set 7 : firewaller: added handling of machine units changes #
Total comments: 6
Patch Set 8 : firewaller: added handling of machine units changes #
Total comments: 38
Patch Set 9 : firewaller: added handling of machine units changes #
Total comments: 8
Patch Set 10 : firewaller: added handling of machine units changes #Patch Set 11 : firewaller: added handling of machine units changes #
MessagesTotal messages: 14
Please take a look.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:101: if change.stateChange != nil { when could change.stateChange be nil? https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:153: func (m *machine) loop(fw *Firewaller) { perhaps a comment on this function? // loop waits for changes to the machine's units // and funnels them to the firewall's machineUnitsChanges // channel.
Sign in to reply to this message.
Added comments, more later. https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:101: if change.stateChange != nil { On 2012/07/16 16:58:02, rog wrote: > when could change.stateChange be nil? Interestingly I had it during testing. Will dig deeper, IMHO the initial watcher value. https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:153: func (m *machine) loop(fw *Firewaller) { On 2012/07/16 16:58:02, rog wrote: > perhaps a comment on this function? > > // loop waits for changes to the machine's units > // and funnels them to the firewall's machineUnitsChanges > // channel. Will comment all, I dislike functions and methods without comments, even when private. Until now the comments only separate the internal types.
Sign in to reply to this message.
Essentially, same issues as the prereq. https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:98: fw.tomb.Killf("aggregation of machine units changes failed") This maybe also feels like a panic situation, now I come to look at it. (I know we don't panic in every analogous situation, but maybe we should: closure of a locally-owned channel tat should stay open until the method exits feels like solid evidence of fundamental logic error to me) https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:106: // a not yet managed unit? +1 https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:108: u.watcher.Stop() Doesn't guarantee we won't get more events on unitPortsChanges (see previous review). https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:198: return fw.tomb.Kill(watcher.MustErr(u.watcher)) (uncertainty as in previous review)
Sign in to reply to this message.
There are unhandled reviews on this. I'll wait for the next round.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:98: fw.tomb.Killf("aggregation of machine units changes failed") On 2012/07/16 23:42:29, fwereade wrote: > This maybe also feels like a panic situation, now I come to look at it. (I know > we don't panic in every analogous situation, but maybe we should: closure of a > locally-owned channel tat should stay open until the method exits feels like > solid evidence of fundamental logic error to me) You're right, this is a hard error. https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:101: if change.stateChange != nil { Ah, forgot it that stateChange == nil shows that the tracker died, Changed the code to make it more clear, error handling has been wrong this way. https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:106: // a not yet managed unit? On 2012/07/16 23:42:29, fwereade wrote: > +1 Done. https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:108: u.watcher.Stop() On 2012/07/16 23:42:29, fwereade wrote: > Doesn't guarantee we won't get more events on unitPortsChanges (see previous > review). Done. https://codereview.appspot.com/6404051/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:198: return On 2012/07/16 23:42:29, fwereade wrote: > fw.tomb.Kill(watcher.MustErr(u.watcher)) > > (uncertainty as in previous review) Done.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6404051/diff/11057/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6404051/diff/11057/worker/firewaller/firewalle... worker/firewaller/firewaller.go:64: case change, ok := <-fw.machineUnitsChanges: we never close machineUnitsChanges (trivially verifiable), so why not just: case change := <-fw.machineUnitsChanges: and lose the test? https://codereview.appspot.com/6404051/diff/11057/worker/firewaller/firewalle... worker/firewaller/firewaller.go:69: log.Printf("tracker of machine %d terminated prematurely", change.machine.id) we're losing the error message from the machine tracker termination here. how about: log.Printf("tracker of machine %d terminated prematurely: %v", change.machine.stop()) ? https://codereview.appspot.com/6404051/diff/11057/worker/firewaller/firewalle... worker/firewaller/firewaller.go:136: // newMachineTracker creates a new machine tracker keeping track of it occurs to me that this thing doesn't actually keep track of anything - it simply forwards machine units changes to the central goroutine. perhaps: // newMachineTracker tracks changes unit changes to // the given machine and sends them to the // central firewaller loop. ?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6404051/diff/11057/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6404051/diff/11057/worker/firewaller/firewalle... worker/firewaller/firewaller.go:64: case change, ok := <-fw.machineUnitsChanges: On 2012/07/19 17:04:05, rog wrote: > we never close machineUnitsChanges (trivially verifiable), so why not just: > > case change := <-fw.machineUnitsChanges: > > and lose the test? Done. https://codereview.appspot.com/6404051/diff/11057/worker/firewaller/firewalle... worker/firewaller/firewaller.go:69: log.Printf("tracker of machine %d terminated prematurely", change.machine.id) On 2012/07/19 17:04:05, rog wrote: > we're losing the error message from the machine tracker termination here. how > about: > log.Printf("tracker of machine %d terminated prematurely: %v", > change.machine.stop()) > ? Done. https://codereview.appspot.com/6404051/diff/11057/worker/firewaller/firewalle... worker/firewaller/firewaller.go:136: // newMachineTracker creates a new machine tracker keeping track of On 2012/07/19 17:04:05, rog wrote: > it occurs to me that this thing doesn't actually keep track of anything - it > simply forwards machine units changes to > the central goroutine. > > perhaps: > // newMachineTracker tracks changes unit changes to > // the given machine and sends them to the > // central firewaller loop. > > ? Done.
Sign in to reply to this message.
https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:16: machines map[int]*machineTracker s/machines/machineTrackers https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:18: units map[string]*unitTracker s/units/unitTrackers/ https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:20: services map[string]*serviceTracker s/services/serviceTrackers https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:48: for _, removedMachine := range change.Removed { s/removedMachine/machine/ https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:49: mt, ok := fw.machines[removedMachine.Id()] s/mt/machinet/ https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:59: for _, addedMachine := range change.Added { s/addedMachine/machine/ https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:60: mt := newMachineTracker(addedMachine, fw) s/mt/machinet/ https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:65: if change.change == nil { As we talked, this seems to just create extra churn. I think we can drop it, and change the sending side so it never sends nil stuff. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:70: for _, removedUnit := range change.change.Removed { s/removedUnit/unit/ https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:71: ut, ok := fw.units[removedUnit.Name()] s/ut/unitt/ https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:79: log.Debugf("firewaller: stopped tracking unit %s", removedUnit.Name()) Shouldn't the unit ports in this machine be closed here? https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:81: for _, addedUnit := range change.change.Added { s/addedUnit/unit/ https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:82: ut := newUnitTracker(addedUnit, fw) s/ut/unitt/ https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:120: machine *machineTracker s/machine/machinet/ https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:121: change *state.MachineUnitsChange s/change //; so that you can use the fields of the Change type directly rather than doing change.change.etc, which looks suboptimal. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:130: ports map[state.Port]*unitTracker As we talked, there's no one-to-one port<=>unit relation in the context of a whole machine. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:178: unit *unitTracker s/unit/unitt/ https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:179: change []state.Port s/change/ports/; so we get change.ports rather than change.change https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:220: if !ok { Same comment as above regarding late handling of ok.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:16: machines map[int]*machineTracker On 2012/07/23 18:58:05, niemeyer wrote: > s/machines/machineTrackers Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:18: units map[string]*unitTracker On 2012/07/23 18:58:05, niemeyer wrote: > s/units/unitTrackers/ Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:20: services map[string]*serviceTracker On 2012/07/23 18:58:05, niemeyer wrote: > s/services/serviceTrackers Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:48: for _, removedMachine := range change.Removed { On 2012/07/23 18:58:05, niemeyer wrote: > s/removedMachine/machine/ Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:49: mt, ok := fw.machines[removedMachine.Id()] On 2012/07/23 18:58:05, niemeyer wrote: > s/mt/machinet/ Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:59: for _, addedMachine := range change.Added { On 2012/07/23 18:58:05, niemeyer wrote: > s/addedMachine/machine/ Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:60: mt := newMachineTracker(addedMachine, fw) On 2012/07/23 18:58:05, niemeyer wrote: > s/mt/machinet/ Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:65: if change.change == nil { On 2012/07/23 18:58:05, niemeyer wrote: > As we talked, this seems to just create extra churn. I think we can drop it, and > change the sending side so it never sends nil stuff. Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:70: for _, removedUnit := range change.change.Removed { On 2012/07/23 18:58:05, niemeyer wrote: > s/removedUnit/unit/ Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:71: ut, ok := fw.units[removedUnit.Name()] On 2012/07/23 18:58:05, niemeyer wrote: > s/ut/unitt/ Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:79: log.Debugf("firewaller: stopped tracking unit %s", removedUnit.Name()) On 2012/07/23 18:58:05, niemeyer wrote: > Shouldn't the unit ports in this machine be closed here? Added a TODO, next branch will do it. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:81: for _, addedUnit := range change.change.Added { On 2012/07/23 18:58:05, niemeyer wrote: > s/addedUnit/unit/ Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:82: ut := newUnitTracker(addedUnit, fw) On 2012/07/23 18:58:05, niemeyer wrote: > s/ut/unitt/ Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:120: machine *machineTracker On 2012/07/23 18:58:05, niemeyer wrote: > s/machine/machinet/ Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:121: change *state.MachineUnitsChange On 2012/07/23 18:58:05, niemeyer wrote: > s/change //; so that you can use the fields of the Change type directly rather > than doing change.change.etc, which looks suboptimal. Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:130: ports map[state.Port]*unitTracker On 2012/07/23 18:58:05, niemeyer wrote: > As we talked, there's no one-to-one port<=>unit relation in the context of a > whole machine. Removed as it's not yet used in this branch. Solutions as discussed will e changed in follow-up. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:178: unit *unitTracker On 2012/07/23 18:58:05, niemeyer wrote: > s/unit/unitt/ Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:179: change []state.Port On 2012/07/23 18:58:05, niemeyer wrote: > s/change/ports/; so we get change.ports rather than change.change Done. https://codereview.appspot.com/6404051/diff/13004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:220: if !ok { On 2012/07/23 18:58:05, niemeyer wrote: > Same comment as above regarding late handling of ok. Done.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6404051/diff/10058/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6404051/diff/10058/worker/firewaller/firewalle... worker/firewaller/firewaller.go:16: machineDatas map[int]*machineData As discussed: machineds, unitds, serviceds https://codereview.appspot.com/6404051/diff/10058/worker/firewaller/firewalle... worker/firewaller/firewaller.go:17: machineUnitsChanges chan *machineUnitsChange s/machineUnitsChanges/unitsChange/ (both in the type name and in the field name) https://codereview.appspot.com/6404051/diff/10058/worker/firewaller/firewalle... worker/firewaller/firewaller.go:19: unitPortsChanges chan *unitPortsChange s/unitPortsChanges/portsChange/ (both in the type and in the field name) https://codereview.appspot.com/6404051/diff/10058/worker/firewaller/firewalle... worker/firewaller/firewaller.go:73: log.Printf("unit data %s returned error when stopping: %v", unit.Name(), err) s/%s/%q/ s/data/watcher/
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6404051/diff/10058/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6404051/diff/10058/worker/firewaller/firewalle... worker/firewaller/firewaller.go:16: machineDatas map[int]*machineData On 2012/07/24 13:29:21, niemeyer wrote: > As discussed: machineds, unitds, serviceds Done. https://codereview.appspot.com/6404051/diff/10058/worker/firewaller/firewalle... worker/firewaller/firewaller.go:17: machineUnitsChanges chan *machineUnitsChange On 2012/07/24 13:29:21, niemeyer wrote: > s/machineUnitsChanges/unitsChange/ (both in the type name and in the field name) Done. https://codereview.appspot.com/6404051/diff/10058/worker/firewaller/firewalle... worker/firewaller/firewaller.go:19: unitPortsChanges chan *unitPortsChange On 2012/07/24 13:29:21, niemeyer wrote: > s/unitPortsChanges/portsChange/ (both in the type and in the field name) Done. https://codereview.appspot.com/6404051/diff/10058/worker/firewaller/firewalle... worker/firewaller/firewaller.go:73: log.Printf("unit data %s returned error when stopping: %v", unit.Name(), err) On 2012/07/24 13:29:21, niemeyer wrote: > s/%s/%q/ > s/data/watcher/ Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted: firewaller: added handling of machine units changes The second iteration of the firewaller adds the handling of changes of the machine units. Here the adding of services is missing and will be one of the next steps. The next step itself will be the handling of unit ports changes. R=rog, fwereade, niemeyer CC= https://codereview.appspot.com/6404051
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
