Issue 6374069: worker: started implementation of the firewaller (Closed)
Descriptionworker: started implementation of the firewaller
Based on the work of Dave and Roger this proposal implements
the first part of the firewaller worker. The major loop now
contains a machines watcher beside the environment watcher.
This machines watcher adds and removes the watchers for the
individual machine units watchers. The changes of those watchers
will be handled with the next iteration.
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: 28
Patch Set 2 : worker: started implementation of the firewaller #
Total comments: 33
Patch Set 3 : worker: started implementation of the firewaller #
Total comments: 13
Patch Set 4 : worker: started implementation of the firewaller #
Total comments: 11
Patch Set 5 : worker: started implementation of the firewaller #
MessagesTotal messages: 19
Please take a look.
Sign in to reply to this message.
mostly LGTM apart from the watcher stopping below, and the unsafe testing access. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:42: // Get environment first. defer watcher.Stop(fw.machineUnitsChanges, &fw.tomb) https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:43: fw.environWatcher = fw.st.WatchEnvironConfig() defer watcher.Stop(fw.environWatcher, &fw.tomb) https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:54: return this isn't killing the machines watcher https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:69: return this isn't killing the environs watcher https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:90: // TODO(mue) fill with life. i like the image :-) https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller_te... File worker/firewaller/firewaller_test.go (right): https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller_te... worker/firewaller/firewaller_test.go:100: allMachines := fw.AllMachines() i liked this initially, but after a moment's thought, i'm not sure about it - it accesses fw.machines in a non-thread-safe way and it makes the test slower than it should be. i'll have a chat online about this - i have an idea :-)
Sign in to reply to this message.
https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:16: environWatcher *state.ConfigWatcher Shouldn't be a field, only used inside loop(). Also, I think I mentioned this somewhere before... why do the firewaller and the provisioner each hold independent instances of what should be the same Environ? Given that we can (AIUI) change environ config, and use the environ itself, safely, at any time in any goroutine, I don't see the benefit of duplicating the get-me-an-environ-and-keep-it-up-to-date logic. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:17: machinesWatcher *state.MachinesWatcher Shouldn't be a field, only used inside loop(). https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:42: // Get environment first. On 2012/07/16 16:40:46, rog wrote: > defer watcher.Stop(fw.machineUnitsChanges, &fw.tomb) defer close(fw.machineUnitsChanges) defer watcher.Stop(fw.machinesWatcher, &fw.tomb) (although as stated above I don't think machinesWatcher needs to be a field) https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:53: } fw.tomb.Kill(watcher.MustErr(fw.environWatcher)) (except it shouln't be a field, and IMO shouldn't even exist here) https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:68: } watcher.MustErr, as above. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:75: // case of not managed machine? Feels like a panic situation to me. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:86: fw.machines[addedMachine.Id()] = m go machine.loop() In fact, plausibly, m := newMachine(addedMachine) https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:129: if !ok { fw.tomb.Kill(watcher.MustErr(m.watcher))?
Sign in to reply to this message.
I think there are several lurking gotchas in the sub-goroutines. While I'm not *certain* that my analogous implementations in https://codereview.appspot.com/6405044/ and https://codereview.appspot.com/6402048/ are *bulletproof*, I'm pretty sure they ensure events happen in the right order (ie no changes after a depart, etc), and that this code doesn't. Ping me tomorrow if you want to talk about it, the comments are a little scattered. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:77: m.watcher.Stop() Don't we need to wait until the *machine* has finished, not just the watcher? ie calling m.watcher.Stop() here does *not* guarantee that we won't see any more events from it on machineUnitsChanges: we actually need to be sure that the machine.loop goroutine has exited. I think we need tombs for the sub-goroutines, as I have in the relation-units-watcher and relation-unit branches (although in his case the tomb and Stop() should probably be on machine) https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:129: if !ok { On 2012/07/16 23:23:59, fwereade wrote: > fw.tomb.Kill(watcher.MustErr(m.watcher))? Feels a bit over-familiar, but better than dropping errors on the floor.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:16: environWatcher *state.ConfigWatcher On 2012/07/16 23:23:59, fwereade wrote: > Shouldn't be a field, only used inside loop(). > > Also, I think I mentioned this somewhere before... why do the firewaller and the > provisioner each hold independent instances of what should be the same Environ? > Given that we can (AIUI) change environ config, and use the environ itself, > safely, at any time in any goroutine, I don't see the benefit of duplicating the > get-me-an-environ-and-keep-it-up-to-date logic. Done. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:17: machinesWatcher *state.MachinesWatcher On 2012/07/16 23:23:59, fwereade wrote: > Shouldn't be a field, only used inside loop(). Done. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:42: // Get environment first. On 2012/07/16 23:23:59, fwereade wrote: > On 2012/07/16 16:40:46, rog wrote: > > defer watcher.Stop(fw.machineUnitsChanges, &fw.tomb) > > defer close(fw.machineUnitsChanges) > defer watcher.Stop(fw.machinesWatcher, &fw.tomb) > > (although as stated above I don't think machinesWatcher needs to be a field) Done. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:43: fw.environWatcher = fw.st.WatchEnvironConfig() On 2012/07/16 16:40:46, rog wrote: > defer watcher.Stop(fw.environWatcher, &fw.tomb) Removed environWatcher. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:53: } On 2012/07/16 23:23:59, fwereade wrote: > fw.tomb.Kill(watcher.MustErr(fw.environWatcher)) > > (except it shouln't be a field, and IMO shouldn't even exist here) Removed. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:54: return On 2012/07/16 16:40:46, rog wrote: > this isn't killing the machines watcher Removed environWatcher. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:68: } On 2012/07/16 23:23:59, fwereade wrote: > watcher.MustErr, as above. Done. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:69: return On 2012/07/16 16:40:46, rog wrote: > this isn't killing the environs watcher Removed environWatcher. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:75: // case of not managed machine? On 2012/07/16 23:23:59, fwereade wrote: > Feels like a panic situation to me. Done. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:86: fw.machines[addedMachine.Id()] = m On 2012/07/16 23:23:59, fwereade wrote: > go machine.loop() > > In fact, plausibly, m := newMachine(addedMachine) Yep, had forgotten. But I added it next branch. But I'll switch there to constructor functions too. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:90: // TODO(mue) fill with life. On 2012/07/16 16:40:46, rog wrote: > i like the image :-) Hehe, step by step. https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go... worker/firewaller/firewaller.go:129: if !ok { On 2012/07/16 23:23:59, fwereade wrote: > fw.tomb.Kill(watcher.MustErr(m.watcher))? Done.
Sign in to reply to this message.
This is going in a great direction. Some initial comments. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:11: // Firewaller manages the opening and closing of ports. // Firewaller watches the state for ports open or closed // and reflects those changes onto the backing environment. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:52: if err != nil { I believe we want to execute the line below no matter what. The err is nil, the MustErr is supposed to explode pointing out the inconsistency. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:60: panic("trying to remove unmanaged machine") "trying to remove machine that wasn't added" https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:63: panic("can't stop machine tracker") We can't panic here. stop can fail in pretty usual circumstances that are outside of the application's control (e.g. network issues). This kind of error condition must be properly managed as usual. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:105: type machine struct { s/machine/machineTracker/ https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:115: func newMachine(mst *state.Machine, fw *Firewaller, changes chan *machineUnitsChange) *machine { newMachineTracker This is not creating a machine. Please fix docs accordingly. Also, the channel should be qualified with <- to make it clear that this is an output channel. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:141: return case <-m.tomb.Dying(): https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:143: if !ok { Checking for ok after using the received change? https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:151: // stop lets the machine tracker stop working. // stop stops the machine tracker. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:157: type service struct { A whole type just to enclose a bool feels like overkill. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:161: type unit struct { Is this a unit tracker too?
Sign in to reply to this message.
https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:44: defer close(machineUnitsChanges) i think this line should go before the machinesWatcher stop, otherwise we'll see eof on the channel before the firewaller has properly stopped. maybe that doesn't matter; i'm not sure. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:52: if err != nil { On 2012/07/17 16:13:50, niemeyer wrote: > I believe we want to execute the line below no matter what. The err is nil, the > MustErr is supposed to explode pointing out the inconsistency. in fact, if there's EOF from the machinesWatcher, it implies that the machines watcher has already stopped, so there's no need to call Stop. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:118: changes: changes, we could store this in firewaller to make it obvious that the changes channel is shared between all machines.
Sign in to reply to this message.
https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:52: if err != nil { On 2012/07/17 17:26:24, rog wrote: > On 2012/07/17 16:13:50, niemeyer wrote: > > I believe we want to execute the line below no matter what. The err is nil, > the > > MustErr is supposed to explode pointing out the inconsistency. > > in fact, if there's EOF from the machinesWatcher, it implies that the machines > watcher has already stopped, so there's no need to call Stop. Good point.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:11: // Firewaller manages the opening and closing of ports. On 2012/07/17 16:13:50, niemeyer wrote: > // Firewaller watches the state for ports open or closed > // and reflects those changes onto the backing environment. Done. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:44: defer close(machineUnitsChanges) On 2012/07/17 17:26:24, rog wrote: > i think this line should go before the machinesWatcher stop, otherwise we'll see > eof on the channel before the firewaller has properly stopped. maybe that > doesn't matter; i'm not sure. Done. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:52: if err != nil { On 2012/07/17 17:26:24, rog wrote: > On 2012/07/17 16:13:50, niemeyer wrote: > > I believe we want to execute the line below no matter what. The err is nil, > the > > MustErr is supposed to explode pointing out the inconsistency. > > in fact, if there's EOF from the machinesWatcher, it implies that the machines > watcher has already stopped, so there's no need to call Stop. Done. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:60: panic("trying to remove unmanaged machine") On 2012/07/17 16:13:50, niemeyer wrote: > "trying to remove machine that wasn't added" Done. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:63: panic("can't stop machine tracker") On 2012/07/17 16:13:50, niemeyer wrote: > We can't panic here. stop can fail in pretty usual circumstances that are > outside of the application's control (e.g. network issues). This kind of error > condition must be properly managed as usual. Changed into logging and keeping the machine tracker, so that a future removing won't fail or at least the finish of the firewaller will again try to stop it. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:105: type machine struct { On 2012/07/17 16:13:50, niemeyer wrote: > s/machine/machineTracker/ Done. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:115: func newMachine(mst *state.Machine, fw *Firewaller, changes chan *machineUnitsChange) *machine { On 2012/07/17 16:13:50, niemeyer wrote: > newMachineTracker > > This is not creating a machine. Please fix docs accordingly. > > Also, the channel should be qualified with <- to make it clear that this is an > output channel. Done. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:141: return On 2012/07/17 16:13:50, niemeyer wrote: > case <-m.tomb.Dying(): Done. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:143: if !ok { On 2012/07/17 16:13:50, niemeyer wrote: > Checking for ok after using the received change? In case of ok == false I need to send &machineUnitsChange{m, nil}. Doing that explicitely I would need the same select as regular above. This way change is nil, it would be send and aferwards the loop is left. The drawback is, that it's looking unfamiliar. Is commenting enough? Outherwise I would change it. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:151: // stop lets the machine tracker stop working. On 2012/07/17 16:13:50, niemeyer wrote: > // stop stops the machine tracker. Done. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:157: type service struct { On 2012/07/17 16:13:50, niemeyer wrote: > A whole type just to enclose a bool feels like overkill. Mentioned on IRC, will be used in the next branches. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:161: type unit struct { On 2012/07/17 16:13:50, niemeyer wrote: > Is this a unit tracker too? Yes.
Sign in to reply to this message.
Very close to LGTM, but I'd like to see more testing of the error paths. What happens when zookeeper goes down, for example? And I'm still unhappy about the non-thread-safe access to firewaller variables in the tests. https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:145: // Had been an error, so end the loop. // The watcher terminated prematurely, so end the loop. https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... File worker/firewaller/firewaller_test.go (right): https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller_test.go:81: c.Assert(addedMachines, DeepEquals, allMachines) c.Assert(allMachines, DeepEquals, addedMachines) https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller_test.go:91: c.Assert(addedMachines, DeepEquals, allMachines) c.Assert(allMachines, DeepEquals, addedMachines)
Sign in to reply to this message.
On 2012/07/18 12:26:39, rog wrote: > Very close to LGTM, but I'd like to see more testing of the error paths. > What happens when zookeeper goes down, for example? from IRC: [13:32:33] <rog> TheMue: here's a thought: currently the firewaller opens its own state. how about we change the signature to NewFirewaller(*state.State)? [13:33:01] <rog> TheMue: then we can pass in a state that we have a handle to, and can close that and see what happens [13:33:34] <rog> TheMue: in fact, that enables both the PA and the firewaller to use the same state object, which is probably a good thing.
Sign in to reply to this message.
On Wed, Jul 18, 2012 at 9:35 AM, <rogpeppe@gmail.com> wrote: > from IRC: > > [13:32:33] <rog> TheMue: here's a thought: currently the firewaller > opens its own state. how about we change the signature to > NewFirewaller(*state.State)? > [13:33:01] <rog> TheMue: then we can pass in a state that we have a > handle to, and can close that and see what happens > [13:33:34] <rog> TheMue: in fact, that enables both the PA and the > firewaller to use the same state object, which is probably a good thing. +1! We should change all workers to behave like that (in their respective CLs). -- gustavo @ http://niemeyer.net
Sign in to reply to this message.
One more little round. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:63: panic("can't stop machine tracker") On 2012/07/18 07:52:44, TheMue wrote: > On 2012/07/17 16:13:50, niemeyer wrote: > > We can't panic here. stop can fail in pretty usual circumstances that are > > outside of the application's control (e.g. network issues). This kind of error > > condition must be properly managed as usual. > > Changed into logging and keeping the machine tracker, so that a future removing > won't fail or at least the finish of the firewaller will again try to stop it. It will not try to stop it again, as far as I can see. You got a Removed notice for the respective node, and it won't show up again in the future. There's also no good reason for a machineTracker to fail to stop, unless there's a real problem. Unless you have something else in mind, I think this should be a real error rather than a logged message. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:143: if !ok { On 2012/07/18 07:52:44, TheMue wrote: > In case of ok == false I need to send &machineUnitsChange{m, nil}. Doing that > explicitely I would need the same select as regular above. This way change is > nil, it would be send and aferwards the loop is left. Ok, a comment sounds fine. https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:11: // Firewaller watches the state for ports open or closed Sorry, my typo: s/open/opened/ https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:139: case mt.changes <- &machineUnitsChange{mt, change}: What happens if mt.changes is closed? We have to keep that kind of concern in mind when working with channels.
Sign in to reply to this message.
https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:139: case mt.changes <- &machineUnitsChange{mt, change}: On 2012/07/18 13:56:01, niemeyer wrote: > What happens if mt.changes is closed? > > We have to keep that kind of concern in mind when working with channels. AFAICS mt.changes is the same channel as machineUnitsChanges, which there's no reason to close ever IMO (although I see that it is closed in fact - i'd remove that). I suggest putting machineUnitsChanges inside the firewaller type, so this line would be: case mt.firewaller.machineUnitsChanges <- ... which makes the mux logic more clear, i think.
Sign in to reply to this message.
https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:63: panic("can't stop machine tracker") As I mentioned on IRC, I think your approach of logging is actually right. What's misguiding here is the comment: the machine tracker *is* stopped if we got here. The message should look like: "machine tracker returned error when stopping: %v" https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:63: delete(fw.machines, removedMachine.Id()) This should also be moved above the if block.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:63: panic("can't stop machine tracker") On 2012/07/18 14:11:25, niemeyer wrote: > As I mentioned on IRC, I think your approach of logging is actually right. > What's misguiding here is the comment: the machine tracker *is* stopped if we > got here. The message should look like: > > "machine tracker returned error when stopping: %v" Done. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:118: changes: changes, On 2012/07/17 17:26:24, rog wrote: > we could store this in firewaller to make it obvious that the changes channel is > shared between all machines. Done. https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller... worker/firewaller/firewaller.go:143: if !ok { On 2012/07/18 13:56:01, niemeyer wrote: > On 2012/07/18 07:52:44, TheMue wrote: > > In case of ok == false I need to send &machineUnitsChange{m, nil}. Doing that > > explicitely I would need the same select as regular above. This way change is > > nil, it would be send and aferwards the loop is left. > > Ok, a comment sounds fine. Done. https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:11: // Firewaller watches the state for ports open or closed On 2012/07/18 13:56:01, niemeyer wrote: > Sorry, my typo: s/open/opened/ Done. https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:63: delete(fw.machines, removedMachine.Id()) On 2012/07/18 14:11:25, niemeyer wrote: > This should also be moved above the if block. Done. https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:139: case mt.changes <- &machineUnitsChange{mt, change}: On 2012/07/18 14:10:01, rog wrote: > On 2012/07/18 13:56:01, niemeyer wrote: > > What happens if mt.changes is closed? > > > > We have to keep that kind of concern in mind when working with channels. > > AFAICS mt.changes is the same channel as machineUnitsChanges, which there's no > reason to close ever IMO (although I see that it is closed in fact - i'd remove > that). > I suggest putting machineUnitsChanges inside the firewaller type, so this line > would be: > case mt.firewaller.machineUnitsChanges <- ... > > which makes the mux logic more clear, i think. Done. https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:145: // Had been an error, so end the loop. On 2012/07/18 12:26:39, rog wrote: > // The watcher terminated prematurely, so end the loop. Done. https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... File worker/firewaller/firewaller_test.go (right): https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller_test.go:81: c.Assert(addedMachines, DeepEquals, allMachines) On 2012/07/18 12:26:39, rog wrote: > c.Assert(allMachines, DeepEquals, addedMachines) Done. https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewalle... worker/firewaller/firewaller_test.go:91: c.Assert(addedMachines, DeepEquals, allMachines) On 2012/07/18 12:26:39, rog wrote: > c.Assert(allMachines, DeepEquals, addedMachines) Done.
Sign in to reply to this message.
LGTM with the following handled: https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:21: func NewFirewaller(st *state.State) (*Firewaller, error) { Nice to see how this was simplified by moving logic out. https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:33: defer fw.finish() Nice cleanup as well! https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:51: log.Debugf("firewaller: remove-machine %v", removedMachine.Id()) Either this should be dropped, or clarified to state what's being reported. No machines are being removed here. I suggest just dropping. https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:56: log.Debugf("firewaller: add-machine %v", m.id) Ditto. https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:118: case <-mt.firewaller.tomb.Dying(): Why do we need this here? If the firewaller dies, it will stop the tracker. https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:126: case <-mt.firewaller.tomb.Dying(): Ditto.
Sign in to reply to this message.
https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:51: log.Debugf("firewaller: remove-machine %v", removedMachine.Id()) On 2012/07/19 00:16:16, niemeyer wrote: > Either this should be dropped, or clarified to state what's being reported. No > machines are being removed here. I suggest just dropping. Oops. I now realize you're using this to test the logic, which sounds fine. Please reword it as: "firewaller: started tracking machine %d" and "firewaller: stopped tracking machine %d"
Sign in to reply to this message.
*** Submitted: worker: started implementation of the firewaller Based on the work of Dave and Roger this proposal implements the first part of the firewaller worker. The major loop now contains a machines watcher beside the environment watcher. This machines watcher adds and removes the watchers for the individual machine units watchers. The changes of those watchers will be handled with the next iteration. R=rog, fwereade, niemeyer, gustavo@niemeyer.net CC= https://codereview.appspot.com/6374069 https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:51: log.Debugf("firewaller: remove-machine %v", removedMachine.Id()) On 2012/07/19 00:20:19, niemeyer wrote: > On 2012/07/19 00:16:16, niemeyer wrote: > > Either this should be dropped, or clarified to state what's being reported. No > > machines are being removed here. I suggest just dropping. > > Oops. I now realize you're using this to test the logic, which sounds fine. > > Please reword it as: > > "firewaller: started tracking machine %d" > > and > > "firewaller: stopped tracking machine %d" Done. https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:56: log.Debugf("firewaller: add-machine %v", m.id) On 2012/07/19 00:16:16, niemeyer wrote: > Ditto. Done. https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:118: case <-mt.firewaller.tomb.Dying(): On 2012/07/19 00:16:16, niemeyer wrote: > Why do we need this here? If the firewaller dies, it will stop the tracker. Done. https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewalle... worker/firewaller/firewaller.go:126: case <-mt.firewaller.tomb.Dying(): On 2012/07/19 00:16:16, niemeyer wrote: > Ditto. Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
