BS: Add propagator #2616
BS: Add propagator #2616
Conversation
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @oncilla and @scrye)
a discussion (no related file):
I looked into onehop/sender.go the method CreatePath has a bug it doesn't use the now argument. Could you please also fix this as part of this PR?
go/beacon_srv/internal/beaconing/propagator.go, line 37 at r1 (raw file):
Registrar
Propagator
go/beacon_srv/internal/beaconing/propagator.go, line 104 at r1 (raw file):
// propagated to. In a core AS, these are all active core links. In a non-core // AS, these are all active child links. func (p *Propagator) activeIntfs() []common.IFIDType {
In the Originator we have the same logic in the method originateBeacons do you think it would make sense to create a utility method out of it and share it?
go/beacon_srv/internal/beaconing/propagator.go, line 145 at r1 (raw file):
} var success ctr wg := sync.WaitGroup{}
Hm is there a specific reason why we use a separate (from the top one) wait group here? If the sending to one neighbour AS blocks it blocks the sending of all beacons to neighbors. With just one WG it wouldn't matter if sending to one AS blocks the rest.
go/beacon_srv/internal/beaconing/propagator.go, line 160 at r1 (raw file):
return common.NewBasicError("None propagated", nil, "beacon", origBeacon) } log.Info("[Propagator] Successfully propagated", "beacon", origBeacon, "count", success.c)
Should we maybe also print how many it should have sent?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @oncilla and @scrye)
a discussion (no related file):
Please update the package doc.go
This change adds a beacon propagator. The propagator fetches the beacons to propagate from the beacon provider and sends them on all active applicable interfaces. In a core BS, these are the core links. In a non-core BS, these are child links. In case the beacon would create a loop on a given interface, the beacon is not forwarded to that interface. fixes scionproto#2563
oncilla
force-pushed
the
pub-bs-beacon-propagator
branch
from
211586b to
8677772
Compare
There was a problem hiding this comment.
Reviewable status: 3 of 11 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker, @oncilla, and @scrye)
go/beacon_srv/internal/beaconing/propagator.go, line 37 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Registrar
Propagator
Done.
go/beacon_srv/internal/beaconing/propagator.go, line 104 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
In the
Originatorwe have the same logic in the methodoriginateBeaconsdo you think it would make sense to create a utility method out of it and share it?
Done.
go/beacon_srv/internal/beaconing/propagator.go, line 145 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Hm is there a specific reason why we use a separate (from the top one) wait group here? If the sending to one neighbour AS blocks it blocks the sending of all beacons to neighbors. With just one WG it wouldn't matter if sending to one AS blocks the rest.
The wait-group is necessary to synchronize the access to the success counter
One slow AS will not block other beacons from propagating:
startPropagate starts for each beacon to propagate a new go routine that loops through all active interfaces, creates the beacons and sends them in a separate go routine.
The hierarchy of the go routines has 2 levels, thus we also need 2 levels of wait-groups.
go/beacon_srv/internal/beaconing/propagator.go, line 160 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Should we maybe also print how many it should have sent?
Done.
There was a problem hiding this comment.
Reviewable status: 2 of 12 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker and @scrye)
a discussion (no related file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
I looked into
onehop/sender.gothe methodCreatePathhas a bug it doesn't use thenowargument. Could you please also fix this as part of this PR?
Oh, nice catch!
Done.
a discussion (no related file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Please update the package
doc.go
Done.
There was a problem hiding this comment.
Reviewed 7 of 8 files at r2, 3 of 3 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @scrye)
go/beacon_srv/internal/beaconing/propagator.go, line 145 at r1 (raw file):
Previously, Oncilla wrote…
The wait-group is necessary to synchronize the access to the success counter
One slow AS will not block other beacons from propagating:
startPropagatestarts for each beacon to propagate a new go routine that loops through all active interfaces, creates the beacons and sends them in a separate go routine.
The hierarchy of the go routines has 2 levels, thus we also need 2 levels of wait-groups.
Yeah of course, must have been to early in the morning :P
This change adds a beacon propagator.
The propagator fetches the beacons to propagate from the beacon
provider and sends them on all active applicable interfaces.
In a core BS, these are the core links.
In a non-core BS, these are child links.
In case the beacon would create a loop on a given interface, the beacon
is not forwarded to that interface.
fixes #2563
This change is