On Mon, Nov 11, 2019 at 8:02 PM Mark Michelson <[email protected]> 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. 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 <[email protected]> > > --- > > 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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
