Hi, Adrian, Thanks a lot for the thorough review, apologize for being late on this response. 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) to address your comments, please also see my reply below inline.
From: Adrian Farrel [mailto:[email protected]] Sent: Sunday, January 26, 2025 6:12 AM To: 'James Cumming (Nokia)' <[email protected]>; [email protected] Cc: 'NETMOD WG Chairs' <[email protected]>; [email protected] Subject: RE: [netmod] WG Last Call - A Common YANG Data Model for Scheduling Hi, Let me start by thanking the authors because I believe this YANG model will be a useful building block for a number of operational features that rely on the ability to schedule functions within the network. I'd like to see this document progress through WGLC, but I did find a lot of (relatively small) comments during my review. I'm sorry, but I did not check my comments for duplication of Reshad's review. No worries. I actually did not see any duplication;-) I did not review the examples, but congratulations on not having any wrapping lines! Cheers, Adrian = Comments = I note some tension between "scheduling" which is a planning process, and "reporting" which makes a statement about what has happened in the past. The document sems to mainly live up to its title (i.e., scheduling), but I find text such as (in A.9): At the time of 12:15 PM, 2025-12-01 (UTC), the recurring event occurred at (note that occurrence at 10:20 AM is excluded): 9:00, 9:20, 9:40, 10:00, 10:40, 11:00, 11:20, 11:40, 12:00. The last occurrence was at 12:00, the upcoming one is at 12:20. It seems a very small step from reporting what scheduled things happened to reporting what unscheduled things happened. I'm not sure that there is anything to be done here, but it is possible that the Abstract and Introduction need to call out more specifically the fact that the module can also report what has happened as well as what is to happen. Make sense to me. The Abstract and Introduction have included the following sentence: "It also defines groupings for validating requested schedules and reporting scheduling status." --- The Abstract says that the granularity levels vary from basic to advanced. Is that true? Or is it, as stated in 3.1, the representation of the recurrence rule that varies from basic to advanced? The authors have made the following update: OLD: For the sake of better modularity, the module includes a set of recurrence related groupings with varying granularity levels (i.e., from basic to advanced). NEW: For the sake of better modularity, the module includes a set of recurrence related groupings with varying levels of representation (i.e., from basic to advanced) to accommodate a variety of requirements. Feel free to propose text if you prefer. --- Introduction has: Section 5 discusses relationship with the managed objects defined in [RFC3231]. Very good, but a little hint would be helpful. Such as: Section 5 discusses the relationship with the Management Information Base (MIB) managed objects for scheduling management operations defined in [RFC3231]. Fixed. --- Instead of Section 3 being called "Groupings" it might be more helpfully named "Scheduling Groupings". Similarly, Appendix A might be better as "Examples of Scheduling Format Representation" Fixed. --- I hate that 5545 used "SECONDLY" and "MINUTELY". It makes me twitch every time I read them. But you are right to use the terms defined there. I do wonder whether using upper case, as in 5545, might make the normal reader less ill. Sure, we use the upper case as value in the normative text, but also add the note that frequency values defined as identities in the YANG module are used in lowercase. Note that in 3.2 you use "per second, per minute, etc.". Perhaps this should be "secondly, minutely, etc.". Fixed. --- 3.1 says: Refer to Section 3.4 for the use of these features. But I think these features are discussed in subsections of 3.3. It's true that 3.4 is on "Feature Use" but it only contains 8 lines of text. Features are related to the groupings defined in sec3.3. fixed as: Refer to Sections 3.4 and B.1 for the use of these features. --- I think maybe re-order "frequency-type" and "schedule-type" because frequency is only relevant for recurring schedules. This could also apply in the module, itself. That's fair. Fixed. --- As I understand "schedule-state" it is used to read the current state of a schedule, but also to control it. I really think you should not overload and confuse this. Just as we would have done in SMI, you should have two objects: intended-schedule-state and current-schedule-state. I am less sure about this. By leveraging NMDA, the device could define <intended> to hold configuration that the system attempts to apply, and also <operational> to hold the configuration that is actually in use. This would not require you model the same node for twice. Note we give the choice for the schedule-state to be defined as RW, but implementers can use it simply as RO if they like. --- The description of "discard-action" is mangled in the document, I find: 3.2 * "discard-action": Specifies the action to perform when a schedule is discarded (e.g., generate a warning or an error message). 3.3.1 The "discard-action" parameter specifies the action if a requested schedule is considered inactive or out-of-date. 6 identity discard-action { description "Indicates that a schedule will be discarded."; } 6 leaf discard-action { type identityref { base discard-action; } description "Specifies the behavior when a schedule is discarded when enforcing the guards in this grouping or it is received out-of-date."; } Now, I'll grant that these are *similar* but they don't feel to be the same. I'd suggest: - In 3.2, spend a little time explaining what "discarded" means. Perhaps the text from 3.3.1 would do. Fixed in sec.3.2 as the following: * "discard-action-type": Specifies the action for the responder to take (e.g., generate a warning or an error message) when a requested schedule cannot be accepted for any reason and is discarded. - Fix the description of the discard-action identity, as this is definitely wrong. Fixed as the following: description "Indicates the action for the responder to take when a requested schedule cannot be accepted for any reason and is discarded."; - Consider whether the description of the discard-action leaf appears to limit the cases of discard leaving other types of discard unaddressed. You are right, we should not limit the cases of discard, it should be generic enough now. - Worry about having a leaf with the same name as the identity. Named as identity discard-action-type. Better? --- 3.3.1 has: The "time-zone-identifier" parameter, if provided, specifies the time zone reference of the date and time values with local time format. The use of "with local time format" made me think that the format of the time-zone-identifier was a local matter. But this is not, I think, what you mean. Perhaps: The "time-zone-identifier" parameter, if provided, specifies the time zone reference [RFC7317] of the local date and time values. Fixed. --- 3.3.1 has: The "validity" parameter specifies the date and time after which a schedule will be considered as invalid. It determines the latest time that a schedule can be executed by a system and takes precedence over similar attributes that are provided at the schedule instance itself. Now, consider a schedule event that takes a finite amount of time to be executed. Do you mean that execution must complete before or at the validity time or that execution must start before or at the validity time? See the updated description for this parameter, I hope it is clear now. It is about the time that execution must start (not complete). So do other parameters (i.e., max/min-allowed-start, max-allowed-end). --- 3.3.2 The "period-of-time" grouping (Figure 3) represents a time period using either a start ("period-start") and end date and time ("period- end"), or a start ("period-start") and a positive time duration ("duration"). Why is period-end described as "end date and time" but period-start is just described as "start"? Surely it should be "start date and time". Fixed. --- Duration I came to this when reading 3.3.2 The "period-of-time" grouping (Figure 3) represents a time period using either a start ("period-start") and end date and time ("period- end"), or a start ("period-start") and a positive time duration ("duration"). For the first format, the start of the period MUST be before the end of the period. There is an obvious imbalance that made me think "can duration be zero? and if so, why must the start be before the end (and not before or equal to)? So I searched "duration" in the document and I found a real mixture. It may be useable, but I wonder why it is necessary to have all the following ways of expressing duration. 3.3.4 and 3.3.6 | +-- duration? uint32 Which allows zero. 6 typedef duration { type string { pattern '((\+)?|\-)P((([0-9]+)D)?(T(0[0-9]|1[0-9]|2[0-3])' + ':[0-5][0-9]:[0-5][0-9]))|P([0-9]+)W'; } Which allows +/-/0 6 case duration { description "A period of time is defined by a start and a positive duration of time."; leaf duration { type duration { pattern 'P((([0-9]+)D)?(T(0[0-9]|1[0-9]|2[0-3])' + ':[0-5][0-9]:[0-5][0-9]))|P([0-9]+)W'; } Which is +/0 although the text says it is a positive duration. Now fixed as: The end time MUST be no later than the end time, and the duration is non-negative. This way I think it is consistent with the pattern definition. --- 3.3 Note that per Section 4.13 of [I-D.ietf-netmod-rfc8407bis], no "default" substatement is used here because there are cases (e.g., profiling) where the use of the default is problematic. This feels like [I-D.ietf-netmod-rfc8407bis] is being used as a normative reference. Shouldn't be an issue as that draft is a little ahead in the processing queue. True. --- 3.3.4 The "duration" parameter specifies, in units of seconds, the time period of the first occurrence. Unless specified otherwise, the "duration" also applies to subsequent recurrence instances. Unless specified otherwise how? In most cases the duration should apply to all occurrences, but the authors want to give flexibility here. We fixed this as following: Unless specified otherwise (e.g., described in the "description" statement), the "duration" also applies to subsequent recurrence instances. --- 3.3.4 Note that the "interval" and "duration" cover two distinct properties of a schedule event. The interval specifies when a schedule will occur, combined with the frequency parameter; while the duration indicates how long an occurrence will last. Clear. But what happens if the interval is shorter than the duration? Is that allowed? Good point. I don't think we should restrict this case, although this might be rare. The following statement added in the end: This document allows the interval between occurrences to be shorter than the duration of each occurrence (e.g., a recurring event is scheduled to start every day for a duration of 2 days). Clear? --- Count The various textual descriptions and tree diagrams made me see that 0 was allowed (unit32) but that there was an assumption that the first occurrence would always occur. Only when I got the module itself did I find leaf count { type uint32; description "The positive number of occurrences at which to terminate the recurrence."; } Perhaps the text (in all the cases) should include "MUST be GTE 1" Fixed now: The value of the "count" node MUST be greater than 1. --- Direction Not totally happy that the scoping -53..-1|1..53 can create some confusion of the 53rd Monday in the month, etc. I see your point, but this does not seem to be fixable in YANG. Some new text includes the following: Valid values of "direction" are 1 to 5 or -5 to -1 within a "monthly" recurrence rule; and 1 to 53 or -53 to -1 within a "yearly" recurrence rule. Better? --- failure-counter Do we need to know when this counter was last reset? Currently we don't provide a method to reset this counter, so it is only reset when the schedule starts at the beginning. The following statement added to specify this: Unless new parameters/operations are defined to allow the count of failures to be reset, "failure-counter" is reset by default only when the schedule starts. --- 4. * Schedules received with a starting time in the past with respect to current time SHOULD be ignored. Please give the alternative "MAY" behaviour. The following alternative "MAY" behaviour added: When a local policy is provided, an implementation MAY omit the past occurrences and start immediately (e.g., for a period-based schedule) or starts from the date and time when the recurrence pattern is first satisfied from the current time (e.g., for a recurrence-based schedule). --- 7. has: This section uses the template described in Section 3.7 of [I-D.ietf-netmod-rfc8407bis]. This is slightly risky with the draft as an Informational reference because the template might change under your feet and this statement would cease to be true. I think that either you remove this statement (and, indeed, why is it there except because you are tired of justifying the content and format of Security Considerations sections of YANG documents that you write!) or make the reference normative and risk a delay while that draft completes the process as well as needing to come back later to check that you are still using the correct template. Hopefully the template has been stable, but the security template in 8407bis has given the guide to list it as informative reference. As you said, this shouldn't be an issue as that draft is a little ahead of this one in the processing queue. --- I believe that section 7 is missing clear instructions to future module that make use of this module. Please review the update in section 7, and let us know if you have further comments. = Nits = Section 2 s/entity that host/entity that hosts/ Fixed. --- I found 3.3 a bit over the top. Is Figure 1 in any way helpful? Having given the cross-references to the section that describes each grouping, do you need to say "Each of these groupings is presented in the following subsections." Good point! It's been much more concise now. --- 3.3.1 s/handles the schedule requests/handles schedule requests/ Fixed. --- Throughout, please consider s/invalid/not valid/ All fixed now. --- 2.2 Interval: Refers to an integer that specifies at which intervals a recurrence rule repeats. Values are taken from "INTERVAL" rule in Section 3.3.10 of [RFC5545]. 3.3.3 Consistent with Section 3.3.10 of [RFC5545], the interval ("interval") represents at which intervals the recurrence rule repeats. Is that really "intervals" or should it be "interval"? Fixed as "interval". --- 3.3.6 s/must not/MUST NOT/ s/e.g./i.e./ Fixed. --- 4. OLD * The second MUST have the value "60" at the end of months in which a leap second occurs for date and time values. NEW * The second for date and time values MUST have the value "60" at the end of months in which a leap second occurs. END Fixed. --- 5. OLD Despite no data nodes are defined in this document, Table 1 lists how main objects in the DISMAN-SCHEDULE-MIB can be mapped to YANG parameters. NEW Although no data nodes are defined in this document, Table 1 lists how the main objects in the DISMAN-SCHEDULE-MIB can be mapped to YANG parameters. END Fixed. --- Thanks again for this very careful review! Best Regards, Qiufang From: James Cumming (Nokia) <[email protected]<mailto:[email protected]>> Sent: 16 January 2025 22:38 To: [email protected]<mailto:[email protected]> Cc: NETMOD WG Chairs <[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]
