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

ddm tunnel routing #77

Merged
merged 16 commits into from
Jan 23, 2024
Merged

ddm tunnel routing #77

merged 16 commits into from
Jan 23, 2024

Conversation

rcgoodfellow
Copy link
Collaborator

@rcgoodfellow rcgoodfellow commented Aug 7, 2023

Introduced tunnel routing as described in RFD 404. It's strongly encouraged to read that short RFD before reviewing this PR.

Depends on

The primary thing this PR does is add a new tunnel route type and associated advertisement mechanics. Support for adding tunnel routes to OPTE from ddmd is also added.

Copy link
Contributor

@Nieuwejaar Nieuwejaar left a comment

Choose a reason for hiding this comment

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

Tiny nit: most (all?) of the files have 2022 Copyrights that should be updated.

I pointed out a few specific examples, but in general there are a lot of pub struct types that could use top-level and per-field comments.

It seems like there is a fair bit of boilerplate code converting between structs that are identical, other than their names (e.g., TunnelVector and TunnelOrigin). It's not clear to me why we're not just sharing a common structure.

ddm/src/admin.rs Outdated Show resolved Hide resolved
ddm/src/db.rs Outdated Show resolved Hide resolved
ddm/src/db.rs Show resolved Hide resolved
ddm/src/db.rs Show resolved Hide resolved
ddm/src/db.rs Show resolved Hide resolved
ddmadm/src/main.rs Show resolved Hide resolved
ddm/src/exchange.rs Show resolved Hide resolved
ddm/src/exchange.rs Outdated Show resolved Hide resolved
@rcgoodfellow rcgoodfellow force-pushed the ddm-tunnel branch 3 times, most recently from dd10dc5 to f2fc0dd Compare January 9, 2024 04:04
@rcgoodfellow rcgoodfellow force-pushed the ddm-tunnel branch 2 times, most recently from 49bc8f2 to 295107c Compare January 10, 2024 20:04
@rcgoodfellow
Copy link
Collaborator Author

most (all?) of the files have 2022 Copyrights that should be updated.

This has been fixed.

@rcgoodfellow
Copy link
Collaborator Author

It seems like there is a fair bit of boilerplate code converting between structs that are identical, other than their names (e.g., TunnelVector and TunnelOrigin). It's not clear to me why we're not just sharing a common structure.

TunnelVector has been squashed into TunnelOrigin

@rcgoodfellow
Copy link
Collaborator Author

@Nieuwejaar this is ready for another look. The primary things that have changed since you last looked are:

  • A static routing API has been added to mgd. In updates for tunnel routing omicron#3859 we shift so using this API instead of sending static routes directly to dpd. This gives mgd visibility of the entire RIB.
  • Because mgd now has visibility of the entire RIB, it can manage tunnel advertisements autonomously. Previously, the mg-lower subsystem only monitored changes in the RIB from BGP and made the corresponding routing table updates to dpd. Now, changes to the RIB come from BGP and static routing, and mg-lower will also make tunnel endpoint updates via the local ddm daemon, based on changes in the RIB.

Copy link
Contributor

@Nieuwejaar Nieuwejaar left a comment

Choose a reason for hiding this comment

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

I feel like I've only been able to do a tree-level review of this, and it looks good. There's enough here, and it's been long enough since my first look, that I want to revisit the RFD and make another pass to make sure I also have a forest-level view.

Cargo.toml Outdated Show resolved Hide resolved
ddm/src/db.rs Show resolved Hide resolved
ddm/src/db.rs Outdated Show resolved Hide resolved
ddm/src/sys.rs Outdated Show resolved Hide resolved
ddm/src/sys.rs Outdated Show resolved Hide resolved
mg-lower/src/dendrite.rs Outdated Show resolved Hide resolved
mg-lower/src/lib.rs Outdated Show resolved Hide resolved
mgd/src/static_admin.rs Show resolved Hide resolved
ddm/src/exchange.rs Outdated Show resolved Hide resolved
ddm/src/exchange.rs Show resolved Hide resolved
@rcgoodfellow rcgoodfellow enabled auto-merge (squash) January 23, 2024 21:09
@rcgoodfellow rcgoodfellow merged commit 869cf80 into main Jan 23, 2024
9 checks passed
@rcgoodfellow rcgoodfellow deleted the ddm-tunnel branch January 23, 2024 21:13
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.

Implement ddm tunnel routing
3 participants