-
Notifications
You must be signed in to change notification settings - Fork 0
[feature] ASIC-focused multicast replication and dendrite API #14
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
base: main
Are you sure you want to change the base?
Conversation
4e2fc4b
to
1ce4908
Compare
Includes: * Multicast Group API management: add, modify, delete, reset, SSM handling * sidecar.p4 tofino_asic updates to integrate multicast packet replication * Table mangagement for ASIC switch tables for multicast * integration tests and test utility additions
1ce4908
to
df62508
Compare
2426e18
to
4a945a8
Compare
I'll add some notes to the PR tomorrow morning (to explain through pieces). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Zeeshan. I've provided some comments on the API and am about half way through the P4 code. Have to run, but wanted to get these initial comments in.
( 0, false, true, true, _, USER_SPACE_SERVICE_PORT, true, _, _, _, _, _, _ ) : forward_from_userspace; | ||
( 0, false, false, _, _, _, false, true, _, _, _, _, _ ) : forward_to_userspace; | ||
( 0, false, false, _, true, _, _, _, _, _, _, _, _ ) : forward_to_userspace; | ||
// Link-local multicast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're changing quite a bit around link-local multicast, we need to test this e2e with ddm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this Zeeshan, some notes so far. I haven't yet made it through dpd/src/mcast.rs
and the table management code yet.
Includes: * make sure to avoid switch table apply on drop-specific code in sidecar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Zeeshan, I've had a look mainly focussing on the new changes and dpd/drc/mcast/*.rs
. I do like the removal of v4 multicast from the routing table, and then relying on the NAT target to make use of it from external ports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for being patient. I think I'm happy with the API shape, just some points on cleanup and lock hygiene.
b09c63c
to
c9224db
Compare
578d926
to
21d9274
Compare
Includes: * kyle's patch on scopedguard/group changes (after modifications) with slight mod on del
2c50521
to
6f4082e
Compare
@FelixMcFelix should now be in a good spot API-wise. |
dpd/src/mcast/mod.rs
Outdated
DpdError::Invalid( | ||
"external group ID should have been pre-allocated".to_string(), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like an invariant maintained server-side, rather than something derived from poor user input. DpdError::Invalid
will convert into an HTTP 400, whereas this seems like a 500 on that note. DpdError::Other
might fill that niche?
dpd/src/mcast/mod.rs
Outdated
DpdError::Invalid( | ||
"underlay group ID should have been pre-allocated".to_string(), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here on something which can convert to a 500?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not likely to spot much more at this point, but I think things are generally in good shape. The last error code changes are minor. Thanks!
To determine if we can target Basic TCP:
|
Re-targeted to
|
Pinging @Nieuwejaar and @rcgoodfellow for any other looks here beyond @FelixMcFelix's extensive review! |
Unless @Nieuwejaar has any more feedback I'd say let's get this landed. |
This brings multicast group management and hardware-accelerated forwarding to Dendrite, providing the foundation for our multicast networking stack.
The PR includes:
API Layer:
Hardware Integration:
Network Processing:
Validation:
Context:
Associated PRs