feedback

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.

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",

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

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

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
+}

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.

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)
-		}
-	}()
-}

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)

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 {

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()