BS: Fast recovery

[oncilla]

This PR adds fast recovery for multiple cases:

1. When an interface becomes active, the associated revocations are
   deleted from the beacon store and the border routers.
2. When the propagator has not propagated beacons in a while on an
   interface, it will do so before the beaconing interval.
3. When the registrar has not registered beacons in a while, it will
   do so when beacons become available.
4. When the originator has not originated beacons in a while on an
   interface, it will do so before the beaconing interval.

---

This change is 

[lukedirtwalker]

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @oncilla)

a discussion (no related file):
can you integrate the changes also in the design doc.

---

---

go/beacon_srv/internal/beaconing/originator.go, line 53 at r1 (raw file):

}

func (t *tick) passed() bool {

maybe add a comment:
passed returns whether the last timestamp is further away from now than the period or something like that.

---

go/beacon_srv/internal/ifstate/ifstate.go, line 217 at r1 (raw file):

// Originate sets the time this interface has been originated on last.
func (intf *Interface) Originate(now time.Time) {
	intf.mu.RLock()

Don't we need to use the write lock for this?

Also pease use defer to unlock

---

go/beacon_srv/internal/ifstate/ifstate.go, line 230 at r1 (raw file):

// Propagate sets the time this interface has been propagated on last.
func (intf *Interface) Propagate(now time.Time) {

ditto write lock & defer

---

go/beacon_srv/internal/ifstate/pusher_test.go, line 66 at r1 (raw file):

a,

is that gomock.Eq or what happens internally if you just pass a value to a mock function? Maybe gomock.Eq would be a bit more explicit.

---

go/beacon_srv/internal/ifstate/revoker.go, line 153 at r1 (raw file):

}

func (r *Revoker) sendToBr(ctx context.Context, id string, a net.Addr,

so that is the same thing as in the pusher code.
Maybe create a function sendIfStateToAllBRs?

[oncilla]

Reviewable status: 8 of 18 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker)

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

can you integrate the changes also in the design doc.

They already are (except for originator)
Done.

---

---

go/beacon_srv/internal/beaconing/originator.go, line 53 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

maybe add a comment:
passed returns whether the last timestamp is further away from now than the period or something like that.

Moved to new file.
Done.

---

go/beacon_srv/internal/ifstate/ifstate.go, line 217 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Don't we need to use the write lock for this?

Also pease use defer to unlock

Right.

I think we should only use defer where it makes sense and helps readability.
Here we have 3 lines anyway and not multiple return statements that break the execution flow.
Defers come with a computational overhead, I think is not justified here.

---

go/beacon_srv/internal/ifstate/ifstate.go, line 230 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

ditto write lock & defer

Done.

---

go/beacon_srv/internal/ifstate/pusher_test.go, line 66 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

a,

is that gomock.Eq or what happens internally if you just pass a value to a mock function? Maybe gomock.Eq would be a bit more explicit.

Internally it uses gomock.Eq.

---

go/beacon_srv/internal/ifstate/revoker.go, line 153 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

so that is the same thing as in the pusher code.
Maybe create a function sendIfStateToAllBRs?

Done.

[lukedirtwalker]

Reviewed 10 of 10 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)

---

go/beacon_srv/internal/beaconing/tick.go, line 21 at r2 (raw file):

is

not needed

---

go/beacon_srv/internal/ifstate/ifstate.go, line 217 at r1 (raw file):

Previously, Oncilla wrote…

Right.

I think we should only use defer where it makes sense and helps readability.
Here we have 3 lines anyway and not multiple return statements that break the execution flow.
Defers come with a computational overhead, I think is not justified here.

Not sure, then I would rather not do it when reading (or do we write more than read?).
Previously we ran into bugs while refactoring code that didn't use defer so unless it really is on a critical path I don't see the overhead as problematic. I'd rather go for something consistent.
If you insist you can also leave it like this.

[oncilla]

Reviewable status: 16 of 18 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)

---

go/beacon_srv/internal/beaconing/tick.go, line 21 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

is

not needed

Done.

---

go/beacon_srv/internal/ifstate/ifstate.go, line 217 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Not sure, then I would rather not do it when reading (or do we write more than read?).
Previously we ran into bugs while refactoring code that didn't use defer so unless it really is on a critical path I don't see the overhead as problematic. I'd rather go for something consistent.
If you insist you can also leave it like this.

You are correct, it is not on the critical path. For the reading, i think it makes readability far better than:

func (intf *Interface) LastOriginate() time.Time {
	intf.mu.RLock()
	last := intf.lastOriginate
	intf.mu.RUnlock()
	return last
}

Updated, since you feel it is justified.

[lukedirtwalker]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

[lukedirtwalker]

Reviewed 3 of 3 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved