| Message ID | 1485422430-3963-7-git-send-email-mickeys.dev@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 26 January 2017 at 01:20, Mickey Spiegel <mickeys.dev@gmail.com> wrote: > Currently in OVN, the "nat-addresses" in the "options" column of a > logical switch port of type "router" must be specified manually. > Typically the user would specify as "nat-addresses" all of the NAT > external IP addresses and load balancer IP addresses that have > already been specified separately on the router. > > This patch allows the logical switch port's "nat-addresses" to be > specified as the string "router". When ovn-northd sees this string, > it automatically copies the following into the southbound > Port_Binding's "nat-addresses" in the "options" column: > The options:router-port's MAC address. > Each NAT external IP address (of any NAT type) specified on the > logical router of options:router-port. > Each load balancer IP address specified on the logical router of > options:router-port. > This will cause the controller where the gateway router resides to > issue gratuitous ARPs for each NAT external IP address and for each > load balancer IP address specified on the gateway router. > > This patch is written as if it will be included in OVS 2.7. If it > is deferred to OVS 2.8, then the OVS version mentioned in ovn-nb.xml > will need to be updated from OVS 2.7 to OVS 2.8. > > Signed-off-by: Mickey Spiegel <mickeys.dev@gmail.com> > --- > ovn/northd/ovn-northd.c | 114 ++++++++++++++++++++++++++++++ > ++++++++---------- > ovn/ovn-nb.xml | 42 +++++++++++++++--- > tests/ovn.at | 60 +++++++++++++++++++++++++ > 3 files changed, 185 insertions(+), 31 deletions(-) > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index e24ff8f..efb8a74 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -1436,6 +1436,86 @@ join_logical_ports(struct northd_context *ctx, > } > > static void > +ip_address_and_port_from_lb_key(const char *key, char **ip_address, > + uint16_t *port); > + > +static void > +get_router_load_balancer_ips(const struct ovn_datapath *od, > + struct sset *all_ips) > +{ > + if (!od->nbr) { > + return; > + } > + > + for (int i = 0; i < od->nbr->n_load_balancer; i++) { > + struct nbrec_load_balancer *lb = od->nbr->load_balancer[i]; > + struct smap *vips = &lb->vips; > + struct smap_node *node; > + > + SMAP_FOR_EACH (node, vips) { > + /* node->key contains IP:port or just IP. */ > + char *ip_address = NULL; > + uint16_t port; > + > + ip_address_and_port_from_lb_key(node->key, &ip_address, > &port); > + if (!ip_address) { > + continue; > + } > + > + if (!sset_contains(all_ips, ip_address)) { > + sset_add(all_ips, ip_address); > + } > + > + free(ip_address); > + } > + } > +} > + > +/* Returns a string consisting of the port's MAC address followed by the > + * external IP addresses of all NAT rules defined on that router and the > + * VIPs of all load balancers defined on that router. */ > A comment that the called has to free the returned string is useful. Also, the load balancer IP address can also be the IP address of the router itself. For e.g: when it is ROUTER_IP:port. I guess, that is not a problem when a GARP is sent for the router IP too. Acked-by: Gurucharan Shetty <guru@ovn.org> > +static char * > +get_nat_addresses(const struct ovn_port *op) > +{ > + struct eth_addr mac; > + if (!op->nbrp || !op->od || !op->od->nbr > + || (!op->od->nbr->n_nat && !op->od->nbr->n_load_balancer) > + || !eth_addr_from_string(op->nbrp->mac, &mac)) { > + return NULL; > + } > + > + struct ds addresses = DS_EMPTY_INITIALIZER; > + ds_put_format(&addresses, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac)); > + > + /* Get NAT IP addresses. */ > + for (int i = 0; i < op->od->nbr->n_nat; i++) { > + const struct nbrec_nat *nat; > + nat = op->od->nbr->nat[i]; > + > + ovs_be32 ip, mask; > + > + char *error = ip_parse_masked(nat->external_ip, &ip, &mask); > + if (error || mask != OVS_BE32_MAX) { > + free(error); > + continue; > + } > + ds_put_format(&addresses, " %s", nat->external_ip); > + } > + > + /* A set to hold all load-balancer vips. */ > + struct sset all_ips = SSET_INITIALIZER(&all_ips); > + get_router_load_balancer_ips(op->od, &all_ips); > + > + const char *ip_address; > + SSET_FOR_EACH(ip_address, &all_ips) { > + ds_put_format(&addresses, " %s", ip_address); > + } > + sset_destroy(&all_ips); > + > + return ds_steal_cstr(&addresses); > +} > + > +static void > ovn_port_update_sbrec(const struct ovn_port *op, > struct hmap *chassis_qdisc_queues) > { > @@ -1524,7 +1604,15 @@ ovn_port_update_sbrec(const struct ovn_port *op, > > const char *nat_addresses = smap_get(&op->nbsp->options, > "nat-addresses"); > - if (nat_addresses) { > + if (nat_addresses && !strcmp(nat_addresses, "router")) { > + if (op->peer && op->peer->nbrp) { > + char *nats = get_nat_addresses(op->peer); > + if (nats) { > + smap_add(&new, "nat-addresses", nats); > + free(nats); > + } > + } > + } else if (nat_addresses) { > struct lport_addresses laddrs; > if (!extract_lsp_addresses(nat_addresses, &laddrs)) { > static struct vlog_rate_limit rl = > @@ -3869,29 +3957,7 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > > /* A set to hold all load-balancer vips that need ARP responses. > */ > struct sset all_ips = SSET_INITIALIZER(&all_ips); > - > - for (int i = 0; i < op->od->nbr->n_load_balancer; i++) { > - struct nbrec_load_balancer *lb = > op->od->nbr->load_balancer[i]; > - struct smap *vips = &lb->vips; > - struct smap_node *node; > - > - SMAP_FOR_EACH (node, vips) { > - /* node->key contains IP:port or just IP. */ > - char *ip_address = NULL; > - uint16_t port; > - > - ip_address_and_port_from_lb_key(node->key, &ip_address, > &port); > - if (!ip_address) { > - continue; > - } > - > - if (!sset_contains(&all_ips, ip_address)) { > - sset_add(&all_ips, ip_address); > - } > - > - free(ip_address); > - } > - } > + get_router_load_balancer_ips(op->od, &all_ips); > > const char *ip_address; > SSET_FOR_EACH(ip_address, &all_ips) { > diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml > index 2af46b6..57fbc98 100644 > --- a/ovn/ovn-nb.xml > +++ b/ovn/ovn-nb.xml > @@ -242,13 +242,41 @@ > </column> > > <column name="options" key="nat-addresses"> > - MAC address of the <code>router-port</code> followed by a list > of > - SNAT and DNAT IP addresses. This is used to send gratuitous > ARPs for > - SNAT and DNAT IP addresses via <code>localnet</code> and is > valid for > - only L3 gateway ports. Example: <code>80:fa:5b:06:72:b7 > 158.36.44.22 > - 158.36.44.24</code>. This would result in generation of > gratuitous > - ARPs for IP addresses 158.36.44.22 and 158.36.44.24 with a MAC > - address of 80:fa:5b:06:72:b7. > + <p> > + This is used to send gratuitous ARPs for SNAT and DNAT IP > + addresses via <code>localnet</code> and is valid for only L3 > + gateway ports. > + </p> > + > + <p> > + This must take one of the following forms: > + </p> > + > + <dl> > + <dt><code>router</code></dt> > + <dd> > + <p> > + Gratuitous ARPs will be sent for all SNAT and DNAT > external IP > + addresses and for all load balancer IP addresses defined > on the > + <ref column="options" key="router-port"/>'s logical > router, > + using the <ref column="options" key="router-port"/>'s MAC > + address. > + </p> > + > + <p> > + Supported only in OVN 2.7 and later. Earlier versions > required > + NAT addresses to be manually synchronized. > + </p> > + </dd> > + > + <dt><code>Ethernet address followed by one or more IPv4 > addresses</code></dt> > + <dd> > + Example: <code>80:fa:5b:06:72:b7 158.36.44.22 > + 158.36.44.24</code>. This would result in generation of > + gratuitous ARPs for IP addresses 158.36.44.22 and > 158.36.44.24 > + with a MAC address of 80:fa:5b:06:72:b7. > + </dd> > + </dl> > </column> > </group> > > diff --git a/tests/ovn.at b/tests/ovn.at > index 6eed020..febe00a 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -5219,6 +5219,66 @@ OVN_CLEANUP([hv1]) > > AT_CLEANUP > > +AT_SETUP([ovn -- send gratuitous arp with nat-addresses router in > localnet]) > +AT_SKIP_IF([test $HAVE_PYTHON = no]) > +ovn_start > +# Create logical switch > +ovn-nbctl ls-add ls0 > +# Create gateway router > +ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1 > +# Add router port to gateway router > +ovn-nbctl lrp-add lr0 lrp0 f0:00:00:00:00:01 192.168.0.1/24 > +ovn-nbctl lsp-add ls0 lrp0-rp -- set Logical_Switch_Port lrp0-rp \ > + type=router options:router-port=lrp0-rp addresses='"f0:00:00:00:00:01" > ' > +# Add nat-address option > +ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0 nat-addresses="router" > +# Add NAT rules > +AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 192.168.0.1 10.0.0.0/24]) > +AT_CHECK([ovn-nbctl lr-nat-add lr0 dnat 192.168.0.2 10.0.0.1]) > +# Add load balancers > +AT_CHECK([ovn-nbctl lb-add lb0 192.168.0.3:80 10.0.0.2:80,10.0.0.3:80]) > +AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0]) > +AT_CHECK([ovn-nbctl lb-add lb1 192.168.0.3:8080 10.0.0.2:8080, > 10.0.0.3:8080]) > +AT_CHECK([ovn-nbctl lr-lb-add lr0 lb1]) > + > +net_add n1 > +sim_add hv1 > +as hv1 > +ovs-vsctl \ > + -- add-br br-phys \ > + -- add-br br-eth0 > + > +ovn_attach n1 br-phys 192.168.0.1 > + > +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge- > mappings=physnet1:br-eth0]) > +AT_CHECK([ovs-vsctl add-port br-eth0 snoopvif -- set Interface snoopvif > options:tx_pcap=hv1/snoopvif-tx.pcap options:rxq_pcap=hv1/snoopvif- > rx.pcap]) > + > +# Create a localnet port. > +AT_CHECK([ovn-nbctl lsp-add ls0 ln_port]) > +AT_CHECK([ovn-nbctl lsp-set-addresses ln_port unknown]) > +AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet]) > +AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1]) > + > + > +# Wait for packet to be received. > +OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 50]) > +trim_zeros() { > + sed 's/\(00\)\{1,\}$//' > +} > +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | > trim_zeros > packets > +expected="fffffffffffff0000000000108060001080006040001f00000000001c0a8 > 0001000000000000c0a80001" > +echo $expected > expout > +expected="fffffffffffff0000000000108060001080006040001f00000000001c0a8 > 0002000000000000c0a80002" > +echo $expected >> expout > +expected="fffffffffffff0000000000108060001080006040001f00000000001c0a8 > 0003000000000000c0a80003" > +echo $expected >> expout > +AT_CHECK([sort packets], [0], [expout]) > +cat packets > + > +OVN_CLEANUP([hv1]) > + > +AT_CLEANUP > + > AT_SETUP([ovn -- delete mac bindings]) > ovn_start > net_add n1 > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Thanks for the review. On Fri, Jan 27, 2017 at 10:20 AM, Guru Shetty <guru@ovn.org> wrote: > > > On 26 January 2017 at 01:20, Mickey Spiegel <mickeys.dev@gmail.com> wrote: > >> Currently in OVN, the "nat-addresses" in the "options" column of a >> logical switch port of type "router" must be specified manually. >> Typically the user would specify as "nat-addresses" all of the NAT >> external IP addresses and load balancer IP addresses that have >> already been specified separately on the router. >> >> This patch allows the logical switch port's "nat-addresses" to be >> specified as the string "router". When ovn-northd sees this string, >> it automatically copies the following into the southbound >> Port_Binding's "nat-addresses" in the "options" column: >> The options:router-port's MAC address. >> Each NAT external IP address (of any NAT type) specified on the >> logical router of options:router-port. >> Each load balancer IP address specified on the logical router of >> options:router-port. >> This will cause the controller where the gateway router resides to >> issue gratuitous ARPs for each NAT external IP address and for each >> load balancer IP address specified on the gateway router. >> >> This patch is written as if it will be included in OVS 2.7. If it >> is deferred to OVS 2.8, then the OVS version mentioned in ovn-nb.xml >> will need to be updated from OVS 2.7 to OVS 2.8. >> >> Signed-off-by: Mickey Spiegel <mickeys.dev@gmail.com> >> --- >> ovn/northd/ovn-northd.c | 114 ++++++++++++++++++++++++++++++ >> ++++++++---------- >> ovn/ovn-nb.xml | 42 +++++++++++++++--- >> tests/ovn.at | 60 +++++++++++++++++++++++++ >> 3 files changed, 185 insertions(+), 31 deletions(-) >> >> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c >> index e24ff8f..efb8a74 100644 >> --- a/ovn/northd/ovn-northd.c >> +++ b/ovn/northd/ovn-northd.c >> @@ -1436,6 +1436,86 @@ join_logical_ports(struct northd_context *ctx, >> } >> >> static void >> +ip_address_and_port_from_lb_key(const char *key, char **ip_address, >> + uint16_t *port); >> + >> +static void >> +get_router_load_balancer_ips(const struct ovn_datapath *od, >> + struct sset *all_ips) >> +{ >> + if (!od->nbr) { >> + return; >> + } >> + >> + for (int i = 0; i < od->nbr->n_load_balancer; i++) { >> + struct nbrec_load_balancer *lb = od->nbr->load_balancer[i]; >> + struct smap *vips = &lb->vips; >> + struct smap_node *node; >> + >> + SMAP_FOR_EACH (node, vips) { >> + /* node->key contains IP:port or just IP. */ >> + char *ip_address = NULL; >> + uint16_t port; >> + >> + ip_address_and_port_from_lb_key(node->key, &ip_address, >> &port); >> + if (!ip_address) { >> + continue; >> + } >> + >> + if (!sset_contains(all_ips, ip_address)) { >> + sset_add(all_ips, ip_address); >> + } >> + >> + free(ip_address); >> + } >> + } >> +} >> + >> +/* Returns a string consisting of the port's MAC address followed by the >> + * external IP addresses of all NAT rules defined on that router and the >> + * VIPs of all load balancers defined on that router. */ >> > A comment that the called has to free the returned string is useful. > I can add that. > > Also, the load balancer IP address can also be the IP address of the > router itself. For e.g: when it is ROUTER_IP:port. I guess, that is not a > problem when a GARP is sent for the router IP too. > If the router IP is used as a load balancer IP address or as a SNAT address, then sending GARP for the router IP is a good thing. Mickey > > Acked-by: Gurucharan Shetty <guru@ovn.org> > > > > >> +static char * >> +get_nat_addresses(const struct ovn_port *op) >> +{ >> + struct eth_addr mac; >> + if (!op->nbrp || !op->od || !op->od->nbr >> + || (!op->od->nbr->n_nat && !op->od->nbr->n_load_balancer) >> + || !eth_addr_from_string(op->nbrp->mac, &mac)) { >> + return NULL; >> + } >> + >> + struct ds addresses = DS_EMPTY_INITIALIZER; >> + ds_put_format(&addresses, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac)); >> + >> + /* Get NAT IP addresses. */ >> + for (int i = 0; i < op->od->nbr->n_nat; i++) { >> + const struct nbrec_nat *nat; >> + nat = op->od->nbr->nat[i]; >> + >> + ovs_be32 ip, mask; >> + >> + char *error = ip_parse_masked(nat->external_ip, &ip, &mask); >> + if (error || mask != OVS_BE32_MAX) { >> + free(error); >> + continue; >> + } >> + ds_put_format(&addresses, " %s", nat->external_ip); >> + } >> + >> + /* A set to hold all load-balancer vips. */ >> + struct sset all_ips = SSET_INITIALIZER(&all_ips); >> + get_router_load_balancer_ips(op->od, &all_ips); >> + >> + const char *ip_address; >> + SSET_FOR_EACH(ip_address, &all_ips) { >> + ds_put_format(&addresses, " %s", ip_address); >> + } >> + sset_destroy(&all_ips); >> + >> + return ds_steal_cstr(&addresses); >> +} >> + >> +static void >> ovn_port_update_sbrec(const struct ovn_port *op, >> struct hmap *chassis_qdisc_queues) >> { >> @@ -1524,7 +1604,15 @@ ovn_port_update_sbrec(const struct ovn_port *op, >> >> const char *nat_addresses = smap_get(&op->nbsp->options, >> "nat-addresses"); >> - if (nat_addresses) { >> + if (nat_addresses && !strcmp(nat_addresses, "router")) { >> + if (op->peer && op->peer->nbrp) { >> + char *nats = get_nat_addresses(op->peer); >> + if (nats) { >> + smap_add(&new, "nat-addresses", nats); >> + free(nats); >> + } >> + } >> + } else if (nat_addresses) { >> struct lport_addresses laddrs; >> if (!extract_lsp_addresses(nat_addresses, &laddrs)) { >> static struct vlog_rate_limit rl = >> @@ -3869,29 +3957,7 @@ build_lrouter_flows(struct hmap *datapaths, struct >> hmap *ports, >> >> /* A set to hold all load-balancer vips that need ARP responses. >> */ >> struct sset all_ips = SSET_INITIALIZER(&all_ips); >> - >> - for (int i = 0; i < op->od->nbr->n_load_balancer; i++) { >> - struct nbrec_load_balancer *lb = >> op->od->nbr->load_balancer[i]; >> - struct smap *vips = &lb->vips; >> - struct smap_node *node; >> - >> - SMAP_FOR_EACH (node, vips) { >> - /* node->key contains IP:port or just IP. */ >> - char *ip_address = NULL; >> - uint16_t port; >> - >> - ip_address_and_port_from_lb_key(node->key, &ip_address, >> &port); >> - if (!ip_address) { >> - continue; >> - } >> - >> - if (!sset_contains(&all_ips, ip_address)) { >> - sset_add(&all_ips, ip_address); >> - } >> - >> - free(ip_address); >> - } >> - } >> + get_router_load_balancer_ips(op->od, &all_ips); >> >> const char *ip_address; >> SSET_FOR_EACH(ip_address, &all_ips) { >> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml >> index 2af46b6..57fbc98 100644 >> --- a/ovn/ovn-nb.xml >> +++ b/ovn/ovn-nb.xml >> @@ -242,13 +242,41 @@ >> </column> >> >> <column name="options" key="nat-addresses"> >> - MAC address of the <code>router-port</code> followed by a list >> of >> - SNAT and DNAT IP addresses. This is used to send gratuitous >> ARPs for >> - SNAT and DNAT IP addresses via <code>localnet</code> and is >> valid for >> - only L3 gateway ports. Example: <code>80:fa:5b:06:72:b7 >> 158.36.44.22 >> - 158.36.44.24</code>. This would result in generation of >> gratuitous >> - ARPs for IP addresses 158.36.44.22 and 158.36.44.24 with a MAC >> - address of 80:fa:5b:06:72:b7. >> + <p> >> + This is used to send gratuitous ARPs for SNAT and DNAT IP >> + addresses via <code>localnet</code> and is valid for only L3 >> + gateway ports. >> + </p> >> + >> + <p> >> + This must take one of the following forms: >> + </p> >> + >> + <dl> >> + <dt><code>router</code></dt> >> + <dd> >> + <p> >> + Gratuitous ARPs will be sent for all SNAT and DNAT >> external IP >> + addresses and for all load balancer IP addresses defined >> on the >> + <ref column="options" key="router-port"/>'s logical >> router, >> + using the <ref column="options" key="router-port"/>'s MAC >> + address. >> + </p> >> + >> + <p> >> + Supported only in OVN 2.7 and later. Earlier versions >> required >> + NAT addresses to be manually synchronized. >> + </p> >> + </dd> >> + >> + <dt><code>Ethernet address followed by one or more IPv4 >> addresses</code></dt> >> + <dd> >> + Example: <code>80:fa:5b:06:72:b7 158.36.44.22 >> + 158.36.44.24</code>. This would result in generation of >> + gratuitous ARPs for IP addresses 158.36.44.22 and >> 158.36.44.24 >> + with a MAC address of 80:fa:5b:06:72:b7. >> + </dd> >> + </dl> >> </column> >> </group> >> >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 6eed020..febe00a 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -5219,6 +5219,66 @@ OVN_CLEANUP([hv1]) >> >> AT_CLEANUP >> >> +AT_SETUP([ovn -- send gratuitous arp with nat-addresses router in >> localnet]) >> +AT_SKIP_IF([test $HAVE_PYTHON = no]) >> +ovn_start >> +# Create logical switch >> +ovn-nbctl ls-add ls0 >> +# Create gateway router >> +ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1 >> +# Add router port to gateway router >> +ovn-nbctl lrp-add lr0 lrp0 f0:00:00:00:00:01 192.168.0.1/24 >> +ovn-nbctl lsp-add ls0 lrp0-rp -- set Logical_Switch_Port lrp0-rp \ >> + type=router options:router-port=lrp0-rp >> addresses='"f0:00:00:00:00:01"' >> +# Add nat-address option >> +ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0 nat-addresses="router" >> +# Add NAT rules >> +AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 192.168.0.1 10.0.0.0/24]) >> +AT_CHECK([ovn-nbctl lr-nat-add lr0 dnat 192.168.0.2 10.0.0.1]) >> +# Add load balancers >> +AT_CHECK([ovn-nbctl lb-add lb0 192.168.0.3:80 10.0.0.2:80,10.0.0.3:80]) >> +AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0]) >> +AT_CHECK([ovn-nbctl lb-add lb1 192.168.0.3:8080 10.0.0.2:8080, >> 10.0.0.3:8080]) >> +AT_CHECK([ovn-nbctl lr-lb-add lr0 lb1]) >> + >> +net_add n1 >> +sim_add hv1 >> +as hv1 >> +ovs-vsctl \ >> + -- add-br br-phys \ >> + -- add-br br-eth0 >> + >> +ovn_attach n1 br-phys 192.168.0.1 >> + >> +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappin >> gs=physnet1:br-eth0]) >> +AT_CHECK([ovs-vsctl add-port br-eth0 snoopvif -- set Interface snoopvif >> options:tx_pcap=hv1/snoopvif-tx.pcap options:rxq_pcap=hv1/snoopvif- >> rx.pcap]) >> + >> +# Create a localnet port. >> +AT_CHECK([ovn-nbctl lsp-add ls0 ln_port]) >> +AT_CHECK([ovn-nbctl lsp-set-addresses ln_port unknown]) >> +AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet]) >> +AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1]) >> + >> + >> +# Wait for packet to be received. >> +OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 50]) >> +trim_zeros() { >> + sed 's/\(00\)\{1,\}$//' >> +} >> +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | >> trim_zeros > packets >> +expected="fffffffffffff0000000000108060001080006040001f0000 >> 0000001c0a80001000000000000c0a80001" >> +echo $expected > expout >> +expected="fffffffffffff0000000000108060001080006040001f0000 >> 0000001c0a80002000000000000c0a80002" >> +echo $expected >> expout >> +expected="fffffffffffff0000000000108060001080006040001f0000 >> 0000001c0a80003000000000000c0a80003" >> +echo $expected >> expout >> +AT_CHECK([sort packets], [0], [expout]) >> +cat packets >> + >> +OVN_CLEANUP([hv1]) >> + >> +AT_CLEANUP >> + >> AT_SETUP([ovn -- delete mac bindings]) >> ovn_start >> net_add n1 >> -- >> 1.9.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > >
On Fri, Jan 27, 2017 at 10:29 AM, Mickey Spiegel <mickeys.dev@gmail.com> wrote: > Thanks for the review. > > On Fri, Jan 27, 2017 at 10:20 AM, Guru Shetty <guru@ovn.org> wrote: > >> >> >> On 26 January 2017 at 01:20, Mickey Spiegel <mickeys.dev@gmail.com> >> wrote: >> >>> Currently in OVN, the "nat-addresses" in the "options" column of a >>> logical switch port of type "router" must be specified manually. >>> Typically the user would specify as "nat-addresses" all of the NAT >>> external IP addresses and load balancer IP addresses that have >>> already been specified separately on the router. >>> >>> This patch allows the logical switch port's "nat-addresses" to be >>> specified as the string "router". When ovn-northd sees this string, >>> it automatically copies the following into the southbound >>> Port_Binding's "nat-addresses" in the "options" column: >>> The options:router-port's MAC address. >>> Each NAT external IP address (of any NAT type) specified on the >>> logical router of options:router-port. >>> Each load balancer IP address specified on the logical router of >>> options:router-port. >>> This will cause the controller where the gateway router resides to >>> issue gratuitous ARPs for each NAT external IP address and for each >>> load balancer IP address specified on the gateway router. >>> >>> This patch is written as if it will be included in OVS 2.7. If it >>> is deferred to OVS 2.8, then the OVS version mentioned in ovn-nb.xml >>> will need to be updated from OVS 2.7 to OVS 2.8. >>> >>> Signed-off-by: Mickey Spiegel <mickeys.dev@gmail.com> >>> --- >>> ovn/northd/ovn-northd.c | 114 ++++++++++++++++++++++++++++++ >>> ++++++++---------- >>> ovn/ovn-nb.xml | 42 +++++++++++++++--- >>> tests/ovn.at | 60 +++++++++++++++++++++++++ >>> 3 files changed, 185 insertions(+), 31 deletions(-) >>> >>> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c >>> index e24ff8f..efb8a74 100644 >>> --- a/ovn/northd/ovn-northd.c >>> +++ b/ovn/northd/ovn-northd.c >>> @@ -1436,6 +1436,86 @@ join_logical_ports(struct northd_context *ctx, >>> } >>> >>> static void >>> +ip_address_and_port_from_lb_key(const char *key, char **ip_address, >>> + uint16_t *port); >>> + >>> +static void >>> +get_router_load_balancer_ips(const struct ovn_datapath *od, >>> + struct sset *all_ips) >>> +{ >>> + if (!od->nbr) { >>> + return; >>> + } >>> + >>> + for (int i = 0; i < od->nbr->n_load_balancer; i++) { >>> + struct nbrec_load_balancer *lb = od->nbr->load_balancer[i]; >>> + struct smap *vips = &lb->vips; >>> + struct smap_node *node; >>> + >>> + SMAP_FOR_EACH (node, vips) { >>> + /* node->key contains IP:port or just IP. */ >>> + char *ip_address = NULL; >>> + uint16_t port; >>> + >>> + ip_address_and_port_from_lb_key(node->key, &ip_address, >>> &port); >>> + if (!ip_address) { >>> + continue; >>> + } >>> + >>> + if (!sset_contains(all_ips, ip_address)) { >>> + sset_add(all_ips, ip_address); >>> + } >>> + >>> + free(ip_address); >>> + } >>> + } >>> +} >>> + >>> +/* Returns a string consisting of the port's MAC address followed by the >>> + * external IP addresses of all NAT rules defined on that router and the >>> + * VIPs of all load balancers defined on that router. */ >>> >> A comment that the called has to free the returned string is useful. >> > > I can add that. > > >> >> Also, the load balancer IP address can also be the IP address of the >> router itself. For e.g: when it is ROUTER_IP:port. I guess, that is not a >> problem when a GARP is sent for the router IP too. >> > > If the router IP is used as a load balancer IP address or as a SNAT > address, then sending GARP for the router IP is a good thing. > I should clarify that statement. It is a good thing if the chassis changes, for example if doing simple high availability. The GARP packet will fix L2 learning. As I think about it, if anyone uses logical routers without NAT or load balancers, and the chassis changes, then GARP of the router IP would still be useful. I guess a user could always specify the router IP and MAC in "nat-addresses" to make that happen. I am not sure if anyone is deploying OVN that way. Mickey > Mickey > > >> >> Acked-by: Gurucharan Shetty <guru@ovn.org> >> >
> > > > I should clarify that statement. It is a good thing if the chassis > changes, for example if doing simple high availability. The GARP > packet will fix L2 learning. > > As I think about it, if anyone uses logical routers without NAT > or load balancers, and the chassis changes, then GARP of the > router IP would still be useful. I guess a user could always > specify the router IP and MAC in "nat-addresses" to make that > happen. I am not sure if anyone is deploying OVN that way. > > Mickey > I applied the first 5 patches of the series to master. I couldn't apply it to 2.7 because of some conflicts. Would you mind reposting it just for 2.7 with "branch-2.7" in the subject . For e.g: [PATCH branch-2.7] > > >> Mickey >> >> >>> >>> Acked-by: Gurucharan Shetty <guru@ovn.org> >>> >>
On Fri, Jan 27, 2017 at 11:16 AM, Guru Shetty <guru@ovn.org> wrote: > >> >> I should clarify that statement. It is a good thing if the chassis >> changes, for example if doing simple high availability. The GARP >> packet will fix L2 learning. >> >> As I think about it, if anyone uses logical routers without NAT >> or load balancers, and the chassis changes, then GARP of the >> router IP would still be useful. I guess a user could always >> specify the router IP and MAC in "nat-addresses" to make that >> happen. I am not sure if anyone is deploying OVN that way. >> >> Mickey >> > > I applied the first 5 patches of the series to master. I couldn't apply it > to 2.7 because of some conflicts. Would you mind reposting it just for 2.7 > with "branch-2.7" in the subject . For e.g: [PATCH branch-2.7] > Some previous patches made it into master but not yet branch-2.7: https://github.com/openvswitch/ovs/commit/ba8d3816e88f7a702635c09111f58352ecad6506 https://github.com/openvswitch/ovs/commit/41a15b71ed1ef35aa612a1128082219fbfc3f327 Next are a bunch of blp's patches that need to go in before these 5 patches: https://github.com/openvswitch/ovs/commit/80b6743d0ab3a39884fe873dd616cb49b6f55fab https://github.com/openvswitch/ovs/commit/b3bd2c33e83e2039d75e830368a64d596f820aaa https://github.com/openvswitch/ovs/commit/ebb467ff1c255813d6ba27d91ef6180e9a20fe0a https://github.com/openvswitch/ovs/commit/8f5de08322673f4e60f44d599fa7ee4de65bc078 https://github.com/openvswitch/ovs/commit/c571f48c36223de360ba0fa4d89104a7da14dbca https://github.com/openvswitch/ovs/commit/4c99cb181b6937efb3819cffc9765999fd7b7796 https://github.com/openvswitch/ovs/commit/db0e819be065c1474ceef232dcc1260c9a2e7c0e blp said that he could help with the backport: https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327884.html Mickey > > >> >> >>> Mickey >>> >>> >>>> >>>> Acked-by: Gurucharan Shetty <guru@ovn.org> >>>> >>> >
On Fri, Jan 27, 2017 at 11:49:25AM -0800, Mickey Spiegel wrote: > On Fri, Jan 27, 2017 at 11:16 AM, Guru Shetty <guru@ovn.org> wrote: > > > > >> > >> I should clarify that statement. It is a good thing if the chassis > >> changes, for example if doing simple high availability. The GARP > >> packet will fix L2 learning. > >> > >> As I think about it, if anyone uses logical routers without NAT > >> or load balancers, and the chassis changes, then GARP of the > >> router IP would still be useful. I guess a user could always > >> specify the router IP and MAC in "nat-addresses" to make that > >> happen. I am not sure if anyone is deploying OVN that way. > >> > >> Mickey > >> > > > > I applied the first 5 patches of the series to master. I couldn't apply it > > to 2.7 because of some conflicts. Would you mind reposting it just for 2.7 > > with "branch-2.7" in the subject . For e.g: [PATCH branch-2.7] > > > > Some previous patches made it into master but not yet > branch-2.7: > > https://github.com/openvswitch/ovs/commit/ba8d3816e88f7a702635c09111f58352ecad6506 > https://github.com/openvswitch/ovs/commit/41a15b71ed1ef35aa612a1128082219fbfc3f327 > > Next are a bunch of blp's patches that need to go in > before these 5 patches: > > https://github.com/openvswitch/ovs/commit/80b6743d0ab3a39884fe873dd616cb49b6f55fab > https://github.com/openvswitch/ovs/commit/b3bd2c33e83e2039d75e830368a64d596f820aaa > https://github.com/openvswitch/ovs/commit/ebb467ff1c255813d6ba27d91ef6180e9a20fe0a > https://github.com/openvswitch/ovs/commit/8f5de08322673f4e60f44d599fa7ee4de65bc078 > https://github.com/openvswitch/ovs/commit/c571f48c36223de360ba0fa4d89104a7da14dbca > https://github.com/openvswitch/ovs/commit/4c99cb181b6937efb3819cffc9765999fd7b7796 > https://github.com/openvswitch/ovs/commit/db0e819be065c1474ceef232dcc1260c9a2e7c0e > > blp said that he could help with the backport: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327884.html I've now cherry-picked all the patches you mention to master. Guru, does the series now apply to branch-2.7? Or let me know if you'd rather have me work on it.
On 31 January 2017 at 14:30, Ben Pfaff <blp@ovn.org> wrote: > On Fri, Jan 27, 2017 at 11:49:25AM -0800, Mickey Spiegel wrote: > > On Fri, Jan 27, 2017 at 11:16 AM, Guru Shetty <guru@ovn.org> wrote: > > > > > > > >> > > >> I should clarify that statement. It is a good thing if the chassis > > >> changes, for example if doing simple high availability. The GARP > > >> packet will fix L2 learning. > > >> > > >> As I think about it, if anyone uses logical routers without NAT > > >> or load balancers, and the chassis changes, then GARP of the > > >> router IP would still be useful. I guess a user could always > > >> specify the router IP and MAC in "nat-addresses" to make that > > >> happen. I am not sure if anyone is deploying OVN that way. > > >> > > >> Mickey > > >> > > > > > > I applied the first 5 patches of the series to master. I couldn't > apply it > > > to 2.7 because of some conflicts. Would you mind reposting it just for > 2.7 > > > with "branch-2.7" in the subject . For e.g: [PATCH branch-2.7] > > > > > > > Some previous patches made it into master but not yet > > branch-2.7: > > > > https://github.com/openvswitch/ovs/commit/ba8d3816e88f7a702635c09111f583 > 52ecad6506 > > https://github.com/openvswitch/ovs/commit/41a15b71ed1ef35aa612a112808221 > 9fbfc3f327 > > > > Next are a bunch of blp's patches that need to go in > > before these 5 patches: > > > > https://github.com/openvswitch/ovs/commit/80b6743d0ab3a39884fe873dd616cb > 49b6f55fab > > https://github.com/openvswitch/ovs/commit/b3bd2c33e83e2039d75e830368a64d > 596f820aaa > > https://github.com/openvswitch/ovs/commit/ebb467ff1c255813d6ba27d91ef618 > 0e9a20fe0a > > https://github.com/openvswitch/ovs/commit/8f5de08322673f4e60f44d599fa7ee > 4de65bc078 > > https://github.com/openvswitch/ovs/commit/c571f48c36223de360ba0fa4d89104 > a7da14dbca > > https://github.com/openvswitch/ovs/commit/4c99cb181b6937efb3819cffc97659 > 99fd7b7796 > > https://github.com/openvswitch/ovs/commit/db0e819be065c1474ceef232dcc126 > 0c9a2e7c0e > > > > blp said that he could help with the backport: > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327884.html > > I've now cherry-picked all the patches you mention to master. > > Guru, does the series now apply to branch-2.7? Or let me know if you'd > rather have me work on it. > It does, thanks!. I applied this series to 2.7 too.
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index e24ff8f..efb8a74 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -1436,6 +1436,86 @@ join_logical_ports(struct northd_context *ctx, } static void +ip_address_and_port_from_lb_key(const char *key, char **ip_address, + uint16_t *port); + +static void +get_router_load_balancer_ips(const struct ovn_datapath *od, + struct sset *all_ips) +{ + if (!od->nbr) { + return; + } + + for (int i = 0; i < od->nbr->n_load_balancer; i++) { + struct nbrec_load_balancer *lb = od->nbr->load_balancer[i]; + struct smap *vips = &lb->vips; + struct smap_node *node; + + SMAP_FOR_EACH (node, vips) { + /* node->key contains IP:port or just IP. */ + char *ip_address = NULL; + uint16_t port; + + ip_address_and_port_from_lb_key(node->key, &ip_address, &port); + if (!ip_address) { + continue; + } + + if (!sset_contains(all_ips, ip_address)) { + sset_add(all_ips, ip_address); + } + + free(ip_address); + } + } +} + +/* Returns a string consisting of the port's MAC address followed by the + * external IP addresses of all NAT rules defined on that router and the + * VIPs of all load balancers defined on that router. */ +static char * +get_nat_addresses(const struct ovn_port *op) +{ + struct eth_addr mac; + if (!op->nbrp || !op->od || !op->od->nbr + || (!op->od->nbr->n_nat && !op->od->nbr->n_load_balancer) + || !eth_addr_from_string(op->nbrp->mac, &mac)) { + return NULL; + } + + struct ds addresses = DS_EMPTY_INITIALIZER; + ds_put_format(&addresses, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac)); + + /* Get NAT IP addresses. */ + for (int i = 0; i < op->od->nbr->n_nat; i++) { + const struct nbrec_nat *nat; + nat = op->od->nbr->nat[i]; + + ovs_be32 ip, mask; + + char *error = ip_parse_masked(nat->external_ip, &ip, &mask); + if (error || mask != OVS_BE32_MAX) { + free(error); + continue; + } + ds_put_format(&addresses, " %s", nat->external_ip); + } + + /* A set to hold all load-balancer vips. */ + struct sset all_ips = SSET_INITIALIZER(&all_ips); + get_router_load_balancer_ips(op->od, &all_ips); + + const char *ip_address; + SSET_FOR_EACH(ip_address, &all_ips) { + ds_put_format(&addresses, " %s", ip_address); + } + sset_destroy(&all_ips); + + return ds_steal_cstr(&addresses); +} + +static void ovn_port_update_sbrec(const struct ovn_port *op, struct hmap *chassis_qdisc_queues) { @@ -1524,7 +1604,15 @@ ovn_port_update_sbrec(const struct ovn_port *op, const char *nat_addresses = smap_get(&op->nbsp->options, "nat-addresses"); - if (nat_addresses) { + if (nat_addresses && !strcmp(nat_addresses, "router")) { + if (op->peer && op->peer->nbrp) { + char *nats = get_nat_addresses(op->peer); + if (nats) { + smap_add(&new, "nat-addresses", nats); + free(nats); + } + } + } else if (nat_addresses) { struct lport_addresses laddrs; if (!extract_lsp_addresses(nat_addresses, &laddrs)) { static struct vlog_rate_limit rl = @@ -3869,29 +3957,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, /* A set to hold all load-balancer vips that need ARP responses. */ struct sset all_ips = SSET_INITIALIZER(&all_ips); - - for (int i = 0; i < op->od->nbr->n_load_balancer; i++) { - struct nbrec_load_balancer *lb = op->od->nbr->load_balancer[i]; - struct smap *vips = &lb->vips; - struct smap_node *node; - - SMAP_FOR_EACH (node, vips) { - /* node->key contains IP:port or just IP. */ - char *ip_address = NULL; - uint16_t port; - - ip_address_and_port_from_lb_key(node->key, &ip_address, &port); - if (!ip_address) { - continue; - } - - if (!sset_contains(&all_ips, ip_address)) { - sset_add(&all_ips, ip_address); - } - - free(ip_address); - } - } + get_router_load_balancer_ips(op->od, &all_ips); const char *ip_address; SSET_FOR_EACH(ip_address, &all_ips) { diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml index 2af46b6..57fbc98 100644 --- a/ovn/ovn-nb.xml +++ b/ovn/ovn-nb.xml @@ -242,13 +242,41 @@ </column> <column name="options" key="nat-addresses"> - MAC address of the <code>router-port</code> followed by a list of - SNAT and DNAT IP addresses. This is used to send gratuitous ARPs for - SNAT and DNAT IP addresses via <code>localnet</code> and is valid for - only L3 gateway ports. Example: <code>80:fa:5b:06:72:b7 158.36.44.22 - 158.36.44.24</code>. This would result in generation of gratuitous - ARPs for IP addresses 158.36.44.22 and 158.36.44.24 with a MAC - address of 80:fa:5b:06:72:b7. + <p> + This is used to send gratuitous ARPs for SNAT and DNAT IP + addresses via <code>localnet</code> and is valid for only L3 + gateway ports. + </p> + + <p> + This must take one of the following forms: + </p> + + <dl> + <dt><code>router</code></dt> + <dd> + <p> + Gratuitous ARPs will be sent for all SNAT and DNAT external IP + addresses and for all load balancer IP addresses defined on the + <ref column="options" key="router-port"/>'s logical router, + using the <ref column="options" key="router-port"/>'s MAC + address. + </p> + + <p> + Supported only in OVN 2.7 and later. Earlier versions required + NAT addresses to be manually synchronized. + </p> + </dd> + + <dt><code>Ethernet address followed by one or more IPv4 addresses</code></dt> + <dd> + Example: <code>80:fa:5b:06:72:b7 158.36.44.22 + 158.36.44.24</code>. This would result in generation of + gratuitous ARPs for IP addresses 158.36.44.22 and 158.36.44.24 + with a MAC address of 80:fa:5b:06:72:b7. + </dd> + </dl> </column> </group> diff --git a/tests/ovn.at b/tests/ovn.at index 6eed020..febe00a 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -5219,6 +5219,66 @@ OVN_CLEANUP([hv1]) AT_CLEANUP +AT_SETUP([ovn -- send gratuitous arp with nat-addresses router in localnet]) +AT_SKIP_IF([test $HAVE_PYTHON = no]) +ovn_start +# Create logical switch +ovn-nbctl ls-add ls0 +# Create gateway router +ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1 +# Add router port to gateway router +ovn-nbctl lrp-add lr0 lrp0 f0:00:00:00:00:01 192.168.0.1/24 +ovn-nbctl lsp-add ls0 lrp0-rp -- set Logical_Switch_Port lrp0-rp \ + type=router options:router-port=lrp0-rp addresses='"f0:00:00:00:00:01"' +# Add nat-address option +ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0 nat-addresses="router" +# Add NAT rules +AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 192.168.0.1 10.0.0.0/24]) +AT_CHECK([ovn-nbctl lr-nat-add lr0 dnat 192.168.0.2 10.0.0.1]) +# Add load balancers +AT_CHECK([ovn-nbctl lb-add lb0 192.168.0.3:80 10.0.0.2:80,10.0.0.3:80]) +AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0]) +AT_CHECK([ovn-nbctl lb-add lb1 192.168.0.3:8080 10.0.0.2:8080,10.0.0.3:8080]) +AT_CHECK([ovn-nbctl lr-lb-add lr0 lb1]) + +net_add n1 +sim_add hv1 +as hv1 +ovs-vsctl \ + -- add-br br-phys \ + -- add-br br-eth0 + +ovn_attach n1 br-phys 192.168.0.1 + +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=physnet1:br-eth0]) +AT_CHECK([ovs-vsctl add-port br-eth0 snoopvif -- set Interface snoopvif options:tx_pcap=hv1/snoopvif-tx.pcap options:rxq_pcap=hv1/snoopvif-rx.pcap]) + +# Create a localnet port. +AT_CHECK([ovn-nbctl lsp-add ls0 ln_port]) +AT_CHECK([ovn-nbctl lsp-set-addresses ln_port unknown]) +AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet]) +AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1]) + + +# Wait for packet to be received. +OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 50]) +trim_zeros() { + sed 's/\(00\)\{1,\}$//' +} +$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | trim_zeros > packets +expected="fffffffffffff0000000000108060001080006040001f00000000001c0a80001000000000000c0a80001" +echo $expected > expout +expected="fffffffffffff0000000000108060001080006040001f00000000001c0a80002000000000000c0a80002" +echo $expected >> expout +expected="fffffffffffff0000000000108060001080006040001f00000000001c0a80003000000000000c0a80003" +echo $expected >> expout +AT_CHECK([sort packets], [0], [expout]) +cat packets + +OVN_CLEANUP([hv1]) + +AT_CLEANUP + AT_SETUP([ovn -- delete mac bindings]) ovn_start net_add n1
Currently in OVN, the "nat-addresses" in the "options" column of a logical switch port of type "router" must be specified manually. Typically the user would specify as "nat-addresses" all of the NAT external IP addresses and load balancer IP addresses that have already been specified separately on the router. This patch allows the logical switch port's "nat-addresses" to be specified as the string "router". When ovn-northd sees this string, it automatically copies the following into the southbound Port_Binding's "nat-addresses" in the "options" column: The options:router-port's MAC address. Each NAT external IP address (of any NAT type) specified on the logical router of options:router-port. Each load balancer IP address specified on the logical router of options:router-port. This will cause the controller where the gateway router resides to issue gratuitous ARPs for each NAT external IP address and for each load balancer IP address specified on the gateway router. This patch is written as if it will be included in OVS 2.7. If it is deferred to OVS 2.8, then the OVS version mentioned in ovn-nb.xml will need to be updated from OVS 2.7 to OVS 2.8. Signed-off-by: Mickey Spiegel <mickeys.dev@gmail.com> --- ovn/northd/ovn-northd.c | 114 ++++++++++++++++++++++++++++++++++++++---------- ovn/ovn-nb.xml | 42 +++++++++++++++--- tests/ovn.at | 60 +++++++++++++++++++++++++ 3 files changed, 185 insertions(+), 31 deletions(-)