Validate interfaces neighbors via LLDP - #292
Merged
Merged
Conversation
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
4 times, most recently
from
April 9, 2026 14:34
1afee7c to
49d83c2
Compare
hardikdr
added
the
area/switch-automation
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
2 times, most recently
from
April 13, 2026 15:39
5ac6e21 to
ccc8336
Compare
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
7 times, most recently
from
April 13, 2026 22:08
fa7115c to
911f23b
Compare
nikatza
changed the title
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
from
April 13, 2026 22:15
911f23b to
c4b7087
Compare
nikatza
marked this pull request as ready for review
April 13, 2026 22:27
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
6 times, most recently
from
April 20, 2026 18:46
065f914 to
d21b806
Compare
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
2 times, most recently
from
April 22, 2026 09:04
f3262aa to
1f391b0
Compare
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
from
May 4, 2026 10:30
cab65dc to
af8dc8f
Compare
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
6 times, most recently
from
May 4, 2026 11:42
14f5cfd to
54616d1
Compare
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
from
May 4, 2026 12:21
54616d1 to
0d7b708
Compare
nikatza
marked this pull request as ready for review
May 4, 2026 12:33
Fetch the configured hostname from the device and populate it in the Device status. The hostname is a configuration item (not state), so it is fetched via GetConfig rather than GetState. This implies one additional "unpacked" gNMI call to the switch. As per [1], it is not possible to issue a `GetRequest` message with different types in the set of requested paths. Note that it is possible to leave the `type` empty and thereby get all data (`CONFIG`, `STATE`, and `OPERATIONAL`). We could implement a `client.GetAny()` method for this case and retrieve all data at once since the models referenced `DeviceInfo` do not have excessive data. However, as this is the only case for such an optimization for now, and the `Device` is infrequently reconciled, this does not seem necessary for now. [1] https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-specification.md#331-the-getrequest-message Signed-off-by: Pujol <enric.pujol@sap.com>
The `dhcprelay_controller` was not aligned with the design of the other controllers. When the device is locked it should requeue using jitter and also with priority `LockWaitPriorityDefault`. Adds a test to verify that reconciliation is triggered when an interface gets configured (after it reconciles once a pending `vrf` resource is created). Signed-off-by: Pujol <enric.pujol@sap.com>
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
from
May 4, 2026 13:34
0d7b708 to
3641530
Compare
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
from
May 5, 2026 08:45
3641530 to
9dfe5cf
Compare
felix-kaestner
approved these changes
May 5, 2026
Add a `neighbors` field to the Interface status containing LLDP neighbor information derived from TLVs: chassis ID, port ID, system name, and expiration time based on TTL. As per 52eae24, users can annotate or label an interface resource with expected neighbor information. This can be cross-validated against the LLDP data that is now stored in the status. We change the annotation format to accept either the chassis ID field or the system name. According to the standard [1], the chassis ID itself can represent different types, like the MAC address, the interface name or the chassis component among others (see Sec. "8.5.2.2 Chassis ID subtype" for details). As information like MAC is not always immediately available, we opt for also allowing the user to use the sysName instead. The rationale is that the hostname is typically configured by the operator, the user, or a known provisioning process. As a result, controller will check the value in the annotation for both cases. The exact mechanism is detailed in the next commit. The status includes a validation field summarizing whether the neighbor could be validated and its result. While TTL is a mandatory field in LLDPDUs, we exclude it from the status because its value decreases continuously on the device. Instead, we compute an ExpirationTime (current time + TTL) which is more meaningful for users. Note that this value is recomputed on each reconcile - a predicate is added later to prevent this from causing infinite reconciliation loops. This design slightly deviates from OpenConfig [2]. In there all LLDP information is contained in the `lldp` subtree. We have decided that configuration is provided by the `lldp` resource. However, the adjacency data is put into the `interface` status and retrieved by its controller. This simplifies the design by removing a dependency towards the `lldp` resource. Notice that if a user enables LLDP by means other than the operator, the data will appear in the interface even if the `lldp` resource does not exist. [1] https://ieeexplore.ieee.org/document/7433915 [2] https://openconfig.net/projects/models/schemadocs/yangdoc/openconfig-lldp.html#lldp-interfaces-interface-neighbors-neighbor-id Signed-off-by: Pujol <enric.pujol@sap.com>
Instruct the interface controller to retrieve the LLDP adjacency information via the provider and populate the status accordingly. The neighbor validation is implemented as a non-blocking operation. Errors are logged but they don't prevent reconciliation of the interface. The validation checks first if the resource has a label. The label is used to perform a validation against an interface resource. If this fails, then the controller falls back to check the annotation. The annotation is used to validate neighbors that are not a kubernetes resource (see 52eae24). The operator does not actively track TTL expiration. Instead, it relies on the device to remove expired neighbors and the periodic requeue interval to sync the status. The ExpirationTime field is informational for users to know when a neighbor will disappear. Note that neighbor entries may become stale if the interface is not requeued in time - the status reflects the last fetched state, not real-time data. This commit also adds a missing watch for the LLDP controller. Now, if the LLDP resource references an interface resource that has not been created, the LLDP resource will be reconciled once the missing dependency is created. This was problematic during bootstraps, as the lldp feature was not installed if a single interface was missing. We also add tests for LLDP operational status degradation, verifying that the controller correctly sets OperationalCondition to False when the device reports LLDP is down, and recovers when it comes back up. Signed-off-by: Pujol <enric.pujol@sap.com>
Retrieve LLDP neighbor information while fetching interface status. As of now, this is only performed on physical interfaces with the use case of cabling validation in mind. The GetStatus response now includes a slice of adjacencies with a subset of the Cisco model for NXOS [1]. Also adds `InterfaceNameEqual` to the InterfaceProvider interface, allowing provider-specific logic to compare interface names. This is needed because of interface naming conventions differ across vendors (e.g., NX-OS uses forms like "eth1/1", "Ethernet1/1"). [1] https://pubhub.devnetcloud.com/media/dme-docs-10-4-3/docs/Discovery%20Protocols/lldp%3AAdjEp/ Signed-off-by: Pujol <enric.pujol@sap.com>
Each reconcile computes ExpirationTime from current time plus TTL. The status patch triggers another reconcile, causing an infinite loop. This predicate filters Update events on the Interface watch, skipping reconciliation when the only change is ExpirationTime. Includes a conditions-count check to ensure initial condition setup completes before filtering kicks in. Signed-off-by: Pujol <enric.pujol@sap.com>
Signed-off-by: Pujol <enric.pujol@sap.com>
nikatza
force-pushed
the
add-lldp-to-iface-status
branch
from
May 5, 2026 14:15
337e48a to
c3f333c
Compare
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
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
3 participants
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.
I am opening this PR even though there is still one action item missing: making the provider reject some of the naming schemes for the physical interfaces. I fear this would make this PR even longer than it is. So I will add this in a follow up PR. As for now, validation via labels and annotations work in the lab in both cases.
This is what an interface resource looks like with neighbor validation using an annotation (i skip the label as it is similar):
I have added context in the commit message, but please let me know if something is not clear.