On Wed, Jul 6, 2016 at 5:14 PM, Numan Siddique <nusid...@redhat.com> wrote:
>
>
> On Tue, Jul 5, 2016 at 12:04 PM, Zong Kai LI <zealo...@gmail.com> wrote:
>
>> + /* Logical switch ingress table 10 and 11: DHCP options and response
>>> + * priority 100 flows. */
>>> + HMAP_FOR_EACH (op, key_node, ports) {
>>> + if (!op->nbs) {
>>> + continue;
>>> + }
>>> +
>>> + if (!lsp_is_enabled(op->nbs) || !strcmp(op->nbs->type,
>>> "router")) {
>>> + /* Don't add the DHCP flows if the port is not enabled or
>>> if the
>>> + * port is a router port. */
>>> + continue;
>>> + }
>>> +
>>> + if (!op->nbs->dhcpv4_options) {
>>> + /* CMS has disabled native DHCPv4 for this lport. */
>>> + continue;
>>> + }
>>> +
>>> + for (size_t i = 0; i < op->nbs->n_addresses; i++) {
>>> + struct lport_addresses laddrs;
>>> + if (!extract_lsp_addresses(op->nbs->addresses[i], &laddrs,
>>> + false)) {
>>> + continue;
>>> + }
>>> +
>>> + for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
>>> + struct ds options_action = DS_EMPTY_INITIALIZER;
>>> + struct ds response_action = DS_EMPTY_INITIALIZER;
>>> + if (build_dhcpv4_action(op, laddrs.ipv4_addrs[j].addr,
>>> + &options_action,
>>> &response_action)) {
>>> + struct ds match = DS_EMPTY_INITIALIZER;
>>> + ds_put_format(
>>> + &match, "inport == %s && eth.src ==
>>> "ETH_ADDR_FMT
>>> + " && ip4.src == 0.0.0.0 && "
>>> + "ip4.dst == 255.255.255.255 && udp.src == 68 &&
>>> "
>>> + "udp.dst == 67", op->json_key,
>>> + ETH_ADDR_ARGS(laddrs.ea));
>>> +
>>> + ovn_lflow_add(lflows, op->od,
>>> S_SWITCH_IN_DHCP_OPTIONS,
>>> + 100, ds_cstr(&match),
>>> + ds_cstr(&options_action));
>>> + /* If REGBIT_DHCP_OPTS_RESULT is set, it means the
>>> + * put_dhcp_opts action is successful */
>>> + ds_put_cstr(&match, " && "REGBIT_DHCP_OPTS_RESULT);
>>> + ovn_lflow_add(lflows, op->od,
>>> S_SWITCH_IN_DHCP_RESPONSE,
>>> + 100, ds_cstr(&match),
>>> + ds_cstr(&response_action));
>>> + ds_destroy(&match);
>>> + ds_destroy(&options_action);
>>> + ds_destroy(&response_action);
>>> + break;
>>> + }
>>> + }
>>> + free(laddrs.ipv4_addrs);
>>> + }
>>> + }
>>> +
>>> + /* Ingress table 10 and 11: DHCP options and response, by default
>>> goto next.
>>> + * (priority 0). */
>>> +
>>> + HMAP_FOR_EACH (od, key_node, datapaths) {
>>> + if (!od->nbs) {
>>> + continue;
>>> + }
>>> +
>>> + ovn_lflow_add(lflows, od, S_SWITCH_IN_DHCP_OPTIONS, 0, "1",
>>> "next;");
>>> + ovn_lflow_add(lflows, od, S_SWITCH_IN_DHCP_RESPONSE, 0, "1",
>>> "next;");
>>> + }
>>>
>>
>> Great work, but only one thing makes me feel uncomfortable.
>> DHCP Options flows are generated per each port, but not per each switch
>> datapath.
>> Since the resumed packet copies L2-L4 fields from original packet, and
>> DHCP options fields are the same for each port in the same switch. It
>> doesn't make sense to build DHCP Options flows for each ports.
>> The inport and eth.src in DHCP Options flows, I think we need logical
>> switch metadata field indeed.
>>
>>
>
> Since DHCP options are associated with logical ports, I don't see
> any other way. Also the IP address to be offered in the DHCP response
> comes from the Logical_Switch_Port.addresses right ?
>
>
>
>
>> Thanks and have a nice day.
>> Zong Kai, LI
>>
>
> I think I missed your discussion in
http://patchwork.ozlabs.org/patch/635772/ .
In some side, this is the simplest way to implement DHCP options in DB, and
it's flexible to allow port from the same net to have different DHCP
options.
But even we follow this design, there are still something need to fix for
port has multiple IPv4 addresses case.
And yes, for response_actions, we do need to get IPv4 address from port, so
it makes sense to build response_action per each port. But I'm not sure it
comes the same to options_action. I'm thinking on it, not a good
explanation found yet.
While processing DHCP options for multiple IPv4 addresses case, I would
suggest a variant solution:
1, add column dhcpv4_options to table Logical_Switch, and it should be a
list of ref. This will be default options for all ports on a logical switch.
2, keep column dhcpv4_options in Logical_Switch_Port, if a port want to has
some customized DHCP options, it can set this. (higher priority than
default options)
3, add column enable_dhcp_options(bool) to table Logical_Switch_Port, if
admin wants to disable DHCP_Options for a port, just set this to false.
4, Logical_Switch.dhcpv4_options is empty while
Logical_Switch_Port.dhcpv4_options is not empty, this means logical switch
doesn't have default DHCP options, but a port can have its owner DHCP
options.
What's benefit?
1, more columns added, but less data in total for logical switch ports.
2, less logical flows for native DHCPv4, logical switch default options
flows will work for ports using default DHCP options.
3, it keeps the flexible ability to allow ports have different DHCP options
in the same switch.
BTW, in commit message:
- A logical flow which implements the DHCP reponder by sending
reponder => responder.
Thanks and have a nice day. :)
Zong Kai, LI
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
