[ovs-dev,ovn] ovn-controller: Fix inject-pkt command error response.

From: Han Zhou <hzhou8@ebay.com>

When using unixctl command inject-packet, it always respond with
failure "server not ready", although the command was actually executed
successfully.

Fixes: 0bd4d85c36ef ("ovn-controller: Initial use of incremental engine - quiet mode.")
Signed-off-by: Han Zhou <hzhou8@ebay.com>
---
 controller/ovn-controller.c | 12 +++++++-----
 tests/ovn.at                |  2 +-
 2 files changed, 8 insertions(+), 6 deletions(-)

On Mon, Aug 5, 2019 at 9:29 AM Han Zhou <zhouhan@gmail.com> wrote:
>
> From: Han Zhou <hzhou8@ebay.com>
>
> When using unixctl command inject-packet, it always respond with
> failure "server not ready", although the command was actually executed
> successfully.
>
> Fixes: 0bd4d85c36ef ("ovn-controller: Initial use of incremental engine - quiet mode.")
> Signed-off-by: Han Zhou <hzhou8@ebay.com>

Hi Han,

I had already sent the exact same patch here :)
https://patchwork.ozlabs.org/patch/1140480/

Nevertheless:
Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks,
Dumitru

> ---
>  controller/ovn-controller.c | 12 +++++++-----
>  tests/ovn.at                |  2 +-
>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index ad33d17..9df598f 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2079,12 +2079,14 @@ main(int argc, char *argv[])
>                          unixctl_command_reply_error(pending_pkt.conn, error);
>                          free(error);
>                      } else {
> -                        VLOG_DBG("Pending_pkt conn but br_int %p or chassis "
> -                                 "%p not ready. run-id: %"PRIu64, br_int,
> -                                 chassis, engine_run_id);
> -                        unixctl_command_reply_error(pending_pkt.conn,
> -                            "ovn-controller not ready.");
> +                        unixctl_command_reply(pending_pkt.conn, NULL);
>                      }
> +                } else {
> +                    VLOG_DBG("Pending_pkt conn but br_int %p or chassis "
> +                             "%p not ready. run-id: %"PRIu64, br_int,
> +                             chassis, engine_run_id);
> +                    unixctl_command_reply_error(pending_pkt.conn,
> +                        "ovn-controller not ready.");
>                  }
>                  pending_pkt.conn = NULL;
>                  free(pending_pkt.flow_s);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index e88cffa..b85e549 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -3723,7 +3723,7 @@ sleep 1
>  packet="inport==\"ls1-lp1\" && eth.src==$ls1_lp1_mac && eth.dst==$rp_ls1_mac &&
>          ip4 && ip.ttl==64 && ip4.src==$ls1_lp1_ip && ip4.dst==$ls2_lp1_ip &&
>          udp && udp.src==53 && udp.dst==4369"
> -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
> +AT_CHECK([as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"])
>
>
>  echo "---------NB dump-----"
> --
> 2.1.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Mon, Aug 5, 2019 at 1:43 PM Dumitru Ceara <dceara@redhat.com> wrote:

> On Mon, Aug 5, 2019 at 9:29 AM Han Zhou <zhouhan@gmail.com> wrote:
> >
> > From: Han Zhou <hzhou8@ebay.com>
> >
> > When using unixctl command inject-packet, it always respond with
> > failure "server not ready", although the command was actually executed
> > successfully.
> >
> > Fixes: 0bd4d85c36ef ("ovn-controller: Initial use of incremental engine
> - quiet mode.")
> > Signed-off-by: Han Zhou <hzhou8@ebay.com>
>
> Hi Han,
>
> I had already sent the exact same patch here :)
> https://patchwork.ozlabs.org/patch/1140480/
>
> Nevertheless:
> Acked-by: Dumitru Ceara <dceara@redhat.com>
>

Thanks Han and Dumitru (for the review). I applied this patch to master as
this one had a test.

Thanks
Numan


> Thanks,
> Dumitru
>
> > ---
> >  controller/ovn-controller.c | 12 +++++++-----
> >  tests/ovn.at                |  2 +-
> >  2 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index ad33d17..9df598f 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -2079,12 +2079,14 @@ main(int argc, char *argv[])
> >                          unixctl_command_reply_error(pending_pkt.conn,
> error);
> >                          free(error);
> >                      } else {
> > -                        VLOG_DBG("Pending_pkt conn but br_int %p or
> chassis "
> > -                                 "%p not ready. run-id: %"PRIu64,
> br_int,
> > -                                 chassis, engine_run_id);
> > -                        unixctl_command_reply_error(pending_pkt.conn,
> > -                            "ovn-controller not ready.");
> > +                        unixctl_command_reply(pending_pkt.conn, NULL);
> >                      }
> > +                } else {
> > +                    VLOG_DBG("Pending_pkt conn but br_int %p or chassis
> "
> > +                             "%p not ready. run-id: %"PRIu64, br_int,
> > +                             chassis, engine_run_id);
> > +                    unixctl_command_reply_error(pending_pkt.conn,
> > +                        "ovn-controller not ready.");
> >                  }
> >                  pending_pkt.conn = NULL;
> >                  free(pending_pkt.flow_s);
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index e88cffa..b85e549 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -3723,7 +3723,7 @@ sleep 1
> >  packet="inport==\"ls1-lp1\" && eth.src==$ls1_lp1_mac &&
> eth.dst==$rp_ls1_mac &&
> >          ip4 && ip.ttl==64 && ip4.src==$ls1_lp1_ip &&
> ip4.dst==$ls2_lp1_ip &&
> >          udp && udp.src==53 && udp.dst==4369"
> > -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
> > +AT_CHECK([as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"])
> >
> >
> >  echo "---------NB dump-----"
> > --
> > 2.1.0
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Mon, Aug 5, 2019 at 6:53 AM Numan Siddique <nusiddiq@redhat.com> wrote:
>
>
>
> On Mon, Aug 5, 2019 at 1:43 PM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>> On Mon, Aug 5, 2019 at 9:29 AM Han Zhou <zhouhan@gmail.com> wrote:
>> >
>> > From: Han Zhou <hzhou8@ebay.com>
>> >
>> > When using unixctl command inject-packet, it always respond with
>> > failure "server not ready", although the command was actually executed
>> > successfully.
>> >
>> > Fixes: 0bd4d85c36ef ("ovn-controller: Initial use of incremental
engine - quiet mode.")
>> > Signed-off-by: Han Zhou <hzhou8@ebay.com>
>>
>> Hi Han,
>>
>> I had already sent the exact same patch here :)
>> https://patchwork.ozlabs.org/patch/1140480/
>>
>> Nevertheless:
>> Acked-by: Dumitru Ceara <dceara@redhat.com>

Dumitru, I am so sorry that I missed that email even it had me in cc. I
just updated my filter rules to make sure I won't miss such email again.
Thank you for fixing so many issues recently!

>
>
> Thanks Han and Dumitru (for the review). I applied this patch to master
as this one had a test.
>
> Thanks
> Numan
>
>>
>> Thanks,
>> Dumitru
>>
>> > ---
>> >  controller/ovn-controller.c | 12 +++++++-----
>> >  tests/ovn.at                |  2 +-
>> >  2 files changed, 8 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> > index ad33d17..9df598f 100644
>> > --- a/controller/ovn-controller.c
>> > +++ b/controller/ovn-controller.c
>> > @@ -2079,12 +2079,14 @@ main(int argc, char *argv[])
>> >                          unixctl_command_reply_error(pending_pkt.conn,
error);
>> >                          free(error);
>> >                      } else {
>> > -                        VLOG_DBG("Pending_pkt conn but br_int %p or
chassis "
>> > -                                 "%p not ready. run-id: %"PRIu64,
br_int,
>> > -                                 chassis, engine_run_id);
>> > -                        unixctl_command_reply_error(pending_pkt.conn,
>> > -                            "ovn-controller not ready.");
>> > +                        unixctl_command_reply(pending_pkt.conn, NULL);
>> >                      }
>> > +                } else {
>> > +                    VLOG_DBG("Pending_pkt conn but br_int %p or
chassis "
>> > +                             "%p not ready. run-id: %"PRIu64, br_int,
>> > +                             chassis, engine_run_id);
>> > +                    unixctl_command_reply_error(pending_pkt.conn,
>> > +                        "ovn-controller not ready.");
>> >                  }
>> >                  pending_pkt.conn = NULL;
>> >                  free(pending_pkt.flow_s);
>> > diff --git a/tests/ovn.at b/tests/ovn.at
>> > index e88cffa..b85e549 100644
>> > --- a/tests/ovn.at
>> > +++ b/tests/ovn.at
>> > @@ -3723,7 +3723,7 @@ sleep 1
>> >  packet="inport==\"ls1-lp1\" && eth.src==$ls1_lp1_mac &&
eth.dst==$rp_ls1_mac &&
>> >          ip4 && ip.ttl==64 && ip4.src==$ls1_lp1_ip &&
ip4.dst==$ls2_lp1_ip &&
>> >          udp && udp.src==53 && udp.dst==4369"
>> > -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
>> > +AT_CHECK([as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"])
>> >
>> >
>> >  echo "---------NB dump-----"
>> > --
>> > 2.1.0
>> >
>> > _______________________________________________
>> > dev mailing list
>> > dev@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Mon, Aug 5, 2019 at 5:25 PM Han Zhou <zhouhan@gmail.com> wrote:
>
>
>
> On Mon, Aug 5, 2019 at 6:53 AM Numan Siddique <nusiddiq@redhat.com> wrote:
> >
> >
> >
> > On Mon, Aug 5, 2019 at 1:43 PM Dumitru Ceara <dceara@redhat.com> wrote:
> >>
> >> On Mon, Aug 5, 2019 at 9:29 AM Han Zhou <zhouhan@gmail.com> wrote:
> >> >
> >> > From: Han Zhou <hzhou8@ebay.com>
> >> >
> >> > When using unixctl command inject-packet, it always respond with
> >> > failure "server not ready", although the command was actually executed
> >> > successfully.
> >> >
> >> > Fixes: 0bd4d85c36ef ("ovn-controller: Initial use of incremental engine - quiet mode.")
> >> > Signed-off-by: Han Zhou <hzhou8@ebay.com>
> >>
> >> Hi Han,
> >>
> >> I had already sent the exact same patch here :)
> >> https://patchwork.ozlabs.org/patch/1140480/
> >>
> >> Nevertheless:
> >> Acked-by: Dumitru Ceara <dceara@redhat.com>
>
> Dumitru, I am so sorry that I missed that email even it had me in cc. I just updated my filter rules to make sure I won't miss such email again. Thank you for fixing so many issues recently!

No problem. Your patch was better anyway as you also added a test for the issue.

>
> >
> >
> > Thanks Han and Dumitru (for the review). I applied this patch to master as this one had a test.
> >
> > Thanks
> > Numan
> >
> >>
> >> Thanks,
> >> Dumitru
> >>
> >> > ---
> >> >  controller/ovn-controller.c | 12 +++++++-----
> >> >  tests/ovn.at                |  2 +-
> >> >  2 files changed, 8 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> >> > index ad33d17..9df598f 100644
> >> > --- a/controller/ovn-controller.c
> >> > +++ b/controller/ovn-controller.c
> >> > @@ -2079,12 +2079,14 @@ main(int argc, char *argv[])
> >> >                          unixctl_command_reply_error(pending_pkt.conn, error);
> >> >                          free(error);
> >> >                      } else {
> >> > -                        VLOG_DBG("Pending_pkt conn but br_int %p or chassis "
> >> > -                                 "%p not ready. run-id: %"PRIu64, br_int,
> >> > -                                 chassis, engine_run_id);
> >> > -                        unixctl_command_reply_error(pending_pkt.conn,
> >> > -                            "ovn-controller not ready.");
> >> > +                        unixctl_command_reply(pending_pkt.conn, NULL);
> >> >                      }
> >> > +                } else {
> >> > +                    VLOG_DBG("Pending_pkt conn but br_int %p or chassis "
> >> > +                             "%p not ready. run-id: %"PRIu64, br_int,
> >> > +                             chassis, engine_run_id);
> >> > +                    unixctl_command_reply_error(pending_pkt.conn,
> >> > +                        "ovn-controller not ready.");
> >> >                  }
> >> >                  pending_pkt.conn = NULL;
> >> >                  free(pending_pkt.flow_s);
> >> > diff --git a/tests/ovn.at b/tests/ovn.at
> >> > index e88cffa..b85e549 100644
> >> > --- a/tests/ovn.at
> >> > +++ b/tests/ovn.at
> >> > @@ -3723,7 +3723,7 @@ sleep 1
> >> >  packet="inport==\"ls1-lp1\" && eth.src==$ls1_lp1_mac && eth.dst==$rp_ls1_mac &&
> >> >          ip4 && ip.ttl==64 && ip4.src==$ls1_lp1_ip && ip4.dst==$ls2_lp1_ip &&
> >> >          udp && udp.src==53 && udp.dst==4369"
> >> > -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
> >> > +AT_CHECK([as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"])
> >> >
> >> >
> >> >  echo "---------NB dump-----"
> >> > --
> >> > 2.1.0
> >> >
> >> > _______________________________________________
> >> > dev mailing list
> >> > dev@openvswitch.org
> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev