On Thu, Jan 16, 2020 at 7:40 PM Numan Siddique <num...@ovn.org> wrote:
>
> On Tue, Jan 14, 2020 at 4:38 PM Lorenzo Bianconi
> <lorenzo.bianc...@redhat.com> wrote:
> >
> > Introduce logical flows in ovn router pipeline in order to parse dhcpv6
> > advertise/reply from IPv6 prefix delegation router.
> > Do not overwrite ipv6_ra_pd_list info in options column of SB port_binding
> > table written by ovn-controller
> > Introduce ipv6_prefix column in NB Logical_router_port table to report
> > IPv6 prefix received from delegation router to the CMS
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
>
> Hi Lorenzo,
>
> Please see below for few comments.
>
> > ---
> > northd/ovn-northd.c | 99 +++++++++++++++++++++++++++++++++-
> > ovn-nb.ovsschema | 5 +-
> > ovn-nb.xml | 22 ++++++++
> > tests/atlocal.in | 5 +-
> > tests/system-ovn.at | 127 ++++++++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 255 insertions(+), 3 deletions(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index b6dc809d7..031792706 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -2645,6 +2645,41 @@ copy_gw_chassis_from_nbrp_to_sbpb(
> > free(sb_ha_chassis);
> > }
> >
> > +static void
> > +ovn_port_update_ipv6_prefix(struct northd_context *ctx,
> > + const struct ovn_port *op,
> > + struct smap *sb_option)
> > +{
> > + const char *ipv6_pd_list = smap_get(&op->sb->options,
> > "ipv6_ra_pd_list");
> > + if (!ipv6_pd_list) {
> > + return;
> > + }
> > +
> > + smap_add(sb_option, "ipv6_ra_pd_list", ipv6_pd_list);
> > +
> > + const struct nbrec_logical_router_port *lrp = NULL, *iter;
> > + /* update logical_router_port table */
> > + NBREC_LOGICAL_ROUTER_PORT_FOR_EACH (iter, ctx->ovnnb_idl) {
> > + if (!strcmp(iter->name, op->sb->logical_port)) {
> > + lrp = iter;
> > + break;
> > + }
> > + }
>
> I don't think you need to run this for loop to get the lrp. I think
> "struct ovn_port" should have a
> pointer to "struct nbrec_logical_router_port".
>
>
> > + if (!lrp) {
> > + return;
> > + }
> > +
> > + struct sset ipv6_prefix_set = SSET_INITIALIZER(&ipv6_prefix_set);
> > + sset_add_array(&ipv6_prefix_set, lrp->ipv6_prefix, lrp->n_ipv6_prefix);
> > + if (!sset_contains(&ipv6_prefix_set, ipv6_pd_list)) {
> > + sset_add(&ipv6_prefix_set, ipv6_pd_list);
> > + nbrec_logical_router_port_set_ipv6_prefix(lrp,
> > + sset_array(&ipv6_prefix_set),
> > + sset_count(&ipv6_prefix_set));
> > + }
>
> I think it's better to just copy whatever is there in the
> port_binding.options:ipv6_ra_pd_list
> to logical_router_port.ipv6_prefix, instead of appending.
>
> This function ovn_port_update_ipv6_prefix() is called from multiple places in
> ovn_port_update_sbrec(). I think this can be called just once right ?
>
> Also ovn_port_update_sbrec() doesn't seem like the right place to
> update the nb db.
> May be a new function - sync_ipv6_pd or something similar ?
>
> Can you please update ovn-northd.8.xml about the new flow being added ?
>
> Thanks
> Numan
>
> > + sset_destroy(&ipv6_prefix_set);
> > +}
> > +
> > static void
> > ovn_port_update_sbrec(struct northd_context *ctx,
> > struct ovsdb_idl_index *sbrec_chassis_by_name,
> > @@ -2653,6 +2688,7 @@ ovn_port_update_sbrec(struct northd_context *ctx,
> > struct sset *active_ha_chassis_grps)
> > {
> > sbrec_port_binding_set_datapath(op->sb, op->od->sb);
> > +
> > if (op->nbrp) {
> > /* If the router is for l3 gateway, it resides on a chassis
> > * and its port type is "l3gateway". */
> > @@ -2775,6 +2811,9 @@ ovn_port_update_sbrec(struct northd_context *ctx,
> > smap_add(&new, "l3gateway-chassis", chassis_name);
> > }
> > }
> > +
> > + ovn_port_update_ipv6_prefix(ctx, op, &new);
> > +
> > sbrec_port_binding_set_options(op->sb, &new);
> > smap_destroy(&new);
> >
> > @@ -2824,6 +2863,9 @@ ovn_port_update_sbrec(struct northd_context *ctx,
> > smap_add_format(&options,
> > "qdisc_queue_id", "%d", queue_id);
> > }
> > +
> > + ovn_port_update_ipv6_prefix(ctx, op, &options);
> > +
> > sbrec_port_binding_set_options(op->sb, &options);
> > smap_destroy(&options);
> > if (ovn_is_known_nb_lsp_type(op->nbsp->type)) {
> > @@ -2873,6 +2915,9 @@ ovn_port_update_sbrec(struct northd_context *ctx,
> > if (chassis) {
> > smap_add(&new, "l3gateway-chassis", chassis);
> > }
> > +
> > + ovn_port_update_ipv6_prefix(ctx, op, &new);
> > +
> > sbrec_port_binding_set_options(op->sb, &new);
> > smap_destroy(&new);
> > } else {
> > @@ -7129,6 +7174,11 @@ copy_ra_to_sb(struct ovn_port *op, const char
> > *address_mode)
> > }
> > ds_put_format(&s, "%s/%u ", addrs->network_s, addrs->plen);
> > }
> > +
> > + const char *ra_pd_list = smap_get(&op->sb->options, "ipv6_ra_pd_list");
> > + if (ra_pd_list) {
> > + ds_put_cstr(&s, ra_pd_list);
> > + }
> > /* Remove trailing space */
> > ds_chomp(&s, ' ');
> > smap_add(&options, "ipv6_ra_prefixes", ds_cstr(&s));
> > @@ -7851,7 +7901,36 @@ build_lrouter_flows(struct hmap *datapaths, struct
> > hmap *ports,
> > free(snat_ips);
> > }
> >
> > - /* Logical router ingress table 3: IP Input for IPv6. */
> > + /* DHCPv6 reply handling */
> > + HMAP_FOR_EACH (op, key_node, ports) {
> > + if (!op->nbrp) {
> > + continue;
> > + }
> > +
> > + if (op->derived) {
> > + continue;
> > + }
> > +
> > + struct lport_addresses lrp_networks;
> > + if (!extract_lrp_networks(op->nbrp, &lrp_networks)) {
> > + continue;
> > + }
> > +
> > + for (size_t i = 0; i < lrp_networks.n_ipv6_addrs; i++) {
> > + ds_clear(&actions);
> > + ds_clear(&match);
> > + ds_put_format(&match, "ip6.dst == %s && udp.src == 547 &&"
> > + " udp.dst == 546",
> > + lrp_networks.ipv6_addrs[i].addr_s);
> > + ds_put_format(&actions, "reg0 = 0; handle_dhcpv6_reply { "
> > + "eth.dst <-> eth.src; ip6.dst <-> ip6.src; "
> > + "outport <-> inport; output; };");
> > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 100,
> > + ds_cstr(&match), ds_cstr(&actions));
> > + }
> > + }
> > +
> > + /* Logical router ingress table 1: IP Input for IPv6. */
> > HMAP_FOR_EACH (op, key_node, ports) {
> > if (!op->nbrp) {
> > continue;
> > @@ -8652,6 +8731,24 @@ build_lrouter_flows(struct hmap *datapaths, struct
> > hmap *ports,
> > continue;
> > }
> >
> > + struct smap options;
> > + /* enable IPv6 prefix delegation */
> > + bool prefix_delegation = smap_get_bool(&op->nbrp->options,
> > + "prefix_delegation", false);
> > + if (prefix_delegation) {
> > + smap_clone(&options, &op->sb->options);
> > + smap_add(&options, "ipv6_prefix_delegation", "true");
> > + sbrec_port_binding_set_options(op->sb, &options);
> > + smap_destroy(&options);
> > + }
> > +
> > + if (smap_get_bool(&op->nbrp->options, "prefix", false)) {
> > + smap_clone(&options, &op->sb->options);
> > + smap_add(&options, "ipv6_prefix", "true");
> > + sbrec_port_binding_set_options(op->sb, &options);
> > + smap_destroy(&options);
> > + }
> > +
> > const char *address_mode = smap_get(
> > &op->nbrp->ipv6_ra_configs, "address_mode");
> >
> > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> > index 12999a466..8e9d1517f 100644
> > --- a/ovn-nb.ovsschema
> > +++ b/ovn-nb.ovsschema
> > @@ -1,7 +1,7 @@
> > {
> > "name": "OVN_Northbound",
> > "version": "5.18.0",
I missed this in the previous reply. The version number should be bumped.
Thanks
Numan
> > - "cksum": "2806349485 24196",
> > + "cksum": "4171338172 24362",
> > "tables": {
> > "NB_Global": {
> > "columns": {
> > @@ -324,6 +324,9 @@
> > "ipv6_ra_configs": {
> > "type": {"key": "string", "value": "string",
> > "min": 0, "max": "unlimited"}},
> > + "ipv6_prefix": {"type": {"key": "string",
> > + "min": 0,
> > + "max": "unlimited"}},
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > "min": 0, "max": "unlimited"}}},
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index 5ae52bbde..55faca3b1 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -1896,6 +1896,11 @@
> > port has all ingress and egress traffic dropped.
> > </column>
> >
> > + <column name="ipv6_prefix">
> > + This column contains IPv6 prefix obtained by prefix delegation
> > + router according to RFC 3633
> > + </column>
> > +
> > <group title="ipv6_ra_configs">
> > <p>
> > This column defines the IPv6 ND RA address mode and ND MTU Option
> > to be
> > @@ -2142,6 +2147,23 @@
> > to <code>true</code>.
> > </p>
> > </column>
> > +
> > + <column name="options" key="prefix_delegation"
> > + type='{"type": "boolean"}'>
> > + <p>
> > + If set to <code>true</code>, enable IPv6 prefix delegation state
> > + machine on this logical router port (RFC3633). IPv6 prefix
> > + delegation is available just on a gateway router or on a gateway
> > + router port.
> > + </p>
> > + </column>
> > +
> > + <column name="options" key="prefix" type='{"type": "boolean"}'>
> > + <p>
> > + If set to <code>true</code>, this interface will receive an IPv6
> > + prefix according to RFC3663
> > + </p>
> > + </column>
> > </group>
> >
> > <group title="Attachment">
> > diff --git a/tests/atlocal.in b/tests/atlocal.in
> > index 5f14c3da0..8f3ff03b9 100644
> > --- a/tests/atlocal.in
> > +++ b/tests/atlocal.in
> > @@ -157,7 +157,7 @@ find_command()
> > {
> > which $1 > /dev/null 2>&1
> > status=$?
> > - var=HAVE_`echo "$1" | tr '[a-z]' '[A-Z]'`
> > + var=HAVE_`echo "$1" | tr '-' '_' | tr '[a-z]' '[A-Z]'`
> > if test "$status" = "0"; then
> > eval ${var}="yes"
> > else
> > @@ -192,6 +192,9 @@ else
> > DIFF_SUPPORTS_NORMAL_FORMAT=no
> > fi
> >
> > +# Set HAVE_DIBBLER-SERVER
> > +find_command dibbler-server
> > +
> > # Turn off proxies.
> > unset http_proxy
> > unset https_proxy
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index a56d358ea..bb7d8e420 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -3426,3 +3426,130 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
> > patch-.*/d
> > /connection dropped.*/d"])
> >
> > AT_CLEANUP
> > +
> > +AT_SETUP([ovn -- IPv6 prefix delegation])
> > +AT_SKIP_IF([test $HAVE_DIBBLER_SERVER = no])
> > +AT_KEYWORDS([ovn-ipv6-prefix_d])
> > +
> > +ovn_start
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +
> > +ADD_BR([br-int])
> > +ADD_BR([br-ext])
> > +
> > +ovs-ofctl add-flow br-ext action=normal
> > +# Set external-ids in br-int needed for ovn-controller
> > +ovs-vsctl \
> > + -- set Open_vSwitch . external-ids:system-id=hv1 \
> > + -- set Open_vSwitch .
> > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> > + -- set bridge br-int fail-mode=secure
> > other-config:disable-in-band=true
> > +
> > +# Start ovn-controller
> > +start_daemon ovn-controller
> > +
> > +ovn-nbctl lr-add R1
> > +
> > +ovn-nbctl ls-add sw0
> > +ovn-nbctl ls-add sw1
> > +ovn-nbctl ls-add public
> > +
> > +ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24
> > +ovn-nbctl lrp-add R1 rp-sw1 00:00:03:01:02:03 192.168.2.1/24
> > +ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 \
> > + -- set Logical_Router_Port rp-public options:redirect-chassis=hv1
> > +
> > +ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \
> > + type=router options:router-port=rp-sw0 \
> > + -- lsp-set-addresses sw0-rp router
> > +ovn-nbctl lsp-add sw1 sw1-rp -- set Logical_Switch_Port sw1-rp \
> > + type=router options:router-port=rp-sw1 \
> > + -- lsp-set-addresses sw1-rp router
> > +
> > +ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port public-rp \
> > + type=router options:router-port=rp-public \
> > + -- lsp-set-addresses public-rp router
> > +
> > +ADD_NAMESPACES(sw01)
> > +ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \
> > + "192.168.1.1")
> > +ovn-nbctl lsp-add sw0 sw01 \
> > + -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2"
> > +
> > +ADD_NAMESPACES(sw11)
> > +ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \
> > + "192.168.2.1")
> > +ovn-nbctl lsp-add sw1 sw11 \
> > + -- lsp-set-addresses sw11 "f0:00:00:02:02:03 192.168.2.2"
> > +
> > +ADD_NAMESPACES(server)
> > +ADD_VETH(s1, server, br-ext, "2001:db8:3333::2/64", "f0:00:00:01:02:05", \
> > + "2001:db8:3333::1")
> > +
> > +OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep 2001:db8:3333::2
> > | grep tentative)" = ""])
> > +OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep fe80 | grep
> > tentative)" = ""])
> > +
> > +AT_CHECK([ovs-vsctl set Open_vSwitch .
> > external-ids:ovn-bridge-mappings=phynet:br-ext])
> > +ovn-nbctl lsp-add public public1 \
> > + -- lsp-set-addresses public1 unknown \
> > + -- lsp-set-type public1 localnet \
> > + -- lsp-set-options public1 network_name=phynet
> > +
> > +ovn-nbctl set logical_router_port rp-public options:prefix_delegation=true
> > +ovn-nbctl set logical_router_port rp-public options:prefix=true
> > +ovn-nbctl set logical_router_port rp-sw0 options:prefix=true
> > +ovn-nbctl set logical_router_port rp-sw1 options:prefix=true
> > +
> > +# reset dibbler state
> > +sed s/eth0/s1/g -i /etc/dibbler/server.conf
> > +cat > /var/lib/dibbler/server-AddrMgr.xml <<EOF
> > +<AddrMgr>
> > + <timestamp>1575481348</timestamp>
> > + <replayDetection>0</replayDetection>
> > +</AddrMgr>
> > +EOF
> > +cat > /var/lib/dibbler/server-CfgMgr.xml <<EOF
> > +<SrvCfgMgr>
> > + <workDir>/var/lib/dibbler</workDir>
> > + <LogName>Server</LogName>
> > + <LogLevel>8</LogLevel>
> > + <InactiveMode>0</InactiveMode>
> > + <GuessMode>0</GuessMode>
> > +</SrvCfgMgr>
> > +EOF
> > +
> > +NS_CHECK_EXEC([server], [dibbler-server run > dibbler.log &])
> > +ovn-nbctl --wait=hv sync
> > +
> > +sleep 10
> > +kill $(pidof dibbler-server)
> > +
> > +OVS_WAIT_UNTIL([ovn-nbctl list logical_router_port rp-public | grep
> > ipv6_prefix])
> > +OVS_WAIT_UNTIL([ovn-nbctl list logical_router_port rp-sw0 | grep
> > ipv6_prefix])
> > +OVS_WAIT_UNTIL([ovn-nbctl list logical_router_port rp-sw1 | grep
> > ipv6_prefix])
> > +AT_CHECK([ovn-nbctl get logical_router_port rp-public ipv6_prefix], [0],
> > [dnl
> > +[["2001:db8:3333::6a2f:0:0/96"]]
> > +])
> > +AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix], [0], [dnl
> > +[["2001:db8:3333::5b81:0:0/96"]]
> > +])
> > +AT_CHECK([ovn-nbctl get logical_router_port rp-sw1 ipv6_prefix], [0], [dnl
> > +[["2001:db8:3333::42f1:0:0/96"]]
> > +])
> > +
> > +kill $(pidof ovn-controller)
> > +
> > +as ovn-sb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as ovn-nb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as northd
> > +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> > +
> > +as
> > +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
> > +/.*terminating with signal 15.*/d"])
> > +AT_CLEANUP
> > --
> > 2.21.1
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
