-
Notifications
You must be signed in to change notification settings - Fork 690
Lldp intf defaults #1262
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: master
Are you sure you want to change the base?
Lldp intf defaults #1262
Conversation
This reverts commit 3be8ac2.
|
What you are describing in this change-set I believe should be discussed in a wider context as it is not unique to LLDP and can relate to the realm of templating. It would be good to drive a consistent approach as this can apply in quite a few protocol domains not only in this fashion but also as template expansion, apply-group like scenarios, etc... something that has not been focused much in OpenConfig as the assumption is full expansion all the time vs. device behavior expansion. You are also selecting a default here that not all implementations support equally so either a YANG deviation is acceptable or there is an underlying ask to change each implementations default behavior to comply. |
|
Two questions:
or is the assumption that the /lldp/interfaces/ list by default is empty and this leaf is supposed to populate it?
|
|
I try to solve 2 problems.
|
I try to make it clear in description.
|
There are two defaults.
The reference above shows p.2.
My interpretation is that interface state should follow the yang defaults (which is enabled), when LLDP enabled on a protocol level. And the third implementation just doesn't support that default. I'm not convinced that we need to change the OC LLDP yang by creating new data structures to accommodate for that.
Yes, that is a potential scenario where this might be useful. To me it feels a bit superficial (trying to find justifications for the proposed solution), but maybe operator community will give different feedback.
If that's the issue we're trying to solve, then point made by @earies even more important. LLDP is just one thing, but OC uses /interfaces/ subtree in many contexts: acl, qos, lacp, sampling. Any decisions in this area require thorough planning and will have a strategic impact on the OC model design as a whole. |
|
My reading, as the OC model is currently defined, is that you need to explicitly list all of the interfaces that you want LLDP to run over. You can add an entry lldp/interfaces/interface list and set enabled to false, but that would be the same as just not having it in the list at all. I think that this would also end up being a breaking change because you are modifying the global default. |
IMO there are a few possible scenarios, depending on the implementation.
I'd argue that the proposed PR is conceptually aligned only with implementations that follow p.3 but have a different default value (disabled). For implementations that follow p.1 or p.2 (if there are any), I would agree with @rgwilton's point above. |
Change Scope
This change is adding container where LLDP attributes can be configured for all interfaces in system.
The system may have 1000's of interfaces, and operator may need to enable LLDP on all of them exept a few. Or oppodite. Depending on network design.
The default value of
enabledis defined as TRUE. This matches industry common behaviour.Platform Implementations
"When enabling LLDP, it is enabled on all interfaces by default..."
"When you enable LLDP globally using the lldp command, LLDP is not enabled on subinterfaces or bundle subinterfaces by default." - hence is enabled on interfaces and bundles.
Note: Junos implicit default is disabled lldp on all interfaces - in contrast to OpenConfig and other vendors.