Re: [alto] Yangdoctors early review of draft-ietf-alto-oam-yang-06

[Andy Bierman <[e-mail]>]

On Tue, Apr 11, 2023 at 7:52 AM Jensen Zhang <[e-mail]>
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 <
> [e-mail]> 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.
>
>