Hi Selva, Thanks for the patch. Please see my comments below.
On Tue, Feb 8, 2022 at 9:18 AM Selvaraj Palaniyappan <selvara...@nutanix.com> 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 <mmich...@redhat.com<mailto: mmich...@redhat.com>> 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 <selvara...@nutanix.com<mailto: selvara...@nutanix.com>> > --- > northd/northd.c | 1 + > ovn-nb.xml | 6 ++++++ > ovn-sb.xml | 3 ++- > tests/ovn-northd.at<http://ovn-northd.at> | 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<http://ovn-northd.at> b/tests/ ovn-northd.at<http://ovn-northd.at> > index 84e52e701..f9c5259f1 100644 > --- a/tests/ovn-northd.at<http://ovn-northd.at> > +++ b/tests/ovn-northd.at<http://ovn-northd.at> > @@ -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 > +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 <hz...@ovn.org> Thanks, Han > + > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD([ > AT_SETUP([check IPv6 RA config propagation to SBDB]) > ovn_start > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
