Open
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
With nwop 0.2.x we introduced a flag in Layer2 networks to limit GRO of the packets for specific VLANs. This is needed by apps that use AF_PACKET with a fixed buffer size (#174). This adds the flag and adds it to the reference implementation
chdxD1
force-pushed
the
feature/disable-segmentation
branch
from
bff3e8d to
f4f7915
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new Layer2 configuration flag to disable segmentation offload (GRO/GSO/TSO) on VLAN interfaces, intended to keep packet sizes bounded by MTU for AF_PACKET consumers.
Changes:
- Introduces
disableSegmentationto the Layer2-related APIs/CRDs and wires it into the CRA-FRR netlink configuration payload. - Implements VLAN offload toggling via
github.com/safchain/ethtoolin thepkg/nlreconciliation path, with accompanying unit tests and test hooks. - Adjusts several Layer2 iteration loops to take stable element addresses (index-based loop).
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/reconciler/operator/layer2.go | Iteration change for Layer2 revisions when building NodeNetworkConfig and netplan VLANs |
| pkg/reconciler/operator/bgp.go | Iteration change when scanning Layer2 revisions for BGP peering VLAN lookup |
| pkg/reconciler/agent-cra-frr/nodenetworkconfig_reconciler.go | Adds DisableSegmentation to the netlink Layer2Information payload |
| pkg/nl/layer2.go | Adds DisableSegmentation to Layer2Information and applies it during L2 reconciliation via ethtool |
| pkg/nl/ethtool_interface.go | Introduces an ethtool interface + factory for production and test injection |
| pkg/nl/mock_ethtool.go | Adds a mock ethtool implementation used by unit tests |
| pkg/nl/nl_test.go | Mocks ethtool for existing ReconcileL2 success test; adds new unit tests for reconcileSegmentation() |
| api/v1alpha1/nodenetworkconfig_types.go | Adds DisableSegmentation to the per-node Layer2 API type |
| api/v1alpha1/layer2networkconfiguration_types.go | Adds DisableSegmentation to Layer2NetworkConfigurationSpec |
| config/crd/bases/network.t-caas.telekom.com_layer2networkconfigurations.yaml | CRD schema + controller-gen annotation update |
| config/crd/bases/network.t-caas.telekom.com_networkconfigrevisions.yaml | CRD schema + controller-gen annotation update |
| config/crd/bases/network.t-caas.telekom.com_nodenetworkconfigs.yaml | CRD schema + controller-gen annotation update |
| config/crd/bases/network.t-caas.telekom.com_nodenetplanconfigs.yaml | controller-gen annotation update |
| config/crd/bases/network.t-caas.telekom.com_vrfrouteconfigurations.yaml | controller-gen annotation update |
| config/crd/bases/network.t-caas.telekom.com_bgppeerings.yaml | controller-gen annotation update |
| go.mod | Adds github.com/safchain/ethtool dependency |
| go.sum | Adds checksums for github.com/safchain/ethtool |
| Makefile | Bumps controller-gen version used to generate manifests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Move MockEthtool to test file to exclude from production binaries - Propagate DisableSegmentation field into NodeNetworkConfig - Guard reconcileSegmentation to only run when flag is set - Apply segmentation settings in CreateL2 for initial VLAN creation
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Reconcile segmentation unconditionally in ReconcileL2 so toggling DisableSegmentation back to false re-enables GRO/GSO/TSO - Include VLAN interface name in reconcileSegmentation error messages - Reuse createVLAN return value in setupVXLAN, assign to info.vlanInterface for CleanupL2, remove redundant LinkByName
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With nwop 0.2.x we introduced a flag in Layer2 networks to limit GRO of the packets for specific VLANs. This is needed by apps that use AF_PACKET with a fixed buffer size (#174).
This adds the flag and adds it to the reference implementation