Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(42)

Issue 6374069: worker: started implementation of the firewaller (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 9 months ago by TheMue
Modified:
7 years, 8 months ago
Reviewers:
niemeyer, mp+114938, rog
Visibility:
Public.

Description

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. 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+299 lines, -0 lines) Patch
A [revision details] View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
A worker/firewaller/export_test.go View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments Download
A worker/firewaller/firewaller.go View 1 2 3 4 1 chunk +150 lines, -0 lines 0 comments Download
A worker/firewaller/firewaller_test.go View 1 2 3 4 1 chunk +140 lines, -0 lines 0 comments Download

Messages

Total messages: 19
TheMue
Please take a look.
7 years, 9 months ago (2012-07-13 20:09:54 UTC) #1
rog
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 ...
7 years, 8 months ago (2012-07-16 16:40:45 UTC) #2
fwereade
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#newcode16 worker/firewaller/firewaller.go:16: environWatcher *state.ConfigWatcher Shouldn't be a field, only used inside ...
7 years, 8 months ago (2012-07-16 23:23:58 UTC) #3
fwereade
I think there are several lurking gotchas in the sub-goroutines. While I'm not *certain* that ...
7 years, 8 months ago (2012-07-16 23:39:52 UTC) #4
TheMue
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#newcode16 worker/firewaller/firewaller.go:16: environWatcher *state.ConfigWatcher On 2012/07/16 23:23:59, ...
7 years, 8 months ago (2012-07-17 15:42:03 UTC) #5
niemeyer
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.go#newcode11 ...
7 years, 8 months ago (2012-07-17 16:13:50 UTC) #6
rog
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.go#newcode44 worker/firewaller/firewaller.go:44: defer close(machineUnitsChanges) i think this line should go before ...
7 years, 8 months ago (2012-07-17 17:26:24 UTC) #7
niemeyer
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.go#newcode52 worker/firewaller/firewaller.go:52: if err != nil { On 2012/07/17 17:26:24, rog ...
7 years, 8 months ago (2012-07-17 18:13:44 UTC) #8
TheMue
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.go#newcode11 worker/firewaller/firewaller.go:11: // Firewaller manages the opening ...
7 years, 8 months ago (2012-07-18 07:52:44 UTC) #9
rog
Very close to LGTM, but I'd like to see more testing of the error paths. ...
7 years, 8 months ago (2012-07-18 12:26:39 UTC) #10
rog
On 2012/07/18 12:26:39, rog wrote: > Very close to LGTM, but I'd like to see ...
7 years, 8 months ago (2012-07-18 12:35:53 UTC) #11
gustavo_niemeyer.net
On Wed, Jul 18, 2012 at 9:35 AM, <rogpeppe@gmail.com> wrote: > from IRC: > > ...
7 years, 8 months ago (2012-07-18 13:51:48 UTC) #12
niemeyer
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.go#newcode63 worker/firewaller/firewaller.go:63: panic("can't stop machine tracker") On ...
7 years, 8 months ago (2012-07-18 13:56:01 UTC) #13
rog
https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewaller.go#newcode139 worker/firewaller/firewaller.go:139: case mt.changes <- &machineUnitsChange{mt, change}: On 2012/07/18 13:56:01, niemeyer ...
7 years, 8 months ago (2012-07-18 14:10:01 UTC) #14
niemeyer
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.go#newcode63 worker/firewaller/firewaller.go:63: panic("can't stop machine tracker") As I mentioned on IRC, ...
7 years, 8 months ago (2012-07-18 14:11:25 UTC) #15
TheMue
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.go#newcode63 worker/firewaller/firewaller.go:63: panic("can't stop machine tracker") On ...
7 years, 8 months ago (2012-07-18 16:26:40 UTC) #16
niemeyer
LGTM with the following handled: https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewaller.go#newcode21 worker/firewaller/firewaller.go:21: func NewFirewaller(st *state.State) (*Firewaller, ...
7 years, 8 months ago (2012-07-19 00:16:16 UTC) #17
niemeyer
https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewaller.go#newcode51 worker/firewaller/firewaller.go:51: log.Debugf("firewaller: remove-machine %v", removedMachine.Id()) On 2012/07/19 00:16:16, niemeyer wrote: ...
7 years, 8 months ago (2012-07-19 00:20:19 UTC) #18
TheMue
7 years, 8 months ago (2012-07-19 11:18:13 UTC) #19
*** 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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b