On Tue, Apr 11, 2023 at 7:52 AM Jensen Zhang <[email protected]> wrote:
> Hi Andy, > > Great thanks for your thorough yangdoctors review. We have gone over all > your comments. Please see our responses inline. > > You can also check details of our proposed changes in the [GitHub > issues][1]. > > [1]: > https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues?q=is%3Aissue+is%3Aopen+label%3Ayangdoctors > > Looking forward to your feedback. > > I checked some of the issues. Just responding about the JSON issue (40) The choices are (1) convert JSON to YANG anydata nodes or (2) embed JSON in a string. If the RFC 7951 mapping is sufficient, then use anydata. Seems like it isn't in this case. Thanks, > Jensen > > Andy > > On Tue, Apr 4, 2023 at 9:12 AM Andy Bierman via Datatracker < > [email protected]> wrote: > >> Reviewer: Andy Bierman >> Review result: Almost Ready >> >> # Review: draft-ietf-alto-oam-yang-06.txt >> >> >> ## module ietf-alto >> >> - General: >> >> - very well written >> - large module but quite understandable for implementors >> >> ### List Key Issues >> >> - there are 3 list keys that use unconstrained 'string' >> as the data type >> >> - /alto/alto-client/client-id >> - /alto/alto-server/cost-type/cost-type-name >> - /alto/alto-server/role/role-name >> - /alto/alto-server/data-source/source-id >> >> >> - the 'resource-id' is done correctly, using a well-constrained >> typedef to define the key leaf. >> >> - A similar typedef for each of these 3 types is needed. >> The constraints do not have to be the same. >> Machine-readable stmts are preferred but description >> is just as normative as 'pattern'. >> >> - the typedef should clarify the following issues: >> >> - semantics: >> >> - it should be clear if the name has any >> special meaning or derived from a field defined >> in a document. >> - Use 'reference' if possible to define the linkage. >> >> - whitespace: >> - leading or trailing whitespace allowed? >> - whitespace within the key string value allowed? >> >> - zero-length: if not allowed then a minimum length of 1 >> should be specified. >> >> - max-length: >> - it may be desirable to pick a maximum string length >> - example with limit: range "1 .. 64"; >> - without limit: range "1 .. max"; >> - there are some engineering trade-offs to consider. >> Details are out of scope here. >> >> - allowed characters: >> - use of plain string means the server is expected to >> support the entire character set. If this is not >> what is desired then limit the character set with a >> pattern or a description-stmt. >> > > Agree. We have added typedefs for them. > > See https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/37 > > >> >> ### /alto/alto-client >> >> - mentioned in sec 5, 5.1, 5.2 >> >> - not clear if alto-client and alto-server would be implemented on the >> same device or expected that a device will implement one container >> or the other >> >> > Agree. We have added two features "alto-client" and "alto-server" to > indicate if a device would implement the "alto-client" container and > "alto-server" container. > > See https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/38 > > > >> >> >> ### /alto/alto-server/logging-system/syslog-params/config-file >> >> - It is unusual to have an implementation or OS-specific >> default value for this sort of parameter. >> >> leaf config-file { >> type inet:uri { >> pattern 'file:.*'; >> } >> default "file:/etc/syslog.conf"; >> description >> "The file location of the syslog configuration."; >> } >> >> > The default value has been removed. > > >> >> ### /alto/alto-server/meta >> >> - The referenced RFC text is not sufficient to implement >> this list. >> >> - It appears each list entry is a map entry and >> the meta-key is a JSONString, and meta-value is >> a JSONValue. >> >> - Not clear how arbitrary JSON is encoded here >> >> >> list meta { >> key "meta-key"; >> description >> "Mapping of custom meta information"; >> reference >> "Section 8.4.1 of RFC 7285."; >> leaf meta-key { >> type string; >> description >> "Custom meta key"; >> } >> leaf meta-value { >> type string; >> mandatory true; >> description >> "Custom meta value"; >> } >> } >> >> - the entire referenced section >> >> Meta information is encoded as a map object for flexibility. >> Specifically, ResponseMeta is defined as: >> >> object-map { JSONString -> JSONValue } ResponseMeta; >> >> > That is a very good catch. We proposed 2 potential options to address this: > > - Option 1: use `binary` to encode the arbitrary JSONValue using base64. > This requires the server to validate the syntax of the encoded value. > - Option 2: use `anydata` statement. But yang data is not fully equivalent > to JSON. This may need the implementors to provide documents to explain how > the yang data will be translated to JSON. > > We would like to see your opinion. > > Please also see > https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/40 > > >> >> ### /alto/alto-server/auth-client/authentication >> >> - this choice is not mandatory and there is no default. >> - either add a mandatory-stmt, or a default-stmt, or explain >> what should happen if no authentication method is configured. >> >> > If a client has no authentication method, it should be ignored because the > server cannot identify it. We have updated the description to clarify this. > > See https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/41 > > >> >> ### /alto/alto-server/data-source/source-params >> >> - Not much guidance or description on this placeholder choice. >> I understand you do not want to reference the 'example' >> YANG modules that augment this choice. >> >> - Please add a reference or description mentioning the >> section in this RFC that explains the source params >> augmentation model. >> >> >> choice source-params { >> description >> "Data source specific configuration."; >> } >> >> ### grouping algorithm >> >> - Same comment as source-params. >> - Provide reference or other guidance how this mandatory >> empty choice is used. >> >> >> grouping algorithm { >> description >> "This grouping defines the base data model for information >> resource creation algorithm."; >> choice algorithm { >> mandatory true; >> description >> "Information resource creation algorithm to be augmented."; >> } >> } >> >> > We have added references to these two placeholders. > > See https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/42 > > >> >> ### /alto/alto-server/data-source/feed-interval >> >> - Add a units statement >> - Is value zero allowed? If so what does it mean? >> If not then use a range-stmt >> >> >> ### /alto/alto-server/data-source/poll-interval >> >> - Add a units statement >> - Is value zero allowed? If so what does it mean? >> If not then use a range-stmt >> >> > We have added units to these two nodes and updated descriptions to explain > the semantics of the zero value. > > See https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/43 > > >> >> ### /alto/alto-server/resource/alto-costmap-params/cost-type-names >> >> - Why is the type 'string' instead of a leafref >> to /alto/alto-server/cost-type/cost-type-name? >> >> - Do not use a plural name for a leaf-list. (Use cost-type-name here) >> >> ### >> /alto/alto-server/resource/alto-costmap-params/testable-cost-type-names >> >> - Why is the type 'string' instead of a leafref >> to /alto/alto-server/cost-type/cost-type-name? >> >> - Do not use a plural name for a leaf-list. >> > > We have defined a new typedef "cost-type-ref" for leafref of > cost-type-name. > > All the plural names of leaf-lists have been fixed. > > See https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/44 > > >> >> ### /alto/alto-server/resource/alto-endpointprop-params/prop-types >> >> - Is there some other YANG data structure that this list >> of property types references? >> >> - What are the allowed values for this string? >> Add a reference or description. >> >> - Do not use a plural name for a leaf-list. >> >> >> leaf-list prop-types { >> type string; >> min-elements 1; >> description >> "Supported endpoint properties."; >> } >> > > We have added a new typedef "endpoint-property" to narrow the values. > > See https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/44 > > >> >> ## module ietf-alto-stats >> >> ### container for statistics >> >> - Suggest using a container (e.g., 'statistics') instead of >> adding this many counter leafs directly to a list entry. >> This makes retrieval of all counters easier. >> >> - augment "/alto:alto/alto:alto-server" >> - augment "/alto:alto/alto:alto-server/alto:resource" >> > > We have put all the server-level and resource-level stats into the > containers. > > See https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/45 > > >> >> ### 5 minute counters >> >> - Should be type gauge64, not counter64 because the value can go down. >> >> - num-total-last-req >> - num-total-last-succ >> - num-total-last-fail >> >> ### size counters >> >> - Should be type uint64, not counter64 because the value is a size. >> >> - res-mem-size >> - res-enc-size >> >> ### update event counters >> >> - It is not clear what the correct data type should be. Not counter64. >> >> - num-event-max >> - num-event-min >> >> - Are these leafs maintained over a 5 minute interval perhaps? >> Then they should be gauge64. >> >> - The procedure to determine min and max should be specified or >> referenced. >> >> > We have corrected the types of those stats. > For "num-total-last-*" and "num-event-*", Med and I think "gauge64" should > be good. But Qin suggests "uint32". I have no strong opinion about this. We > are willing to see your suggestion. > > About how to determine max and min stats, we have updated descriptions of > the metrics to clarify them. > > See https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/15 > for the concrete changes. > > The reporting interval can be configured right now instead of a fixed > value (5 minutes). > > See https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/19 > > >> >> ## Example modules >> >> ### Leafref Typedefs >> >> - There should be typedefs defined in ietf-alto to simplify >> usage in an extension module. >> >> - source-datastore >> - top-name >> >> - Example: source-datastore >> >> leaf source-datastore { >> type leafref { >> path '/alto:alto/alto:alto-server/alto:data-source' >> + '/alto:source-id'; >> } >> >> - Add a typedef to ietf-alto.yang >> >> typedef data-source-ref { >> type leafref { >> path '/alto:alto/alto:alto-server/alto:data-source' >> + '/alto:source-id'; >> } >> } >> >> - Change the leaf to use the typedef >> >> leaf source-datastore { >> type alto:data-source-ref; >> } >> >> - The topo-name leafref definition should be a typedef. >> It is not clear which module should define the typedef. >> >> leaf topo-name { >> type leafref { >> path '/alto:alto/alto:alto-server/alto:data-source' >> + '[alto:source-id = current()/../source-datastore]' >> + '/alto-ds:yang-datastore-source-params' >> + '/alto-ds:target-paths/alto-ds:name'; >> } >> description >> "The name of the IETF layer 3 unicast topology."; >> } >> > > I agree that a typedef for the leafref of source-id can be useful for > other modules. It has been added. > > See https://github.com/ietf-wg-alto/draft-ietf-alto-oam-yang/issues/46 > > However, topo-name can only be used when the "when" condition of the > source-datastore leaf is true. They are combined for the > implementation-specific data source. I cannot imagine how it can be reused > by other modules. > >
_______________________________________________ alto mailing list [email protected] https://www.ietf.org/mailman/listinfo/alto
