Supports FRR-VRRP configuration #2949
Conversation
Signed-off-by: philo <[email protected]>
| if "/" not in ip_addr: | ||
| ctx.fail("IP address {} is missing a mask. Such as xx.xx.xx.xx/yy or xx:xx::xx/yy".format(str(ip_addr))) |
There was a problem hiding this comment.
This needs to be removed as vrrpsyncd expects a VIP to be without netmask as it eventually concatenate /32 for ipv4 and /128 length for ipv6 IP at the time of creating VRRP_TABLE entry in APLL_DB.
There was a problem hiding this comment.
The purpose of bring the mask is to enable the Linux kernel to learn neighbors properly. If we do not bring it, the default value of ipv4 will be set to /32, and ipv6 will be set to /128 . Because of this, arp or nd cannot be learned properly. The chip layer does not care about the arp interaction process and it only needs to upload the corresponding dip data packets to the cpu.
|
@nmoray pls signoff and approve if you have no further comments |
config/main.py
Outdated
| @vrrp.command("adv_interval") | ||
| @click.argument('interface_name', metavar='<interface_name>', required=True) | ||
| @click.argument('vrrp_id', metavar='<vrrp_id>', required=True, type=click.IntRange(1, 255)) | ||
| @click.argument('interval', metavar='<interval>', required=True, type=click.IntRange(10, 40950), default=1000) |
There was a problem hiding this comment.
I believe there is an incongruence.
HLD and sonic-net/sonic-buildimage#20383 fix the limit for both VRRP2 and VRRP3 to uint8.
Thought it was some kind of SONiC requirement but not sure now.
There was a problem hiding this comment.
BTW, neither are valid, I believe the correct should be:
+--rw (advertise-interval-choice)?
| +--:(v2)
| | +--rw advertise-interval-sec? uint8
| +--:(v3)
| +--rw advertise-interval-centi-sec? uint16
There is an on-going work (DRAFT) form the IETF, it might be helpful use it as reference: https://datatracker.ietf.org/doc/draft-acee-rtgwg-vrrp-rfc8347bis/
There was a problem hiding this comment.
This is a good suggestion. We plan to first modify it to align with the HLD, and later, we will further discuss with Broadcom to explore how to set it up more appropriately.Thanks.
Signed-off-by: philo <[email protected]>
Signed-off-by: philo <[email protected]>
Signed-off-by: philo <[email protected]>
Signed-off-by: philo <[email protected]>
Signed-off-by: philo <[email protected]>
config/main.py
Outdated
| if len(vrrp_keys) >= 128: | ||
| ctx.fail("Has already configured 128 IPs") | ||
| intf_cfg = 0 | ||
| for key in vrrp_keys: | ||
| if key[1] == str(vrrp_id): | ||
| ctx.fail("The vrrp instance {} has already configured!".format(vrrp_id)) | ||
| if key[0] == interface_name: | ||
| intf_cfg += 1 | ||
| if intf_cfg >= 16: | ||
| ctx.fail("{} has already configured 16 vrrp instances!".format(interface_name)) | ||
| vrrp_entry["vid"] = vrrp_id |
There was a problem hiding this comment.
Sorry, I believe there is another incongruence.
Are 128 instances per interface and 16 VIPs per instance the scalibility requirements described in the HLD?
If I'm not wrong, they are:
Scalability Requirements
Max number of VRRP instances: 254
Max number of VIPS per instance: 4
Regards,
config/main.py
Outdated
| def is_vaild_intf_ip_addr(ip_addr) -> bool: | ||
| """Check whether the ip address is valid""" | ||
| try: | ||
| ip_address = ipaddress.ip_interface(ip_addr) | ||
| except ValueError as err: | ||
| click.echo("IP address {} is not valid: {}".format(ip_addr, err)) | ||
| return False | ||
|
|
||
| if ip_address.version == 6: | ||
| if ip_address.is_unspecified: | ||
| click.echo("IPv6 address {} is unspecified".format(str(ip_address))) | ||
| return False | ||
| elif ip_address.version == 4: | ||
| if str(ip_address.ip) == "0.0.0.0": | ||
| click.echo("IPv4 address {} is Zero".format(str(ip_address))) | ||
| return False | ||
|
|
||
| if ip_address.is_multicast: | ||
| click.echo("IP address {} is multicast".format(str(ip_address))) | ||
| return False | ||
|
|
||
| return True |
There was a problem hiding this comment.
Two obsevations here:
- I believe the user can configure a loopback IP (127.0.0.0/8): do we want to prevent it?
- IETF defines other reserved addresses, do we want to make a consideration regarding them?
Regards,
There was a problem hiding this comment.
Loopback address check has been added. Other reserved address checks are not planned for this time; further consideration and verification are needed. Thank you.
Signed-off-by: philo <[email protected]>
|
Looks fine, please go ahead with merge. |
|
@qiluo-msft please help merge this |
|
@qiluo-msft please help merge this, thanks. |
hld:sonic-net/SONiC#1446