-
Notifications
You must be signed in to change notification settings - Fork 1
[mcast] Lifecycle + API changes for Omicron impl #109
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
15f31df
to
076815a
Compare
0fc844d
to
55b4d32
Compare
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. Full disclosure that I haven't yet looked at the integration tests. I think the new rollback machinery is pretty neat -- obviously it's geared toward just multicast, but I think the model of maintaining a snapshot of the old target state and moving back to it is pretty useful.
I think overall I'm a bit confused by the mention of External
forwarding groups being used with instances/guests, but most things here are nits.
This change strengthens the multicast implementation with always-allocated group IDs, better API validation, and comprehensive test improvements for Omicron integration. This update no longer generates multicast group IDs optionally. They are always allocated during group creation, following how multicast groups are configured in the Omicron CP In Omicron, multicast groups are created first, without members, and then members are added as instances are configured for a multicast group. Replication configuration is only written to tables when members are added, but IDs are always generated for the 1:1 mapping between underlay and external (overlay) associated groups. Includes: * **Core ID Management Changes:** - Remove Option<MulticastGroupId> - IDs are always allocated during group creation - Establish 1:1 mapping between underlay and external (overlay) groups - External groups now use IDs from corresponding NAT target (Omicron keeps the true relational mapping) * **API Changes and Validation:** - Remove sources field from internal group APIs (MulticastGroupCreateEntry, MulticastGroupUpdateEntry) - Internal groups cannot have sources or NAT targets - cleaner separation of concerns - External groups retain sources for proper SSM (Source-Specific Multicast) validation - Now fail outright on reset if cleanup is not used properly, which helps on the Omicron side. * **Rollback & Error Handling:** - The addition of a rollback module (and trait) for a more functional approach to rollback on creation or updates involving tables, ports, etc - Improved error propagation in test cleanup to catch resource leaks early - Better validation of group ID relationships to match tables and allocation states * **Test Infrastructure Improvements:** - Enhanced cleanup_test_group() to fail explicitly on deletion errors (prevents test pollution), and ensures proper 1:1 deletion mapping - New tests for rollback, empty members upon multicast group creation/update * **Replication Management:** - Configure replication only when groups have members (change made expecting empty groups in Omicron CP initially) - Reconfigure replication tables when transitioning between empty/populated groups Key aspects this commit covers: 1. ID Management to match expectations in Omicron's multicast impl 2. Validation: Enhanced API validation, group ID relationship checks, SSM validation 3. Rollback: Reset operations now fail explicitly, better error propagation 4. Testing: Comprehensive test improvements, better error handling, standardized cleanup
…nsistentcy This includes `MulticastUnderlayGroupResponse` and `MulticastExternalGroupResponse`, and a unified response type for lists, mixed result calls `MulticastGroupResponse`. We also added an AdminScoped type for underlay and consistent naming throughout. We also rename structs for consistency, and handle rollback at the boundary calls to internal fns. This PR has been updated to accomodate the new API trait, oxidecomputer/omicron#8922, so it adjusts a lot from the previous code and commit.
55b4d32
to
b11823a
Compare
@FelixMcFelix Sorry for the additional changes, as 2daa552 went in after the review. With it changing all the type handling, I went ahead and just made the API more consistent (and properly restrictive) across the board. |
This change strengthens the multicast implementation with always-allocated group IDs, better API validation, and comprehensive test improvements for Omicron integration.
This update no longer generates multicast group IDs optionally. They are always allocated during group creation, following how multicast groups are configured in the Omicron CP
In Omicron, multicast groups are created first, without members, and then members are added as instances are configured for a multicast group.
Replication configuration is only written to tables when members are added, but IDs are always generated for the 1:1 mapping between underlay and external (overlay) associated groups.
Includes:
Core ID Management Changes:
group creation
keeps the true relational mapping)
API Changes and Validation:
APIs (MulticastGroupCreateEntry, MulticastGroupUpdateEntry)
MulticastUnderlayGroupResponse
andMulticastExternalGroupResponse
, and unified for listsMulticastGroupResponse
separation of concerns
Multicast) validation
helps on the Omicron side
Rollback & Error Handling:
functional approach to rollback on creation or updates involving
tables, ports, etc
allocation states
Test Infrastructure Improvements:
(prevents test pollution), and ensures proper 1:1 deletion mapping
Replication Management:
expecting empty groups in Omicron CP initially)
Key aspects this commit covers:
SSM validation
standardized cleanup