| Message ID | 20200806145139.881591-1-mmichels@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] Allow bare ct_commits when no nested actions are required. | expand |
Bleep bloop. Greetings Mark Michelson, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: sha1 information is lacking or useless (lib/actions.c). error: could not build fake ancestor hint: Use 'git am --show-current-patch' to see the failed patch Patch failed at 0001 Allow bare ct_commits when no nested actions are required. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Thu, Aug 6, 2020 at 8:22 PM Mark Michelson <mmichels@redhat.com> wrote: > In the fixes commit below, ct_commit was changed to use nested actions. > This requires that curly braces be present for all ct_commits. When > adjusting ovn-northd, some ct_commits were not updated to have them. > This commit changes the behavior of the ct_commit action not to require > curly braces if there are no nested actions required. > > Fixes: 6cfb44a76c61("Used nested actions in ct_commit") > Signed-off-by: Mark Michelson <mmichels@redhat.com> > Thanks for the fix. Acked-by: Numan Siddique <numans@ovn.org> The system test case - 29: ovn --Test packet drops due to incorrect flows in physical table 33 FAILED (system-ovn.at:4538) is failing on my setup. But this patch is not the issue. I see failure with the master too. Just FYI. Thanks Numan > --- > lib/actions.c | 20 ++++++++++++++++---- > tests/ovn.at | 5 ++++- > 2 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/lib/actions.c b/lib/actions.c > index 05fa44b60..4afc23d66 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -630,15 +630,27 @@ ovnact_ct_next_free(struct ovnact_ct_next *a > OVS_UNUSED) > static void > parse_CT_COMMIT(struct action_context *ctx) > { > - > - parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip", > - WR_CT_COMMIT); > + if (ctx->lexer->token.type == LEX_T_LCURLY) { > + parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip", > + WR_CT_COMMIT); > + } else { > + /* Add an empty nested action to allow for "ct_commit;" syntax */ > + add_prerequisite(ctx, "ip"); > + struct ovnact_nest *on = ovnact_put(ctx->ovnacts, > OVNACT_CT_COMMIT, > + OVNACT_ALIGN(sizeof *on)); > + on->nested_len = 0; > + on->nested = NULL; > + } > } > > static void > format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s) > { > - format_nested_action(on, "ct_commit", s); > + if (on->nested_len) { > + format_nested_action(on, "ct_commit", s); > + } else { > + ds_put_cstr(s, "ct_commit;"); > + } > } > > static void > diff --git a/tests/ovn.at b/tests/ovn.at > index b0179a8db..7236eeb8e 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1050,8 +1050,11 @@ ct_next; > has prereqs ip > > # ct_commit > +ct_commit; > + encodes as ct(commit,zone=NXM_NX_REG13[0..15]) > + has prereqs ip > ct_commit { }; > - formats as ct_commit { drop; }; > + formats as ct_commit; > encodes as ct(commit,zone=NXM_NX_REG13[0..15]) > has prereqs ip > ct_commit { ct_mark=1; }; > -- > 2.25.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 8/6/20 12:16 PM, Numan Siddique wrote: > > > On Thu, Aug 6, 2020 at 8:22 PM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> wrote: > > In the fixes commit below, ct_commit was changed to use nested actions. > This requires that curly braces be present for all ct_commits. When > adjusting ovn-northd, some ct_commits were not updated to have them. > This commit changes the behavior of the ct_commit action not to require > curly braces if there are no nested actions required. > > Fixes: 6cfb44a76c61("Used nested actions in ct_commit") > Signed-off-by: Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> > > > Thanks for the fix. > Acked-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > The system test case - 29: ovn --Test packet drops due to incorrect > flows in physical table 33 FAILED (system-ovn.at:4538 > <http://system-ovn.at:4538>) is failing > on my setup. But this patch is not the issue. I see failure with the > master too. Just FYI. OK. Just to throw out another data point, that test passes on my setup. > > Thanks > Numan > > --- > lib/actions.c | 20 ++++++++++++++++---- > tests/ovn.at <http://ovn.at> | 5 ++++- > 2 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/lib/actions.c b/lib/actions.c > index 05fa44b60..4afc23d66 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -630,15 +630,27 @@ ovnact_ct_next_free(struct ovnact_ct_next *a > OVS_UNUSED) > static void > parse_CT_COMMIT(struct action_context *ctx) > { > - > - parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip", > - WR_CT_COMMIT); > + if (ctx->lexer->token.type == LEX_T_LCURLY) { > + parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip", > + WR_CT_COMMIT); > + } else { > + /* Add an empty nested action to allow for "ct_commit;" > syntax */ > + add_prerequisite(ctx, "ip"); > + struct ovnact_nest *on = ovnact_put(ctx->ovnacts, > OVNACT_CT_COMMIT, > + OVNACT_ALIGN(sizeof *on)); > + on->nested_len = 0; > + on->nested = NULL; > + } > } > > static void > format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s) > { > - format_nested_action(on, "ct_commit", s); > + if (on->nested_len) { > + format_nested_action(on, "ct_commit", s); > + } else { > + ds_put_cstr(s, "ct_commit;"); > + } > } > > static void > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at> > index b0179a8db..7236eeb8e 100644 > --- a/tests/ovn.at <http://ovn.at> > +++ b/tests/ovn.at <http://ovn.at> > @@ -1050,8 +1050,11 @@ ct_next; > has prereqs ip > > # ct_commit > +ct_commit; > + encodes as ct(commit,zone=NXM_NX_REG13[0..15]) > + has prereqs ip > ct_commit { }; > - formats as ct_commit { drop; }; > + formats as ct_commit; > encodes as ct(commit,zone=NXM_NX_REG13[0..15]) > has prereqs ip > ct_commit { ct_mark=1; }; > -- > 2.25.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, Aug 6, 2020 at 11:03 PM Mark Michelson <mmichels@redhat.com> wrote: > > On 8/6/20 12:16 PM, Numan Siddique wrote: > > > > > > On Thu, Aug 6, 2020 at 8:22 PM Mark Michelson <mmichels@redhat.com > > <mailto:mmichels@redhat.com>> wrote: > > > > In the fixes commit below, ct_commit was changed to use nested actions. > > This requires that curly braces be present for all ct_commits. When > > adjusting ovn-northd, some ct_commits were not updated to have them. > > This commit changes the behavior of the ct_commit action not to require > > curly braces if there are no nested actions required. > > > > Fixes: 6cfb44a76c61("Used nested actions in ct_commit") > > Signed-off-by: Mark Michelson <mmichels@redhat.com > > <mailto:mmichels@redhat.com>> > > > > > > Thanks for the fix. > > Acked-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > > > The system test case - 29: ovn --Test packet drops due to incorrect > > flows in physical table 33 FAILED (system-ovn.at:4538 > > <http://system-ovn.at:4538>) is failing > > on my setup. But this patch is not the issue. I see failure with the > > master too. Just FYI. > > OK. Just to throw out another data point, that test passes on my setup. It's a timing issue and adding the below before the check fixes the issue for me and tests passes all the time when I run in a loop. OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1-f) = xup]) OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2-f) = xup]) OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xup]) I'll submit the patch to fix it later. Thanks Numan > > > > > > Thanks > > Numan > > > > --- > > lib/actions.c | 20 ++++++++++++++++---- > > tests/ovn.at <http://ovn.at> | 5 ++++- > > 2 files changed, 20 insertions(+), 5 deletions(-) > > > > diff --git a/lib/actions.c b/lib/actions.c > > index 05fa44b60..4afc23d66 100644 > > --- a/lib/actions.c > > +++ b/lib/actions.c > > @@ -630,15 +630,27 @@ ovnact_ct_next_free(struct ovnact_ct_next *a > > OVS_UNUSED) > > static void > > parse_CT_COMMIT(struct action_context *ctx) > > { > > - > > - parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip", > > - WR_CT_COMMIT); > > + if (ctx->lexer->token.type == LEX_T_LCURLY) { > > + parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip", > > + WR_CT_COMMIT); > > + } else { > > + /* Add an empty nested action to allow for "ct_commit;" > > syntax */ > > + add_prerequisite(ctx, "ip"); > > + struct ovnact_nest *on = ovnact_put(ctx->ovnacts, > > OVNACT_CT_COMMIT, > > + OVNACT_ALIGN(sizeof *on)); > > + on->nested_len = 0; > > + on->nested = NULL; > > + } > > } > > > > static void > > format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s) > > { > > - format_nested_action(on, "ct_commit", s); > > + if (on->nested_len) { > > + format_nested_action(on, "ct_commit", s); > > + } else { > > + ds_put_cstr(s, "ct_commit;"); > > + } > > } > > > > static void > > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at> > > index b0179a8db..7236eeb8e 100644 > > --- a/tests/ovn.at <http://ovn.at> > > +++ b/tests/ovn.at <http://ovn.at> > > @@ -1050,8 +1050,11 @@ ct_next; > > has prereqs ip > > > > # ct_commit > > +ct_commit; > > + encodes as ct(commit,zone=NXM_NX_REG13[0..15]) > > + has prereqs ip > > ct_commit { }; > > - formats as ct_commit { drop; }; > > + formats as ct_commit; > > encodes as ct(commit,zone=NXM_NX_REG13[0..15]) > > has prereqs ip > > ct_commit { ct_mark=1; }; > > -- > > 2.25.4 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto: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 8/6/20 1:47 PM, Numan Siddique wrote: > On Thu, Aug 6, 2020 at 11:03 PM Mark Michelson <mmichels@redhat.com> wrote: >> >> On 8/6/20 12:16 PM, Numan Siddique wrote: >>> >>> >>> On Thu, Aug 6, 2020 at 8:22 PM Mark Michelson <mmichels@redhat.com >>> <mailto:mmichels@redhat.com>> wrote: >>> >>> In the fixes commit below, ct_commit was changed to use nested actions. >>> This requires that curly braces be present for all ct_commits. When >>> adjusting ovn-northd, some ct_commits were not updated to have them. >>> This commit changes the behavior of the ct_commit action not to require >>> curly braces if there are no nested actions required. >>> >>> Fixes: 6cfb44a76c61("Used nested actions in ct_commit") >>> Signed-off-by: Mark Michelson <mmichels@redhat.com >>> <mailto:mmichels@redhat.com>> >>> >>> >>> Thanks for the fix. >>> Acked-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> >>> >>> The system test case - 29: ovn --Test packet drops due to incorrect >>> flows in physical table 33 FAILED (system-ovn.at:4538 >>> <http://system-ovn.at:4538>) is failing >>> on my setup. But this patch is not the issue. I see failure with the >>> master too. Just FYI. >> >> OK. Just to throw out another data point, that test passes on my setup. > > > It's a timing issue and adding the below before the check fixes the > issue for me and tests passes all the > time when I run in a loop. > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1-f) = xup]) > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2-f) = xup]) > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xup]) > > I'll submit the patch to fix it later. > > Thanks > Numan OK, cool. In the meantime, I pushed this patch to master. > >> >> >>> >>> Thanks >>> Numan >>> >>> --- >>> lib/actions.c | 20 ++++++++++++++++---- >>> tests/ovn.at <http://ovn.at> | 5 ++++- >>> 2 files changed, 20 insertions(+), 5 deletions(-) >>> >>> diff --git a/lib/actions.c b/lib/actions.c >>> index 05fa44b60..4afc23d66 100644 >>> --- a/lib/actions.c >>> +++ b/lib/actions.c >>> @@ -630,15 +630,27 @@ ovnact_ct_next_free(struct ovnact_ct_next *a >>> OVS_UNUSED) >>> static void >>> parse_CT_COMMIT(struct action_context *ctx) >>> { >>> - >>> - parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip", >>> - WR_CT_COMMIT); >>> + if (ctx->lexer->token.type == LEX_T_LCURLY) { >>> + parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip", >>> + WR_CT_COMMIT); >>> + } else { >>> + /* Add an empty nested action to allow for "ct_commit;" >>> syntax */ >>> + add_prerequisite(ctx, "ip"); >>> + struct ovnact_nest *on = ovnact_put(ctx->ovnacts, >>> OVNACT_CT_COMMIT, >>> + OVNACT_ALIGN(sizeof *on)); >>> + on->nested_len = 0; >>> + on->nested = NULL; >>> + } >>> } >>> >>> static void >>> format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s) >>> { >>> - format_nested_action(on, "ct_commit", s); >>> + if (on->nested_len) { >>> + format_nested_action(on, "ct_commit", s); >>> + } else { >>> + ds_put_cstr(s, "ct_commit;"); >>> + } >>> } >>> >>> static void >>> diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at> >>> index b0179a8db..7236eeb8e 100644 >>> --- a/tests/ovn.at <http://ovn.at> >>> +++ b/tests/ovn.at <http://ovn.at> >>> @@ -1050,8 +1050,11 @@ ct_next; >>> has prereqs ip >>> >>> # ct_commit >>> +ct_commit; >>> + encodes as ct(commit,zone=NXM_NX_REG13[0..15]) >>> + has prereqs ip >>> ct_commit { }; >>> - formats as ct_commit { drop; }; >>> + formats as ct_commit; >>> encodes as ct(commit,zone=NXM_NX_REG13[0..15]) >>> has prereqs ip >>> ct_commit { ct_mark=1; }; >>> -- >>> 2.25.4 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org <mailto: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 >
diff --git a/lib/actions.c b/lib/actions.c index 05fa44b60..4afc23d66 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -630,15 +630,27 @@ ovnact_ct_next_free(struct ovnact_ct_next *a OVS_UNUSED) static void parse_CT_COMMIT(struct action_context *ctx) { - - parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip", - WR_CT_COMMIT); + if (ctx->lexer->token.type == LEX_T_LCURLY) { + parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip", + WR_CT_COMMIT); + } else { + /* Add an empty nested action to allow for "ct_commit;" syntax */ + add_prerequisite(ctx, "ip"); + struct ovnact_nest *on = ovnact_put(ctx->ovnacts, OVNACT_CT_COMMIT, + OVNACT_ALIGN(sizeof *on)); + on->nested_len = 0; + on->nested = NULL; + } } static void format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s) { - format_nested_action(on, "ct_commit", s); + if (on->nested_len) { + format_nested_action(on, "ct_commit", s); + } else { + ds_put_cstr(s, "ct_commit;"); + } } static void diff --git a/tests/ovn.at b/tests/ovn.at index b0179a8db..7236eeb8e 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1050,8 +1050,11 @@ ct_next; has prereqs ip # ct_commit +ct_commit; + encodes as ct(commit,zone=NXM_NX_REG13[0..15]) + has prereqs ip ct_commit { }; - formats as ct_commit { drop; }; + formats as ct_commit; encodes as ct(commit,zone=NXM_NX_REG13[0..15]) has prereqs ip ct_commit { ct_mark=1; };
In the fixes commit below, ct_commit was changed to use nested actions. This requires that curly braces be present for all ct_commits. When adjusting ovn-northd, some ct_commits were not updated to have them. This commit changes the behavior of the ct_commit action not to require curly braces if there are no nested actions required. Fixes: 6cfb44a76c61("Used nested actions in ct_commit") Signed-off-by: Mark Michelson <mmichels@redhat.com> --- lib/actions.c | 20 ++++++++++++++++---- tests/ovn.at | 5 ++++- 2 files changed, 20 insertions(+), 5 deletions(-)