Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

router: make room for multiple underlay impls phase 1 #4658

Merged
merged 15 commits into from
Feb 4, 2025

Conversation

jiceatscion
Copy link
Contributor

@jiceatscion jiceatscion commented Nov 29, 2024

The goal is to eventually remove all underlay-specific code from the main router code and place it in a plugin, such that there can then be multiple underlay plugins. This PR isn't attempting to fully accomplish that. It avoids moving some code, so that the diff is easier to read. The price to pay is a few extra spaghetti left between main code and plugin.

The code that should eventually move to the underlay is:

  • runReceiver and runForwarder.
  • likely some of the BFD code.
  • opening of connections (currently done by connector.go)

Other changes being planned:

  • Stop reusing the internal connection for sibling links (so we can take advantage of bound connections).
  • Add knowledge of multiple underlay into the configurator.
  • Make underlay addresses opaque to the router.
  • Demultiplex to links on ingest, so ifID and srcAddress (when they are defined by the link) are obtained in the most efficient way (directly from the link's fields for example).

Contributes to: #4593

@jiceatscion
Copy link
Contributor Author

This change is Reviewable

jiceatscion and others added 3 commits January 16, 2025 11:10
The underlay code is now the udpip underlay provider.

The refactoring is not complete. The assumption that underlay addresses are
udp/ip addresses is still part of the router. Also, the main receiver and
sender tasks are still where they are and use the underlay connections via
a temporary API. The reason for that is to make the diff somewhat more
reviewable by not moving large swaths of code from one file to another.
@jiceatscion jiceatscion changed the title WIP: router refactoring. Make room for multiple underlay impls. router: make room for multiple underlay impls Jan 24, 2025
@jiceatscion jiceatscion marked this pull request as ready for review January 24, 2025 18:56
Copy link
Contributor

@tzaeschke tzaeschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iḿ not really familiar with this module and I'm probably missing a lot of context, so my review is almost exclusively about comments and naming.

:lgtm:

Reviewed 1 of 11 files at r2.
Reviewable status: 1 of 11 files reviewed, 22 unresolved discussions (waiting on @jiceatscion)


-- commits line 16 at r3:
Since it is incomplete, maybe add some points about what exactly still needs to be done?


router/underlay.go line 33 at r3 (raw file):

// Link embodies the router's idea of a point to point connection. A link associates the underlay
// connection, with a bfdSession, a destination address, etc. It also allows the concrete send

connection with

Code quote:

connection, with

router/underlay.go line 48 at r3 (raw file):

	IsUp() bool
	GetIfID() uint16
	GetRemote() netip.AddrPort // incremental refactoring: using code will move to underlay.

TODO?


router/underlay.go line 59 at r3 (raw file):

//
// Incremental refactoring: addresses are still explicitly IP/port. In the next step, we have to
// make them opaque; to be interpreted only by the underlay implementation.

TODO?


router/underlayproviders/udpip.go line 27 at r3 (raw file):

// This is currently the only implementation. The goal of splitting out this code from the router
// is to enable other implementations. However, as a first step, we continue assuming that the
// batchConn is given to us and is a Udp socket and that, in the case of externalLink, it is bound.

UDP

Code quote:

Udp

router/underlayproviders/udpip.go line 49 at r3 (raw file):

func (u *provider) GetConnections() map[netip.AddrPort]router.UnderlayConnection {
	// a map of interfaces and a map of concrete implementations aren't compatible.

A

Code quote:

a

router/underlayproviders/udpip.go line 50 at r3 (raw file):

func (u *provider) GetConnections() map[netip.AddrPort]router.UnderlayConnection {
	// a map of interfaces and a map of concrete implementations aren't compatible.
	// For the same reason, we cannot have the map of concrete as our return type; it

concrete implementations as

Code quote:

concrete as

router/underlayproviders/udpip.go line 55 at r3 (raw file):

	// Since we do not want to store our own things as interfaces, we have to translate.
	// Good thing it doesn't happen often.
	m := make(map[netip.AddrPort]router.UnderlayConnection)

How often is map() called? Is it worth caching it?


router/underlayproviders/udpip.go line 90 at r3 (raw file):

}

// Incremental refactoring: The following implements UnderlayConnection so some of the code

TODO?


router/underlayproviders/udpip.go line 202 at r3 (raw file):

// to erase the separation between link and connection for this implementation. Side effect
// of moving the address:link here: the router does not know if there is an existing link. As
// a result it has to give us a bfdSession in all cases and if we might throuw it away (there

throw

Code quote:

throuw

router/underlayproviders/udpip.go line 204 at r3 (raw file):

// a result it has to give us a bfdSession in all cases and if we might throuw it away (there
// are no permanent resources attached to it). This will be fixed by moving some bfd related code
// in-here; but later.

TODO? Maybe a ticket?


router/underlayproviders/udpip.go line 219 at r3 (raw file):

	c, exists := u.allConnections[netip.AddrPort{}]
	if !exists {
		// That doesn't actually happen and we'll get rid of this soon.

TODO?


router/dataplane.go line 232 at r3 (raw file):

	unsupportedUnspecifiedAddress = errors.New("unsupported unspecified address")
	noBFDSessionFound             = errors.New("no BFD session was found")
	// noBFDSessionConfigured        = errors.New("no BFD sessions have been configured")

Maybe remove this?


router/dataplane.go line 356 at r3 (raw file):

	}

	l := d.underlay.NewInternalLink(conn, d.RunConfig.BatchSize)

rename to "link" or something longer than "l" (which I first read as "1")?


router/dataplane.go line 392 at r3 (raw file):

	}
	lk := d.underlay.NewExternalLink(conn, d.RunConfig.BatchSize, bfd, dst.Addr, ifID)
	d.interfaces[ifID] = lk

lk -> link?


router/dataplane.go line 648 at r3 (raw file):

	}

	for a, l := range d.underlay.GetLinks() {

Use slightly more descriptive variable names?


router/dataplane.go line 741 at r3 (raw file):

		pkt.rawPacket = pkt.rawPacket[:size] // Update size; readBatch does not.
		// Incremental refactoring: We should begin with finding the link and get the ifID

Is it common in scionproto to insert a TODO here?


router/dataplane.go line 1023 at r3 (raw file):

// Big issue with metrics and ifID. If an underlay connection must be shared between links
// (for example, libling links), then we don't have a specific ifID in the connection per se. It

sibling

Code quote:

libling

router/dataplane.go line 1035 at r3 (raw file):

//     metrics... might not be much cheaper than the naive way.
//   - Use one fw queue per ifID in each connection... but then have to round-robin for fairness....
//     smaller batches?

That's also a TODO, isn't it?


router/dataplane.go line 1203 at r3 (raw file):

	// If this is an inter-AS BFD, it can via an interface we own. So the ifID matches one link
	// and the ifID better be valid. In the future that will be checked upstream from here.
	l, exists := p.d.interfaces[p.pkt.ingress]

l is an interface? Maybe rename to intf (or link) or something?


router/dataplane.go line 1528 at r3 (raw file):

// SrcIA checks by disguising packets as transit traffic.
//
// Incremental refactoring: All or part of this check should move to the underlay.

TODO?


router/dataplane.go line 1548 at r3 (raw file):

}

// Validates the egress interface referenced by the current hop. This  is not called for

This is not

Code quote:

This  is not

@jiceatscion jiceatscion requested a review from a team January 27, 2025 15:51
@jiceatscion jiceatscion changed the title router: make room for multiple underlay impls router: make room for multiple underlay impls phase 1 Jan 28, 2025
Copy link
Contributor Author

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 11 files reviewed, 22 unresolved discussions (waiting on @tzaeschke)


-- commits line 16 at r3:

Previously, tzaeschke (Tilmann) wrote…

Since it is incomplete, maybe add some points about what exactly still needs to be done?

Done.


router/dataplane.go line 232 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

Maybe remove this?

Done.


router/dataplane.go line 356 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

rename to "link" or something longer than "l" (which I first read as "1")?

Done.


router/dataplane.go line 392 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

lk -> link?

Done.


router/dataplane.go line 648 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

Use slightly more descriptive variable names?

Done.


router/dataplane.go line 741 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

Is it common in scionproto to insert a TODO here?

Replaced all "incremantal refactoring" with TODO(multi_underlay). Better? Note that, if you and other reviewers prefer, I can do another round of changes on this same PR after it is reviewed. All I'm trying to do is to not combine changes and moves too much in a single review cycle.


router/dataplane.go line 1023 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

sibling

Done.


router/dataplane.go line 1035 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

That's also a TODO, isn't it?

Done.


router/dataplane.go line 1203 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

l is an interface? Maybe rename to intf (or link) or something?

Done.


router/dataplane.go line 1548 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

This is not

Done.


router/underlay.go line 33 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

connection with

Done.


router/underlayproviders/udpip.go line 27 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

UDP

Done.


router/underlayproviders/udpip.go line 49 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

A

Done.


router/underlayproviders/udpip.go line 50 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

concrete implementations as

Done.


router/underlayproviders/udpip.go line 55 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

How often is map() called? Is it worth caching it?

Called once.


router/underlayproviders/udpip.go line 202 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

throw

Done.


router/underlayproviders/udpip.go line 204 at r3 (raw file):

Previously, tzaeschke (Tilmann) wrote…

TODO? Maybe a ticket?

It's part of the continuation of the refactoring. The TODO should be enough.

Copy link
Contributor

@tzaeschke tzaeschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

Copy link
Contributor

@marcfrei marcfrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 11 files at r2, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @jiceatscion)


router/dataplane.go line 81 at r4 (raw file):

)

type BfdSession interface {

BFDSession

https://go.dev/wiki/CodeReviewComments#initialisms

Code quote:

BfdSession

router/dataplane.go line 472 at r4 (raw file):

}

func (d *DataPlane) addBFDController(ifID uint16, s bfd.Sender, cfg control.BFD,

This doesn't really add a BFDController (or BFDSession) anymore. Does it make sense to rename the method and maybe even make it a free-standing function instead of a method on DataPlane? The same might also apply to some of the methods using this one.

Code quote:

addBFDController

router/dataplane.go line 2474 at r4 (raw file):

	// If the packet is sent to an external router, we need to increment the
	// path to prepare it for the next hop.
	// This is an scmp response to pkt, so egress will be pkt.ingress.

SCMP?

Code quote:

scmp

router/underlayproviders/udpip.go line 63 at r4 (raw file):

func (u *provider) GetLinks() map[netip.AddrPort]router.Link {
	return u.allLinks

Should we copy at least the map here? I tend towards 'yes'.

Code quote:

	return u.allLinks

router/underlay.go line 44 at r4 (raw file):

// the link is a sibling link. If the interface ID is zero, then the link is the internal link.
type Link interface {
	GetScope() LinkScope

Scope

Code quote:

	GetScope

router/underlay.go line 45 at r4 (raw file):

type Link interface {
	GetScope() LinkScope
	GetBfdSession() BfdSession

BFDSession

Code quote:

	GetBfdSession

router/underlay.go line 47 at r4 (raw file):

	GetBfdSession() BfdSession
	IsUp() bool
	GetIfID() uint16

IfID

Code quote:

	GetIfID

router/underlay.go line 48 at r4 (raw file):

	IsUp() bool
	GetIfID() uint16
	GetRemote() netip.AddrPort // TODO(multi_underlay): using code will move to underlay.

Remote or RemoteAddrPort

Code quote:

	GetRemote

router/underlay.go line 50 at r4 (raw file):

	GetRemote() netip.AddrPort // TODO(multi_underlay): using code will move to underlay.
	Send(p *Packet) bool
	BlockSend(p *Packet)

Should this really be part of the interface? It's only used for testing, right?

Code quote:

	BlockSend(p *Packet)

router/underlay.go line 86 at r4 (raw file):

	// dataplane code for now. There may be fewer connections than links. For example, right now
	// all sibling links and the internal link use a shared un-bound connection.
	GetConnections() map[netip.AddrPort]UnderlayConnection

Connections

Code quote:

	GetConnections

router/underlay.go line 93 at r4 (raw file):

	// There may be fewer links than ifIDs. For example, all interfaces owned by one given sibling
	// router are connected via the same link because the remote address is the same.
	GetLinks() map[netip.AddrPort]Link

Links

Code quote:

	GetLinks

router/underlay.go line 101 at r4 (raw file):

	// matched with a link), on ingest by the underlay. That would imply moving a part of the
	// runReceiver routine to the underlay. We will do that in the next step.
	GetLink(netip.AddrPort) Link

Link or LinkFor

Code quote:

	GetLink

router/underlay.go line 109 at r4 (raw file):

// TODO(multi_underlay): this will eventually be reduced to nothing at all because the sender
// receiver tasks will be part of the underlay.
type UnderlayConnection interface {

UnderlayConn and the first method then BatchConn()?

Code quote:

UnderlayConnection

Copy link
Contributor Author

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 11 files reviewed, 9 unresolved discussions (waiting on @marcfrei and @tzaeschke)


router/dataplane.go line 81 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

BFDSession

https://go.dev/wiki/CodeReviewComments#initialisms

Done (but we might have to revert the name back if it moves to underlay and no-longer needs to be exported (Unlikely, though).


router/dataplane.go line 472 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

This doesn't really add a BFDController (or BFDSession) anymore. Does it make sense to rename the method and maybe even make it a free-standing function instead of a method on DataPlane? The same might also apply to some of the methods using this one.

Done. I agree that this in not a method and not an Add anymore. Renamed "NewBFDController" for the same reason. renamed AddNextHopBFD and AddExternalInterfaceBFD (although they are still methods od DataPlane. One thing I noticed is that newBFDController doesn't seem like it even belong in DataPlane anymore. Should we move it to bfd/session.go ?


router/dataplane.go line 2474 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

SCMP?

done


router/underlay.go line 44 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Scope

Done.


router/underlay.go line 45 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

BFDSession

Done.


router/underlay.go line 47 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

IfID

Done.


router/underlay.go line 48 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remote or RemoteAddrPort

Done.


router/underlay.go line 50 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Should this really be part of the interface? It's only used for testing, right?

Yes, it is only used by tests. Unfortunately, there's no reasonably simple way of implementing that only in test code. It would take a ridiculous amount of plumbing. Anything wrong about actually having a BlockSend method, even if only for testability? Otherwise, any suggestion?


router/underlay.go line 86 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Connections

Done.


router/underlay.go line 93 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Links

Done.


router/underlay.go line 101 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Link or LinkFor

Done.


router/underlay.go line 109 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

UnderlayConn and the first method then BatchConn()?

I don't much like the abbreviation "Conn". I find it ugly. I didn't chose the name BatchConn. Anyway, this not worth arguing. I changed the name. Good thing this interface will soon disappear, given that underlayconn is also how we name the underlay/conn package on import.


router/underlayproviders/udpip.go line 63 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Should we copy at least the map here? I tend towards 'yes'.

done. I didn't bother because the whole method will go away soon. But then, Go now has maps.clone() so it's little effort and the cost doesn't mater.

Copy link
Contributor

@marcfrei marcfrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 4 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jiceatscion)


router/dataplane.go line 472 at r4 (raw file):

Previously, jiceatscion wrote…

Done. I agree that this in not a method and not an Add anymore. Renamed "NewBFDController" for the same reason. renamed AddNextHopBFD and AddExternalInterfaceBFD (although they are still methods od DataPlane. One thing I noticed is that newBFDController doesn't seem like it even belong in DataPlane anymore. Should we move it to bfd/session.go ?

Yes, moving to bfd/session.go sounds good. And should we rename it to newBFDSession?

The other question would be whether we could also directly use *bfd.Session instead of having the interface BFDSession. bfd.Session has already abstracted away the sending and receiving part. But maybe that's for another refactoring.


router/underlay.go line 50 at r4 (raw file):

Previously, jiceatscion wrote…

Yes, it is only used by tests. Unfortunately, there's no reasonably simple way of implementing that only in test code. It would take a ridiculous amount of plumbing. Anything wrong about actually having a BlockSend method, even if only for testability? Otherwise, any suggestion?

Ok. But I would at least rename it to SendBlocking. I think that's more clear and it also shows that it's a variant of `Send


router/underlayproviders/udpip.go line 95 at r6 (raw file):

// next step

// Name returns the name (for logging) associated with a connection.

Remove comment or fix it.


router/underlayproviders/udpip.go line 100 at r6 (raw file):

}

// Name returns the name (for logging) associated with a connection.

Remove comment or fix it.


router/underlayproviders/udpip.go line 105 at r6 (raw file):

}

// Name returns the name (for logging) associated with a connection.

Remove comment?


router/underlayproviders/udpip.go line 110 at r6 (raw file):

}

// Name returns the name (for logging) associated with a connection.

Remove comment or fix it.


router/underlayproviders/udpip.go line 255 at r6 (raw file):

func (l *siblingLink) Send(p *router.Packet) bool {
	// We use an unbound connection but we offer a connected-oriented service. So, we need to

connection-oriented?

Code quote:

connected-oriented

router/underlayproviders/udpip.go line 267 at r6 (raw file):

func (l *siblingLink) BlockSend(p *router.Packet) {
	// We use an unbound connection but we offer a connected-oriented service. So, we need to

connection-oriented?

Code quote:

connected-oriented

Copy link
Contributor Author

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 13 files reviewed, 6 unresolved discussions (waiting on @marcfrei and @tzaeschke)


router/dataplane.go line 472 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Yes, moving to bfd/session.go sounds good. And should we rename it to newBFDSession?

The other question would be whether we could also directly use *bfd.Session instead of having the interface BFDSession. bfd.Session has already abstracted away the sending and receiving part. But maybe that's for another refactoring.

With you on both accounts. Moved/renamed, but keeping interface for now.


router/underlay.go line 50 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Ok. But I would at least rename it to SendBlocking. I think that's more clear and it also shows that it's a variant of `Send

Done.


router/underlayproviders/udpip.go line 95 at r6 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove comment or fix it.

Done moved doc strings to the interface and fixed them all.


router/underlayproviders/udpip.go line 100 at r6 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove comment or fix it.

Done.


router/underlayproviders/udpip.go line 105 at r6 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove comment?

Done.


router/underlayproviders/udpip.go line 110 at r6 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove comment or fix it.

Done.


router/underlayproviders/udpip.go line 267 at r6 (raw file):

Previously, marcfrei (Marc Frei) wrote…

connection-oriented?

Done.

Copy link
Contributor

@marcfrei marcfrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 6 files at r7, 3 of 3 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion)


router/bfd/session.go line 167 at r8 (raw file):

}

func NewBFDSession(ifID uint16, s Sender, cfg control.BFD,

Looking at the function now that it is in package bfd I'm not sure anymore whether this was a good idea in the first place. Multiple concerns:

  • It should be called NewSession now
  • Argument ifID is not used
  • It pulls in the dependency on github.com/scionproto/scion/router/control
  • How general is this random discriminator functionality?
  • Hardcoded ReceiveQueueSize

Not sure how unacceptable these points really are, but in doubt I'd probably keep the function (as newBFDSession) in dataplane.go for now. Sorry.

New name NewSession() since BFD in the name was redundant.
Removed ifID argument, which wasn't needed.
Added a doc string.
Copy link
Contributor Author

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


router/bfd/session.go line 167 at r8 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Looking at the function now that it is in package bfd I'm not sure anymore whether this was a good idea in the first place. Multiple concerns:

  • It should be called NewSession now
  • Argument ifID is not used
  • It pulls in the dependency on github.com/scionproto/scion/router/control
  • How general is this random discriminator functionality?
  • Hardcoded ReceiveQueueSize

Not sure how unacceptable these points really are, but in doubt I'd probably keep the function (as newBFDSession) in dataplane.go for now. Sorry.

I think that it is better to fix these things and leave the code here.

  • NewSession: Done. Indeed.
  • ifID not used: Gone. It was no-longer used before; I should have removed it anyway.
  • dependency on controller (sure, that's normal, it provides the BFD configuration). Controller.go will need a bit of refactor in the next phase anyway, I might want to move the creation of bfd sessions down to underlay.go anyway, so we'll see to the config then, if we need to.
  • the discriminator is no worse or better than before. It actually doesn't mater; we don't really use it. This is one thing that could stay in the router, given that it's the router that doesn't need it... but then, having a reasonable built-in is more convenient (and ensures protocol compliance).
  • queue size: it is better hard-coded here than in the caller. It is a matter of implementation. The caller should certainly not know what to use.

I think that your concerns stem from the fact that bfd now looks more like a general purpose library than before, but thanks to Go's 1-level package hierarchy, it already was exported; just without a constructor. Doesn't mean that we need to support non-router usage. Given that it was already exposed to such non-router use, this is actually an improvement.

Copy link
Contributor

@marcfrei marcfrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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


router/bfd/session.go line 167 at r8 (raw file):

Previously, jiceatscion wrote…

I think that it is better to fix these things and leave the code here.

  • NewSession: Done. Indeed.
  • ifID not used: Gone. It was no-longer used before; I should have removed it anyway.
  • dependency on controller (sure, that's normal, it provides the BFD configuration). Controller.go will need a bit of refactor in the next phase anyway, I might want to move the creation of bfd sessions down to underlay.go anyway, so we'll see to the config then, if we need to.
  • the discriminator is no worse or better than before. It actually doesn't mater; we don't really use it. This is one thing that could stay in the router, given that it's the router that doesn't need it... but then, having a reasonable built-in is more convenient (and ensures protocol compliance).
  • queue size: it is better hard-coded here than in the caller. It is a matter of implementation. The caller should certainly not know what to use.

I think that your concerns stem from the fact that bfd now looks more like a general purpose library than before, but thanks to Go's 1-level package hierarchy, it already was exported; just without a constructor. Doesn't mean that we need to support non-router usage. Given that it was already exposed to such non-router use, this is actually an improvement.

All clear, let's try it like this then.

@jiceatscion jiceatscion merged commit ca467d2 into scionproto:master Feb 4, 2025
5 checks passed
@jiceatscion jiceatscion deleted the router_multi_io_3 branch February 4, 2025 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants