On Thu, Aug 26, 2021 at 7:38 AM Han Zhou <[email protected]> wrote: > > > > On Thu, Aug 19, 2021 at 4:09 AM Frode Nordahl <[email protected]> > wrote: > > > > To allow for use of optional plugging support we add a new > > plugged_by column with weakRef to the Chassis table. The > > ovn-controller can monitor this column and only process events > > for its chassis UUID. > > > > ovn-northd will fill this column with UUID of Chassis referenced > > in Logical_Switch_Port options:requested-chassis when > > options:plug-type is defined. > > Thanks Frode. Please see my comments below.
Thank you for taking the time to review Han, much appreciated. > > > > Signed-off-by: Frode Nordahl <[email protected]> > > --- > > northd/ovn-northd.c | 31 ++++++++++++++++++++++++++++++ > > ovn-nb.xml | 38 ++++++++++++++++++++++++++++++++++++ > > ovn-sb.ovsschema | 10 +++++++--- > > ovn-sb.xml | 15 +++++++++++++++ > > tests/ovn-northd.at | 47 +++++++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 138 insertions(+), 3 deletions(-) > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 3d8e21a4f..a66f92ddd 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -3222,6 +3222,35 @@ ovn_port_update_sbrec(struct northd_context *ctx, > > * ha_chassis_group cleared in the same transaction. */ > > sbrec_port_binding_set_ha_chassis_group(op->sb, NULL); > > } > > + > > + const char *plug_type; /* May be NULL. */ > > + const char *requested_chassis; /* May be NULL. */ > > + bool reset_plugged_by = false; > > + plug_type = smap_get(&op->nbsp->options, "plug-type"); > > + requested_chassis = smap_get(&op->nbsp->options, > > + "requested-chassis"); > > + if (plug_type && requested_chassis) { > > + const struct sbrec_chassis *chassis; /* May be NULL. */ > > + chassis = chassis_lookup_by_name(sbrec_chassis_by_name, > > + requested_chassis); > > + if (chassis) { > > + sbrec_port_binding_set_plugged_by(op->sb, chassis); > > Why do we need a separate column for this while we already have the value in > the SB port-binding options? The challenge is that we want the ovn-controllers to do as little processing as possible, and only react to unbound ports that are destined for them. In the initial RFC [0] I did the lookup based on Port_Binding:options, but that proved to not be very effective as you have to monitor all unbound ports and you can't use an index (you would only get the records with exactly and only the option you look for, which is normally not the case). In deployments with lots of load balancers there will be a lot of unbound ports. I also think you suggested using a separate column in the review of the initial RFC ;) 0: https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ > > + } else { > > + reset_plugged_by = true; > > + static struct vlog_rate_limit rl = > > VLOG_RATE_LIMIT_INIT( > > + 1, 1); > > + VLOG_WARN_RL( > > + &rl, > > + "Unknown chassis '%s' set as " > > + "options:requested-chassis on LSP '%s'.", > > + requested_chassis, op->nbsp->name); > > Would it be normal that the chassis is not registered to SB when the NB > option is set? I would not think so, but it is a safeguard in the event the CMS puts an invalid value into the requested-chassis option. We need to make sure chassis != NULL and I thought it would be reasonable to log the fact if it ever happened. > > + } > > + } else if (op->sb->plugged_by) { > > + reset_plugged_by = true; > > + } > > + if (reset_plugged_by) { > > + sbrec_port_binding_set_plugged_by(op->sb, NULL); > > + } > > } else { > > const char *chassis = NULL; > > if (op->peer && op->peer->od && op->peer->od->nbr) { > > @@ -15066,6 +15095,8 @@ main(int argc, char *argv[]) > > add_column_noalert(ovnsb_idl_loop.idl, > > &sbrec_port_binding_col_nat_addresses); > > ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > &sbrec_port_binding_col_chassis); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > + &sbrec_port_binding_col_plugged_by); > > ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > &sbrec_port_binding_col_gateway_chassis); > > ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index 8a942b54c..2812a0d11 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -1020,6 +1020,44 @@ > > DHCP reply. > > </p> > > </column> > > + > > + <group title="VIF Plugging Options"> > > + <p> > > + These options apply to logical ports with > > + <ref column="options" key="requested-chassis"/> and > > Now that the "requested-chassis" is used for two different scenarios, it > would be better to describe both use cases at one place instead of two, for > the reader can easily miss one of these two sections when reading. Ack, I agree and will fix. > > + <ref column="options" key="plug-type"/> set. > > + </p> > > + > > + <column name="options" key="plug-type"> > > + If set, OVN will attempt to to perform plugging of this VIF. > > In > > + order to get this port plugged by the OVN controller, OVN must > > be > > + built with support for VIF plugging. The default behavior is > > for > > + the CMS to do the VIF plugging. > > + Supported values: representor > > + </column> > > + > > + <column name="options" key="plug-pf-mac"> > > + MAC address for identifying PF device. When > > + <ref column="options" key="plug-vf-num"/> is also set, this > > + option is used to identify PF to use as base to locate the > > correct > > + VF representor port. When > > + <ref column="options" key="plug-vf-num"/> is not set this > > + option is used to locate a PF representor port. > > + </column> > > + > > + <column name="options" key="plug-vf-num"> > > + Logical VF number relative to PF device specified in > > + <ref column="options" key="plug-pf-mac"/>. > > + </column> > > + > > + <column name="options" key="plug-mtu-request"> > > + Requested MTU for plugged interfaces. When set the OVN > > controller > > + will fill the <ref table="Interface" column="mtu_request"/> > > column > > + of the Open vSwitch database's > > + <ref table="Interface" db="vswitch"/> table. This in turn will > > + make OVS vswitchd update the MTU of the linked interface. > > + </column> > > For the above 3 options, they are used only for plug-type "representor", > right? So it is better to have a naming convention that for each type, the > options should have a prefix: "plug:<plug type>:<option name for the plug > type>", e.g. "plug:representor:pf-mac". The additional namespacing you are proposing does make sense to me, will add. Thx! -- Frode Nordahl > Thanks, > Han > > > + </group> > > </group> > > > > <group title="Virtual port Options"> > > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > > index e5ab41db9..4df326ff4 100644 > > --- a/ovn-sb.ovsschema > > +++ b/ovn-sb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Southbound", > > - "version": "20.20.0", > > - "cksum": "605270161 26670", > > + "version": "20.21.0", > > + "cksum": "888060012 26935", > > "tables": { > > "SB_Global": { > > "columns": { > > @@ -232,7 +232,11 @@ > > "external_ids": {"type": {"key": "string", > > "value": "string", > > "min": 0, > > - "max": "unlimited"}}}, > > + "max": "unlimited"}}, > > + "plugged_by": {"type": {"key": {"type": "uuid", > > + "refTable": "Chassis", > > + "refType": "weak"}, > > + "min": 0, "max": 1}}}, > > "indexes": [["datapath", "tunnel_key"], ["logical_port"]], > > "isRoot": true}, > > "MAC_Binding": { > > diff --git a/ovn-sb.xml b/ovn-sb.xml > > index 687555c47..d32e9b6f2 100644 > > --- a/ovn-sb.xml > > +++ b/ovn-sb.xml > > @@ -2983,6 +2983,21 @@ tcp.flags = RST; > > </dd> > > </dl> > > </column> > > + <column name="plugged_by"> > > + Chassis that should plug this port, this column must be a > > + <ref table="Chassis"/> record. This is populated by > > + <code>ovn-northd</code> when the <ref > > + table="Logical_Switch_Port" > > + column="options" > > + key="requested-chassis" > > + db="OVN_Northbound"/> and <ref > > + table="Logical_Switch_Port" > > + column="options" > > + key="plug-type" > > + db="OVN_Northbound"/> is defined. In order to get this port > > plugged by > > + the OVN controller, OVN must be built with support for VIF > > plugging. > > + The default behavior is for the CMS to do the VIF plugging. > > + </column> > > </group> > > > > <group title="Patch Options"> > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 93bb0e1da..5324f484b 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -5208,3 +5208,50 @@ AT_CHECK([grep lr_in_gw_redirect lrflows | grep > > cr-DR | sort], [0], [dnl > > > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([check requested-chassis and plug-type fills plugged_by col]) > > +AT_KEYWORDS([plugging]) > > +ovn_start NORTHD_TYPE > > + > > +# Add chassis ch1. > > +check ovn-sbctl chassis-add ch1 geneve 127.0.0.2 > > +check ovn-sbctl chassis-add ch2 geneve 127.0.0.3 > > +check ovn-sbctl chassis-add ch3 geneve 127.0.0.4 > > + > > +wait_row_count Chassis 3 > > + > > +ch1_uuid=`ovn-sbctl --bare --columns _uuid find Chassis name="ch1"` > > +ch2_uuid=`ovn-sbctl --bare --columns _uuid find Chassis name="ch2"` > > +ch3_uuid=`ovn-sbctl --bare --columns _uuid find Chassis name="ch3"` > > + > > +ovn-nbctl ls-add S1 > > +ovn-nbctl --wait=sb lsp-add S1 S1-vm1 > > + > > +ovn-nbctl --wait=sb set logical_switch_port S1-vm1 \ > > + options:requested-chassis=ch1 > > + > > +wait_row_count Port_Binding 1 logical_port=S1-vm1 plugged_by!=$ch1_uuid > > + > > +ovn-nbctl --wait=sb set logical_switch_port S1-vm1 \ > > + options:plug-type=representor > > + > > +wait_row_count Port_Binding 1 logical_port=S1-vm1 plugged_by=$ch1_uuid > > + > > +ovn-nbctl --wait=sb set logical_switch_port S1-vm1 \ > > + options:requested-chassis=ch2 options:plug-type=representor > > + > > +wait_row_count Port_binding 1 logical-port=S1-vm1 plugged_by=$ch2_uuid > > + > > +ovn-nbctl --wait=sb set logical_switch_port S1-vm1 \ > > + options:requested-chassis=ch3 options:plug-type=representor > > + > > +wait_row_count Port_binding 1 logical-port=S1-vm1 plugged_by=$ch3_uuid > > + > > +ovn-nbctl --wait=sb remove logical_switch_port S1-vm1 \ > > + options plug-type=representor > > + > > +wait_row_count Port_binding 1 logical-port=S1-vm1 plugged_by!=$ch3_uuid > > + > > +AT_CLEANUP > > +]) > > -- > > 2.32.0 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
