-
Notifications
You must be signed in to change notification settings - Fork 284
📖 proposal: add support for defining trunk subports #2862
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
Conversation
Signed-off-by: Bharath Nallapeta <nr.bharath97@gmail.com>
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/cc @lentzi90 |
|
@lentzi90 do you have some time to take a look at this? |
smoshiur1237
left a comment
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.
Some comments regarding formatting of md files. I was getting the following minor issues in the docs . It will improve the md file formatting .
| - Maintain backward compatibility with existing port configurations | ||
| - Follow OpenStack Networking (Neutron) trunk API patterns | ||
|
|
||
| ### Non-Goals |
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.
Need one empty line after the title
|
|
||
| ## Motivation | ||
|
|
||
| ### Goals |
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.
Need one empty line after the title
|
|
||
| ### User Stories | ||
|
|
||
| #### Story 1: NFV Workloads |
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.
Need one empty line after the title
| #### Story 1: NFV Workloads | ||
| As a user deploying Network Function Virtualization (NFV) workloads, I need to attach my VM to multiple VLANs through a single trunk interface to separate control plane and data plane traffic without consuming multiple physical interfaces or PCI slots. | ||
|
|
||
| #### Story 2: Multi-Tenant Networking |
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.
Need one empty line after the title
| ```go | ||
| type PortOpts struct { | ||
| // ... existing fields ... | ||
|
|
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.
remove the empty line
| 1. **`EnsureTrunkSubPorts` Method**: | ||
| - Iterates through the desired ports. | ||
| - If `trunk: true` is set, it checks for `Subports`. | ||
| - For each subport: | ||
| - Creates a regular Neutron port using `CommonPortOpts`. | ||
| - Adds the port as a subport to the parent trunk using the `AddSubports` API, specifying the `SegmentationID` and `SegmentationType`. |
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 think you have used space 4. please use space 2
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.
So far we have not used so strict formatting in CAPO. I think we need to discuss this first and agree on a style. Maybe also go through older docs and reformat them so we can actually enforce the style. Otherwise it will just be inconsistent.
| - **Manual Configuration**: Users could manually create trunks and subports using the OpenStack CLI and reference them by ID. | ||
| - *Pros*: No code changes in CAPO. | ||
| - *Cons*: Breaks the "Infrastructure as Code" model; manual steps are error-prone and hard to automate in CAPI workflows. | ||
| - **Separate CRD**: Introduce a new CRD for Trunks/Subports. | ||
| - *Pros*: Decouples networking from Machine. | ||
| - *Cons*: Increases API surface area and complexity for common use cases. Embedding in `OpenStackMachine` is more ergonomic for the 90% case. |
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.
Please confirm with space 2. In my editor it is showing space 4
lentzi90
left a comment
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.
Sorry for the delay!
I don't have any immediate concerns. It looks like a useful feature.
However, I think we should coordinate this with ORC so we don't end up making things unnecessarily painful later.
There is already a Port resource in ORC, but I don't think it has Trunk support yet.
@mandre could you check this? Do you see any issues with this if we later want to adopt ORC Ports? Would it be better to go for a separate CRD (perhaps implemented in ORC)?
/cc @mandre
| 1. **`EnsureTrunkSubPorts` Method**: | ||
| - Iterates through the desired ports. | ||
| - If `trunk: true` is set, it checks for `Subports`. | ||
| - For each subport: | ||
| - Creates a regular Neutron port using `CommonPortOpts`. | ||
| - Adds the port as a subport to the parent trunk using the `AddSubports` API, specifying the `SegmentationID` and `SegmentationType`. |
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.
So far we have not used so strict formatting in CAPO. I think we need to discuss this first and agree on a style. Maybe also go through older docs and reformat them so we can actually enforce the style. Otherwise it will just be inconsistent.
|
|
||
| 2. **Lifecycle Management**: | ||
| - Subports are created after the parent port and trunk are created. | ||
| - Deletion of the `OpenStackMachine` will naturally clean up the ports, but explicit cleanup logic for trunks and subports ensures no orphaned resources. |
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 is unclear to me. Do we need to clean up or will it happen automatically? If it happens automatically, why would manual cleaning ensure no orphaned resources? Wouldn't that be ensured anyway?
Hi @lentzi90! ORC already has a work still in progress to add a new Trunk controller on this PR. Since it is still in progress, I believe it can be aligned with CAPO interests. (sorry for meddling) |
|
@winiciusallan Thanks for the info. I checked out the current ongoing work on Trunk controller. While it implements the feature I have proposed here, the approach is totally different, but I guess that's a bigger question about the design itself (multiple, individual custom resources (Port, Trunk, Machine)). @lentzi90 @mandre I see two paths moving forward here.
Thoughts? I am inclined towards letting this go at this point and plan for the bigger migration work. Please let me know. |
|
The first option sounds good to me, as long as you can wait for it. |
What this PR does / why we need it:
Proposal doc for #2861
/hold