Hi Bo! On Sun, Jan 26, 2025 at 12:35 PM Wubo (lana) <lana.wubo@xxxxxxxxxx> wrote: > > Hi Per, > > > > Thank you for the valuable review. The -19 version has been submitted to address all the comments: > > Diff: https://author-tools.ietf.org/iddiff?url2=draft-ietf-teas-ietf-network-slice-nbi-yang-19 This import has changed prefix from what is in the originat module ietf-te-packet-types from RFC 8776 from "te-packet-types" to "te-pkt". import ietf-te-packet-types { - prefix te-pkt; + prefix te-packet-types; I suggest to revert this change and use the modules prefix. The benefit in a familiar module prefix is much larger than the cost of the extra characters. > Major issues: > > > > I wonder how the /network-slice-services/slice-service/sdps/sdp\ > > /service-match-criteria/match-criterion/match-type/value leaf-list values will be used in practice. The type is "string" without any constrains and the description states "Provides a value for the Slice Service match criteria, e.g., IP prefix, VLAN ID, or ACL name." > > > > As a model consumer I would expect the type to enforce the valid values depending on type, e.g. choice with inet:dscp, inet:ip-prefix, string with length "1..64" for acl name etc. Since there are no restrictions on valid values, what happens when an invalid value is entered; e.g. > > empty string? Having the constraints defined in YANG enable errors to be found early. > > > > There is no discussion of error cases for the service-match-critera, only the examples in the appendix. > > [Bo Wu]Thank you for identifying this critical issue and providing modification suggestions. In the -19 version, the new YANG code has been added as follows: Good! > Minor issues: > > > > The node "compute-only" might be better recognized if it was named "dry-run". This is commonly used in network products and also in other computer science and engineering fields. > > > > Also, the <test-option> element mentioned needs to be supported, i.e. > > the :validate:1.1 capability needs to be advertised). Suggest mentioning that this only works if it is supported by the server. > > > > [Bo Wu]Thank you for this suggestion. In the -19 version, we are considering to use “test-only” to replace "compute-only", because the AC service model, approaching RFC publication, named the same mechanism as ‘test-only,’ aligning with the NETCONF. > > On the ‘:validate:1.1’ capability, we think this constraints is already described in the NETCONF ‘test-only’ referenced section, which is clear. Good to me! > Figure 9: Application of Match Critera; The formatting is off for the overarching NSS descriptor and it would be beneficial with a legend explaining the "x" and "%" symbols. I guess they indicate different matching criterias per connectivity construct between SDP16 and SDP17? > > > > [Bo Wu]Thanks for the suggestion. The -19 version has been updated accordingly. Thanks! > The example in Figure 23 for establishing a YANG-Push subscription over RESTCONF is wrong. Since YANG-Push is used, "ietf-yang-push:datastore" > > needs to be in the input parameters and the correct subtree filter tag is "ietf-yang-push:datastore-subtree-filter" and not "stream-subtree-filter". > > > > Furthermore, since the example is using YANG-Push over RESTCONF, add a normative reference to RFC 8650. > > [Bo Wu] Thanks for catching this issue. We have corrected with "ietf-yang-push:datastore-subtree-filter". You still need to add the "ietf-yang-push:datastore" node to the establish-subscription RPC input parameters, e.g. something like "ietf-yang-push:datastore":"ietf-datastores:running" > And we also added RFC 8650 as informative reference, since this is only a example and the YANG model not limited to RESTCONF and NETCONF, also RESTful etc. Of course, I just want the example in Figure 23 to be correct YANG-Push. > The example in Figure 35 includes the JSON > > > > "status": {} > > > > But in ac-common "status" is defined as a non-presence container. It should probably not be visible if no child node exists. I know that some implementations might show NP containers even when they have no child nodes, but this is confusing as far as an example in an RFC goes IMHO. > > > > [Bo Wu] Thanks again for identifying this issue. In -19 version, all the examples in the appendix, we added slice-level “admin-status” and removed the empty “status” at other levels. We hope this is clear. > > "status": { > > "admin-status": { > > "status": "ietf-vpn-common:admin-up" > > } Yes, good! > The following idnits warnings should be attended: > > draft-ietf-teas-ietf-network-slice-nbi-yang-17.txt: > > > > Miscellaneous warnings: > > ---------------------------------------------------------------------------- > > > > == The copyright year in the IETF Trust and authors Copyright Line does not > > match the current year > > > > == Line 5329 has weird spacing: '... cos-id uin...' > > > > == Line 5347 has weird spacing: '... cos-id uin...' > > > > == Line 5390 has weird spacing: '... cos-id uin...' > > > > == Line 5408 has weird spacing: '... cos-id uin...' > > > > -- The document date (20 December 2024) is 32 days in the past. Is this > > intentional? > > > > > > Checking references for intended status: Proposed Standard > > ---------------------------------------------------------------------------- > > > > (See RFCs 3967 and 4897 for information about using normative references > > to lower-maturity documents in RFCs) > > > > == Outdated reference: A later version (-19) exists of > > draft-ietf-opsawg-teas-attachment-circuit-18 > > > > == Outdated reference: A later version (-14) exists of > > draft-ietf-opsawg-teas-common-ac-13 > > > > ** Downref: Normative reference to an Informational RFC: RFC 9543 > > > > > > Summary: 1 error (**), 0 flaws (~~), 7 warnings (==), 1 comment (--). > > > > [Bo Wu] Thank you for reminding us. There is one issue not sure that the spacing issues are from the YANG tree generated by pyang. Perhaps wrong warning? > > We fixed copyright to 2025. And the AC references will be fixed when the AC document is published. I just thought I would at least mention them, thanks! I don't know about the YANG warning, maybe that should be looked into by YANG Doctors. -- Per -- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
