Thanks Han for reviewing the diff. I have addressed your review comments and uploaded the new diff.
Kind Regards, Selva On 23-Feb-2022, at 8:16 AM, Han Zhou <[email protected]<mailto:[email protected]>> wrote: Hi Selva, Thanks for the patch. Please see my comments below. On Tue, Feb 8, 2022 at 9:18 AM Selvaraj Palaniyappan <[email protected]<mailto:[email protected]>> wrote: > > Hello Mark, > > Thanks for reviewing the diff. Please find the background info related to > this patch. > > "The ML2 Plugin can add some useful info to NB database of Logical router > port(LRP) table's external-ids that can be propagated to SB Port_binding > table. Some module on compute node can consume these external-ids from > Port_binding entries. The useful info could be subnet type on LRP and other > data.” Please keep in mind that any separate connections to SB DB could add pressure to the SB DB at scale. With this considered, I am ok with this patch, since we are doing the same for LSPs already. > > Regarding subject line, I have used "git send-mail” with format patch o/p to > upload the patch. Complete description has been taken from format patch o/p. > If needed, let me send the new patch. Let me know your input. > Not sure what's wrong, but you could run the format-patch command first and check if it generates the patch file properly. (you can also send email by git send-email followed by the file name) Please see a minor comment on the test case. > Thanks, > Selva > > On 05-Feb-2022, at 12:38 AM, Mark Michelson > <[email protected]<mailto:[email protected]><mailto:[email protected]<mailto:[email protected]>>> > wrote: > > Hello, > > I had a look, and while the code does what it claims, it's not clear why you > want to do this. Can you provide some background? > > Also, it appears in this version of the patch, you put the entire description > in the subject line :) > > Thanks > > On 1/27/22 07:50, Selvaraj Palaniyappan wrote: > Signed-off-by: Selvaraj Palaniyappan > <[email protected]<mailto:[email protected]><mailto:[email protected]<mailto:[email protected]>>> > --- > northd/northd.c | 1 + > ovn-nb.xml | 6 ++++++ > ovn-sb.xml | 3 ++- > tests/ovn-northd.at > [ovn-northd.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn-2Dnorthd.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=gIgsMKu2DWUEq3SxefjOJ7t5jlMAOBbL8nYw2MkwHDc&e=><http://ovn-northd.at > > [ovn-northd.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn-2Dnorthd.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=gIgsMKu2DWUEq3SxefjOJ7t5jlMAOBbL8nYw2MkwHDc&e=>> > | 14 ++++++++++++++ > 4 files changed, 23 insertions(+), 1 deletion(-) > diff --git a/northd/northd.c b/northd/northd.c > index fc7a64f99..090922ae2 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3240,6 +3240,7 @@ ovn_port_update_sbrec(struct northd_input *input_data, > ds_destroy(&s); > struct smap ids = SMAP_INITIALIZER(&ids); > + smap_clone(&ids, &op->nbrp->external_ids); > sbrec_port_binding_set_external_ids(op->sb, &ids); > sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 6a6972856..293d25b32 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -2895,6 +2895,12 @@ > <group title="Common Columns"> > <column name="external_ids"> > See <em>External IDs</em> at the beginning of this document. > + <p> > + The <code>ovn-northd</code> program copies all these pairs into the > + <ref column="external_ids"/> column of the > + <ref table="Port_Binding"/> table in <ref db="OVN_Southbound"/> > + database. > + </p> > </column> > </group> > </table> > diff --git a/ovn-sb.xml b/ovn-sb.xml > index 9ddacdf09..f7c41ccdc 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -3354,7 +3354,8 @@ tcp.flags = RST; > <p> > The <code>ovn-northd</code> program populates this column with > all entries into the <ref column="external_ids"/> column of the > - <ref table="Logical_Switch_Port"/> table of the > + <ref table="Logical_Switch_Port"/> and > + <ref table="Logical_Router_Port"/> tables of the > <ref db="OVN_Northbound"/> database. > </p> > </column> > diff --git a/tests/ovn-northd.at > [ovn-northd.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn-2Dnorthd.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=gIgsMKu2DWUEq3SxefjOJ7t5jlMAOBbL8nYw2MkwHDc&e=><http://ovn-northd.at > > [ovn-northd.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn-2Dnorthd.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=gIgsMKu2DWUEq3SxefjOJ7t5jlMAOBbL8nYw2MkwHDc&e=>> > b/tests/ovn-northd.at > [ovn-northd.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn-2Dnorthd.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=gIgsMKu2DWUEq3SxefjOJ7t5jlMAOBbL8nYw2MkwHDc&e=><http://ovn-northd.at > > [ovn-northd.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn-2Dnorthd.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=gIgsMKu2DWUEq3SxefjOJ7t5jlMAOBbL8nYw2MkwHDc&e=>> > index 84e52e701..f9c5259f1 100644 > --- a/tests/ovn-northd.at > [ovn-northd.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn-2Dnorthd.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=gIgsMKu2DWUEq3SxefjOJ7t5jlMAOBbL8nYw2MkwHDc&e=><http://ovn-northd.at > > [ovn-northd.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn-2Dnorthd.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=gIgsMKu2DWUEq3SxefjOJ7t5jlMAOBbL8nYw2MkwHDc&e=>> > +++ b/tests/ovn-northd.at > [ovn-northd.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn-2Dnorthd.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=gIgsMKu2DWUEq3SxefjOJ7t5jlMAOBbL8nYw2MkwHDc&e=><http://ovn-northd.at > > [ovn-northd.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn-2Dnorthd.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=gIgsMKu2DWUEq3SxefjOJ7t5jlMAOBbL8nYw2MkwHDc&e=>> > @@ -144,6 +144,20 @@ AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup]) > AT_CLEANUP > ]) > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([check external id propagation to SBDB]) The title of the test case is too general, because all the tables have an external-ids column. Better to call out the "logical router port, or LRP". > +ovn_start > + > +ovn-nbctl lr-add ro > +ovn-nbctl lrp-add ro lrp0 00:00:00:00:00:01 192.168.1.1/24 > [192.168.1.1]<https://urldefense.proofpoint.com/v2/url?u=http-3A__192.168.1.1_24&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=zj5Nf8VkD1D8KQGU04RRZX0uZE-AKNSEzlpCBO8Wv48&e=> > +ovn-nbctl --wait=sb set logical_router_port lrp0 external_ids=test=123 > +AT_CHECK([ovn-sbctl --columns=external_ids --bare find Port_Binding > logical_port=lrp0], > +[0], [test=123 > +]) Just a hint, you can also use the check_column function which is slightly more concise: check_column "test=123" sb:Port_Binding external_ids logical_port=lrp0 With the minor comments addressed: Acked-by: Han Zhou <[email protected]<mailto:[email protected]>> Thanks, Han > + > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD([ > AT_SETUP([check IPv6 RA config propagation to SBDB]) > ovn_start > > > _______________________________________________ > dev mailing list > [email protected]<mailto:[email protected]> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > [mail.openvswitch.org]<https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=s-LBGxFeMigvGGdbcJ8ZlFHWTAGRMXX30AvpXtJVEZs&m=yYOIUFTjMzJXPByd-7KigARqaZnpV5ILO5YH7SWSHNAFMqWPPfKivXYy2Bx1A7gi&s=KNcc0e49zdQ047hRaUmQKefK3N0-aZJCEVPEfs1mgvY&e=> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
