feat(controller): support per-pod NIC granular DHCP control via annotations#6475
feat(controller): support per-pod NIC granular DHCP control via annotations#6475
Conversation
dosubot
bot
added
size:XL
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances network configuration flexibility by introducing granular DHCP control at the individual pod network interface level. Previously, DHCP options were applied uniformly across entire subnets. With this change, users can now specify unique DHCP settings for each pod's network interface using Kubernetes annotations, overriding subnet-level configurations and enabling more precise network management for complex deployments, including those utilizing multiple network interfaces. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Pull Request Test Coverage Report for Build 23631840176Details
💛 - Coveralls |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for per-pod DHCP option configuration via annotations, enhancing granular network control. The implementation is robust, with well-structured changes across the controller and OVN layers, complemented by necessary mock updates and new tests. The refactoring to unify subnet-level and port-level DHCP option handling is a notable improvement. I have a couple of suggestions to further enhance the code by reducing duplication and addressing a known limitation mentioned in the pull request description.
There was a problem hiding this comment.
Pull request overview
This PR adds per-pod / per-network-interface DHCP option overrides (via annotations) by creating OVN per-port DHCP_Options rows and updating LSP dhcpv4_options / dhcpv6_options references, enabling finer-grained behavior than subnet-level DHCP settings.
Changes:
- Added new DHCP annotation templates for v4/v6 options scoped by provider name.
- Extended OVN NB client to support per-port DHCP options CRUD and targeted LSP DHCP pointer updates.
- Updated controller reconciliation/GC logic to apply and clean up per-port DHCP options; added unit tests and regenerated mocks.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/const.go | Adds provider-scoped DHCP annotation templates. |
| pkg/ovs/ovn-nb.go | Introduces PortKey ExternalID constant for per-port DHCP_Options discrimination. |
| pkg/ovs/ovn-nb-suite_test.go | Wires new per-port DHCP options test into the suite. |
| pkg/ovs/ovn-nb-logical_switch_port.go | Adds SetLogicalSwitchPortDHCPOptions to update only LSP DHCP pointers. |
| pkg/ovs/ovn-nb-dhcp_options.go | Refactors DHCP_Options helpers; adds per-port update/delete APIs and unified getter. |
| pkg/ovs/interface.go | Extends public interfaces for new per-port DHCP and LSP update APIs. |
| pkg/controller/subnet.go | Extracts getSubnetMTU() helper used by DHCP option updates. |
| pkg/controller/pod.go | Applies per-port DHCP options on allocation and reconciles DHCP annotations on updates; cleans per-port DHCP on pod delete. |
| pkg/controller/gc.go | Extends DHCP_Options GC to remove orphaned per-port entries when the referenced LSP no longer exists. |
| mocks/pkg/ovs/interface.go | Regenerates mocks for new interface methods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
oilbeater
force-pushed
the
worktree-sorted-yawning-sun
branch
from
9ebddfb to
15d797f
Compare
dosubot
bot
added
size:XXL
…ations Allow pod annotations to override subnet-level DHCP settings at the per-LSP (logical switch port) granularity. Annotations follow the provider-prefixed format: <provider>.kubernetes.io/dhcp-v4-options <provider>.kubernetes.io/dhcp-v6-options Key behaviors: - Pod annotation takes highest priority and overrides subnet enableDHCP - Supports multi-NIC (Multus) by scoping annotations per provider name - Override semantics: annotation completely replaces subnet-level DHCP - GC extended to clean up orphaned per-port DHCP_Options entries - reconcilePodDHCPOptions handles annotation changes on running pods Implementation: - Unified internal OVN DHCP_Options helpers (portName="" = subnet-level, portName!="" = per-port) to eliminate code duplication - Added PortKey ExternalID to discriminate per-port from subnet entries - Extracted getSubnetMTU() helper for reuse across controllers - Added SetLogicalSwitchPortDHCPOptions for targeted LSP DHCP field update Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Fix all issues raised in code review: Bug fixes: - Fix error messages: "logical router name" -> "logical switch name" - Add portName validation in DeleteDHCPOptionsForPort to prevent accidentally deleting all per-port entries when portName is empty - Add lsName/portName validation and dual-stack bounds check in UpdateDHCPOptionsForPort - Fix per-family override semantics: on dual-stack subnets, only create per-port DHCP_Options for families with a non-empty annotation value; fall back to subnet-level UUIDs for un-annotated families - Add missing nil assertion in GetDHCPOptions isolation test Improvements: - Add no-op fast path in SetLogicalSwitchPortDHCPOptions to avoid unnecessary OVN NB transactions when DHCP pointers are unchanged - Handle annotation removal on running pods: reconcilePodDHCPOptions now detects stale per-port entries and reverts to subnet-level DHCP - Extract computePodPortDHCPOptions helper to eliminate duplicated DHCP option preparation logic between reconcileAllocateSubnets and reconcilePodDHCPOptions - Add test case for single-family annotation on dual-stack subnet Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
- Fix computePodPortDHCPOptions to accept subnet parameter instead of always using podNet.Subnet; in reconcileAllocateSubnets pass the subnet returned by acquireAddress to correctly handle static IPs allocated from a different subnet in multi-subnet namespaces - Fix test error messages: "logical router name" -> "logical switch name" to match the corrected error messages in getDHCPOptionsEntry Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
…fecycle Instead of iterating all DHCP options in gcLogicalSwitch, clean them up at the point of resource deletion: - Subnet-level: already handled by handleDeleteLogicalSwitch via DeleteDHCPOptions - Per-port: now cleaned up in markAndCleanLSP when GC deletes orphaned LSPs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Move per-port DHCP management from scattered controller calls into a unified ReconcilePortDHCPOptions method on the OVS layer. This uses the LSP's DHCP UUID pointers (compared against subnet-level UUIDs) to detect stale per-port entries, eliminating the ListDHCPOptions cache scan and unnecessary DeleteDHCPOptionsForPort transactions for non-DHCP pods. Key changes: - Add ReconcilePortDHCPOptions to LSP interface and implement it - Embed per-port DHCP cleanup into DeleteLogicalSwitchPort - Remove computePodPortDHCPOptions from controller - Simplify reconcilePodDHCPOptions and reconcileAllocateSubnets - Remove explicit DeleteDHCPOptionsForPort calls from handleDeletePod/GC Performance: non-DHCP pod creation now has zero OVSDB transactions (was 1x DeleteDHCPOptionsForPort); non-DHCP pod update does O(1) GetLogicalSwitchPort + UUID comparison instead of O(N) cache scan. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
oilbeater
force-pushed
the
worktree-sorted-yawning-sun
branch
from
053912f to
e65b5c0
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds per-pod/per-NIC DHCP option overrides via pod annotations by creating OVN per-port DHCP_Options rows (distinguished via ExternalIDs) and updating LSP dhcpv4_options/dhcpv6_options pointers, so pod-level settings can override subnet-level DHCP behavior.
Changes:
- Introduces new DHCP annotation templates for v4/v6 options (scoped by provider name).
- Extends OVN NB DHCP options management to support per-port DHCP_Options creation/update/delete and to exclude per-port entries from subnet-level getters.
- Updates controller pod/subnet/gc flows to apply per-port DHCP options during allocation and reconcile running pods / cleanup stale entries.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/const.go | Adds annotation key templates for DHCP v4/v6 options. |
| pkg/ovs/ovn-nb.go | Introduces PortKey constant used in ExternalIDs to tag per-port DHCP rows. |
| pkg/ovs/ovn-nb-dhcp_options.go | Adds per-port DHCP options CRUD and unifies DHCP_Options lookup behavior. |
| pkg/ovs/ovn-nb-dhcp_options_test.go | Updates/extends unit tests to cover new per-port DHCP behaviors. |
| pkg/ovs/ovn-nb-logical_switch_port.go | Adds targeted LSP DHCP pointer update helper for running pods. |
| pkg/ovs/ovn-nb-logical_switch_port_test.go | Adds tests around LSP deletion / DHCP cleanup and related behavior. |
| pkg/ovs/ovn-nb-suite_test.go | Wires new DHCP-related tests into the suite. |
| pkg/ovs/interface.go | Extends OVN client interfaces to expose new DHCP APIs. |
| pkg/controller/subnet.go | Extracts subnet MTU computation helper reused by DHCP logic. |
| pkg/controller/pod.go | Reads DHCP annotations and applies per-port DHCP options during allocation and pod updates. |
| pkg/controller/gc.go | Ensures per-port DHCP options are cleaned when orphaned ports are GC’d. |
| mocks/pkg/ovs/interface.go | Updates generated mocks for new OVN client interfaces. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if _, _, err := c.OVNNbClient.ReconcilePortDHCPOptions( | ||
| subnet.Name, portName, subnetDHCPOptionsUUIDs(subnet), |
There was a problem hiding this comment.
computePodPortDHCPOptions calls DeleteDHCPOptionsForPort(portName) whenever the pod has no DHCP annotations. This triggers an OVSDB delete transaction for every port allocation even when no per-port DHCP_Options rows exist (the common case), which can add significant control-plane overhead. Consider making this conditional (e.g., only delete when a cache lookup shows per-port entries exist, similar to reconcilePodDHCPOptions), or moving the cleanup entirely into the “annotation removed / stale entries exist” path.
| dhcpOptionsUUIDs := &DHCPOptionsUUIDs{} | ||
| // Only process a family when the corresponding annotation is set (options non-empty). | ||
| if len(v4CIDR) != 0 && v4Options != "" { | ||
| uuid, err := c.updateDHCPv4Options(lsName, portName, v4CIDR, v4Gateway, v4Options, mtu) | ||
| if err != nil { | ||
| klog.Error(err) | ||
| return nil, fmt.Errorf("update per-port IPv4 dhcp options for port %s: %w", portName, err) | ||
| } | ||
| dhcpOptionsUUIDs.DHCPv4OptionsUUID = uuid | ||
| } | ||
|
|
||
| if len(v6CIDR) != 0 && v6Options != "" { | ||
| uuid, err := c.updateDHCPv6Options(lsName, portName, v6CIDR, v6Options) | ||
| if err != nil { | ||
| klog.Error(err) | ||
| return nil, fmt.Errorf("update per-port IPv6 dhcp options for port %s: %w", portName, err) | ||
| } | ||
| dhcpOptionsUUIDs.DHCPv6OptionsUUID = uuid | ||
| } |
There was a problem hiding this comment.
UpdateDHCPOptionsForPort silently ignores a family when its CIDR is absent from cidrBlock (e.g., v4Options set on an IPv6-only subnet), returning empty UUIDs without any error. This makes misconfigured annotations hard to detect and can lead to users thinking per-pod DHCP overrides are applied when they are not. Consider validating that any non-empty v4Options/v6Options correspond to an available family in cidrBlock, and return a clear error when they don’t.
Summary
<provider>.kubernetes.io/dhcp-v4-optionsand<provider>.kubernetes.io/dhcp-v6-optionsenableDHCPsetting entirelyMotivation
Current DHCP control in kube-ovn is at subnet granularity — all pods on a subnet share the same DHCP options. This PR enables fine-grained per-pod-NIC control by leveraging OVN's LSP-level
dhcpv4_options/dhcpv6_optionsweak references.Changes
New Annotations
OVN Layer (
pkg/ovs/)portName=""= subnet-level,portName!=""= per-port, eliminating code duplicationPortKey = "port"ExternalID to discriminate per-port from subnet-level entriesUpdateDHCPOptionsForPort,DeleteDHCPOptionsForPort,SetLogicalSwitchPortDHCPOptionsController Layer (
pkg/controller/)reconcileAllocateSubnets: reads DHCP annotations during initial pod allocationreconcilePodDHCPOptions: new function handles annotation changes on already-running podsgetSubnetMTU(): extracted helper reused by both subnet and pod controllersTest plan
Test_UpdateDHCPOptionsForPortwith 7 sub-cases covering create, idempotent update, isolation from subnet-level entries, dual-stack, delete, and idempotent deletemake build-goandmake lintpass cleanlyKnown Limitations
🤖 Generated with Claude Code