From 5c19fd45c476a6eb86c369b946065eae30816931 Mon Sep 17 00:00:00 2001 From: roos Date: Mon, 13 May 2019 12:45:05 +0200 Subject: [PATCH] feedback --- doc/BeaconService.md | 2 + go/beacon_srv/internal/beaconing/BUILD.bazel | 1 + go/beacon_srv/internal/beaconing/doc.go | 12 +++- .../internal/beaconing/originator.go | 16 ----- go/beacon_srv/internal/beaconing/tick.go | 39 +++++++++++ go/beacon_srv/internal/ifstate/ifstate.go | 8 +-- go/beacon_srv/internal/ifstate/pusher.go | 53 +++++++-------- go/beacon_srv/internal/ifstate/pusher_test.go | 7 +- go/beacon_srv/internal/ifstate/revoker.go | 67 ++++++++++++------- go/beacon_srv/main.go | 4 +- 10 files changed, 127 insertions(+), 82 deletions(-) create mode 100644 go/beacon_srv/internal/beaconing/tick.go diff --git a/doc/BeaconService.md b/doc/BeaconService.md index 1b98304c86..cc35c1e45f 100644 --- a/doc/BeaconService.md +++ b/doc/BeaconService.md @@ -264,6 +264,8 @@ Core BSes *(uses: Interfaces.All)* Core BSes originate new beacons to child and neighboring core ASes. +The periodic task has an interval that is less than the OriginateTime. +Beacons are originated on all interfaces that do not have a origination in the last OriginateTime. 1. Create a new PCB using the local IA and the current timestamp 1. Propagate the PCB on all core and child interfaces. diff --git a/go/beacon_srv/internal/beaconing/BUILD.bazel b/go/beacon_srv/internal/beaconing/BUILD.bazel index 75bd6a230a..698120059e 100644 --- a/go/beacon_srv/internal/beaconing/BUILD.bazel +++ b/go/beacon_srv/internal/beaconing/BUILD.bazel @@ -10,6 +10,7 @@ go_library( "originator.go", "propagator.go", "registrar.go", + "tick.go", "util.go", ], importpath = "github.com/scionproto/scion/go/beacon_srv/internal/beaconing", diff --git a/go/beacon_srv/internal/beaconing/doc.go b/go/beacon_srv/internal/beaconing/doc.go index cae01080e1..443c76539d 100644 --- a/go/beacon_srv/internal/beaconing/doc.go +++ b/go/beacon_srv/internal/beaconing/doc.go @@ -25,18 +25,24 @@ // // The originator should only be instantiated by core beacon servers. It // periodically creates fresh beacons and propagates them on all core and child -// links. +// links. In case the task is run before a full period has passed, beacons are +// originated on all interfaces that have last been originated on more than one +// period ago. // // Registrar // // The registrar is a periodic task to register segments with the appropriate // path server. Core and Up segments are registered with the local path server. -// Down segments are registered with the originating core AS. +// Down segments are registered with the originating core AS. In case the task +// is run before a full period has passed, segments are only registered, if +// there has not been a successful registration in the last period. // // Propagator // // The propagator is a periodic task to propagate beacons to the appropriate // neighboring ASes. In a core AS, the beacons are propagated to the neighbors // on all core link, unless they will create an AS loop. In a non-core AS, the -// beacons are propagated to the neighbors on all child links. +// beacons are propagated to the neighbors on all child links. In case the task +// is run before a full period has passed, beacons are propagated on all +// interfaces that have last been propagated on more than one period ago. package beaconing diff --git a/go/beacon_srv/internal/beaconing/originator.go b/go/beacon_srv/internal/beaconing/originator.go index 95f2c3d686..fb96886a0b 100644 --- a/go/beacon_srv/internal/beaconing/originator.go +++ b/go/beacon_srv/internal/beaconing/originator.go @@ -38,22 +38,6 @@ type OriginatorConf struct { Period time.Duration } -type tick struct { - now time.Time - last time.Time - period time.Duration -} - -func (t *tick) updateLast() { - if t.passed() { - t.last = t.now - } -} - -func (t *tick) passed() bool { - return t.now.Sub(t.last) >= t.period -} - // Originator originates beacons. It should only be used by core ASes. type Originator struct { *segExtender diff --git a/go/beacon_srv/internal/beaconing/tick.go b/go/beacon_srv/internal/beaconing/tick.go new file mode 100644 index 0000000000..5ef423f349 --- /dev/null +++ b/go/beacon_srv/internal/beaconing/tick.go @@ -0,0 +1,39 @@ +// Copyright 2019 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package beaconing + +import ( + "time" +) + +// tick is keeps track whether the period has passed compared to the last time. +type tick struct { + now time.Time + last time.Time + period time.Duration +} + +// updateLast updates the last time to the current time, if the period has +// passed since last. +func (t *tick) updateLast() { + if t.passed() { + t.last = t.now + } +} + +// passed returns whether the last timestamp is further away from now than the period +func (t *tick) passed() bool { + return t.now.Sub(t.last) >= t.period +} diff --git a/go/beacon_srv/internal/ifstate/ifstate.go b/go/beacon_srv/internal/ifstate/ifstate.go index 0453620f82..7b2bf7d3f7 100644 --- a/go/beacon_srv/internal/ifstate/ifstate.go +++ b/go/beacon_srv/internal/ifstate/ifstate.go @@ -214,9 +214,9 @@ func (intf *Interface) State() State { // Originate sets the time this interface has been originated on last. func (intf *Interface) Originate(now time.Time) { - intf.mu.RLock() + intf.mu.Lock() intf.lastOriginate = now - intf.mu.RUnlock() + intf.mu.Unlock() } // LastOriginate indicates the last time this interface has been originated on. @@ -228,9 +228,9 @@ func (intf *Interface) LastOriginate() time.Time { // Propagate sets the time this interface has been propagated on last. func (intf *Interface) Propagate(now time.Time) { - intf.mu.RLock() + intf.mu.Lock() intf.lastPropagate = now - intf.mu.RUnlock() + intf.mu.Unlock() } // LastPropagate indicates the last time this interface has been propagated on. diff --git a/go/beacon_srv/internal/ifstate/pusher.go b/go/beacon_srv/internal/ifstate/pusher.go index 6e7ad91a04..0fd4b531a0 100644 --- a/go/beacon_srv/internal/ifstate/pusher.go +++ b/go/beacon_srv/internal/ifstate/pusher.go @@ -16,29 +16,45 @@ package ifstate import ( "context" - "net" "sync" "github.com/scionproto/scion/go/lib/common" "github.com/scionproto/scion/go/lib/ctrl/path_mgmt" "github.com/scionproto/scion/go/lib/infra" - "github.com/scionproto/scion/go/lib/infra/messenger" "github.com/scionproto/scion/go/lib/log" - "github.com/scionproto/scion/go/lib/snet" "github.com/scionproto/scion/go/lib/topology" ) -// Pusher pushes interface state infos to all border routers to remove the -// revocations. It is called when an interface comes back up. -type Pusher struct { +// PusherConf is the configuration to create a new pusher. +type PusherConf struct { TopoProvider topology.Provider Intfs *Interfaces Msgr infra.Messenger } +// Pusher pushes interface state infos to all border routers to remove the +// revocations. It is called when an interface comes back up. +type Pusher struct { + topoProvider topology.Provider + intfs *Interfaces + pusher brPusher +} + +// New creates a new interface state pusher. +func (cfg PusherConf) New() *Pusher { + return &Pusher{ + topoProvider: cfg.TopoProvider, + intfs: cfg.Intfs, + pusher: brPusher{ + msgr: cfg.Msgr, + logger: log.New("mode", "pusher"), + }, + } +} + // Push removes the revocation for the given interface from all border routers. func (p *Pusher) Push(ctx context.Context, ifid common.IFIDType) { - intf := p.Intfs.Get(ifid) + intf := p.intfs.Get(ifid) if intf == nil || intf.State() != Active { return } @@ -48,28 +64,7 @@ func (p *Pusher) Push(ctx context.Context, ifid common.IFIDType) { Active: true, }}, } - topo := p.TopoProvider.Get() wg := &sync.WaitGroup{} - for id, br := range topo.BR { - a := &snet.Addr{ - IA: topo.ISD_AS, - Host: br.CtrlAddrs.PublicAddr(br.CtrlAddrs.Overlay), - NextHop: br.CtrlAddrs.OverlayAddr(br.CtrlAddrs.Overlay), - } - p.sendToBr(ctx, id, a, msg, wg) - } + p.pusher.sendIfStateToAllBRs(ctx, msg, p.topoProvider.Get(), wg) wg.Wait() } - -func (p *Pusher) sendToBr(ctx context.Context, id string, a net.Addr, - msg *path_mgmt.IFStateInfos, wg *sync.WaitGroup) { - - wg.Add(1) - go func() { - defer log.LogPanicAndExit() - defer wg.Done() - if err := p.Msgr.SendIfStateInfos(ctx, msg, a, messenger.NextId()); err != nil { - log.Error("[Pusher] Failed to send IfStateInfo to BR", "br", id, "err", err) - } - }() -} diff --git a/go/beacon_srv/internal/ifstate/pusher_test.go b/go/beacon_srv/internal/ifstate/pusher_test.go index 5dacd11aae..d02450ae79 100644 --- a/go/beacon_srv/internal/ifstate/pusher_test.go +++ b/go/beacon_srv/internal/ifstate/pusher_test.go @@ -35,11 +35,11 @@ func TestPusherPush(t *testing.T) { topoProvider := xtest.TopoProviderFromFile(t, "testdata/topology.json") msgr := mock_infra.NewMockMessenger(mctrl) intfs := NewInterfaces(topoProvider.Get().IFInfoMap, Config{}) - p := Pusher{ + p := PusherConf{ TopoProvider: topoProvider, Intfs: intfs, Msgr: msgr, - } + }.New() expectedMsg := &path_mgmt.IFStateInfos{ Infos: []*path_mgmt.IFStateInfo{{ IfID: 101, @@ -63,7 +63,8 @@ func TestPusherPush(t *testing.T) { Host: br.CtrlAddrs.PublicAddr(br.CtrlAddrs.Overlay), NextHop: br.CtrlAddrs.OverlayAddr(br.CtrlAddrs.Overlay), } - msgr.EXPECT().SendIfStateInfos(gomock.Any(), expectedMsg, a, gomock.Any()) + msgr.EXPECT().SendIfStateInfos(gomock.Any(), gomock.Eq(expectedMsg), + gomock.Eq(a), gomock.Any()) } } p.Push(context.Background(), 101) diff --git a/go/beacon_srv/internal/ifstate/revoker.go b/go/beacon_srv/internal/ifstate/revoker.go index 7da471dc45..2d1423da47 100644 --- a/go/beacon_srv/internal/ifstate/revoker.go +++ b/go/beacon_srv/internal/ifstate/revoker.go @@ -68,13 +68,20 @@ var _ periodic.Task = (*Revoker)(nil) // Revoker issues revocations for interfaces that have timed out. // Revocations for already revoked interfaces are renewed periodically. type Revoker struct { - cfg RevokerConf + cfg RevokerConf + pusher brPusher } -// NewRevoker creates a new revoker from the given arguments. +// New creates a new revoker from the given arguments. func (cfg RevokerConf) New() *Revoker { cfg.RevConfig.InitDefaults() - return &Revoker{cfg: cfg} + return &Revoker{ + cfg: cfg, + pusher: brPusher{ + msgr: cfg.Msgr, + logger: log.New("mode", "revoker"), + }, + } } // Run issues revocations for interfaces that have timed out @@ -133,34 +140,13 @@ func (r *Revoker) createSignedRev(ifid common.IFIDType) (*path_mgmt.SignedRevInf func (r *Revoker) pushRevocationsToBRs(ctx context.Context, revs map[common.IFIDType]*path_mgmt.SignedRevInfo, wg *sync.WaitGroup) { - topo := r.cfg.TopoProvider.Get() msg := &path_mgmt.IFStateInfos{ Infos: make([]*path_mgmt.IFStateInfo, 0, len(revs)), } for ifid := range revs { msg.Infos = append(msg.Infos, infoFromInterface(ifid, r.cfg.Intfs.Get(ifid))) } - for id, br := range topo.BR { - a := &snet.Addr{ - IA: topo.ISD_AS, - Host: br.CtrlAddrs.PublicAddr(br.CtrlAddrs.Overlay), - NextHop: br.CtrlAddrs.OverlayAddr(br.CtrlAddrs.Overlay), - } - r.sendToBr(ctx, id, a, msg, wg) - } -} - -func (r *Revoker) sendToBr(ctx context.Context, id string, a net.Addr, - msg *path_mgmt.IFStateInfos, wg *sync.WaitGroup) { - - wg.Add(1) - go func() { - defer log.LogPanicAndExit() - defer wg.Done() - if err := r.cfg.Msgr.SendIfStateInfos(ctx, msg, a, messenger.NextId()); err != nil { - log.Error("[Revoker] Failed to send revocations to BR", "br", id, "err", err) - } - }() + r.pusher.sendIfStateToAllBRs(ctx, msg, r.cfg.TopoProvider.Get(), wg) } func (r *Revoker) pushRevocationsToPS(ctx context.Context, @@ -189,6 +175,37 @@ func (r *Revoker) pushRevocationsToPS(ctx context.Context, } } +type brPusher struct { + msgr infra.Messenger + logger log.Logger +} + +func (p *brPusher) sendIfStateToAllBRs(ctx context.Context, msg *path_mgmt.IFStateInfos, + topo *topology.Topo, wg *sync.WaitGroup) { + + for id, br := range topo.BR { + a := &snet.Addr{ + IA: topo.ISD_AS, + Host: br.CtrlAddrs.PublicAddr(br.CtrlAddrs.Overlay), + NextHop: br.CtrlAddrs.OverlayAddr(br.CtrlAddrs.Overlay), + } + p.sendIfStateToBr(ctx, msg, id, a, wg) + } +} + +func (p *brPusher) sendIfStateToBr(ctx context.Context, msg *path_mgmt.IFStateInfos, + id string, a net.Addr, wg *sync.WaitGroup) { + + wg.Add(1) + go func() { + defer log.LogPanicAndExit() + defer wg.Done() + if err := p.msgr.SendIfStateInfos(ctx, msg, a, messenger.NextId()); err != nil { + log.Error("Failed to send interface state to BR", "br", id, "err", err) + } + }() +} + func toSlice(revs map[common.IFIDType]*path_mgmt.SignedRevInfo) []*path_mgmt.SignedRevInfo { res := make([]*path_mgmt.SignedRevInfo, 0, len(revs)) for _, rev := range revs { diff --git a/go/beacon_srv/main.go b/go/beacon_srv/main.go index 547e49a4e8..533ff075ba 100644 --- a/go/beacon_srv/main.go +++ b/go/beacon_srv/main.go @@ -160,11 +160,11 @@ func realMain() int { msgr.AddHandler(infra.IfId, keepalive.NewHandler(topo.ISD_AS, intfs, keepalive.StateChangeTasks{ RevDropper: store, - IfStatePusher: &ifstate.Pusher{ + IfStatePusher: ifstate.PusherConf{ Intfs: intfs, Msgr: msgr, TopoProvider: itopo.Provider(), - }, + }.New(), }), ) cfg.Metrics.StartPrometheus()