Hi, Rob, Thanks a lot for the review, apologize for the delay. The authors have submitted -04 (https://datatracker.ietf.org/doc/html/draft-ietf-netmod-schedule-yang-04, diff is available at https://author-tools.ietf.org/iddiff?url2=draft-ietf-netmod-schedule-yang-04) which incorporates some of your concerns/comments below, please also see my reply below inline. My co-authors may want to comment more if they want.
From: Rob Wilton (rwilton) [mailto:[email protected]] Sent: Tuesday, February 4, 2025 6:28 PM To: James Cumming (Nokia) <[email protected]>; [email protected] Cc: NETMOD WG Chairs <[email protected]>; [email protected] Subject: Re: WG Last Call - A Common YANG Data Model for Scheduling Hi authors, WG, Sorry for a late WGLC review. I would like to thank the authors working on this document. Like Adrian, I also think that this document will be useful, but I do have a few concerns/comments. Moderate level comments: (1) p 4, sec 3.1. Features Refer to Section 3.4 for the use of these features. I'm not sure that these YANG features are really useful/helpful. Given this module only defines groupings, then if modules using these groupings are also predicated by these features then the same constraint on basic-recurrence or icalendar-recurrence will affect all modules. Will it always be the case that they will want a shared fate, or would it be better for the modules using these groupings to define their own module specific feature statements? I.e., this draft could suggest that uses of the groupings define their own features if needed, or just say nothing at all and assume that they will do the right thing. I am unsure about this, currently we have different complex levels of recurrence representation, features are defined to allow implementations to declare which one they support (e.g., through YANG-library). You are right that consumer modules can define their own specific feature statement, but the initial consideration for defining features here is for better usage consistency. Does the clarification help? (2) p 7, sec 3.3.1. The "generic-schedule-params" Grouping grouping generic-schedule-params: +-- description? string Most of the definitions include a description, but I would have thought that in many instances the description would likely be superfluous to requirements. Of course, when the groupings are used the description statement could be deviated, but I think that it may be better to elide the description from the groupings and then it could be augmented in during the uses statements, if needed by the uses of these groupings. The description parameter could be useful to record any other comments and details that cannot be available in the structure. The authors had some internal discussion and we incline to keep this in -04, but would also welcome any other feedback from the WG. (3) p 7, sec 3.3.1. The "generic-schedule-params" Grouping +-- time-zone-identifier? sys:timezone-name +-- validity? yang:date-and-time +-- max-allowed-start? yang:date-and-time +-- min-allowed-start? yang:date-and-time +-- max-allowed-end? yang:date-and-time Quite a few of the definitions include both a time-zone-identifier and also various leaves using the yang:date-and-time. I think that the text description of these leaves needs to be very explicit regarding what time-offset should be used, what valid values are, etc. E.g., if I say that the time-zone is PST but then use data-and-time values at +01:00, then what does that mean? Possibly, you might want to consider introducing a date-and-time-no-zone type to avoid the ambiguity. Good point, I agree we fail to make it clear. Some usage requirements of this parameter have been added in -04 in the section where this parameter is defined. For example: "The "time-zone-identifier" parameter, if provided, specifies the time zone reference [RFC7317] of the local date and time values. This parameter MUST be specified if any of the date and time values are in the format of local time. It MUST NOT be applied to date and time values which are specified in the format of UTC or time zone offset to UTC." Note we did not define a type for date-and-time-no-zone as we still want some flexibility here to also allow other formats to be used inside this grouping. (4) p 14, sec 3.3.6. The "recurrence-utc-with-date-times" Grouping grouping generic-schedule-params: ... grouping period-of-time: ... grouping recurrence-basic: ... grouping recurrence-utc: ... grouping recurrence-with-time-zone: ... grouping recurrence-utc-with-date-times: +-- recurrence-first | +-- start-time-utc? yang:date-and-time | +-- duration? uint32 +-- (recurrence-end)? | +--:(until) | | +-- utc-until? yang:date-and-time | +--:(count) | +-- count? uint32 +-- recurrence-description? string +-- frequency identityref +-- interval? uint32 +-- period-timeticks* [period-start] +-- period-start yang:timeticks +-- period-end? yang:timeticks I found the naming of this group and associated description to be somewhat confusing. I.e., I can see that this recurrence uses utc and date-times, but then so does recurrence-utc ... Would it be better if this was called something like recurrence-utc-with-periods? A similar comment applies to recurrence-time-zone-with-date-times? Sure. Renamed as "recurrence-utc-with-periods" and "recurrence-time-zone-with-periods" in -04. Related text and examples are also fixed correspondingly. Minor level comments: (5) p 7, sec 3.3.1. The "generic-schedule-params" Grouping +-- discard-action? identityref grouping period-of-time: ... grouping recurrence-basic: ... grouping recurrence-utc: ... grouping recurrence-with-time-zone: ... grouping recurrence-utc-with-date-times: ... grouping recurrence-time-zone-with-date-times: ... grouping icalendar-recurrence: ... grouping schedule-status: ... grouping schedule-status-with-name: ... This module defines a lot of very similar groupings. I am slightly concerned that this gives too many options and variants for doing it in different ways. I think that it may be nicer if we offered fewer choices for the recurrence groups, which would presumably mean that when it is used, it is used in a more standard way. I.e., perhaps recurrence-basic, recurrence-utc, and recurrence-time-zone-with-date-times would be sufficient, and then the module using these grouping could deviate remove the parts that they don't need? I see your concern, currently we have six of recurrence related groupings, but this is for the consideration of modularity (i.e., a more comprehensive grouping is the result of using a simple one and then defining more parameters on top of that) and to accommodate different requirements. We only have one of them at the very beginning, but then the authors of tvr-schedule-yang requests a grouping with all parameters using UTC time format to ensure consistency and machine readability; while authors of opsawg-scheduling-oam-tests wish to have a time-zone-identifier parameter to improve readability and maintainability; and also authors of opsawg-ucl-acl prefer to use icalendar-recurrence to facilitate the enterprise network users to define the recurrence rule based on week, work day, etc. It's true that all these drafts could use the simplest one and then add their own parameters in their drafts, but these parameters seem generic enough to represent recurrence rule and should be covered in this draft. Does this make sense? (6) p 13, sec 3.3.5. The "recurrence-with-time-zone" Grouping grouping generic-schedule-params: ... grouping period-of-time: ... grouping recurrence-basic: ... grouping recurrence-utc: ... grouping recurrence-with-time-zone: +-- recurrence-first | +-- start-time? yang:date-and-time | +-- duration? duration | +-- time-zone-identifier? sys:timezone-name +-- (recurrence-end)? | +--:(until) | | +-- until? yang:date-and-time | +--:(count) | +-- count? uint32 +-- recurrence-description? string +-- frequency identityref +-- interval? uint32 Does the time-zone-identifier only relate the start-time (because it is under the recurrence-first container) or also the until time? Good point. It's been moved out of the recurrence-first container in -04. This parameter does not only relate to the start-time, but also until time, if either of them is declared in the format of local-time. Nit level comments: (7) p 41, sec 7. Security Considerations The "ietf-schedule" module defines a set of types and groupings. These nodes are intended to be reused by other YANG modules. The module by itself does not expose any data nodes that are writable, data nodes that contain read-only state, or RPCs. As such, there are no additional security issues related to the "ietf- schedule" module that need to be considered. you seem to have an extra space lurking in "ietf- schedule". Fixed. Thanks a lot! Kind regards, Rob Best Regards, Qiufang From: James Cumming (Nokia) <[email protected]<mailto:[email protected]>> Date: Thursday, 16 January 2025 at 22:39 To: [email protected]<mailto:[email protected]> <[email protected]<mailto:[email protected]>> Cc: NETMOD WG Chairs <[email protected]<mailto:[email protected]>>, [email protected]<mailto:[email protected]> <[email protected]<mailto:[email protected]>> Subject: [netmod] WG Last Call - A Common YANG Data Model for Scheduling All, This starts working group last call on the Common YANG Data Model for Scheduling draft. https://datatracker.ietf.org/doc/draft-ietf-netmod-schedule-yang/03/ The IPR call prior to WGLC is concluded with no IPR was previously disclosed. (Thread: https://mailarchive.ietf.org/arch/search/?as=1&email_list=&end_date=&q=subject%3A%28+IPR+call+prior+to+WGLC+-+A+Common+YANG+Data+Model+for+Scheduling%29&qdr=a&start_date=) The working group last call ends on January 31st. Please send your comments to the working group mailing list. Positive comments, e.g., "I've reviewed this document and believe it is ready for publication", are welcome! This is useful and important, even from authors. Thank you, James (Document shepherd)
_______________________________________________ netmod mailing list -- [email protected] To unsubscribe send an email to [email protected]
