| Message ID | 56780C44.8090902@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
On 12/21/2015 09:27 AM, Numan Siddique wrote: > Instead add only if > - the port is up or > - if port type is router > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > --- > ovn/northd/ovn-northd.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 270b116..3d4ce78 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -951,6 +951,12 @@ lport_is_enabled(const struct nbrec_logical_port *lport) > } > > static bool > +lport_is_up(const struct nbrec_logical_port *lport) > +{ > + return !lport->up || *lport->up; > +} > + > +static bool > has_stateful_acl(struct ovn_datapath *od) > { > for (size_t i = 0; i < od->nbs->n_acls; i++) { > @@ -1152,6 +1158,15 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, > continue; > } > > + /* > + * Add ARP reply flows if either the > + * - port is up or > + * - port type is router > + */ > + if (!lport_is_up(op->nbs) && strcmp(op->nbs->type, "router")) { > + continue; > + } > + > for (size_t i = 0; i < op->nbs->n_addresses; i++) { > struct eth_addr ea; > ovs_be32 ip; > What problem are you addressing here? It seems we would want the flows created, even when the port is not up yet. Otherwise, when the port does come up, ovn-controller has an incomplete logical flow table. I think the logical flow table should be independent of port state and location. Thanks,
On 12/21/2015 08:13 PM, Russell Bryant wrote: > On 12/21/2015 09:27 AM, Numan Siddique wrote: >> Instead add only if >> - the port is up or >> - if port type is router >> >> Signed-off-by: Numan Siddique <nusiddiq@redhat.com> >> --- >> ovn/northd/ovn-northd.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c >> index 270b116..3d4ce78 100644 >> --- a/ovn/northd/ovn-northd.c >> +++ b/ovn/northd/ovn-northd.c >> @@ -951,6 +951,12 @@ lport_is_enabled(const struct nbrec_logical_port *lport) >> } >> >> static bool >> +lport_is_up(const struct nbrec_logical_port *lport) >> +{ >> + return !lport->up || *lport->up; >> +} >> + >> +static bool >> has_stateful_acl(struct ovn_datapath *od) >> { >> for (size_t i = 0; i < od->nbs->n_acls; i++) { >> @@ -1152,6 +1158,15 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, >> continue; >> } >> >> + /* >> + * Add ARP reply flows if either the >> + * - port is up or >> + * - port type is router >> + */ >> + if (!lport_is_up(op->nbs) && strcmp(op->nbs->type, "router")) { >> + continue; >> + } >> + >> for (size_t i = 0; i < op->nbs->n_addresses; i++) { >> struct eth_addr ea; >> ovs_be32 ip; >> > What problem are you addressing here? It seems we would want the flows > created, even when the port is not up yet. Otherwise, when the port > does come up, ovn-controller has an incomplete logical flow table. I > think the logical flow table should be independent of port state and > location. > > Thanks, > Presently when I create a port (neutron port-create) , i can get response to arping for the port ip, even if it is not bound to any VM. I thought of addressing this trivial problem. As soon as the port does come up, northd would add the necessary ARP reply logical flow. But I get your point now. Thanks Numan
On 12/21/2015 09:55 AM, Numan Siddique wrote: > On 12/21/2015 08:13 PM, Russell Bryant wrote: >> On 12/21/2015 09:27 AM, Numan Siddique wrote: >> What problem are you addressing here? It seems we would want the flows >> created, even when the port is not up yet. Otherwise, when the port >> does come up, ovn-controller has an incomplete logical flow table. I >> think the logical flow table should be independent of port state and >> location. >> >> Thanks, >> > Presently when I create a port (neutron port-create) , i can get response to arping for the port ip, > even if it is not bound to any VM. > I thought of addressing this trivial problem. As soon as the port does come up, northd would > add the necessary ARP reply logical flow. But I get your point now. Got it. Thanks for clarifying the issue. I wonder what the desired behavior should be here ... I'm tempted to just say the existing behavior is OK. I wonder what others think.
On Mon, Dec 21, 2015 at 10:39:29AM -0500, Russell Bryant wrote: > On 12/21/2015 09:55 AM, Numan Siddique wrote: > > On 12/21/2015 08:13 PM, Russell Bryant wrote: > >> On 12/21/2015 09:27 AM, Numan Siddique wrote: > >> What problem are you addressing here? It seems we would want the flows > >> created, even when the port is not up yet. Otherwise, when the port > >> does come up, ovn-controller has an incomplete logical flow table. I > >> think the logical flow table should be independent of port state and > >> location. > >> > >> Thanks, > >> > > Presently when I create a port (neutron port-create) , i can get response to arping for the port ip, > > even if it is not bound to any VM. > > I thought of addressing this trivial problem. As soon as the port does come up, northd would > > add the necessary ARP reply logical flow. But I get your point now. > > Got it. Thanks for clarifying the issue. > > I wonder what the desired behavior should be here ... I'm tempted to > just say the existing behavior is OK. I wonder what others think. It is surprising behavior to get an ARP response from a port that is not up. I think that it will confuse users and administrators trying to troubleshoot. I don't know the best way to handle this, but this seems like a reasonable place to start.
On 12/21/2015 02:52 PM, Ben Pfaff wrote: > On Mon, Dec 21, 2015 at 10:39:29AM -0500, Russell Bryant wrote: >> On 12/21/2015 09:55 AM, Numan Siddique wrote: >>> On 12/21/2015 08:13 PM, Russell Bryant wrote: >>>> On 12/21/2015 09:27 AM, Numan Siddique wrote: >>>> What problem are you addressing here? It seems we would want the flows >>>> created, even when the port is not up yet. Otherwise, when the port >>>> does come up, ovn-controller has an incomplete logical flow table. I >>>> think the logical flow table should be independent of port state and >>>> location. >>>> >>>> Thanks, >>>> >>> Presently when I create a port (neutron port-create) , i can get response to arping for the port ip, >>> even if it is not bound to any VM. >>> I thought of addressing this trivial problem. As soon as the port does come up, northd would >>> add the necessary ARP reply logical flow. But I get your point now. >> >> Got it. Thanks for clarifying the issue. >> >> I wonder what the desired behavior should be here ... I'm tempted to >> just say the existing behavior is OK. I wonder what others think. > > It is surprising behavior to get an ARP response from a port that is not > up. I think that it will confuse users and administrators trying to > troubleshoot. > > I don't know the best way to handle this, but this seems like a > reasonable place to start. > OK, thanks for the feedback Ben. If you're good with this patch, it's fine with me. I don't have any better ideas for how to handle it if we don't want the ARP response when ports aren't up. The main downside is the increase in churn of the logical flow table. For every new port, we now update the logical flow table twice instead of just once. Just brainstorming here ... We could have logical flow pre-requisites, where we could say to only process this logical flow if a given logical port is bound to a chassis (is up). That way the logical flows don't change and ovn-controller handles the change based on port state. That's a good bit more complicated than this patch, so maybe we should just merge this and keep it as something that could be revisited later if optimizing here seems worthwhile.
On Mon, Dec 21, 2015 at 03:03:42PM -0500, Russell Bryant wrote: > On 12/21/2015 02:52 PM, Ben Pfaff wrote: > > On Mon, Dec 21, 2015 at 10:39:29AM -0500, Russell Bryant wrote: > >> On 12/21/2015 09:55 AM, Numan Siddique wrote: > >>> On 12/21/2015 08:13 PM, Russell Bryant wrote: > >>>> On 12/21/2015 09:27 AM, Numan Siddique wrote: > >>>> What problem are you addressing here? It seems we would want the flows > >>>> created, even when the port is not up yet. Otherwise, when the port > >>>> does come up, ovn-controller has an incomplete logical flow table. I > >>>> think the logical flow table should be independent of port state and > >>>> location. > >>>> > >>>> Thanks, > >>>> > >>> Presently when I create a port (neutron port-create) , i can get response to arping for the port ip, > >>> even if it is not bound to any VM. > >>> I thought of addressing this trivial problem. As soon as the port does come up, northd would > >>> add the necessary ARP reply logical flow. But I get your point now. > >> > >> Got it. Thanks for clarifying the issue. > >> > >> I wonder what the desired behavior should be here ... I'm tempted to > >> just say the existing behavior is OK. I wonder what others think. > > > > It is surprising behavior to get an ARP response from a port that is not > > up. I think that it will confuse users and administrators trying to > > troubleshoot. > > > > I don't know the best way to handle this, but this seems like a > > reasonable place to start. > > > > OK, thanks for the feedback Ben. If you're good with this patch, it's > fine with me. I don't have any better ideas for how to handle it if we > don't want the ARP response when ports aren't up. > > The main downside is the increase in churn of the logical flow table. > For every new port, we now update the logical flow table twice instead > of just once. > > Just brainstorming here ... > > We could have logical flow pre-requisites, where we could say to only > process this logical flow if a given logical port is bound to a chassis > (is up). That way the logical flows don't change and ovn-controller > handles the change based on port state. > > That's a good bit more complicated than this patch, so maybe we should > just merge this and keep it as something that could be revisited later > if optimizing here seems worthwhile. That's roughly my thought process too. First get stuff correct, then worry about making it faster where it is necessary.
On Mon, Dec 21, 2015 at 1:59 PM, Ben Pfaff <blp@ovn.org> wrote: > On Mon, Dec 21, 2015 at 03:03:42PM -0500, Russell Bryant wrote: > > On 12/21/2015 02:52 PM, Ben Pfaff wrote: > > > On Mon, Dec 21, 2015 at 10:39:29AM -0500, Russell Bryant wrote: > > >> On 12/21/2015 09:55 AM, Numan Siddique wrote: > > >>> On 12/21/2015 08:13 PM, Russell Bryant wrote: > > >>>> On 12/21/2015 09:27 AM, Numan Siddique wrote: > > >>>> What problem are you addressing here? It seems we would want the > flows > > >>>> created, even when the port is not up yet. Otherwise, when the port > > >>>> does come up, ovn-controller has an incomplete logical flow table. > I > > >>>> think the logical flow table should be independent of port state and > > >>>> location. > > >>>> > > >>>> Thanks, > > >>>> > > >>> Presently when I create a port (neutron port-create) , i can get > response to arping for the port ip, > > >>> even if it is not bound to any VM. > > >>> I thought of addressing this trivial problem. As soon as the port > does come up, northd would > > >>> add the necessary ARP reply logical flow. But I get your point now. > > >> > > >> Got it. Thanks for clarifying the issue. > > >> > > >> I wonder what the desired behavior should be here ... I'm tempted to > > >> just say the existing behavior is OK. I wonder what others think. > > > > > > It is surprising behavior to get an ARP response from a port that is > not > > > up. I think that it will confuse users and administrators trying to > > > troubleshoot. > > > > > > I don't know the best way to handle this, but this seems like a > > > reasonable place to start. > > > > > > > OK, thanks for the feedback Ben. If you're good with this patch, it's > > fine with me. I don't have any better ideas for how to handle it if we > > don't want the ARP response when ports aren't up. > > > > The main downside is the increase in churn of the logical flow table. > > For every new port, we now update the logical flow table twice instead > > of just once. > > > > Just brainstorming here ... > > > > We could have logical flow pre-requisites, where we could say to only > > process this logical flow if a given logical port is bound to a chassis > > (is up). That way the logical flows don't change and ovn-controller > > handles the change based on port state. > > > > That's a good bit more complicated than this patch, so maybe we should > > just merge this and keep it as something that could be revisited later > > if optimizing here seems worthwhile. > > That's roughly my thought process too. First get stuff correct, then > worry about making it faster where it is necessary. > > I don't think it is strictly incorrect behavior to respond ARP before the port is up, because ARP is just to do the address resolving. Yes, it would confuse admin when doing trouble-shooting in the beginning, but I am not sure if people can get used to it and accept it as how virtual network works. Nevertheless, if for any reason we decide to avoid this, I would suggest to drop the ARP request in this case, instead of letting it flood on all existing ports, because it is not suppose to get any response before the port is up.
On Mon, Dec 21, 2015 at 07:57:16PM +0530, Numan Siddique wrote: > Instead add only if > - the port is up or > - if port type is router > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> Following the discussion, I applied this to master. I updated ovn-northd(8) to describe the new behavior.
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 270b116..3d4ce78 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -951,6 +951,12 @@ lport_is_enabled(const struct nbrec_logical_port *lport) } static bool +lport_is_up(const struct nbrec_logical_port *lport) +{ + return !lport->up || *lport->up; +} + +static bool has_stateful_acl(struct ovn_datapath *od) { for (size_t i = 0; i < od->nbs->n_acls; i++) { @@ -1152,6 +1158,15 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports, continue; } + /* + * Add ARP reply flows if either the + * - port is up or + * - port type is router + */ + if (!lport_is_up(op->nbs) && strcmp(op->nbs->type, "router")) { + continue; + } + for (size_t i = 0; i < op->nbs->n_addresses; i++) { struct eth_addr ea; ovs_be32 ip;
Instead add only if - the port is up or - if port type is router Signed-off-by: Numan Siddique <nusiddiq@redhat.com> --- ovn/northd/ovn-northd.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)