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. Thanks, Jensen On Tue, Apr 4, 2023 at 9:12 AM Andy Bierman via Datatracker < nore...@ietf.org> 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 alto@ietf.org https://www.ietf.org/mailman/listinfo/alto
