dhcp for dual tor: include all vlan intf into downstream intf#6990
dhcp for dual tor: include all vlan intf into downstream intf#69902 commits merged intosonic-net:masterfrom
Conversation
ghost
requested a review
from 
ghost
added
the
| (flags & INTERFACE_DOWNSTREAM ? 'Y' : 'N')); | ||
|
|
||
| + if (flags & INTERFACE_DOWNSTREAM) { | ||
| + if (flags & INTERFACE_DOWNSTREAM || flags & INTERFACE_UPSTREAM) { |
There was a problem hiding this comment.
We can use the existing interfaces which hold list of all interfaces as being done in isc dhcrelay:849
There was a problem hiding this comment.
a) the code above is better: we don't have to include all interfaces, -id will include vlan for this proc, and -iu will include other vlans on the switch; b) the code above is safer: no one knows if some interfaces will have any setting errors by the code, for example, in patch 0010, I already find out that netmask is not correctly setup. So in short, we should keep the list as short as possible
There was a problem hiding this comment.
I agree, however on the other hand, this is a legacy code and reducing the amount of code changes helps limit regression or injecting new bugs given that the code was running for quite some time and functioning. I am indifferent though as it is not a complex change.
There was a problem hiding this comment.
Without comment, it is a bit confusing why we are including upstream interfaces in the downstream interface list. Maybe consider adding a comment.
There was a problem hiding this comment.
sure, but I will add a comment in the same line so that we don't have to re-generate the patch
* include all vlan intf into downstream intf * add a comment
…net#6990) * include all vlan intf into downstream intf * add a comment
…net#6990) * include all vlan intf into downstream intf * add a comment
3 participants
Include all vlan interfaces into downstream interface set to allow each dhcp instance to scan over them. Note that with this change, downstream interface set includes not only vlan interfaces but also portchannels. However, this doesn't matter as dhcp uses giaddr or yiaddr to choose out interface.
Tested and verified on a virtual testbed.