Document: draft-ietf-opsawg-discardmodel Title: Information and Data Models for Packet Discard Reporting Reviewer: Ladislav Lhotka Review result: Ready with Issues
I've done an early review of version -03 of this draft, see https://datatracker.ietf.org/doc/review-ietf-opsawg-discardmodel-03-yangdoctors-early-lhotka-2024-08-28/. The authors addressed most of my comments and suggestions in the present version. **** General comments The document now contains two YANG modules: - ietf-packet-discard-reporting-sx contains two dozen groupings with components of the schema for detailed packet loss statistics as well as several YANG features and identities. This module then also defines a YANG structure [RFC 8791] that defines a complete hierarchical schema. This module – or rather the structure – represents the information model (IM). - ietf-packet-discard-reporting (the data model, DM) then augments three existing modules (ietf-interfaces, ietf-logical-network-element and ietf-routing) with relevant packet loss statistics from the IM. This separation is useful and can indeed aid implementing the same data structures in other protocols such as IPFIX. I would recommend to put the "packet-discard-reporting" structure to a separate module. This way, implementations that want to use the ietf-packet-discard-reporting module (and thus have to import ietf-packet-discard-reporting-sx) needn't parse the structure. **** Specific comments - The ietf-packet-discard-reporting module augments "/rt:routing/rt:control-plane-protocols/rt:control-plane-protocol" with the contents of the "plr-sx:control-plane" grouping. The augment target is a "config true" data node and the grouping doesn't contain "config false", so the augmented statistics are also "config true" (rw), which is probably wrong. - What is the purpose of defining "alias" groupings that only use another grouping and nothing else? This applies to the following pairs of groupings: l2-traffic/basic-frames-bytes and l3-traffic/ip. - I assume "all" identity means "either ipv4 or ipv6 address family". If it is so, I'd suggest to use the following hierarchy of identities: address-family ip ipv4 ipv6 The intermediate "ip" identity would replace "all". This is more flexible because "ipv4" and "ipv6" are also derived from "ip" (unlike "all", and it also allows for adding other address families. _______________________________________________ OPSAWG mailing list -- [email protected] To unsubscribe send an email to [email protected]
