Re: [yang-doctors] [I2nsf] Yangdoctors early review of draft-ietf-i2nsf-nsf-monitoring-data-model-04

[Andy Bierman <[e-mail]>]

On Mon, Mar 1, 2021 at 4:21 PM Linda Dunbar <[e-mail]>
wrote:

> Andy and Reshad,
>
>
>
> Does it help if I request another YANG DR review?
>

Yes.
I think the IESG should specify and document the procedures for follow-up
YANG module reviews.
We should avoid undocumented ad-hoc procedures that rely on email as the
sole collaboration tool.
IMO there should be a new review request for each updated revision.  I
especially want to
know if the request is for an early review or WGLC review, and other
contextual info.


Andy



>
> Thanks, Linda
>
>
>
> *From:* I2nsf <[e-mail]> *On Behalf Of * Reshad Rahman
> *Sent:* Wednesday, February 24, 2021 10:40 AM
> *To:* Andy Bierman <[e-mail]>
> *Cc:* Roman Danyliw <[e-mail]>; [e-mail]; JungSoo Park <
> [e-mail]>; Yunchul Choi <[e-mail]>; YANG Doctors <
> [e-mail]>; Patrick Lingga <[e-mail]>;
> skku-iotlab-members <[e-mail]>; Mr. Jaehoon
> Paul Jeong <[e-mail]>
> *Subject:* Re: [I2nsf] [yang-doctors] Yangdoctors early review of
> draft-ietf-i2nsf-nsf-monitoring-data-model-04
>
>
>
>
>
>
>
> *From: *Andy Bierman <[e-mail]>
> *Date: *Wednesday, February 24, 2021 at 11:30 AM
> *To: *Reshad Rahman <[e-mail]>
> *Cc: *"Mr. Jaehoon Paul Jeong" <[e-mail]>, Roman Danyliw <
> [e-mail]>, "[e-mail]" <[e-mail]>, JungSoo Park <
> [e-mail]>, Yunchul Choi <[e-mail]>, Patrick Lingga <
> [e-mail]>, YANG Doctors <[e-mail]>,
> skku-iotlab-members <[e-mail]>
> *Subject: *Re: [yang-doctors] [I2nsf] Yangdoctors early review of
> draft-ietf-i2nsf-nsf-monitoring-data-model-04
>
>
>
>
>
>
>
> On Mon, Feb 22, 2021 at 7:31 PM Reshad Rahman <[e-mail]> wrote:
>
> *I cannot figure out how to enter a new review in the IETF pages (even
> logged in),*
>
> I have run into the same issue in the past. You need to go to the actual
> review link,
> https://datatracker.ietf.org/doc/draft-ietf-i2nsf-nsf-monitoring-data-model/reviewrequest/13767/
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fdraft-ietf-i2nsf-nsf-monitoring-data-model%2Freviewrequest%2F13767%2F&data=04%7C01%7Clinda.dunbar%40futurewei.com%7C959faa31307f463f868c08d8dc7283ae%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637501731693750332%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=qBtXhMguOMJEGkRkZ%2BBQsLffjtku4CV5x9kyyqiqa2U%3D&reserved=0>
> in this case, and click on the “Correct review” button. My recollection is
> that this will create a newer version of the review.
>
>
>
>
>
> But I am not modifying my review of the old draft.
>
> I am entering my review of the new draft.
>
> <RR> Version 00 will remain as a review of the old rev. The new version
> will be a review of the new draft (you can provide a new reviewed revision).
>
> I agree that correct review implies that you can only change the review of
> the old rev, so the button should probably be “Correct/new review” instead.
> Anyway I believe that, even though the UI is misleading, you can enter a
> new review while keeping the old review.
>
>
>
> Regards,
>
> Reshad.
>
>
>
> I think a better procedure would be for the authors to request a new
> review for the updated draft.
>
> Hopefully the same reviewer can be assigned automatically.
>
>
>
>
>
> Regards,
>
> Reshad.
>
>
>
>
>
> Andy
>
>
>
> *From: *yang-doctors <[e-mail]> on behalf of Andy
> Bierman <[e-mail]>
> *Date: *Saturday, February 20, 2021 at 12:11 PM
> *To: *"Mr. Jaehoon Paul Jeong" <[e-mail]>
> *Cc: *Roman Danyliw <[e-mail]>, "[e-mail]" <[e-mail]>,
> JungSoo Park <[e-mail]>, Yunchul Choi <[e-mail]>, Patrick
> Lingga <[e-mail]>, YANG Doctors <[e-mail]>,
> skku-iotlab-members <[e-mail]>
> *Subject: *Re: [yang-doctors] [I2nsf] Yangdoctors early review of
> draft-ietf-i2nsf-nsf-monitoring-data-model-04
>
>
>
> Hi,
>
>
>
> I have reviewed the module in draft-05.
>
> Thanks for the revision letter. That really helped make the review go fast.
>
> All my draft-04 comments have been addressed.
>
>
>
> I cannot figure out how to enter a new review in the IETF pages (even
> logged in),
>
> so it is attached here. pyang is reporting some minor style issues.
>
>
>
>
>
> Status: Ready with nits (see attached file)
>
>
>
>
>
> Andy
>
>
>
>
>
> On Wed, Feb 17, 2021 at 7:15 AM Mr. Jaehoon Paul Jeong <
> [e-mail]> wrote:
>
> Hi Andy,
>
> Patrick and I have addressed your comments on the following revision:
>
>
>
>
> https://datatracker.ietf.org/doc/draft-ietf-i2nsf-nsf-monitoring-data-model/
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fdraft-ietf-i2nsf-nsf-monitoring-data-model%2F&data=04%7C01%7Clinda.dunbar%40futurewei.com%7C959faa31307f463f868c08d8dc7283ae%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637501731693750332%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3gJHx%2F9sQfQbaiUYFQmxZidapOXeC53MUqVf6N1he%2Bw%3D&reserved=0>
>
> https://tools.ietf.org/html/draft-ietf-i2nsf-nsf-monitoring-data-model-05
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftools.ietf.org%2Fhtml%2Fdraft-ietf-i2nsf-nsf-monitoring-data-model-05&data=04%7C01%7Clinda.dunbar%40futurewei.com%7C959faa31307f463f868c08d8dc7283ae%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637501731693760323%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=jEmAChZfGP2qMxfZScCg7I5jspjawUkF%2FtIMhG6DmXk%3D&reserved=0>
>
>
>
> I attach the revision letter to explain how to address your comments on
> the revision.
>
>
>
> If you have further comments, please let me know.
>
>
>
> If you are satisfied with our revision, please update the YANG doctor
> review status in the following link:
>
>
> https://datatracker.ietf.org/doc/draft-ietf-i2nsf-nsf-monitoring-data-model/
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fdraft-ietf-i2nsf-nsf-monitoring-data-model%2F&data=04%7C01%7Clinda.dunbar%40futurewei.com%7C959faa31307f463f868c08d8dc7283ae%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637501731693760323%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=dYm5Hu%2BAeYV1bBtcMZcACJjKYrtA96cmBAJoZqv2Ca0%3D&reserved=0>
>
>
>
> Thanks.
>
>
>
> Best Regards,
>
> Paul
>
>
>
> On Tue, Oct 6, 2020 at 3:03 AM Andy Bierman via Datatracker <
> [e-mail]> wrote:
>
> Reviewer: Andy Bierman
> Review result: Almost Ready
>
>
>
> Major Issues:
>
>  - None
>
> Moderate Issues:
>
>  - top-level 'counters' container does not follow naming conventions.
>    Should start with 'i2nsf', probably 'i2nsf-state'
>
>  - There do not seem to be any writable objects in the /counters
>    subtree so this container should have a 'config false' statement
>
>  - top-level typedef and grouping description-stmts are self-referential
>    and not useful. Need to rewrite description-stmts and/or add
>    reference-stmts as needed.
>
>  - grouping common-monitoring-data/time-stamp
>    Is this a different time stamp than the one in the NETCONF notification?
>    The 'message generation time' sounds like the standard timestamp.
>    Does this object represent the event detection time?
>
>  - grouping i2nsf-system-alarm-type-content/usage
>  - grouping i2nsf-system-alarm-type-content/threshold
>    These are uint8 leafs with unclear descriptions.
>    Not sure why uint8 is the appropriate type.
>    Needs 1 or more of (reference, units, better description)
>
>  - grouping traffic-rates
>    Add a units statement to each leaf. Not sure what units to use
>    but it should be consistent. (e.g, pps, bps used in descriptions
>    should also be in a units-stmt)
>
>  - grouping i2nsf-system-counter-type-content
>    These counters should use the yang:counter32 type instead of uint32
>
>  - container counters/system-interface
>  - container counters/nsf-firewall
>  - container counters/nsf-policy-hits
>   The descriptions are too terse and confusing, and need a rewrite.
>
>  -  container counters/nsf-firewall
>  -  container counters/nsf-policy-hits
>     - uses i2nsf-nsf-counters-type-content;
>     Many of the fields expanded from this grouping all say
>     they refer to 'the packet'. Why are they in this global
>     container of counters? E.g. (src-ip, dst-ip, src-port, dst-port)
>     Not clear at all how the server is supposed to apply this
>     grouping to these containers.
>
>  - many leafs use "uint32" type for a rate.
>    Should add a units-stmt
>
>  - leaf counters/nsf-policy-hits/hit-times
>    The purpose and type are confusing and generic.
>    If this is a counter then use counter32
>
>  - cut-and-paste for notification-stmt content should be replaced
>    with grouping/uses instead. Applies to the nsf-detection-*
>    and the various logging notifications. Even a grouping that
>    has 1 object in it is better than cut-and-paste 5+ times
>
> Minor Issues:
>
>  - top-level identifiers are too generic
>    should have 'i2nsf-' prefix to be more reusable outside this module
>
>  - quite a lot of identities that an implementation is required to support.
>    If this set of identities might change a lot faster than the
>    notifications and counter objects, then consider putting them
>    in a separate module
>
>  - leaf with same type named differently; both intrusion-attack-type
>    - nsf-detection-intrusion/sub-attack-type
>    - nsf-log-intrusion/attack-type
>
>  - quite a lot of notification event types for a server to implement
>    and a user to manage. All are mandatory (no if-feature statements).
>    Some such as nsf-detection-* subset are very similar.
>    A section or table would be useful that showed the YANG notification
>    names and their purpose -- maybe a reference to another RFC
>    with more details
>
>  - there seems to be notifications for intrusion events and then
>    again for the logging of those events.  This seems excessive
>    but
>
>
>  - grouping common-monitoring-data/time-stamp
>    Is this a different time stamp than the one in the NETCONF notification?
>    The 'message generation time' sounds like the standard timestamp.
>    Is this event detection time?
>
>  - grouping common-monitoring-data/module-name
>    Is this a YANG module or some other type of module?
>
>  - there is no way to configure which notifications should be generated
>    or maybe how often.  YANG Push has its own dampening-period.
>    Since these are event stream subscriptions, not datastore subscriptions,
>    YANG-Push does not apply to this document at all.
>
>    If there are a lot of notifications then a server implementation
>    might drop some
>
>  - grouping i2nsf-nsf-event-type-content-extend/src-zone
>  - grouping i2nsf-nsf-event-type-content-extend/src-zone
>    These use type 'string'. Consider using a typedef that constrains
>    the string.  General comment where unconstrained string is used:
>    The corner-case values such as empty string are often not allowed
>    in implementations.
>
>
>  - grouping i2nsf-nsf-event-type-content-extend/rule-id
>  - grouping i2nsf-nsf-event-type-content-extend/rule-name
>  - grouping i2nsf-nsf-event-type-content-extend/profile
>    These objects seem to reference objects in another YANG module.
>    If so, then leafref types might be more appropriate.
>
>  - grouping i2nsf-nsf-event-type-content/rule-id
>  - grouping i2nsf-nsf-event-type-content/rule-name
>  - grouping i2nsf-nsf-event-type-content/profile
>  - grouping i2nsf-nsf-event-type-content/raw-info
>    These objects are cut-and-paste duplicates from
>    grouping i2nsf-nsf-event-type-content. They should
>    be in a separate grouping used by both. Also applies
>    to some other sets of objects
>
>  - limits issues (e.g. current-session, maximum-session
>    The type is uint8. This is only OK it is impossible for any
>    implementation to ever have or want more than 255 of them.
>    If some other RFC really does limit the values where uint8
>    is used, then that is OK. If so, a reference-stmt would help.
>
>
>
> _______________________________________________
> I2nsf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/i2nsf
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ietf.org%2Fmailman%2Flistinfo%2Fi2nsf&data=04%7C01%7Clinda.dunbar%40futurewei.com%7C959faa31307f463f868c08d8dc7283ae%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637501731693770318%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=btLcI1LT3ujHq73ICfGbJDyCKOP1teOZsuEL9dP7VDg%3D&reserved=0>
>
>
>
> _______________________________________________ yang-doctors mailing list
> [e-mail] https://www.ietf.org/mailman/listinfo/yang-doctors
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ietf.org%2Fmailman%2Flistinfo%2Fyang-doctors&data=04%7C01%7Clinda.dunbar%40futurewei.com%7C959faa31307f463f868c08d8dc7283ae%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637501731693770318%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=%2BFmmp8MqSGlZlpBgTBdbUyvpYiu1Ck3GGMxBgeJFuRA%3D&reserved=0>
>
>