Move VXLAN L2 programming to its own object. #8449

fasaxc · 2024-01-25T11:21:05Z

Description

Remove special-case L2 logic in the RouteTable.
Add new VXLANFDB object to manage it.

Related issues/PRs

CORE-9902

Todos

Tests
Documentation
Release note

Release Note

Clean up: VXLAN ARP and FDB programming is moved to a new sub-component.  This should make it easier to maintain.

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

docs-pr-required: This change requires a change to the documentation that has not been completed yet.
docs-completed: This change has all necessary documentation completed.
docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

release-note-required: This PR has user-facing changes. Most PRs should have this label.
release-note-not-required: This PR has no user-facing changes.

Other optional labels:

cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

- Remove special-case L2 logic in the RouteTable. - Add new VXLANFDB object to manage it.

Realised that the old API could result in closing a Handle before we'd finished trying to use it.

nelljerram

LGTM. A few nits and suggestions, but up to you, and don't think I will need to re-review.

nelljerram · 2024-02-05T14:46:59Z

felix/dataplane/linux/int_dataplane.go

@@ -506,18 +507,22 @@ func NewIntDataplaneDriver(config Config) *InternalDataplane {
 		var routeTableVXLAN routetable.RouteTableInterface
 		if !config.RouteSyncDisabled {
 			log.Debug("RouteSyncDisabled is false.")
-			routeTableVXLAN = routetable.New([]string{"^vxlan.calico$"}, 4, true, config.NetlinkTimeout,
+			routeTableVXLAN = routetable.New([]string{"^vxlan.calico$"}, 4, config.NetlinkTimeout,


Should build the regex here from VXLANIfaceNameV4, and similarly below for the v6 case.

nelljerram · 2024-02-05T14:58:43Z

felix/dataplane/linux/vxlan_mgr.go

@@ -140,7 +151,6 @@ func newVXLANManager(
 			brt = routetable.New(


It looks messy, in the v6 case, to construct a v4 route table and then immediately discard it. Is it worth fixing that too while you're here?

nelljerram · 2024-02-05T15:10:24Z

felix/dataplane/linux/vxlan_mgr_test.go

+func (t *mockVXLANFDB) SetVTEPs(targets []vxlanfdb.VTEP) {
+	log.WithFields(log.Fields{
+		"targets": targets,
+	}).Debug("SetL2Routes")


nelljerram · 2024-02-05T15:18:52Z

felix/netlinkshim/mocknetlink/netlink.go

@@ -127,6 +127,8 @@ const (
 	FailNextRouteDel
 	FailNextAddARP
 	FailNextNeighSet
+	FailNextNeighDel
+	FailNextNeighList


Note, if my counting is correct, we're now using 31 bits here. Only one more spare? Or are we OK to go past 32?

I'll bump it up to a uint64.

nelljerram · 2024-02-05T15:21:40Z

felix/netlinkshim/mocknetlink/netlink.go

+	}
+}
+
+func (d *MockNetlinkDataplane) ExpectNeighs(family int, neigh3s ...netlink.Neigh) {


Is the "3" here a typo?

nelljerram · 2024-02-05T15:32:44Z

felix/vxlanfdb/vxlan_fdb.go

+
+type VTEP struct {
+	// HostIP is the node's real IP address; the IP that we send the
+	// VXLAN packets to.


Can you add "remote" here, if this really means the remote node's IP address.

nelljerram · 2024-02-05T15:45:48Z

felix/vxlanfdb/vxlan_fdb.go

+// kernel what MAC address to use for the inner ethernet frame inside the
+// VXLAN packet.  FDB entries tell the kernel what IP address to use for the
+// outer IP header, given a particular inner MAC.  So, ARP maps IP->(inner)MAC;
+// FDB maps (inner)MAC->(outer)IP.


This is incredibly useful, but I think it could be even better if you added the remote VXLAN route to complete the overall process:

<pod block CIDR> via <VXLAN device> gw <tunnel IP> onlink

ARP table resolves <tunnel IP> to a (fictitious!) <tunnel MAC>

FDB table maps <tunnel MAC> to remote node IP.

Maybe also helpful to point out that the MAC is fictitious. (Assuming I'm right about that!)

The tunnel MAC is actually used in the packet. I think we usually derive it from the IP address but we also support carrying it in the datamodel.

nelljerram · 2024-02-05T16:00:28Z

libcalico-go/lib/logutils/logutils.go

@@ -522,3 +523,34 @@ func SafeParseLogLevel(logLevel string) log.Level {
 	}
 	return defaultedLevel
 }
+
+// TestingTWriter adapts a *testing.T as a Writer so it can be used as a target
+// for logrus.


Could do with more exposition about how this is helpful, and maybe point to an example.

nelljerram · 2024-02-05T16:02:59Z

libcalico-go/lib/logutils/logutils.go

Need copyright update - do you not have a commit hook to check this?

nelljerram · 2024-02-05T17:12:56Z

felix/vxlanfdb/vxlan_fdb.go

+			log.WithField("entry", entry).Debug("Deleting ARP entry.")
+		}
+		neigh := entryToNeigh(entry)
+		if err := nl.NeighDel(neigh); err != nil && !errors.Is(err, unix.ENOENT) {


IIUC, here is where the point that you mentioned comes into play, about NeighDel on a constructed neigh, instead of on one returned by NeighList. Am I right that you could eliminate this delta by passing the whole neigh as the value of the delta tracker, instead of ipMacMapping? If so, I suppose the advantage of ipMacMapping is that you can use the same value for both trackers, whereas with the whole neigh you'd need two slightly different neighs.

I think the code is OK as it is; just commenting to check my understanding, and to point out the possibility in case it was appealing.

Yes, exactly, I could keep the value read from the dataplane but then I'd need a custom comparison function in the DeltaTracker, which adds a few more corner cases to think about.

marvin-tigera added this to the Calico v3.28.0 milestone Jan 25, 2024

marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Jan 25, 2024

fasaxc force-pushed the vxlan-fdb branch from c6d6f4f to 5987494 Compare January 25, 2024 14:09

Move VXLAN L2 programming to its own object.

44181d9

- Remove special-case L2 logic in the RouteTable. - Add new VXLANFDB object to manage it.

fasaxc force-pushed the vxlan-fdb branch from 5987494 to b6944fb Compare January 26, 2024 09:53

fasaxc added 3 commits January 26, 2024 14:44

Add UT for VXLANFDB.

740a6cf

Adjust HandleManager API to do lazy Delete() of handles.

ff837b3

Realised that the old API could result in closing a Handle before we'd finished trying to use it.

Add UT for VXLANFDB.

02d894a

fasaxc force-pushed the vxlan-fdb branch from b6944fb to 02d894a Compare January 26, 2024 14:45

fasaxc added 2 commits January 26, 2024 14:49

Markups.

9600369

Refactor VXLAN FDB to share code between different families.

9c16ce1

fasaxc marked this pull request as ready for review January 26, 2024 15:34

fasaxc requested a review from a team as a code owner January 26, 2024 15:34

fasaxc added docs-not-required Docs not required for this change and removed docs-pr-required Change is not yet documented labels Jan 26, 2024

fasaxc force-pushed the vxlan-fdb branch 2 times, most recently from d72e02c to 64bc9c2 Compare January 26, 2024 17:11

Add IPv6 test.

6ef8c45

fasaxc force-pushed the vxlan-fdb branch from 64bc9c2 to 6ef8c45 Compare January 30, 2024 11:51

nelljerram approved these changes Feb 5, 2024

View reviewed changes

fasaxc added 2 commits February 12, 2024 15:59

Markups.

5bab45d

Merge remote-tracking branch 'origin/master' into vxlan-fdb

156e48e

fasaxc merged commit 29a4062 into projectcalico:master Feb 12, 2024
2 checks passed

fasaxc mentioned this pull request Feb 29, 2024

Remove unused SetL2Routes method. #8578

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move VXLAN L2 programming to its own object. #8449

Move VXLAN L2 programming to its own object. #8449

fasaxc commented Jan 25, 2024 •

edited

Loading

nelljerram left a comment

nelljerram Feb 5, 2024

nelljerram Feb 5, 2024

nelljerram Feb 5, 2024

nelljerram Feb 5, 2024

fasaxc Feb 12, 2024

nelljerram Feb 5, 2024

nelljerram Feb 5, 2024

nelljerram Feb 5, 2024

fasaxc Feb 12, 2024

nelljerram Feb 5, 2024

nelljerram Feb 5, 2024

nelljerram Feb 5, 2024

fasaxc Feb 12, 2024

		@@ -140,7 +151,6 @@ func newVXLANManager(
		brt = routetable.New(

Move VXLAN L2 programming to its own object. #8449

Move VXLAN L2 programming to its own object. #8449

Conversation

fasaxc commented Jan 25, 2024 • edited Loading

Description

Related issues/PRs

Todos

Release Note

Reminder for the reviewer

nelljerram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fasaxc commented Jan 25, 2024 •

edited

Loading