[ovs-dev] [PATCH v2] ovn-northd: Add logical flows to support native DHCPv4

[Numan Siddique]

On Mon, Jun 27, 2016 at 10:35 PM, Zong Kai LI <zealokii at gmail.com> wrote:

> +static void
>> +check_and_add_supported_dhcp_opts_to_sb_db(struct northd_context *ctx)
>> +{
>> +    static bool nothing_to_add = false;
>> +
>> +    if (nothing_to_add) {
>> +        return;
>> +    }
>> +
>> +    struct hmap dhcp_opts_to_add = HMAP_INITIALIZER(&dhcp_opts_to_add);
>> +    for (size_t i = 0; (i < sizeof(supported_dhcp_opts) /
>> +                            sizeof(supported_dhcp_opts[0])); i++) {
>> +        hmap_insert(&dhcp_opts_to_add, &supported_dhcp_opts[i].hmap_node,
>> +                    dhcp_opt_hash(supported_dhcp_opts[i].name));
>> +    }
>> +
>> +    const struct sbrec_dhcp_options *opt_row, *opt_row_next;
>> +    SBREC_DHCP_OPTIONS_FOR_EACH_SAFE(opt_row, opt_row_next,
>> ctx->ovnsb_idl) {
>> +        struct dhcp_opts_map *dhcp_opt =
>> +            dhcp_opts_find(&dhcp_opts_to_add, opt_row->name);
>> +        if (dhcp_opt) {
>> +            hmap_remove(&dhcp_opts_to_add, &dhcp_opt->hmap_node);
>> +        }
>> +        else {
>> +            sbrec_dhcp_options_delete(opt_row);
>> +        }
>> +    }
>> +
>> +
>> ​​
>> if (!dhcp_opts_to_add.n) {
>> +        nothing_to_add = true;
>> +    }
>> +
>> +    struct dhcp_opts_map *opt;
>> +
>> ​​
>> HMAP_FOR_EACH_POP(opt, hmap_node, &dhcp_opts_to_add) {
>> +        struct sbrec_dhcp_options *sbrec_dhcp_option =
>> +            sbrec_dhcp_options_insert(ctx->ovnsb_txn);
>> +        sbrec_dhcp_options_set_name(sbrec_dhcp_option, opt->name);
>> +        sbrec_dhcp_options_set_code(sbrec_dhcp_option, opt->code);
>> +        sbrec_dhcp_options_set_type(sbrec_dhcp_option, opt->type);
>> +    }
>> +
>> +    hmap_destroy(&dhcp_opts_to_add);
>> +}
>> +
>>
>
> After HMAP_FOR_EACH_POP processed, will not dhcp_opts_to_add.n be set to
> 0? For actions left in dhcp_opts_to_add has been added in HMAP_FOR_EACH_POP
> loop.
> If so, how about move dhcp_opts_to_add checking after HMAP_FOR_EACH_POP
> loop?
>

​The idea to check ​

​"
​
if (!
​​
dhcp_opts_to_add.n) {
​" before ​
​
​"
​
HMAP_FOR_EACH_POP
​" is to determine if all the dhcp options are written to database or not.
If
​
dhcp_opts_to_add.n
​ is zero, it means the SB DB "DHCP_Options" table is in sync with the
supported DHCP options.

​

>
>
>> diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
>> index 58f04b2..9587b94 100644
>> --- a/ovn/ovn-nb.ovsschema
>> +++ b/ovn/ovn-nb.ovsschema
>> @@ -1,7 +1,7 @@
>>  {
>>      "name": "OVN_Northbound",
>> -    "version": "3.1.0",
>> -    "cksum": "1426508118 6135",
>> +    "version": "3.2.0",
>> +    "cksum": "27511920 6856",
>>      "tables": {
>>          "Logical_Switch": {
>>              "columns": {
>> @@ -43,6 +43,11 @@
>>                                             "max": "unlimited"}},
>>                  "up": {"type": {"key": "boolean", "min": 0, "max": 1}},
>>                  "enabled": {"type": {"key": "boolean", "min": 0, "max":
>> 1}},
>> +                "dhcpv4_options": {"type": {"key": {"type": "uuid",
>> +                                            "refTable": "DHCP_Options",
>> +                                            "refType": "strong"},
>> +                                 "min": 0,
>> +                                 "max": 1}},
>>
>
> I just recognized that this new column is in table Logical_Switch_Port,
> why not in table Logical_Switch? Or we prefer lsp on the same ls can have
> different dhcp_options?
>

​Please see this thread [1] for more details. The previous versions of this​

​patch were referencing "Subnet" table in the "Logical_Switch" table.

[1] - http://openvswitch.org/pipermail/dev/2016-June/073807.html
​

>
>
>> +static void
>> +nbctl_lsp_set_dhcpv4_options(struct ctl_context *ctx)
>> +{
>> +    const char *id = ctx->argv[1];
>> +    const struct nbrec_logical_switch_port *lsp;
>> +
>> +    lsp = lsp_by_name_or_uuid(ctx, id, true);
>> +    const struct nbrec_dhcp_options *dhcp_opt = NULL;
>> +    if (ctx->argc == 3 ) {
>> +        dhcp_opt = dhcp_options_get(ctx, ctx->argv[2], true);
>> +    }
>> +
>> +    if (dhcp_opt) {
>> +        ovs_be32 ip;
>> +        unsigned int plen;
>> +        char *error = ip_parse_cidr(dhcp_opt->cidr, &ip, &plen);
>> +        if (error){
>> +            free(error);
>> +            VLOG_WARN("DHCP options cidr '%s' is not IPv4",
>> dhcp_opt->cidr);
>> +            return;
>>
>
> how about using ctl_fatal here?
>
>

​Ack.
​


> +static void
>> +nbctl_dhcp_options_create(struct ctl_context *ctx)
>> +{
>> +    /* Validate the cidr */
>> +    ovs_be32 ip;
>> +    unsigned int plen;
>> +    char *error = ip_parse_cidr(ctx->argv[1], &ip, &plen);
>> +    if (error){
>> +        /* check if its IPv6 cidr */
>> +        free(error);
>> +        struct in6_addr ipv6;
>> +        error = ipv6_parse_cidr(ctx->argv[1], &ipv6, &plen);
>> +        if (error) {
>> +            free(error);
>> +            VLOG_WARN("Invalid cidr format '%s'", ctx->argv[1]);
>> +            return;
>>
>
> ditto.
>
>
>> +ovn-nbctl -- --id=@d1 create DHCP_Options cidr=10.0.0.0/24 \
>> +options="\"server_id\"=\"10.0.0.1\" \"server_mac\"=\"ff:10:00:00:00:01\"
>> \
>> +\"lease_time\"=\"3600\" \"router\"=\"10.0.0.1\"" \
>>
>
> how about using
> ​​
> nbctl_dhcp_options_create here?
>

​I think the way it is used is better. If we use ​

​"
​
nbctl_dhcp_options_create
​" then we need to store the
UUID of the created DHCP options in a variable.​
​

>
> +-- add Logical_Switch_Port ls1-lp1 dhcpv4_options @d1 \
>> +-- add Logical_Switch_Port ls1-lp2 dhcpv4_options @d1
>> +
>> +ovn-nbctl -- --id=@d2 create DHCP_Options cidr=30.0.0.0/24 \
>> +options="\"server_id\"=\"30.0.0.1\" \"server_mac\"=\"ff:10:00:00:00:02\"
>> \
>> +\"lease_time\"=\"3600\"" -- add Logical_Switch_Port ls2-lp2
>> dhcpv4_options @d2
>>
>
> ditto
>
> Thanks,
> Zong Kai, LI
>