HI Quentin, See inline.
> On Jul 28, 2023, at 18:53, Acee Lindem <[email protected]> wrote: > > Hi Quentin, > > Thanks much for your detailed review!!! See inline. I’m posting version -08. > > Thanks, > Acee > >> On Jul 28, 2023, at 09:03, Quentin Armitage <[email protected]> wrote: >> >> I have now had the opportunity to read draft 7 of rfc5798bis in detail and >> have the >> following comments (apologies that the list is rather long). The vast >> majority of the >> comments relate to the original RFC5798, but it seems a good opportunity to >> make these >> updates while the RFC is being updated. >> >> 1. All occurrences of "virtual router" should be "Virtual Router", including >> plurals and >> possessives (it is a defined term). > > Deferred. Given the number of occurrences, I’m going to consider this in a > separate edit. Ok - I went ahead with this change and added Virtual Router MAC address to the definitions. I think it is an improvement but would like feedback from others. Thanks again for the extensive review, Acee > >> >> 2. All occurrences of "VRRP router" should be "VRRP Router", including >> plurals and >> possessives (defined term). > > Fixed. > > >> >> 3. All occurrences of "VRRP Active Router" in Abstract, section 1 >> (Introduction) and section >> 9 should be "Active Router" (defined term). > > Fixed. > >> >> 4. All occurrences of "VRRP Backup Router" in Abstract and section 1 >> (Introduction) should >> be "Active Router" (defined term). > > I believe you mean “Backup Router”. Fixed. > >> >> 5. Abstract, final paragraph, insert "to" after "updated". > > Fixed. > >> >> 6. Section 1 (Introduction) second paragraph, insert "to" after "updated". > > Fixed. > >> >> 7. Section 1.1 paragraph 3 should have "for IPv4" inserted at the end. > > Fixed. > >> >> 8. Section 1.1 paragraph 8 should have "to" inserted after "augmented". >> "ethernet" should be >> "Ethernet". > > Fixed. > >> >> 9. Section 1.3 second paragraph delete "be" between "may" and "not". Also >> delete >> "associated" before "protocol" ("associated" is specified 3 words earlier), >> and delete the >> comma between "and" and "such". > > Fixed. > >> >> 10. Section 1.4 The second sentence does not read easily. Suggest replacing >> "The Router >> Advertisements ... than 10 seconds" with "The Router Advertisements are >> multicast >> periodically at a rate such that the hosts will normally take more than 10 >> seconds to learn >> the default routers on a LAN". > > Fixed. > > >> >> 11. Section 1.4 second paragraph delete "(ND)"; this is specified in the >> previous >> paragraph. > > Fixed. > >> >> 12. Section 1.4 last paragraph. Replace "for a failed default router" to >> "from a failed >> Active Router". Change "around 3 seconds" to "3 to 4 seconds". I suggest >> adding at the end >> of that sentence "or within 1/64th second when using the lowest >> Advertisement_Interval". > > I think “take over for” is better than “take over from”. > >> >> 13. Section 1.6 change "guarantees" to "guarantee". > > Fixed. I’m not sure whether the clause was originally intended to refer to > all three components but keeping it all plural is simpler. > >> >> 14. Section 1.7 definition of "Active Router".Change "become the" to "be". > > Ok. > >> >> 15. Section 2.2 replace "preferential router" with "Virtual Router". > > “Most preferred virtual router”. > >> >> 16. Section 2.4 delete ", i.e., sending either IPv4 or IPv6". IPvX is >> defined in section >> 1.2. > > I think it is ok to repeat it with this first usage. > >> >> 17. Section 2.5 second paragraph. Move "may occur" to after "problematic >> scenario". Change >> both occurrences of "VRRP_Advertisement_Interval" to >> "Advertisement_Interval" and add "(see >> section 6.1)" after first occurrence. Change "transmitted onto" to >> "transmitted on" (this is >> consistent with change in previous sentence). Insert "(see section 6.1)" >> after >> "Active_Down_Interval" since it is defined later in the document. Insert >> "original" before >> "Active Router cause a". > > Fixed. > >> >> 18. Section 3 second paragraph. Change "virtual router identifier" to >> "Virtual Router >> Identifier (see section 6.1)". > > Fixed. > >> >> 19. Section 3 fourth paragraph. Change "VRRP Advertisement messages" to >> "VRRP advertisement > > Leaving it since it is a specific message type. > >> messages" (it is not a defined term). Change "unless it has" to "unless the >> Backup Router >> has" ("it" refers to Active Router as the last noun). > > > Fixed. > >> >> 20. Section 3 last paragraph. Insert "Router" after "Backup". "(< 1 second >> )" should be "(< >> 4 seconds when using the default 1 second Advertisement_Interval (see >> section 6.1))" (if the >> Active Router fails, takeover time is 3 * Advertisement_Interval + >> Skew_Time). > > Ok - reworded. > > >> >> 21. Section 4.1 first and second paragraphs after diagram, delete comma >> after "i.e.". > > Nope. “i.e., “ should be followed by a comma. See other RFCs. > > >> >> 22. Section 4.1. Move the final paragraph "Note that ..." to before the >> diagram, and delete >> the words "in both cases". > > I think it fits better where it is. > > >> >> 23. Section 4.1 paragraph starting "In the IPv6 case" change "LAN interface >> for the VRRP >> protocol and" to "LAN interface, and for the VRRP protocoal there is". >> Change "other >> routers" to "Virtual Routers". > > Ok. > >> >> 24. Section 4.1 in two paragraphs starting "In an IPv? VRRP environment" >> where ? is 4 or 6, >> change "virtual router ID" to "VRID", and change "owned by a router " to >> "owned by Router- >> 1". > > Ok > > >> >> 25. Section 4.1 in paragraph starting "In an IPv6 VRRP environment", delete >> the first >> sentence (it duplicates the second sentence). In the (current) second >> sentence delte "Link- >> Local" (the second and subsequent IPv6 address don't need to be link-local). >> Change "with >> both VRIDs" to "with the VRID" (there is only one VRID in this example). > > Ok > > >> >> 26. Section 4.1 paragraphs starting "The IPv? example above", if "IPv4" and >> "IP" in the >> first paragraph are replaced with IPvX, the second of the paragraphs can be >> deleted, . If >> the paragraphs are not combined, the last sentence of the IPv4 example needs >> to be added to >> the IPv6 paragraph. > > Used IPvX. > > >> >> 27. Section 5 first paragraph replace "VRRP packet" with "VRRP >> advertisement". Replace "and >> the state" with ", the Max Advertise Interval and backed up addresses". > > Ok > >> >> 28. Section 5.2.3 After "field" add "(in conjunction with the protocol >> family)". > > Ok > > >> >> 29. Section 5.2.4 third paragraph add sentence at end "Note, each Virtual >> Router for a VRID >> should be configured with different priorities (unless there are more than >> 254 non address- >> owner virtual routers) since if two (or more) Backup Routers are configured >> with the same >> priority, when the Active Router fails, both Backup Routers will transition >> to be an Active >> Router simultaneously, both sending VRRP advertisements and gratuitous >> ARP/unsolicited ND >> messages, causing confusion for learning bridges (see section 3)". > > > Nope. The primary address is used as a tie-breaker. > > > >> >> 30. Section 5.2.4 last paragraph "Active Router to timeout" to >> "Active_Down_Interval (see >> section 6.1) to expire". > > Ok >> >> 31. Section 5.2.7 to end. I do not think that "ADVERTISEMENT" should be >> capitalised. > > Ok. > >> >> 32. Section 5.2.7 second paragraph. Change "VRRP routers" to "Backup >> Routers". > > Ok. > >> >> 33. Section 5.2.8 third paragraph, would the reference to Section 5.2 be >> better as a >> reference to Section 5.2.9? > > No. It refers to the entire message. > >> >> 34. Section 5.2.9 last paragraph replace "VRRP protocol packet" with "IP >> header family" (the >> VRRP PDU does not identify whether the packet is IPv4 or IPv6). > > Fixed. > >> >> 35. Section 6.1 Advertisement_Interval. Add "sent by this Virtual Router" >> after >> (centiseconds)". > > Not sure this is needed but added. > > >> >> 36. Section 6.1 Add definition "IPvX_Addresses" as "IPv4_Addresses or >> IPv6_Addresses" after >> definition of "IPv6_Addresses". > > Fixed. > >> >> 37. Section 6.1 Skew_Time. Replace "priority" with "Priority". > > Fixed > >> >> 38. Section 6.1 Preempt_Mode Note - replace "The" with "the". > > No. > > >> >> 39. Section 6.1 Virtual_Router_MAC_Address change "ARP" to "ARP/ND" and >> change "IPvX >> Addresses to "IPvX_Addresses". > > Fixed. > >> >> 40. Section 6.2 add "(Backup Routers only)" at end of definition of >> "Active_Down_Timer". > > Ok. > >> >> 41. Section 6.2 add "(Active Routers only)" at end of definition of >> "Adver_Timer". > > Ok. > >> >> 42. Section 6.4 second paragraph delete "election". > > Ok. > >> >> 43. Section 6.4.1 (105) change "address" to "address(es)". > > Fixed. > >> >> 44. Section 6.4.1 (120) change "request" to "message" (gratuitous ARP >> messages are not >> requests). > > > Ok. > >> >> 45. Section 6.4.1 (150) change "does not own the virtual address" to "is not >> the address >> owner". > > > Ok. > >> >> 46. Section 6.4.1 (380) change "request" to "message". > > Ok. > >> >> 47. Section 6.4.1 (450) insert "Max" before "Advertise". > > Ok. > > >> >> 48. Section 6.4.1 after (450) add "(452) @ Recompute Skew_Time" (it needs to >> be recomputed >> after every change of Active_Adver_Interval, such as at (750)). > > Fixed. > > >> >> 49. (460) Change "Reset" to "Set" (to match (760)). > > Ok. > >> >> 50. (465) Add "than the local priority" at the end. > > Ok > > >> >> 50. (640) Delete "+". > > Yup. > > >> >> 51. Section 6.4.1 (745) insert "Max" before "Advertise". > > Fixed. > >> >> 52. Section 6.4.1 after (775) add "(778) @ Send an advertisement" (the >> reason for this is to >> update/correct any learning bridges caches and to make the lower priority >> Active Router >> revert to Backup state. This is a change in procedure but not a protocol >> change). > > I don’t think the virtual router transitioning to Backup Router should send > an advertisement here. > > >> >> 53. Section 7.1 after "- MUST verify that the VRRP version is 3" add a new >> check "- MUST >> verify that the VRRP packet Type is 1 - ADVERTISEMENT". > > Ok. > >> >> 54. Section 7.1 paragraph beginning "A receiver SHOULD" change '"Maximum >> Advertisement >> Interval"' to 'Max Advertisement Interval' (i.e. delete "imum" and both >> double quotes). > > > Ok. > >> >> 55. Section 7.2 Insert "- Set the source MAC address to virtual router MAC >> Address" before >> "- If the protected address is an IPv4 address, then:" and delete both >> occurrences of >> "+ Set the source MAC address to virtual router MAC Address" since it >> applies to both IPv4 >> and IPv6 and avoids duplication. Also add "the" before "Virtual Router". > > > Ok > >> >> 56. Section 7.2 In the two lines starting "+ Set the source IPv? address" >> add "the" before >> "interface". > > Ok. > > >> >> 57. Section 7.3 third and fifth paragraphs delete "VRRP" before "Virtual >> Router Identifier". >> Replace "network" with "LAN". > > Ok. > >> >> 58. Section 8.1.2 replace "gratuitous ARP request" with "gratuitous ARP >> message" (both >> singular and plural in following paragraph). > > Ok. > >> >> 59. Section 8.2.2 In "for the Virtual Router IPv6 address" replace "the" >> with "a". > > Ok. > >> >> 60. Section 8.4.2.1.2 and "be" before configured at the end. > > > Fixed. > > Thanks, > Acee > >> >> >> I will send separate emails relating to section 5.2.8 (checksums) and 8.3.2 >> (recommendations >> for priority when multiple Backup Routers), since they are more than simple >> editorial >> changes and probably require more discussion. >> >> With regards, >> >> Quentin Armitage >> > _______________________________________________ rtgwg mailing list [email protected] https://www.ietf.org/mailman/listinfo/rtgwg
