Issue 6875053: firewaller: init machined ports in global mode (Closed)
Descriptionfirewaller: init machined ports in global mode
This is the first one of two CLs for a proper initialization
of the firewaller if the mode is the global mode. This CLs adds
the init of machined.ports when machined is started. Here the
ports field as well as the global ports ref is set based on the
state at the time of start.
https://code.launchpad.net/~themue/juju-core/005-firewaller-machined-ports/+merge/137768
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 1
Patch Set 2 : firewaller: init machined ports in global mode #Patch Set 3 : firewaller: init machined ports in global mode #Patch Set 4 : firewaller: init machined ports in global mode #Patch Set 5 : firewaller: init machined ports in global mode #
Total comments: 19
Patch Set 6 : firewaller: init machined ports in global mode #
Total comments: 12
Patch Set 7 : firewaller: init machined ports in global mode #
Total comments: 24
Patch Set 8 : firewaller: init machined ports in global mode #
Total comments: 9
Patch Set 9 : firewaller: init machined ports in global mode #
Total comments: 2
Patch Set 10 : firewaller: init machined ports in global mode #Patch Set 11 : firewaller: init machined ports in global mode #Patch Set 12 : firewaller: init machined ports in global mode #
Total comments: 4
Patch Set 13 : firewaller: init machined ports in global mode #
Total comments: 37
Patch Set 14 : firewaller: init machined ports in global mode #
Total comments: 2
Patch Set 15 : firewaller: init machined ports in global mode #
MessagesTotal messages: 25
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/6875053/diff/1/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:362: fw.globalPortRef[uport]++ There's still a window in between initGlobalPorts and machineLifeChanged in which the wrong change can lead to uncloseable ports.
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.
Please take a look.
Sign in to reply to this message.
Thanks -- this is *much* more like it. A few comments still, but I think we're on the right track. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:124: m, err := md.machine() if state.IsNotFound(err) { return nil } else if err != nil { // handle err } perhaps? https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:132: change := <-md.unitw.Changes() Need to select on fw.tomb.Dying() as well; and also handle unitw errors. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:134: if err != nil { stop watcher on error https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:147: ports: make([]state.Port, 0), I think this can/should be taken directly from unit.OpenedPorts. (otherwise the data is not correct and complete by the time this method returns.) https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:187: if fw.globalMode { I think I'd find this easier to read if it were split into reconcileGlobal and reconcileInstance or similar. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:545: unitw *state.MachineUnitsWatcher I'd probably pass this through to watchLoop directly myself and save a field, but YMMV. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:605: // managed by the firewaller. Sorry, would you clarify this please? https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:606: var ports []state.Port This should also come from unit.OpenedPorts, so we can discard non-ports changes and avoid sending events we don't need to.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:124: m, err := md.machine() On 2012/12/10 23:01:38, fwereade wrote: > if state.IsNotFound(err) { > return nil > } else if err != nil { > // handle err > } > > perhaps? Done. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:132: change := <-md.unitw.Changes() On 2012/12/10 23:01:38, fwereade wrote: > Need to select on fw.tomb.Dying() as well; and also handle unitw errors. Reacting on a dying signal here leads to a not running watchLoop() which is needed by stopWatchers() when Firewaller.loop() is terminating. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:134: if err != nil { On 2012/12/10 23:01:38, fwereade wrote: > stop watcher on error Done. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:147: ports: make([]state.Port, 0), On 2012/12/10 23:01:38, fwereade wrote: > I think this can/should be taken directly from unit.OpenedPorts. (otherwise the > data is not correct and complete by the time this method returns.) Done. But it has to take care if the service of this unit is already exposed. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:187: if fw.globalMode { On 2012/12/10 23:01:38, fwereade wrote: > I think I'd find this easier to read if it were split into reconcileGlobal and > reconcileInstance or similar. Done. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:545: unitw *state.MachineUnitsWatcher On 2012/12/10 23:01:38, fwereade wrote: > I'd probably pass this through to watchLoop directly myself and save a field, > but YMMV. I need this watcher before starting the watchLoop() to consume the initial event. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:605: // managed by the firewaller. On 2012/12/10 23:01:38, fwereade wrote: > Sorry, would you clarify this please? Done. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:606: var ports []state.Port On 2012/12/10 23:01:38, fwereade wrote: > This should also come from unit.OpenedPorts, so we can discard non-ports changes > and avoid sending events we don't need to. Done, but from the initial ud.ports, see above.
Sign in to reply to this message.
Getting closer :) https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:132: change := <-md.unitw.Changes() On 2012/12/11 12:25:40, TheMue wrote: > On 2012/12/10 23:01:38, fwereade wrote: > > Need to select on fw.tomb.Dying() as well; and also handle unitw errors. > > Reacting on a dying signal here leads to a not running watchLoop() which is > needed by stopWatchers() when Firewaller.loop() is terminating. Is there any reason that the md needs to be put into the machineds map before we've received this event? https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:187: if fw.globalMode { On 2012/12/11 12:25:40, TheMue wrote: > On 2012/12/10 23:01:38, fwereade wrote: > > I think I'd find this easier to read if it were split into reconcileGlobal and > > reconcileInstance or similar. > > Done. Thanks, that's much clearer. https://codereview.appspot.com/6875053/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:545: unitw *state.MachineUnitsWatcher On 2012/12/11 12:25:40, TheMue wrote: > On 2012/12/10 23:01:38, fwereade wrote: > > I'd probably pass this through to watchLoop directly myself and save a field, > > but YMMV. > > I need this watcher before starting the watchLoop() to consume the initial > event. It doesn't need to be stored in a field, though, does it? https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:130: if state.IsNotFound(err) { Whoa, sorry, I should have been clearer. If we're returning nil on not found (which I still think we should), we shouldn't leave a non-started md lying around in machineds. https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:139: fw.tomb.Kill(watcher.MustErr(md.unitw)) Sorry, I've only just focused on this -- is there some reason we have to kill fw.tomb here? Won't an error return bubble up to a sensible place anyway? https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:166: ud.ports = unit.OpenedPorts() I don't get this. I thought service exposure was used to filter out irrelevant units at the last moment -- otherwise, how do we detect a machine's port changes when one of its units' service is exposed? https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:213: collector[port] = true Why aren't we paying attention to service exposure here? https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:346: if unitd.serviced.exposed { Yeah, this bit looks right. Why don't we do this in reconcileGlobal? How does it interact with that service exposure check I'm complaining about in startUnit? https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:629: copy(ports, ud.ports) I don't think that's goroutine-safe, is it? Surely you should be passing a copy of ud.ports into watchLoop, rather than accessing it here...
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:130: if state.IsNotFound(err) { On 2012/12/11 15:20:43, fwereade wrote: > Whoa, sorry, I should have been clearer. If we're returning nil on not found > (which I still think we should), we shouldn't leave a non-started md lying > around in machineds. Done. https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:139: fw.tomb.Kill(watcher.MustErr(md.unitw)) On 2012/12/11 15:20:43, fwereade wrote: > Sorry, I've only just focused on this -- is there some reason we have to kill > fw.tomb here? Won't an error return bubble up to a sensible place anyway? Done. https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:166: ud.ports = unit.OpenedPorts() On 2012/12/11 15:20:43, fwereade wrote: > I don't get this. I thought service exposure was used to filter out irrelevant > units at the last moment -- otherwise, how do we detect a machine's port changes > when one of its units' service is exposed? Done. https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:213: collector[port] = true On 2012/12/11 15:20:43, fwereade wrote: > Why aren't we paying attention to service exposure here? Done. https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:346: if unitd.serviced.exposed { On 2012/12/11 15:20:43, fwereade wrote: > Yeah, this bit looks right. Why don't we do this in reconcileGlobal? How does it > interact with that service exposure check I'm complaining about in startUnit? Done. https://codereview.appspot.com/6875053/diff/16001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:629: copy(ports, ud.ports) On 2012/12/11 15:20:43, fwereade wrote: > I don't think that's goroutine-safe, is it? Surely you should be passing a copy > of ud.ports into watchLoop, rather than accessing it here... Done.
Sign in to reply to this message.
We're well into trivials territory here... just a few more :). https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:132: fw.tomb.Killf("worker/firewaller: cannot watch machine units: %v", err) Shouldn't this just be a `return fmt.Errorf(`? https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:135: fw.machineds[id] = machined This line can move to... (skip a comment...) https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:137: change, ok := <-machined.unitw.Changes() I still think we should be selecting on fw.tomb.Dying() here as well -- I can't be certain offhand that there's no pathological case in which a MachineUnitsWatcher fails to either send or close its Changes chan, and I can be even less certain that no such bug will ever be introduced. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:140: } ...just after the above branch; this makes sure we don't leave unstarted mds lying around in the FW (it probably doesn't actually matter, but I'd prefer not to have to verify that in detail). https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:144: fw.tomb.Killf("worker/firewaller: cannot watch machine units: %v", err) Similarly, I don't think we should be hitting the tomb here -- I find it's easier to think about this sort of type if we keep the tomb-killing in its main loop. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:147: go machined.watchLoop() I'd still prefer to drop the unitw field and pass it in here, but I guess it's not really a big deal :). https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:170: if err != nil { This'll leave an unstarted unitData lying around in fw.unitds. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:176: go unitd.watchLoop(unitd.ports) I think you need to copy this before you send it off to another goroutine... did I miss something? https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:242: if state.IsNotFound(err) { forgetMachine? https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:245: if err != nil { I prefer to smoosh related tests into the same, er, group-of-blocks, with `else`s, but NBD. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:471: for _, unitd := range machined.unitds { I think you can drop this loop... https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:474: if err := fw.flushMachine(machined); err != nil { ...and do the flushMachine after the forgetUnit calls.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:132: fw.tomb.Killf("worker/firewaller: cannot watch machine units: %v", err) On 2012/12/11 16:51:25, fwereade wrote: > Shouldn't this just be a `return fmt.Errorf(`? Done. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:135: fw.machineds[id] = machined On 2012/12/11 16:51:25, fwereade wrote: > This line can move to... (skip a comment...) Done. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:137: change, ok := <-machined.unitw.Changes() On 2012/12/11 16:51:25, fwereade wrote: > I still think we should be selecting on fw.tomb.Dying() here as well -- I can't > be certain offhand that there's no pathological case in which a > MachineUnitsWatcher fails to either send or close its Changes chan, and I can be > even less certain that no such bug will ever be introduced. Yes, the move of the machined registration below made it possible now. Before I had blocking situations when this signal is raised with a registered machined without running loop. In that case stopping the watchers at the end of the Firewaller loop hangs. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:140: } On 2012/12/11 16:51:25, fwereade wrote: > ...just after the above branch; this makes sure we don't leave unstarted mds > lying around in the FW (it probably doesn't actually matter, but I'd prefer not > to have to verify that in detail). Done. Yes, good catch. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:144: fw.tomb.Killf("worker/firewaller: cannot watch machine units: %v", err) On 2012/12/11 16:51:25, fwereade wrote: > Similarly, I don't think we should be hitting the tomb here -- I find it's > easier to think about this sort of type if we keep the tomb-killing in its main > loop. Done. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:147: go machined.watchLoop() On 2012/12/11 16:51:25, fwereade wrote: > I'd still prefer to drop the unitw field and pass it in here, but I guess it's > not really a big deal :). Done. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:170: if err != nil { On 2012/12/11 16:51:25, fwereade wrote: > This'll leave an unstarted unitData lying around in fw.unitds. Done. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:176: go unitd.watchLoop(unitd.ports) On 2012/12/11 16:51:25, fwereade wrote: > I think you need to copy this before you send it off to another goroutine... did > I miss something? Done. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:242: if state.IsNotFound(err) { On 2012/12/11 16:51:25, fwereade wrote: > forgetMachine? Done. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:245: if err != nil { On 2012/12/11 16:51:25, fwereade wrote: > I prefer to smoosh related tests into the same, er, group-of-blocks, with > `else`s, but NBD. Done. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:471: for _, unitd := range machined.unitds { On 2012/12/11 16:51:25, fwereade wrote: > I think you can drop this loop... Done. https://codereview.appspot.com/6875053/diff/13008/worker/firewaller/firewalle... worker/firewaller/firewaller.go:474: if err := fw.flushMachine(machined); err != nil { On 2012/12/11 16:51:25, fwereade wrote: > ...and do the flushMachine after the forgetUnit calls. Done.
Sign in to reply to this message.
LGTM -- only suggestions now. Thanks for all your hard work on this :). https://codereview.appspot.com/6875053/diff/21003/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/21003/worker/firewaller/firewalle... worker/firewaller/firewaller.go:137: return nil I think this is a good place for tomb.ErrDying https://codereview.appspot.com/6875053/diff/21003/worker/firewaller/firewalle... worker/firewaller/firewaller.go:145: watcher.Stop(unitw, &fw.tomb) I think this should just be a plain Stop(), and log the error if there is one. https://codereview.appspot.com/6875053/diff/21003/worker/firewaller/firewalle... worker/firewaller/firewaller.go:207: // units and services with the opened and closed ports globally. ..."and opens and closes the appropriate ports for the whole environment"? or something? https://codereview.appspot.com/6875053/diff/21003/worker/firewaller/firewalle... worker/firewaller/firewaller.go:246: // units and services with the opened and closed ports of the instances. similarly https://codereview.appspot.com/6875053/diff/21003/worker/firewaller/firewalle... worker/firewaller/firewaller.go:628: ports := initialPorts just a thought -- call initialPorts latestPorts, and drop ports?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6875053/diff/21003/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/21003/worker/firewaller/firewalle... worker/firewaller/firewaller.go:137: return nil On 2012/12/12 11:19:00, fwereade wrote: > I think this is a good place for tomb.ErrDying Done. https://codereview.appspot.com/6875053/diff/21003/worker/firewaller/firewalle... worker/firewaller/firewaller.go:207: // units and services with the opened and closed ports globally. On 2012/12/12 11:19:00, fwereade wrote: > ..."and opens and closes the appropriate ports for the whole environment"? > > or something? Done. https://codereview.appspot.com/6875053/diff/21003/worker/firewaller/firewalle... worker/firewaller/firewaller.go:246: // units and services with the opened and closed ports of the instances. On 2012/12/12 11:19:00, fwereade wrote: > similarly Done. https://codereview.appspot.com/6875053/diff/21003/worker/firewaller/firewalle... worker/firewaller/firewaller.go:628: ports := initialPorts On 2012/12/12 11:19:00, fwereade wrote: > just a thought -- call initialPorts latestPorts, and drop ports? Done.
Sign in to reply to this message.
Still LGTM :) https://codereview.appspot.com/6875053/diff/21004/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/21004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:146: log.Printf("worker/firewaller: cannot stop units watcher: %v", werr) Technically we won't return until we do manage to stop it, so "error stopping machine units watcher" might be sensible.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6875053/diff/21004/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/21004/worker/firewaller/firewalle... worker/firewaller/firewaller.go:146: log.Printf("worker/firewaller: cannot stop units watcher: %v", werr) On 2012/12/12 15:27:31, fwereade wrote: > Technically we won't return until we do manage to stop it, so "error stopping > machine units watcher" might be sensible. Done.
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.
Just noticed the below -- I'll do a final pass later today just in case anything else has slipped in. https://codereview.appspot.com/6875053/diff/28001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/28001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:137: return tomb.ErrDying You need to stop the watcher here... https://codereview.appspot.com/6875053/diff/28001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:140: return watcher.MustErr(unitw) ...and here.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6875053/diff/28001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/28001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:137: return tomb.ErrDying On 2012/12/14 08:17:21, fwereade wrote: > You need to stop the watcher here... Done. https://codereview.appspot.com/6875053/diff/28001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:140: return watcher.MustErr(unitw) On 2012/12/14 08:17:21, fwereade wrote: > ...and here. Done.
Sign in to reply to this message.
That looks like a winning approach, Frank. Some comments: https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:84: if !reconciled { Would you mind to put "reconciled = true" here as a first line? It's easier to understand the one-shot semantics. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:137: if err := unitw.Stop(); err != nil { stop("units watcher", unitw) https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:143: if err := unitw.Stop(); err != nil { stop("units watcher", unitw) https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:151: if werr := unitw.Stop(); werr != nil { stop("units watcher", unitw) https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:154: return fmt.Errorf("worker/firewaller: cannot watch machine units: %v", err) "cannot watch"!? We can't possibly tell this is what failed from here. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:159: } func stop(what string, stopper watcher.Stopper) { if err := stopper.Stop(); err != nil { log.Printf("worker/firewaller: error stopping %s: %v", what, err) } } https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:162: // unit and starts watching the unit for port changes. // The provided machineId must be the id for the machine the // unit was last observed to be assigned to. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:180: err := fw.startService(serviceName) This can take the service itself as a parameter. We have the service above, and the first thing the method does it to load it again. This also means that startService's error result can be dropped, so the logic below can go away. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:237: if len(toOpen) > 0 { log.Printf("worker/firewaller: opening global ports %v", toOpen) https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:242: log.Printf("worker/firewaller: initially opened ports %v in environment", toOpen) d https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:244: if len(toClose) > 0 { log.Printf("worker/firewaller: closing global ports %v", toClose) https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:249: log.Printf("worker/firewaller: initially closed ports %v in environment", toClose) d https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:261: return fw.forgetMachine(machined) Removing a machine is a perfectly normal situation. Why would we stop processing stuff and return? https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:280: if len(toOpen) > 0 { log.Printf("worker/firewaller: opening instance ports %v for machine %s", toOpen, machined.id) https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:286: log.Printf("worker/firewaller: opened ports %v on machine %s", toOpen, machined.id) d https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:288: if len(toClose) > 0 { log.Printf("worker/firewaller: closing instance ports %v for machine %s", toClose, machined.id) https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:294: log.Printf("worker/firewaller: closed ports %v on machine %s", toClose, machined.id) d
Sign in to reply to this message.
+1 to niemeyer's suggestions https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:180: err := fw.startService(serviceName) On 2012/12/14 14:23:51, niemeyer wrote: > This can take the service itself as a parameter. We have the service above, and > the first thing the method does it to load it again. This also means that > startService's error result can be dropped, so the logic below can go away. Nice. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:261: return fw.forgetMachine(machined) On 2012/12/14 14:23:51, niemeyer wrote: > Removing a machine is a perfectly normal situation. Why would we stop processing > stuff and return? Whoa, good catch :/. Sorry.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:84: if !reconciled { On 2012/12/14 14:23:51, niemeyer wrote: > Would you mind to put "reconciled = true" here as a first line? It's easier to > understand the one-shot semantics. Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:137: if err := unitw.Stop(); err != nil { On 2012/12/14 14:23:51, niemeyer wrote: > stop("units watcher", unitw) Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:143: if err := unitw.Stop(); err != nil { On 2012/12/14 14:23:51, niemeyer wrote: > stop("units watcher", unitw) Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:151: if werr := unitw.Stop(); werr != nil { On 2012/12/14 14:23:51, niemeyer wrote: > stop("units watcher", unitw) Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:154: return fmt.Errorf("worker/firewaller: cannot watch machine units: %v", err) On 2012/12/14 14:23:51, niemeyer wrote: > "cannot watch"!? We can't possibly tell this is what failed from here. Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:159: } On 2012/12/14 14:23:51, niemeyer wrote: > func stop(what string, stopper watcher.Stopper) { > if err := stopper.Stop(); err != nil { > log.Printf("worker/firewaller: error stopping %s: %v", what, err) > } > } Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:162: // unit and starts watching the unit for port changes. On 2012/12/14 14:23:51, niemeyer wrote: > // The provided machineId must be the id for the machine the > // unit was last observed to be assigned to. Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:180: err := fw.startService(serviceName) On 2012/12/14 14:23:51, niemeyer wrote: > This can take the service itself as a parameter. We have the service above, and > the first thing the method does it to load it again. This also means that > startService's error result can be dropped, so the logic below can go away. Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:237: if len(toOpen) > 0 { On 2012/12/14 14:23:51, niemeyer wrote: > log.Printf("worker/firewaller: opening global ports %v", toOpen) Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:242: log.Printf("worker/firewaller: initially opened ports %v in environment", toOpen) On 2012/12/14 14:23:51, niemeyer wrote: > d Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:244: if len(toClose) > 0 { On 2012/12/14 14:23:51, niemeyer wrote: > log.Printf("worker/firewaller: closing global ports %v", toClose) Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:249: log.Printf("worker/firewaller: initially closed ports %v in environment", toClose) On 2012/12/14 14:23:51, niemeyer wrote: > d Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:261: return fw.forgetMachine(machined) On 2012/12/14 14:23:51, niemeyer wrote: > Removing a machine is a perfectly normal situation. Why would we stop processing > stuff and return? Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:280: if len(toOpen) > 0 { On 2012/12/14 14:23:51, niemeyer wrote: > log.Printf("worker/firewaller: opening instance ports %v for machine %s", > toOpen, machined.id) Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:286: log.Printf("worker/firewaller: opened ports %v on machine %s", toOpen, machined.id) On 2012/12/14 14:23:51, niemeyer wrote: > d Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:288: if len(toClose) > 0 { On 2012/12/14 14:23:51, niemeyer wrote: > log.Printf("worker/firewaller: closing instance ports %v for machine %s", > toClose, machined.id) Done. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:294: log.Printf("worker/firewaller: closed ports %v on machine %s", toClose, machined.id) On 2012/12/14 14:23:51, niemeyer wrote: > d Done.
Sign in to reply to this message.
LGTM assuming the fixes below. Thanks. https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/30002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:180: err := fw.startService(serviceName) On 2012/12/14 14:56:58, TheMue wrote: > On 2012/12/14 14:23:51, niemeyer wrote: > > This can take the service itself as a parameter. We have the service above, > and > > the first thing the method does it to load it again. This also means that > > startService's error result can be dropped, so the logic below can go away. > > Done. Not entirely. Please do drop the error result, and remove the if block below. https://codereview.appspot.com/6875053/diff/25002/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/25002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:155: return fmt.Errorf("worker/firewaller: start watching machine %d faild: %v", id, err) That's a difference sentence with the exact same meaning. The error in this context is unrelated to "watching machine". Please just "return err" here. We can improve that later if needed.
Sign in to reply to this message.
*** Submitted: firewaller: init machined ports in global mode This is the first one of two CLs for a proper initialization of the firewaller if the mode is the global mode. This CLs adds the init of machined.ports when machined is started. Here the ports field as well as the global ports ref is set based on the state at the time of start. R=fwereade, niemeyer CC= https://codereview.appspot.com/6875053 https://codereview.appspot.com/6875053/diff/25002/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6875053/diff/25002/worker/firewaller/firewalle... worker/firewaller/firewaller.go:155: return fmt.Errorf("worker/firewaller: start watching machine %d faild: %v", id, err) On 2012/12/14 16:57:39, niemeyer wrote: > That's a difference sentence with the exact same meaning. The error in this > context is unrelated to "watching machine". > > Please just "return err" here. We can improve that later if needed. Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
