router: small refactoring to remove the need for a batchconn.WriteTo

[jiceatscion]

WriteTo needs to be given the destination address explicitly. WriteBatch, on the other end, can either find it in each packet structure, or rely on the connection's destination. WriteTo is only used to send BFD packets. It turns out that BFD packets can also very easily be sent via the regular forwarders that use WriteBtach.

The motivation to do that is to simplify the interface between the dataplane and the forwarders, in view of supporting multiple underlays with possibly very different interfaces. So the channels around the processor tasks seems like a good universal interface.

In passing, also removed a duplicate field. Slightly off-topic but still in the spirit of noise abatement.

As this is to facilitate the necessary refactoring...
Contributes to #4593

[jiceatscion]

This change is 

[marcfrei]

The actual changes look good to me. But it would be good to have additional reviews from people closer to the dataplane forwarding logic.

Reviewed 3 of 4 files at r1, 2 of 3 files at r2, all commit messages.
Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @jiceatscion)

---

router/dataplane.go line 1830 at r2 (raw file):

	}
	if _, ok := p.d.external[p.pkt.egress]; !ok {
		// the egress router is not this one.

Comment doesn't contribute much, imo. Could be removed.

---

router/dataplane.go line 2423 at r2 (raw file):

	fwChan, ok := b.dataPlane.fwQs[b.ifID]
	if !ok {
		// WTF? May be we should just treat that as a panic-able offense

Remove comment.

---

router/dataplane.go line 2430 at r2 (raw file):

	p.reset()

	// FIXME: would rather build the pkt directly into the rawPacket's buffer.

Since this is new code, I would either do it or remove the comment.

---

router/dataplane.go line 2441 at r2 (raw file):

	case fwChan <- p:
	default:
		b.dataPlane.returnPacketToPool(p) // Do we care enough to have a metric?

Same. Either do it or remove the comment.

[jiceatscion]

Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @marcfrei)

---

router/dataplane.go line 1830 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Comment doesn't contribute much, imo. Could be removed.

I found it a useful reminder to understand what's happening. I find the name external misleading in that context (like the interface is somewhere else, which is the exact opposite of the truth). So, even though it's a note-to-self, it may help others.

---

router/dataplane.go line 2423 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove comment.

This was intended to trigger an opinion in the reviewer. Apparently yours is that it shouldn't be a panic-able offense, right?

---

router/dataplane.go line 2430 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Since this is new code, I would either do it or remove the comment.

Fair point. I do not have a simple solution right now - it might become easier as refactoring progresses. However, I'd like to leave this as a hint for a future maintainer (most likely me). Would a TODO be more acceptable here?

---

router/dataplane.go line 2441 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Same. Either do it or remove the comment.

As before... what's your opinion? Does it mater? Before we didn't have the potential for a queue full.

[marcfrei]

router/dataplane.go line 2430 at r2 (raw file):

Previously, jiceatscion wrote…

Fair point. I do not have a simple solution right now - it might become easier as refactoring progresses. However, I'd like to leave this as a hint for a future maintainer (most likely me). Would a TODO be more acceptable here?

Ok, if it's not actually clear how to do this well, a TODO would be fine, imo.

[marcfrei]

Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @jiceatscion)

---

router/dataplane.go line 2423 at r2 (raw file):

Previously, jiceatscion wrote…

This was intended to trigger an opinion in the reviewer. Apparently yours is that it shouldn't be a panic-able offense, right?

No, didn't get that this is a meta-comment intended to be removed before merging. I think we could indeed panic here.

---

router/dataplane.go line 2441 at r2 (raw file):

Previously, jiceatscion wrote…

As before... what's your opinion? Does it mater? Before we didn't have the potential for a queue full.

I'm not sure actually about incrementing a metric. Up till now we don't do the increment on the slow path, as far as I can see. So maybe this is similar to that case?

[jiceatscion]

Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @jiceatscion and @marcfrei)

---

router/dataplane.go line 2423 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

No, didn't get that this is a meta-comment intended to be removed before merging. I think we could indeed panic here.

I think so too. I'll change the code to let-it-crash.

---

router/dataplane.go line 2430 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Ok, if it's not actually clear how to do this well, a TODO would be fine, imo.

Will do. In the meantime I figured how to do it, but it's a bit ugly and I'd rather wait until more refactoring allows it more cleanly.

---

router/dataplane.go line 2441 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

I'm not sure actually about incrementing a metric. Up till now we don't do the increment on the slow path, as far as I can see. So maybe this is similar to that case?

I'm leaning that way too. So, I'll ditch the comment.

[jiceatscion]

Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @marcfrei)

---

router/dataplane.go line 2441 at r2 (raw file):

Previously, jiceatscion wrote…

I'm leaning that way too. So, I'll ditch the comment.

done

[jiceatscion]

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @marcfrei)

---

router/dataplane.go line 2430 at r2 (raw file):

Previously, jiceatscion wrote…

Will do. In the meantime I figured how to do it, but it's a bit ugly and I'd rather wait until more refactoring allows it more cleanly.

done

[jiceatscion]

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @marcfrei)

---

router/dataplane.go line 2423 at r2 (raw file):

Previously, jiceatscion wrote…

I think so too. I'll change the code to let-it-crash.

done

[jiceatscion]

Thanks for the review. I'll give a chance to more reviewers, then.

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @marcfrei)

[marcfrei]

Thank you!

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)

[lukedirtwalker]

Reviewed 3 of 4 files at r1, 2 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion)

---

router/dataplane.go line 2423 at r2 (raw file):

Previously, jiceatscion wrote…

done

Does it actually panic if you just assign the ok to _, let's drop the , _

[jiceatscion]

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @marcfrei)

---

router/dataplane.go line 2423 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Does it actually panic if you just assign the ok to _, let's drop the , _

You're right; it would panic a few lines down, which isn't helpful.
Fixed.

[lukedirtwalker]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)