[ovs-dev,ovn,3/3] ovn-detrace: Add support for other types of SB cookies.
diff mbox series

Message ID 20191108111537.10100.38432.stgit@dceara.remote.csb
State Superseded
Headers show
Series
  • Improve ovn-detrace support for parsing OpenFlow cookies.
Related show

Commit Message

Dumitru Ceara Nov. 8, 2019, 11:15 a.m. UTC
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@redhat.com>
---
 utilities/ovn-detrace.in |  166 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 117 insertions(+), 49 deletions(-)

Comments

Mark Michelson Nov. 11, 2019, 7:02 p.m. UTC | #1
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.

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@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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Dumitru Ceara Nov. 12, 2019, 4:21 p.m. UTC | #2
On Mon, Nov 11, 2019 at 8:02 PM Mark Michelson <mmichels@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.

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

Patch
diff mbox series

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))
 
-
-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)
+        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