Issue 6820112: firewaller: use new style machine units watcher
Descriptionfirewaller: use new style machine units watcher
https://code.launchpad.net/~aramh/juju-core/120-firewaller-new-watcher-units7/+merge/133269
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : firewaller: use new style machine units watcher #
Total comments: 22
Patch Set 3 : firewaller: use new style machine units watcher #
Total comments: 22
MessagesTotal messages: 9
Please take a look.
Sign in to reply to this message.
I don't think i can lgtm as I don't know enough about the watchers, but this change looks sane. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:109: changed := []*unitData{} suggestion: var changed []*unitdata{} https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:136: panic("unreachable") panic("impossible lifecycle state") https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:142: panic("trying to remove unit that was not added") how unlikely is this ? could they be errors instead ? https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:156: panic("machine of added unit is not watched") ditto
Sign in to reply to this message.
I'm not sure about some of the logic here. Concerns outlined below. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:109: changed := []*unitData{} On 2012/11/09 00:31:35, dfc wrote: > suggestion: > > var changed []*unitdata{} > i also prefer that (without the {} of course) but this idiom is used in many places, so i tend to ignore it. it's perhaps interesting to note that this does actually do an allocation (which the var form doesn't), although it probably won't in the future. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:116: removed = append(removed, name) i think we need to check that the unitds entry isn't present when we add items to removed. otherwise the watcher can send us an Alive message, but by the time we call st.Unit, the unit has been removed (or unassigned, below) so we add to removed even though it was never there, thus triggering the panic below. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:128: if machineId != fw.unitds[name].machined.id || err != nil { i'd swap the two conditions to make it clear we don't care about the machine id if err is not nil. also, we actually care about the error here - it can be an error due to the state going down. i suggest that we change AssignedMachineId to return a known error when the unit is not assigned to a machine and check against that here. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:134: continue d https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:142: panic("trying to remove unit that was not added") On 2012/11/09 00:31:35, dfc wrote: > how unlikely is this ? could they be errors instead ? i tend to agree - it should never happen, but a panic doesn't help much as the relevant stack trace is long gone. a "should never happen" error return seems perhaps nicer. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:153: fw.tomb.Kill(err) this should just return err, i think. but i'm not sure this is necessarily a fatal error - a subordinate unit could be added and then unassigned from its machine - there's no guarantee that we see the removed event before AssignedMachineId might return an error. this is an interesting issue actually. i think that if we see this happen, we should just ignore the unit. actually, why are we even calling AssignedMachineId here? we've just called it above, so we should have no need. perhaps there's no need to have this logic outside of the life switch logic above. i'm not sure what purpose the gathering of the removed and slices is serving now. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:163: return err this is not necessarily a fatal error - what if the unit and its service have been removed since we received the added event? https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... File worker/firewaller/firewaller_test.go (right): https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller_test.go:664: // Change configuration. c.Skip("disabled until firewaller initialization is fixed")
Sign in to reply to this message.
https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:109: changed := []*unitData{} I like yours better, but currently we always do it this way, so unless we decide to change it everywhere I'm leaving it as is, for consistency. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:116: removed = append(removed, name) On 2012/11/09 10:16:05, rog wrote: You're right, fixed. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:128: if machineId != fw.unitds[name].machined.id || err != nil { > i'd swap the two conditions to make it clear we don't care about > the machine id if err is not nil. also, we actually care about > the error here - it can be an error due to the state going down. i > suggest that we change AssignedMachineId to return a known error > when the unit is not assigned to a machine and check against > that here. Sorry, for some reason I was thinking this only works with principals, in which case the code would have been correct (though perhaps too subtle). Of course this is wrong with subordinates. I made AssignedMachineId return a known error instead. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:134: continue On 2012/11/09 10:16:05, rog wrote: > d Done. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:142: panic("trying to remove unit that was not added") Deleted the code, it makes no sense. We only enqueue removals above, and we don't enqueue unknowns units. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:153: fw.tomb.Kill(err) On 2012/11/09 10:16:05, rog wrote: > this should just return err, i think. > > but i'm not sure this is necessarily a fatal error - a subordinate unit could be > added and then unassigned from its machine - there's no guarantee that we see > the removed event before AssignedMachineId might return an error. > > this is an interesting issue actually. i think that if we see this happen, we > should just ignore the unit. > > actually, why are we even calling AssignedMachineId here? we've just called it > above, so we should have no need. > > perhaps there's no need to have this logic outside of the life switch logic > above. i'm not sure what purpose the gathering of the removed and slices is > serving now. Done. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:156: panic("machine of added unit is not watched") On 2012/11/09 00:31:35, dfc wrote: > ditto Done. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:163: return err On 2012/11/09 10:16:05, rog wrote: > this is not necessarily a fatal error - what if the unit and its service have > been removed since we received the added event? Done. https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... File worker/firewaller/firewaller_test.go (right): https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller_test.go:664: // Change configuration. Thank you.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/6820112/diff/1005/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6820112/diff/1005/state/unit.go#newcode417 state/unit.go:417: return &NotFoundError{fmt.Sprintf(format+" is not assigned to a machine", args...)} s/NotFoundError/notAssignedError/ obviously.
Sign in to reply to this message.
LGTM assuming no disagreement below (I think it should be `unit.Life() == state.Dead`). https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:129: if unit == nil || unit.Life() != state.Alive || unassigned || machineId != knownMachineId { Why the Alive check? A unit can still open/close ports when it's Dying, and I think those should be honoured... a unit can take a long time to die, and it should keep working until it does :).
Sign in to reply to this message.
NOT LGTM until these are addressed: https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6820112/diff/1003/worker/firewaller/firewaller... worker/firewaller/firewaller.go:109: changed := []*unitData{} On 2012/11/09 15:52:41, aram wrote: > I like yours better, but currently we always do it this way, so unless we decide > to change it everywhere I'm leaving it as is, for consistency. I believe we're pretty consistent about not doing this for slices. Pretty much everywhere an empty slice is a nil slice, so let's please try to preserve this. I can find a single other occurrence in this file, which may be fixed as well at the same time if you wish. https://codereview.appspot.com/6820112/diff/1005/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6820112/diff/1005/state/unit.go#newcode423 state/unit.go:423: } Please replace all of the logic above by this: type NotAssignedError struct { Unit *Unit } func (e *NotAssignedError) Error() string { return fmt.Errorf("unit %q is not assigned to a machine", e.Unit) } https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:116: var unassigned bool machineId := -1 Please drop unassigned. These two variables overlap in meaning, and the logic below is a lot more confusing because one has to keep in mind that machineId zero is actually not necessarily real. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:125: unassigned = state.IsNotAssigned(err) id, err := unit.AssignedMachineId() if <not found check> { continue } else if err == nil { machineId = id } else { return err } https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:129: if unit == nil || unit.Life() != state.Alive || unassigned || machineId != knownMachineId { On 2012/11/13 16:19:32, fwereade wrote: > Why the Alive check? A unit can still open/close ports when it's Dying, and I > think those should be honoured... a unit can take a long time to die, and it > should keep working until it does :). +1 Also, || knownMachineId != machineId { https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:134: } else if unit != nil && unit.Life() == state.Alive { See William's comment above. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:136: fw.unitds[unit.Name()] = unitd We have "name" in scope, here and below. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:137: if fw.machineds[machineId] == nil || unassigned { Please avoid using variables unless they are initialized. It doesn't matter what the settings for machine id zero are if this is not machine zero. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:141: unitd.machined.unitds[unit.Name()] = unitd serviceName := unit.ServiceName() There are 4 calls to this in the next ~10 lines. We can as well keep it around. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:142: if fw.serviceds[unit.ServiceName()] == nil { unitd.serviced = fw.serviceds[serviceName] if unitd.serviced == nil { ... } https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... File worker/firewaller/firewaller_test.go (right): https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller_test.go:663: c.Skip("disabled until firewaller initialization is fixed") Why is it broken? The comment should have a bug # so we don't have to second guess.
Sign in to reply to this message.
Addressed in new branch: lp:~fwereade/juju-core/aramh-firewaller-changes https://codereview.appspot.com/6820112/diff/1005/state/unit.go File state/unit.go (right): https://codereview.appspot.com/6820112/diff/1005/state/unit.go#newcode423 state/unit.go:423: } On 2012/11/14 18:40:33, niemeyer wrote: > Please replace all of the logic above by this: > > type NotAssignedError struct { > Unit *Unit > } > > func (e *NotAssignedError) Error() string { > return fmt.Errorf("unit %q is not assigned to a machine", e.Unit) > } Done. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller.go File worker/firewaller/firewaller.go (right): https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:116: var unassigned bool On 2012/11/14 18:40:33, niemeyer wrote: > machineId := -1 > > Please drop unassigned. These two variables overlap in meaning, and the logic > below is a lot more confusing because one has to keep in mind that machineId > zero is actually not necessarily real. Done. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:125: unassigned = state.IsNotAssigned(err) On 2012/11/14 18:40:33, niemeyer wrote: > id, err := unit.AssignedMachineId() > if <not found check> { > continue > } else if err == nil { > machineId = id > } else { > return err > } Done. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:129: if unit == nil || unit.Life() != state.Alive || unassigned || machineId != knownMachineId { On 2012/11/14 18:40:33, niemeyer wrote: > On 2012/11/13 16:19:32, fwereade wrote: > > Why the Alive check? A unit can still open/close ports when it's Dying, and I > > think those should be honoured... a unit can take a long time to die, and it > > should keep working until it does :). > > +1 > > Also, || knownMachineId != machineId { Done. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:134: } else if unit != nil && unit.Life() == state.Alive { On 2012/11/14 18:40:33, niemeyer wrote: > See William's comment above. Done. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:136: fw.unitds[unit.Name()] = unitd On 2012/11/14 18:40:33, niemeyer wrote: > We have "name" in scope, here and below. Done. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:137: if fw.machineds[machineId] == nil || unassigned { On 2012/11/14 18:40:33, niemeyer wrote: > Please avoid using variables unless they are initialized. It doesn't matter what > the settings for machine id zero are if this is not machine zero. Done. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:141: unitd.machined.unitds[unit.Name()] = unitd On 2012/11/14 18:40:33, niemeyer wrote: > serviceName := unit.ServiceName() > > There are 4 calls to this in the next ~10 lines. We can as well keep it around. Done. https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller.go:142: if fw.serviceds[unit.ServiceName()] == nil { On 2012/11/14 18:40:33, niemeyer wrote: > unitd.serviced = fw.serviceds[serviceName] > if unitd.serviced == nil { > ... > } I don't think this makes anything clearer or more readable... would you explain please? https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... File worker/firewaller/firewaller_test.go (right): https://codereview.appspot.com/6820112/diff/1005/worker/firewaller/firewaller... worker/firewaller/firewaller_test.go:663: c.Skip("disabled until firewaller initialization is fixed") On 2012/11/14 18:40:33, niemeyer wrote: > Why is it broken? The comment should have a bug # so we don't have to second > guess. Done; it was a MachineUnitsWatcher bug.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
