[design doc] LLDP support#7105
Conversation
bleader
left a comment
bleader
left a comment
There was a problem hiding this comment.
Sounds globally good to me, but I don't have the ocaml knowledge to comment on the objects.
See my inline comments for some discussions points on the bigger picture.
| Values: | ||
|
|
||
| - `default`: follow `pool.lldp_enabled` | ||
| - `enabled`: enable LLDP on the physical NIC of the PIF. Override `pool.lldp_enabled`. |
There was a problem hiding this comment.
"on the physical NIC of the PIF": how will that be handled? PIFs can be a standard network, a vlan network, a bond or a tunnel.
In my mind:
- standard network is what is described already, nothing to see here
- VLANs: LLDP is L2, and lives outside the VLAN world, but in XAPI we do have multiple PIFs backed on the same NIC, but wih VLAN, I guess we need to skip them
- bonds: kind of the same, I think most users need to monitor each link individually and not the bond itself as LLDP is showing the physicality of the devices, but here again we need to skip these PIFs
- tunnels: I'm not sure if Xen Server uses them, on our side that's an overlay, and shoud probably not be part of the LLDP scope, but again we need to skip them somehow.
There was a problem hiding this comment.
This feature is only for physical PIF. VLAN PIF, bond PIF, tunnel PIF, SR-IOV PIF are skipped, although the data structure is there.
I will state this explicitly here. Thanks.
There was a problem hiding this comment.
Thanks, I think we're aligned that's good to know.
| - retrieval of selected received LLDP neighbor fields through `PIF_metrics` | ||
| - protection against enabling LLDP by default on NIC drivers known to conflict with storage-related functionality | ||
|
|
||
| The implementation uses XAPI for configuration, networkd for per-host application of configuration, and [`lldpd`](https://github.com/lldpd/lldpd) as the LLDP agent in dom0 user space. |
There was a problem hiding this comment.
Here we're talking about lldpd specifically but currently XS 8.4 and XCP-ng 8.3 both ship lldpad, but it is unmanaged (in the sense that xapi has nothing to do with it as far as I understand). On our side it is installed, the service runs, but it is not configured to advertise anything and it is up to the administrators to configure it manually (unless I missed something).
My understanding is that there is no package handling lldp on XS9 side as of now. Is the plan to move to lldpd for XS9 only or is that meant to be backported to lcm branch as well? If that's for XS9 only, we could move to lldpd on XCP-ng 9 as well, there is nothing decided as of yet on that front.
If it's meant to be backported, I was about to suggest to have the handling of the actual configuration some kind of abstraction to allow both backend, but it might not be really possible in any way as it seems lldpad seems to be pretty inconvenient to receive advertisements that this design stores in xapi database. In that case we may need to discuss further.
There was a problem hiding this comment.
This feature is going to be in master (XS 9) branch only. One of reasons is just to get the things simpler.
If that's for XS9 only, we could move to lldpd on XCP-ng 9 as well, there is nothing decided as of yet on that front.
It would be great if this could be decided. But it would be fine to have "some kind of abstraction" as well if it would be really needed.
There was a problem hiding this comment.
It's done! We will be going to lldpd for XCP-ng 9, and keep 8.3 as is, so that proposal is all good for me for the grand scheme of things. As I mentionned, I can't talk about the XAPI objects and ocaml as I'm not very well versed in that, I'll see if someone else on our side can have a look as well.
|
|
||
| Default after fresh install: `true` | ||
|
|
||
| ### `PIF.lldp_status` |
There was a problem hiding this comment.
The word "status" means to me something that reflects some property of the system, but here it is used as a configuration parameter set by the user. Better may be something like lldp_mode, lldp_configuration, or just lldp.
There was a problem hiding this comment.
lldp_mode looks better to me 😄
| - `pool`: the pool reference | ||
| - `enabled`: `true` or `false` |
There was a problem hiding this comment.
Here I would use self and value as parameter names, like other setters have.
| - `pif`: the PIF reference; | ||
| - `status`: `default`, `enabled`, or `disabled`; |
There was a problem hiding this comment.
Here also I'd prefer self and value.
|
|
||
| The following new APIs are added. | ||
|
|
||
| ### `host.set_lldp_conf` |
There was a problem hiding this comment.
You could avoid adding this and just call PIF.plug from pool.set_lldp_enabled for all attached, physical PIFs in the pool. The PIF.plug call is idempotent, so if nothing else has changed, then it should just update the LLDP config through Interface.make_config. PIF.set_lldp_mode could do the same for individual PIFs.
There was a problem hiding this comment.
Thanks. I had a wrong understanding on the ~management_interface:false of Nm.bring_pif_up.
I look at it again. It is harmless even if the PIF is really a management interface.
|
|
||
| ### Report received LLDP TLVs | ||
|
|
||
| networkd periodically queries statistics for individual NICs and writes them to the in-memory file `/dev/shm/network_stats`. The file format is defined in `ocaml/xapi-idl/network/network_stats.ml` and is extended with a new field, `lldp_neighbor`. |
There was a problem hiding this comment.
Are other metrics already using this mechanism?
There was a problem hiding this comment.
Yes. Like carrier, speed, and duplex, etc.
There was a problem hiding this comment.
This was RRDD protocol version 0
|
Signed-off-by: Ming Lu <ming.lu@cloud.com>
minglumlu
force-pushed
the
private/mingl/lldp-design
branch
from
29fd715 to
4d73459
Compare
2501b28
4 participants
No description provided.