Sowmini, thanks for your review and the comments. Please see my comments inline.
[EMAIL PROTECTED] wrote: >On (10/22/08 05:08), yifan xu wrote: > > >>The VRRP project team invites you to review our current design. The document >>could >>be found at: >> >>http://www.opensolaris.org/os/project/vrrp/vrrp_design.pdf >> >> >> > >Yifan, > >thanks for writing this up- this will be a useful feature to have. > >Some comments: > >- Section 3.1, first paragraph is a bit confusing and > recursive. ("VRRP virtual router contains multiple VRRP routers"). > Could you please clarify? > I agree the terminology is a bit confusing. A VRRP virtual router is defined in the RFC as an abstract object managed by the protocol and usually consists of multiple hosts participating the virtual router. We call those individual hosts running the protocol "VRRP routers". Because we are implementing the standard only with little extension so we didn't include the details of the RFC terminology in the design document. > >- Section 3.3: it's not clear what the exact syntax of the vrrpadm > command will be, but I assume that this will follow the syntax > of existing *adm commands like dladm, i.e., the verb-object > format, so that the commands will be something like > "vrrapadm create-instance", "vrrp modify-group" etc.? > Yes. I think that's what we should add to the doc. For the details of the vrrpadm usage, you might want to take a look at the CLI specification which could be found at: http://www.opensolaris.org/os/project/vrrp/vrrp_cli_spec.txt We could merge the CLI/API specification into the design document as appendixes. > >- Section 5.1, 5.5: why not use sockaddr_storage instead of explicitly > storing address family and a union of in_addr, in6_addr? At the > very least, you can typedef a new type that contains > typedef struct vrrp_addr { > int vrrp_af; > union { > struct in_addr in4_u; > struct in6_addr in6_u; > } vrrp_ip_addr > }; > instead of duplicating this. > Using sockaddr_storage seems making sense to me. We could change the structure to use it. > >- Section 6: I'd suggest using the nested block-structured > config syntax similar to that used for gated, i.e., > > vrrp_group { > group_name vrgp3 > : > vrrp_instance { > type non-router; > vrid 12; > interface e1000g1; > : > } > vrrp_instance { > type standard; > vrid 13; > interface nge1; > : > } > } > This eliminates the need for some fields like number_of_vr, group_name etc. > Sounds reasonable to me. > >- Section 7.3: "For IPv4, the primary IP address is used as the > source IP address". What is meant by the "primary" IP address? > Is this the one on the ipif with logical number 0? BSD has a > different notion of primary and secondary addresses than Solaris, > so it would be good to define "primary" clearly. > Sorry we didn't make it clear here. Here primary IP address is a terminology in VRRP protocol. It is designated to be used as the source IP address of the VRRP advertisement. But the RFC doesn't specify what algorithm should be used to choose primary IP address for an interface. See: http://www.ietf.org/internet-drafts/draft-ietf-vrrp-unified-spec-02.txt In the implementation we simply choose the first IP address of the link for unspecified primary IP address. > >- Section 8: what is the limitation that prohibits the application of > VRRP on IPMP links (esp. after Clearview IPMP introduces the ipmp > meta-interface)? > The VRRP implementation requires manipulating MAC addresses thus the instances are actually applied on data-link level. See section 7.5. So IPMP links won't be able to support VRRP protocol. Thanks, Yifan _______________________________________________ networking-discuss mailing list [email protected]
