Re: [Lsr] Yangdoctors last call review of draft-ietf-isis-sr-yang-19
HI Reshad,
I've uploaded version -21, which includes the changes we discussed.
Thanks,
Yingzhen
On Mon, Jan 22, 2024 at 11:52 AM Reshad Rahman wrote:
> Thanks Yingzhen. Yes I am good with that.
>
> Regards,
> Reshad.
>
> On Monday, January 22, 2024, 02:39:17 PM EST, Yingzhen Qu <
> yingzhen.i...@gmail.com> wrote:
>
>
> Hi Reshad,
>
> Thanks for the review.
>
> The "sid-binding-tlv" and "mt-sid-binding-tlv" are relatively big with
> more content, so I thought it might be easier to read with a container. But
> you're right, it's not following the YANG traditions, how about the
> following?
>
> container sid-binding-tlvs {
> list sid-binding-tlv {
> key "prefix";
> uses sid-binding-tlv;
> description
> "Sid/label binding TLV, type 149.";
> }
> description
> "List of sid/label binding TLVs.";
> }
> container mt-sid-binding-tlvs {
> list mt-sid-binding-tlv {
> key "prefix mt-id";
> uses sid-binding-tlv;
> leaf mt-id {
> type uint16;
> description
> "A 12-bit field containing the non-zero ID
> of the topology.";
> }
> description
> "Multi-Topology SID/Label binding TLV, type 150.";
> reference
> "RFC 8667 - IS-IS Extensions for Segment Routing,
>Section 2.5";
> }
> description
> "List of multi-topology sid/label binding TLVs.";
> }
>
> Thanks,
> Yingzhen
>
> On Mon, Jan 22, 2024 at 6:07 AM Reshad Rahman wrote:
>
> Hi,
>
> Typically we have a container (plural) including a list (singular). In -20
> it was done the other way round. Since this is read-only, IIRC we don't
> need the container including a list as we do for read-write. Is the
> container there for convenience?
>
> Regards,
> Reshad.
>
>
> augment /rt:routing/rt:control-plane-protocols
> /rt:control-plane-protocol/isis:isis/isis:database
> /isis:levels/isis:lsp:
> +--ro sid-binding-tlvs* []
> | +--ro sid-binding-tlv
> | +--ro prefix?inet:ip-prefix
> | +--ro range? uint16
> | +--ro sid-binding-flags
> | | +--ro flags* identityref
> | +--ro prefix-sid-sub-tlvs* []
> | | +--ro prefix-sid-sub-tlvs
> | | +--ro prefix-sid-sub-tlv* [sid]
> | |+--ro prefix-sid-flags
> | || +--ro flags* identityref
> | |+--ro algorithm? identityref
> | |+--ro sid uint32
> | +--ro sid-sub-tlvs* []
> | | +--ro sid-sub-tlv
> | | +--ro length? uint8
> | | +--ro sid? uint32
> | +--ro unknown-tlvs
> |+--ro unknown-tlv* []
> | +--ro type? uint16
> | +--ro length? uint16
> | +--ro value?yang:hex-string
> +--ro mt-sid-binding-tlvs* []
>+--ro mt-sid-binding-tlvs
> +--ro prefix?inet:ip-prefix
> +--ro range? uint16
> +--ro sid-binding-flags
> | +--ro flags* identityref
> +--ro prefix-sid-sub-tlvs* []
> | +--ro prefix-sid-sub-tlvs
> | +--ro prefix-sid-sub-tlv* [sid]
> |+--ro prefix-sid-flags
> || +--ro flags* identityref
> |+--ro algorithm? identityref
> |+--ro sid uint32
> +--ro sid-sub-tlvs* []
> | +--ro sid-sub-tlv
> | +--ro length? uint8
> | +--ro sid? uint32
> +--ro unknown-tlvs
> | +--ro unknown-tlv* []
> | +--ro type? uint16
> | +--ro length? uint16
> | +--ro value?yang:hex-string
> +--ro mt-id? uint16
>
> On Saturday, January 20, 2024, 06:53:52 PM EST, Reshad Rahman <
> res...@yahoo.com> wrote:
>
>
> [Yingzhen]: Thanks for catching this. I've updated the description.
> I looked at the changes in -20. That grouping is now gone and the
> (mt-)sid-binding-tlvs lists have no key, is that the intent?
> Also container mt-sid-binding-tlvs should be renamed to mt-
> sid-binding-tlv.
>
>
>
___
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr
Re: [Lsr] Yangdoctors last call review of draft-ietf-isis-sr-yang-19
Thanks Yingzhen. Yes I am good with that.
Regards,Reshad.
On Monday, January 22, 2024, 02:39:17 PM EST, Yingzhen Qu
wrote:
Hi Reshad,
Thanks for the review.
The "sid-binding-tlv" and "mt-sid-binding-tlv" are relatively big with more
content, so I thought it might be easier to read with a container. But you're
right, it's not following the YANG traditions, how about the following?
container sid-binding-tlvs { list sid-binding-tlv { key
"prefix"; uses sid-binding-tlv; description "Sid/label
binding TLV, type 149."; } description "List of sid/label
binding TLVs."; } container mt-sid-binding-tlvs { list
mt-sid-binding-tlv { key "prefix mt-id"; uses sid-binding-tlv;
leaf mt-id { type uint16; description "A
12-bit field containing the non-zero ID of the topology."; }
description "Multi-Topology SID/Label binding TLV, type 150.";
reference "RFC 8667 - IS-IS Extensions for Segment Routing,
Section 2.5"; } description "List of multi-topology
sid/label binding TLVs."; }
Thanks,Yingzhen
On Mon, Jan 22, 2024 at 6:07 AM Reshad Rahman wrote:
Hi,
Typically we have a container (plural) including a list (singular). In -20 it
was done the other way round. Since this is read-only, IIRC we don't need the
container including a list as we do for read-write. Is the container there for
convenience?
Regards,Reshad.
augment /rt:routing/rt:control-plane-protocols
/rt:control-plane-protocol/isis:isis/isis:database
/isis:levels/isis:lsp:
+--ro sid-binding-tlvs* []
| +--ro sid-binding-tlv
| +--ro prefix?inet:ip-prefix
| +--ro range? uint16
| +--ro sid-binding-flags
| | +--ro flags* identityref
| +--ro prefix-sid-sub-tlvs* []
| | +--ro prefix-sid-sub-tlvs
| | +--ro prefix-sid-sub-tlv* [sid]
| |+--ro prefix-sid-flags
| || +--ro flags* identityref
| |+--ro algorithm? identityref
| |+--ro sid uint32
| +--ro sid-sub-tlvs* []
| | +--ro sid-sub-tlv
| | +--ro length? uint8
| | +--ro sid? uint32
| +--ro unknown-tlvs
|+--ro unknown-tlv* []
| +--ro type? uint16
| +--ro length? uint16
| +--ro value?yang:hex-string
+--ro mt-sid-binding-tlvs* []
+--ro mt-sid-binding-tlvs
+--ro prefix?inet:ip-prefix
+--ro range? uint16
+--ro sid-binding-flags
| +--ro flags* identityref
+--ro prefix-sid-sub-tlvs* []
| +--ro prefix-sid-sub-tlvs
| +--ro prefix-sid-sub-tlv* [sid]
|+--ro prefix-sid-flags
|| +--ro flags* identityref
|+--ro algorithm? identityref
|+--ro sid uint32
+--ro sid-sub-tlvs* []
| +--ro sid-sub-tlv
| +--ro length? uint8
| +--ro sid? uint32
+--ro unknown-tlvs
| +--ro unknown-tlv* []
| +--ro type? uint16
| +--ro length? uint16
| +--ro value?yang:hex-string
+--ro mt-id? uint16On Saturday, January 20, 2024,
06:53:52 PM EST, Reshad Rahman wrote:
[Yingzhen]: Thanks for catching this. I've updated the description. I
looked at the changes in -20. That grouping is now gone and the
(mt-)sid-binding-tlvs lists have no key, is that the intent?Also container
mt-sid-binding-tlvs should be renamed to mt-sid-binding-tlv.
___
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr
Re: [Lsr] Yangdoctors last call review of draft-ietf-isis-sr-yang-19
Hi Reshad,
Thanks for the review.
The "sid-binding-tlv" and "mt-sid-binding-tlv" are relatively big with more
content, so I thought it might be easier to read with a container. But
you're right, it's not following the YANG traditions, how about the
following?
container sid-binding-tlvs {
list sid-binding-tlv {
key "prefix";
uses sid-binding-tlv;
description
"Sid/label binding TLV, type 149.";
}
description
"List of sid/label binding TLVs.";
}
container mt-sid-binding-tlvs {
list mt-sid-binding-tlv {
key "prefix mt-id";
uses sid-binding-tlv;
leaf mt-id {
type uint16;
description
"A 12-bit field containing the non-zero ID
of the topology.";
}
description
"Multi-Topology SID/Label binding TLV, type 150.";
reference
"RFC 8667 - IS-IS Extensions for Segment Routing,
Section 2.5";
}
description
"List of multi-topology sid/label binding TLVs.";
}
Thanks,
Yingzhen
On Mon, Jan 22, 2024 at 6:07 AM Reshad Rahman wrote:
> Hi,
>
> Typically we have a container (plural) including a list (singular). In -20
> it was done the other way round. Since this is read-only, IIRC we don't
> need the container including a list as we do for read-write. Is the
> container there for convenience?
>
> Regards,
> Reshad.
>
>
> augment /rt:routing/rt:control-plane-protocols
> /rt:control-plane-protocol/isis:isis/isis:database
> /isis:levels/isis:lsp:
> +--ro sid-binding-tlvs* []
> | +--ro sid-binding-tlv
> | +--ro prefix?inet:ip-prefix
> | +--ro range? uint16
> | +--ro sid-binding-flags
> | | +--ro flags* identityref
> | +--ro prefix-sid-sub-tlvs* []
> | | +--ro prefix-sid-sub-tlvs
> | | +--ro prefix-sid-sub-tlv* [sid]
> | |+--ro prefix-sid-flags
> | || +--ro flags* identityref
> | |+--ro algorithm? identityref
> | |+--ro sid uint32
> | +--ro sid-sub-tlvs* []
> | | +--ro sid-sub-tlv
> | | +--ro length? uint8
> | | +--ro sid? uint32
> | +--ro unknown-tlvs
> |+--ro unknown-tlv* []
> | +--ro type? uint16
> | +--ro length? uint16
> | +--ro value?yang:hex-string
> +--ro mt-sid-binding-tlvs* []
>+--ro mt-sid-binding-tlvs
> +--ro prefix?inet:ip-prefix
> +--ro range? uint16
> +--ro sid-binding-flags
> | +--ro flags* identityref
> +--ro prefix-sid-sub-tlvs* []
> | +--ro prefix-sid-sub-tlvs
> | +--ro prefix-sid-sub-tlv* [sid]
> |+--ro prefix-sid-flags
> || +--ro flags* identityref
> |+--ro algorithm? identityref
> |+--ro sid uint32
> +--ro sid-sub-tlvs* []
> | +--ro sid-sub-tlv
> | +--ro length? uint8
> | +--ro sid? uint32
> +--ro unknown-tlvs
> | +--ro unknown-tlv* []
> | +--ro type? uint16
> | +--ro length? uint16
> | +--ro value?yang:hex-string
> +--ro mt-id? uint16
>
> On Saturday, January 20, 2024, 06:53:52 PM EST, Reshad Rahman <
> res...@yahoo.com> wrote:
>
>
> [Yingzhen]: Thanks for catching this. I've updated the description.
> I looked at the changes in -20. That grouping is now gone and the
> (mt-)sid-binding-tlvs lists have no key, is that the intent?
> Also container mt-sid-binding-tlvs should be renamed to mt-
> sid-binding-tlv.
>
>
>
___
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr
Re: [Lsr] Yangdoctors last call review of draft-ietf-isis-sr-yang-19
Hi, Typically we have a container (plural) including a list (singular). In -20 it was done the other way round. Since this is read-only, IIRC we don't need the container including a list as we do for read-write. Is the container there for convenience? Regards,Reshad. augment /rt:routing/rt:control-plane-protocols /rt:control-plane-protocol/isis:isis/isis:database /isis:levels/isis:lsp: +--ro sid-binding-tlvs* [] | +--ro sid-binding-tlv | +--ro prefix?inet:ip-prefix | +--ro range? uint16 | +--ro sid-binding-flags | | +--ro flags* identityref | +--ro prefix-sid-sub-tlvs* [] | | +--ro prefix-sid-sub-tlvs | | +--ro prefix-sid-sub-tlv* [sid] | |+--ro prefix-sid-flags | || +--ro flags* identityref | |+--ro algorithm? identityref | |+--ro sid uint32 | +--ro sid-sub-tlvs* [] | | +--ro sid-sub-tlv | | +--ro length? uint8 | | +--ro sid? uint32 | +--ro unknown-tlvs |+--ro unknown-tlv* [] | +--ro type? uint16 | +--ro length? uint16 | +--ro value?yang:hex-string +--ro mt-sid-binding-tlvs* [] +--ro mt-sid-binding-tlvs +--ro prefix?inet:ip-prefix +--ro range? uint16 +--ro sid-binding-flags | +--ro flags* identityref +--ro prefix-sid-sub-tlvs* [] | +--ro prefix-sid-sub-tlvs | +--ro prefix-sid-sub-tlv* [sid] |+--ro prefix-sid-flags || +--ro flags* identityref |+--ro algorithm? identityref |+--ro sid uint32 +--ro sid-sub-tlvs* [] | +--ro sid-sub-tlv | +--ro length? uint8 | +--ro sid? uint32 +--ro unknown-tlvs | +--ro unknown-tlv* [] | +--ro type? uint16 | +--ro length? uint16 | +--ro value?yang:hex-string +--ro mt-id? uint16On Saturday, January 20, 2024, 06:53:52 PM EST, Reshad Rahman wrote: [Yingzhen]: Thanks for catching this. I've updated the description. I looked at the changes in -20. That grouping is now gone and the (mt-)sid-binding-tlvs lists have no key, is that the intent?Also container mt-sid-binding-tlvs should be renamed to mt-sid-binding-tlv. ___ Lsr mailing list Lsr@ietf.org https://www.ietf.org/mailman/listinfo/lsr
Re: [Lsr] Yangdoctors last call review of draft-ietf-isis-sr-yang-19
Hi Yingzhen,
Please see inline.
On Thursday, January 18, 2024, 04:49:28 PM EST, Yingzhen Qu
wrote:
HI Reshad,
Thanks for the review. I've uploaded version -20 to address your comments.
Details below inline.
Thanks,Yingzhen
On Sat, Jan 13, 2024 at 4:24 PM Reshad Rahman via Datatracker
wrote:
Reviewer: Reshad Rahman
Review result: Ready with Issues
Hi all,
This is my YANG Doctor review of -19. Thanks to the authors for making the
effort to align with draft-ietf-ospf-yang (as previously requested).
Comments
Section 3 (YANG Tree)
- There are a few instances of perfix-sid-flags, should bd prefix-sid-flags
[Yingzhen]: fixed.
- For the list below, can there be overlaps between different entries? If not,
this should be documented (ideally should have been documented in RFC9020)
+--rw protocol-srgb {sr-mpls:protocol-srgb}?
+--rw srgb* [lower-bound upper-bound]
+--rw lower-bound uint32
+--rw upper-bound uint32
[Yingzhen]: There should not be overlaps. Agreed with you, this should have
been documented in RFC 9020.
YANG Model
- The identities such as r-bit, n-bit etc should all have a reference (not just
the document but also the section)
[Yingzhen]: I added reference to each base identity.
- All those identities are called x-bit but the description says flag. Which
terminology is typically used in IS-IS?
[Yingzhen]: changed to -flag.
- Leaf Sid, how do we know whether it’s a 32-bit SID or a 20-bit label (not
sure if we need to know)?
[Yingzhen]: Same as ospf-sr-mpls.yang, added length, and updated sid
description.
- For global-blocks and local-blocks, a reference would help.
[Yingzhen]: Done.
- In grouping sid-sub-tlv, is the container sid-sub-tlv needed?
[Yingzhen]: A container would help to identify the boundary of a TLV. In an
ISIS LSP, there are multiple TLVs and sub-tlvs.
- In grouping srlb , can there be an overlap of the advertised local blocks?
Need some enhanced description here iMO. Same question for sr-capability and
global blocks.
[Yingzhen]: please see the above answers.
- In list global-block, why not add a key? I’m assuming the sid would be
unique? Same comment for local-block.
[Yingzhen]: SRGB is defined in RFC 9020, which is common for both OSPF and
ISIS. Here, we're defining protocol specific SRGB, and the key is defined in
the grouping in the ietf-segment-routing-common.yang. SRLB is defined in RFC
9020, which applies to all protocols.
- In grouping srms-preference, leaf preference, no need for the range 0..255
since it is a uint8.
[Yingzhen]: fixed.
- Nit: a few instances of “This group …”, it should be “This grouping …”
[Yingzhen]: fixed.
- Leaf ‘af’, nit/suggestion: I would rename to address-family.
[Yingzhen] : Done.
- In grouping segment-routing-binding-tlv, leaf range, is that description
accurate?
[Yingzhen]: Thanks for catching this. I've updated the description. I
looked at the changes in -20. That grouping is now gone and the
(mt-)sid-binding-tlvs lists have no key, is that the intent?Also container
mt-sid-binding-tlvs should be renamed to mt-sid-binding-tlv.
- Augment with neighbor-system-id, should the leaf node be mandatory?
[Yingzhen]: added "mandatory true". Also did the same for ospf.
- Container selection-tie-breakers, can both protection types be enabled
simultaneously?
[Yingzhen]: yes, it's possible to enable both of them, one or none.
There should be an appendix with a fairly complete config example and also an
operational state example.
[Yingzhen]: Operational state is not easy to do with the IGP database since we
don't have an implementation yet. I agree that it's not easy but
that's the case for most YANG documents at IETF. I've found yanglint to be a
very useful tool for this. Up to the WG to decide.
Regards,Reshad.
Regards,
Reshad.
___
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr
___
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr
Re: [Lsr] Yangdoctors last call review of draft-ietf-isis-sr-yang-19
HI Reshad,
Thanks for the review. I've uploaded version -20 to address your comments.
Details below inline.
Thanks,
Yingzhen
On Sat, Jan 13, 2024 at 4:24 PM Reshad Rahman via Datatracker <
nore...@ietf.org> wrote:
> Reviewer: Reshad Rahman
> Review result: Ready with Issues
>
> Hi all,
>
> This is my YANG Doctor review of -19. Thanks to the authors for making the
> effort to align with draft-ietf-ospf-yang (as previously requested).
>
> Comments
>
>
> Section 3 (YANG Tree)
>
> - There are a few instances of perfix-sid-flags, should bd prefix-sid-flags
>
[Yingzhen]: fixed.
>
> - For the list below, can there be overlaps between different entries? If
> not,
> this should be documented (ideally should have been documented in RFC9020)
>
>+--rw protocol-srgb {sr-mpls:protocol-srgb}?
> +--rw srgb* [lower-bound upper-bound]
> +--rw lower-bounduint32
> +--rw upper-bounduint32
>
> [Yingzhen]: There should not be overlaps. Agreed with you, this should
have been documented in RFC 9020.
> YANG Model
>
> - The identities such as r-bit, n-bit etc should all have a reference (not
> just
> the document but also the section)
>
[Yingzhen]: I added reference to each base identity.
>
> - All those identities are called x-bit but the description says flag.
> Which
> terminology is typically used in IS-IS?
>
> [Yingzhen]: changed to -flag.
> - Leaf Sid, how do we know whether it’s a 32-bit SID or a 20-bit label (not
> sure if we need to know)?
>
[Yingzhen]: Same as ospf-sr-mpls.yang, added length, and updated sid
description.
> - For global-blocks and local-blocks, a reference would help.
>
> [Yingzhen]: Done.
> - In grouping sid-sub-tlv, is the container sid-sub-tlv needed?
>
> [Yingzhen]: A container would help to identify the boundary of a TLV. In
an ISIS LSP, there are multiple TLVs and sub-tlvs.
> - In grouping srlb , can there be an overlap of the advertised local
> blocks?
> Need some enhanced description here iMO. Same question for sr-capability
> and
> global blocks.
>
> [Yingzhen]: please see the above answers.
- In list global-block, why not add a key? I’m assuming the sid would be
> unique? Same comment for local-block.
>
[Yingzhen]: SRGB is defined in RFC 9020, which is common for both OSPF and
ISIS. Here, we're defining protocol specific SRGB, and the key is defined
in the grouping in the ietf-segment-routing-common.yang. SRLB is defined in
RFC 9020, which applies to all protocols.
>
> - In grouping srms-preference, leaf preference, no need for the range
> 0..255
> since it is a uint8.
>
> [Yingzhen]: fixed.
> - Nit: a few instances of “This group …”, it should be “This grouping …”
>
> [Yingzhen]: fixed.
> - Leaf ‘af’, nit/suggestion: I would rename to address-family.
>
> [Yingzhen] : Done.
> - In grouping segment-routing-binding-tlv, leaf range, is that description
> accurate?
>
> [Yingzhen]: Thanks for catching this. I've updated the description.
> - Augment with neighbor-system-id, should the leaf node be mandatory?
>
> [Yingzhen]: added "mandatory true". Also did the same for ospf.
- Container selection-tie-breakers, can both protection types be enabled
> simultaneously?
>
> [Yingzhen]: yes, it's possible to enable both of them, one or none.
> There should be an appendix with a fairly complete config example and also
> an
> operational state example.
>
> [Yingzhen]: Operational state is not easy to do with the IGP database
since we don't have an implementation yet.
> Regards,
> Reshad.
>
>
>
>
___
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr
[Lsr] Yangdoctors last call review of draft-ietf-isis-sr-yang-19
Reviewer: Reshad Rahman
Review result: Ready with Issues
Hi all,
This is my YANG Doctor review of -19. Thanks to the authors for making the
effort to align with draft-ietf-ospf-yang (as previously requested).
Comments
Section 3 (YANG Tree)
- There are a few instances of perfix-sid-flags, should bd prefix-sid-flags
- For the list below, can there be overlaps between different entries? If not,
this should be documented (ideally should have been documented in RFC9020)
+--rw protocol-srgb {sr-mpls:protocol-srgb}?
+--rw srgb* [lower-bound upper-bound]
+--rw lower-bounduint32
+--rw upper-bounduint32
YANG Model
- The identities such as r-bit, n-bit etc should all have a reference (not just
the document but also the section)
- All those identities are called x-bit but the description says flag. Which
terminology is typically used in IS-IS?
- Leaf Sid, how do we know whether it’s a 32-bit SID or a 20-bit label (not
sure if we need to know)?
- For global-blocks and local-blocks, a reference would help.
- In grouping sid-sub-tlv, is the container sid-sub-tlv needed?
- In grouping srlb , can there be an overlap of the advertised local blocks?
Need some enhanced description here iMO. Same question for sr-capability and
global blocks.
- In list global-block, why not add a key? I’m assuming the sid would be
unique? Same comment for local-block.
- In grouping srms-preference, leaf preference, no need for the range 0..255
since it is a uint8.
- Nit: a few instances of “This group …”, it should be “This grouping …”
- Leaf ‘af’, nit/suggestion: I would rename to address-family.
- In grouping segment-routing-binding-tlv, leaf range, is that description
accurate?
- Augment with neighbor-system-id, should the leaf node be mandatory?
- Container selection-tie-breakers, can both protection types be enabled
simultaneously?
There should be an appendix with a fairly complete config example and also an
operational state example.
Regards,
Reshad.
___
Lsr mailing list
Lsr@ietf.org
https://www.ietf.org/mailman/listinfo/lsr
