[ovs-dev] [PATCH ovn 3/3] ovn-detrace: Add support for other types of SB cookies.

Dumitru Ceara dceara at redhat.com
Tue Nov 12 16:50:36 UTC 2019


On Tue, Nov 12, 2019 at 5:21 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On Mon, Nov 11, 2019 at 8:02 PM Mark Michelson <mmichels at 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 <dceara at 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
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >



More information about the dev mailing list