On Tue, Nov 12, 2019 at 5:21 PM Dumitru Ceara <dce...@redhat.com> wrote: > > On Mon, Nov 11, 2019 at 8:02 PM Mark Michelson <mmich...@redhat.com> wrote: > > > > Hi Dumitru. Everything you've done looks good to me. > > > > However, when reviewing the patch, I educated myself on how ovn-detrace > > is implemented and I found a couple of problems. These problems aren't > > introduced by you. However, I think that since you have broadened the > > scope of the southbound database search area, they should probably be > > addressed. > > Hi Mark, > > Thanks for looking into this. > > I addressed your remarks and sent a v2: > https://patchwork.ozlabs.org/project/openvswitch/list/?series=142383 > > I also refactored a bit more the printing so we don't have to > explicitly specify the 'prefix' every time.
Sorry for the noise.. I had forgotten a stray "%s" in v2. Fixed in v3: https://patchwork.ozlabs.org/project/openvswitch/list/?series=142396 Thanks, Dumitru > > Thanks, > Dumitru > > > > > On 11/8/19 6:15 AM, Dumitru Ceara wrote: > > > Commit eb25a7da639e ("Improve debuggability of OVN to OpenFlow > > > translations.") added cookies for Port_Binding, Mac_Binding, > > > Multicast_Group, Chassis records to the OpenfFlow entries they > > > generate. > > > > > > Add support for parsing such cookies in ovn-detrace too. > > > Also, refactor ovn-detrace to allow a more generic way of defining > > > cookie handlers. > > > > > > Signed-off-by: Dumitru Ceara <dce...@redhat.com> > > > --- > > > utilities/ovn-detrace.in | 166 > > > ++++++++++++++++++++++++++++++++-------------- > > > 1 file changed, 117 insertions(+), 49 deletions(-) > > > > > > diff --git a/utilities/ovn-detrace.in b/utilities/ovn-detrace.in > > > index 34b6b0e..79c6d3b 100755 > > > --- a/utilities/ovn-detrace.in > > > +++ b/utilities/ovn-detrace.in > > > @@ -98,48 +98,112 @@ class OVSDB(object): > > > def find_row_by_partial_uuid(self, table_name, value): > > > return self._find_row(table_name, lambda row: value in > > > str(row.uuid)) > > > > (Note: this problem existed prior to the patch series) > > > > Since the cookie corresponds to the beginning of the southbound record's > > UUID, this lambda should probably be altered to: > > > > lambda row: str(row.uuid).startswith(value) > > > > The existing lambda could match the cookie with a later part of the > > UUID, returning an invalid match. > > > > > > > > - > > > -def get_lflow_from_cookie(ovnsb_db, cookie): > > > - return ovnsb_db.find_row_by_partial_uuid('Logical_Flow', cookie) > > > - > > > - > > > -def print_lflow(lflow, prefix): > > > - ldp_uuid = lflow.logical_datapath.uuid > > > - ldp_name = str(lflow.logical_datapath.external_ids.get('name')) > > > - > > > - print '%sLogical datapath: "%s" (%s) [%s]' % (prefix, > > > - ldp_name, > > > - ldp_uuid, > > > - lflow.pipeline) > > > - print "%sLogical flow: table=%s (%s), priority=%s, " \ > > > - "match=(%s), actions=(%s)" % (prefix, > > > - lflow.table_id, > > > - > > > lflow.external_ids.get('stage-name'), > > > - lflow.priority, > > > - str(lflow.match).strip('"'), > > > - str(lflow.actions).strip('"')) > > > - > > > - > > > -def print_lflow_nb_hint(lflow, prefix, ovnnb_db): > > > - external_ids = lflow.external_ids > > > - if external_ids.get('stage-name') in ['ls_in_acl', > > > - 'ls_out_acl']: > > > - acl_hint = external_ids.get('stage-hint') > > > - if not acl_hint: > > > - return > > > - acl = ovnnb_db.find_row_by_partial_uuid('ACL', acl_hint) > > > - if not acl: > > > +class CookieHandler(object): > > > + def __init__(self, db, table): > > > + self._db = db > > > + self._table = table > > > + > > > + def get_record(self, cookie): > > > + return self._db.find_row_by_partial_uuid(self._table, cookie) > > > + > > > + def print_record(self, record, prefix): > > > + pass > > > + > > > + def print_hint(self, record, prefix, db): > > > + pass > > > + > > > +def datapath_str(datapath): > > > + return '"%s" (%s)' % (str(datapath.external_ids.get('name')), > > > + datapath.uuid) > > > + > > > +def chassis_str(chassis): > > > + if len(chassis) == 0: > > > + return '' > > > + ch = chassis[0] > > > + return 'chassis-name "%s", chassis-str "%s"' % (ch.name, ch.hostname) > > > + > > > +class LogicalFlowHandler(CookieHandler): > > > + def __init__(self, ovnsb_db): > > > + super(LogicalFlowHandler, self).__init__(ovnsb_db, > > > 'Logical_Flow') > > > + > > > + def print_record(self, lflow, prefix): > > > + print('%sLogical datapath: %s [%s]' % > > > + (prefix, datapath_str(lflow.logical_datapath), > > > lflow.pipeline)) > > > + print('%sLogical flow: table=%s (%s), priority=%s, ' > > > + 'match=(%s), actions=(%s)' % > > > + (prefix, lflow.table_id, > > > lflow.external_ids.get('stage-name'), > > > + lflow.priority, > > > + str(lflow.match).strip('"'), > > > + str(lflow.actions).strip('"'))) > > > + > > > + def print_hint(self, lflow, prefix, ovnnb_db): > > > + external_ids = lflow.external_ids > > > + if external_ids.get('stage-name') in ['ls_in_acl', > > > + 'ls_out_acl']: > > > + acl_hint = external_ids.get('stage-hint') > > > + if not acl_hint: > > > + return > > > + acl = ovnnb_db.find_row_by_partial_uuid('ACL', acl_hint) > > > + if not acl: > > > + return > > > + output = '%sACL: %s, priority=%s, ' \ > > > + 'match=(%s), %s' % (prefix, > > > + acl.direction, > > > + acl.priority, > > > + acl.match.strip('"'), > > > + acl.action) > > > + if acl.log: > > > + output += ' (log)' > > > + print(output) > > > + > > > +class PortBindingHandler(CookieHandler): > > > + def __init__(self, ovnsb_db): > > > + super(PortBindingHandler, self).__init__(ovnsb_db, > > > 'Port_Binding') > > > + > > > + def print_record(self, pb, prefix): > > > + ldp_uuid = pb.datapath.uuid > > > + ldp_name = str(pb.datapath.external_ids.get('name')) > > > + > > > + print('%sLogical datapath: %s' % (prefix, > > > datapath_str(pb.datapath))) > > > + print('%sPort Binding: logical_port "%s", tunnel_key %ld, %s' % > > > + (prefix, pb.logical_port, pb.tunnel_key, > > > + chassis_str(pb.chassis))) > > > + > > > +class MacBindingHandler(CookieHandler): > > > + def __init__(self, ovnsb_db): > > > + super(MacBindingHandler, self).__init__(ovnsb_db, 'MAC_Binding') > > > + > > > + def print_record(self, mb, prefix): > > > + print('%sLogical datapath: %s' % (prefix, > > > datapath_str(mb.datapath))) > > > + print('%sMAC Binding: ip "%s", logical_port "%s", mac "%s"' % > > > + (prefix, mb.ip, mb.logical_port, mb.mac)) > > > + > > > +class MulticastGroupHandler(CookieHandler): > > > + def __init__(self, ovnsb_db): > > > + super(MulticastGroupHandler, self).__init__(ovnsb_db, > > > + 'Multicast_Group') > > > + > > > + def print_record(self, mc, prefix): > > > + mc_ports = ', '.join([pb.logical_port for pb in mc.ports]) > > > + > > > + print('%sLogical datapath: %s' % (prefix, > > > datapath_str(mc.datapath))) > > > + print('%sMulticast Group: name "%s", tunnel_key %ld ports: (%s)' > > > % > > > + (prefix, mc.name, mc.tunnel_key, mc_ports)) > > > + > > > +class ChassisHandler(CookieHandler): > > > + def __init__(self, ovnsb_db): > > > + super(ChassisHandler, self).__init__(ovnsb_db, 'Chassis') > > > + > > > + def print_record(self, chassis, prefix): > > > + print('%sChassis: %s' % (prefix, chassis_str([chassis]))) > > > + > > > +def print_sb_record_from_cookie(ovnnb_db, ovnsb_db, cookie_handlers, > > > cookie): > > > + for handler in cookie_handlers: > > > + sb_record = handler.get_record(cookie) > > > > (Note: this problem also existed before this patch series) > > > > handler.get_record() uses a generator expression to retrieve one of > > several potential matches for the cookie. If there are multiple partial > > matches in the database, then each subsequent call to > > handler.get_record() will return the next row with the same partial match. > > > > Perhaps it is a good idea to call handler.get_record() until it returns > > None each time. This way, if there is potential ambiguity we can at > > least present to the user that they're hitting one of several possible > > matching southbound database entries. > > > > > + if sb_record: > > > + handler.print_record(sb_record, " * ") > > > + handler.print_hint(sb_record, " * ", ovnnb_db) > > > return > > > - output = "%sACL: %s, priority=%s, " \ > > > - "match=(%s), %s" % (prefix, > > > - acl.direction, > > > - acl.priority, > > > - acl.match.strip('"'), > > > - acl.action) > > > - if acl.log: > > > - output += ' (log)' > > > - print output > > > - > > > > > > def main(): > > > try: > > > @@ -183,6 +247,14 @@ def main(): > > > ovsdb_ovnsb = OVSDB(ovnsb_db, 'OVN_Southbound') > > > ovsdb_ovnnb = OVSDB(ovnnb_db, 'OVN_Northbound') > > > > > > + cookie_handlers = [ > > > + LogicalFlowHandler(ovsdb_ovnsb), > > > + PortBindingHandler(ovsdb_ovnsb), > > > + MacBindingHandler(ovsdb_ovnsb), > > > + MulticastGroupHandler(ovsdb_ovnsb), > > > + ChassisHandler(ovsdb_ovnsb) > > > + ] > > > + > > > regex_cookie = re.compile(r'^.*cookie 0x([0-9a-fA-F]+)') > > > regex_table_id = re.compile(r'^[0-9]+\.') > > > cookie = None > > > @@ -194,14 +266,10 @@ def main(): > > > > > > line = line.strip() > > > > > > - if cookie: > > > - # print lflow info when the current flow block ends > > > - if regex_table_id.match(line): > > > - lflow = get_lflow_from_cookie(ovsdb_ovnsb, cookie) > > > - if lflow: > > > - print_lflow(lflow, " * ") > > > - print_lflow_nb_hint(lflow, " * ", ovsdb_ovnnb) > > > - cookie = None > > > + # Print SB record info when the current flow block ends. > > > + if cookie and regex_table_id.match(line): > > > + print_sb_record_from_cookie(ovsdb_ovnnb, ovsdb_ovnsb, > > > + cookie_handlers, cookie) > > > > > > print line > > > > > > > > > _______________________________________________ > > > 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