[ovs-dev] [PATCH monitor_cond V2 11/12] python: move Python idl to work with monitor_cond

Russell Bryant russell at ovn.org
Mon Jan 25 20:06:35 UTC 2016


On 01/16/2016 03:16 AM, Liran Schour wrote:
> Python idl works now with "monitor_cond" method. Add test
> for backward compatibility with old "monitor" method.
> 
> Signed-off-by: Liran Schour <lirans at il.ibm.com>

As requested by Andy Zhou, this review is focused on Python specific
issues, and not necessarily a review to make sure it matches the
implementation on the server side.

> diff --git a/python/ovs/db/data.py b/python/ovs/db/data.py
> index 6baff38..0d382e1 100644
> --- a/python/ovs/db/data.py
> +++ b/python/ovs/db/data.py
> @@ -386,6 +386,18 @@ class Datum(object):
>              s.append(tail)
>          return ''.join(s)
>  
> +    def diff(self, datum):
> +        if self.type.n_max > 1 or len(self.values) == 0:
> +            for k, v in datum.values.iteritems():

I'm in the middle of submitting a conversion of this library to be
compatible with Python 3.  One of the patches I just merged was to make
dict iteration Python 3 compatible.  On this line, you should now use:

    for k, v in six.iteritems(datum.values):

> +                if k in self.values and v == self.values[k]:
> +                    del self.values[k]
> +                else:
> +                    self.values[k] = v
> +        else:
> +            return datum
> +
> +        return self
> +
>      def as_list(self):
>          if self.type.is_map():
>              return [[k.value, v.value] for k, v in self.values.iteritems()]
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index 17ed15b..e95f42e 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -30,6 +30,9 @@ ROW_CREATE = "create"
>  ROW_UPDATE = "update"
>  ROW_DELETE = "delete"
>  
> +class Update_version():

Please make new classes inherit from object, like this:

class Update_version(object):

If you install flake8 and the hacking flake8 plugin, it should complain
about this.


> +    OVSDB_UPDATE =  0,
> +    OVSDB_UPDATE2 = 1.

but I'm actually not sure why a class is needed here.  How about just
constants not wrapped in a class?

>  class Idl(object):
>      """Open vSwitch Database Interface Definition Language (OVSDB IDL).
> @@ -83,6 +86,10 @@ class Idl(object):
>        currently being constructed, if there is one, or None otherwise.
>  """
>  
> +    IDL_S_INITIAL = 0
> +    IDL_S_MONITOR_REQUESTED = 1
> +    IDL_S_MONITOR_COND_REQUESTED = 2
> +
>      def __init__(self, remote, schema):
>          """Creates and returns a connection to the database named 'db_name' on
>          'remote', which should be in a form acceptable to
> @@ -113,6 +120,8 @@ class Idl(object):
>          self._monitor_request_id = None
>          self._last_seqno = None
>          self.change_seqno = 0
> +        self.uuid = uuid.uuid1()

I would use uuid.uuid4() here instead of uuid1(), unless you have a
particular reason to need uuid1().

> +        self.state = self.IDL_S_INITIAL
>  
>          # Database locking.
>          self.lock_name = None          # Name of lock we need, None if none.
> @@ -131,6 +140,7 @@ class Idl(object):
>              table.need_table = False
>              table.rows = {}
>              table.idl = self
> +            table.condition = []
>  
>      def close(self):
>          """Closes the connection to the database.  The IDL will no longer
> @@ -177,11 +187,15 @@ class Idl(object):
>              if msg is None:
>                  break
>              if (msg.type == ovs.jsonrpc.Message.T_NOTIFY
> +                and msg.method == "update2"
> +                and len(msg.params) == 2):
> +                # Database contents changed.
> +                self.__parse_update(msg.params[1], Update_version.OVSDB_UPDATE2)
> +            elif (msg.type == ovs.jsonrpc.Message.T_NOTIFY
>                  and msg.method == "update"
> -                and len(msg.params) == 2
> -                and msg.params[0] is None):
> +                and len(msg.params) == 2):
>                  # Database contents changed.
> -                self.__parse_update(msg.params[1])
> +                self.__parse_update(msg.params[1], Update_version.OVSDB_UPDATE)
>              elif (msg.type == ovs.jsonrpc.Message.T_REPLY
>                    and self._monitor_request_id is not None
>                    and self._monitor_request_id == msg.id):
> @@ -190,8 +204,13 @@ class Idl(object):
>                      self.change_seqno += 1
>                      self._monitor_request_id = None
>                      self.__clear()
> -                    self.__parse_update(msg.result)
> -                except error.Error as e:
> +                    if self.state == self.IDL_S_MONITOR_COND_REQUESTED:
> +                        self.__parse_update(msg.result, Update_version.OVSDB_UPDATE2)
> +                    else:
> +                        assert self.state == self.IDL_S_MONITOR_REQUESTED
> +                        self.__parse_update(msg.result, Update_version.OVSDB_UPDATE)
> +
> +                except error.Error, e:
>                      vlog.err("%s: parse error in received schema: %s"
>                                % (self._session.get_name(), e))
>                      self.__error()
> @@ -211,6 +230,11 @@ class Idl(object):
>              elif msg.type == ovs.jsonrpc.Message.T_NOTIFY and msg.id == "echo":
>                  # Reply to our echo request.  Ignore it.
>                  pass
> +            elif (msg.type == ovs.jsonrpc.Message.T_ERROR and
> +                  self.state == self.IDL_S_MONITOR_COND_REQUESTED and
> +                  self._monitor_request_id == msg.id):
> +                      if msg.error == "unknown method":
> +                          self.__send_monitor_request()

The indentation here is a little odd.  I would expect these last 2 lines
to be 4 spaces indented from "elif" above.

>              elif (msg.type in (ovs.jsonrpc.Message.T_ERROR,
>                                 ovs.jsonrpc.Message.T_REPLY)
>                    and self.__txn_process_reply(msg)):
> @@ -225,6 +249,21 @@ class Idl(object):
>  
>          return initial_change_seqno != self.change_seqno
>  
> +    def cond_update(self, table_name, add, remove):
> +        """Change conditions for this IDL session. If session is not already
> +        connected, add condtion to table and submit it on send_monitor_request.
> +        Otherwise  send monitor_cond_update method with the requested changes."""
> +        table = self.tables.get(table_name)
> +        if not table:
> +            raise error.Error('Unknown table "%s"' % table_name)
> +        if self._session.is_connected():
> +            self.__send_cond_update(table, add ,remove)
> +        else:
> +            if remove:
> +                raise error.Error('Non-empty remove condition on unconnected'
> +                                  'idl session')
> +            self.__add_condition(table, add)
> +
>      def wait(self, poller):
>          """Arranges for poller.block() to wake up when self.run() has something
>          to do or when activity occurs on a transaction on 'self'."""
> @@ -280,6 +319,21 @@ class Idl(object):
>          :type updates:  Row
>          """
>  
> +    def __send_cond_update(self, table, add ,remove):
> +        monitor_cond_update = {table.name: [{"added": add,
> +                                            "removed": remove}]}
> +        old_uuid = str(self.uuid)
> +        self.uuid = uuid.uuid1()

As with earlier, is uuid1() intentional, or should you just use uuid4() ?

> +        params = [old_uuid, str(self.uuid), monitor_cond_update]
> +        msg = ovs.jsonrpc.Message.create_request("monitor_cond_update", params)
> +        msg_id = msg.id
> +        self._session.send(msg)
> +
> +    def __add_condition(self, table, add):
> +        for clause in add:
> +            if not clause in table.condition:
> +                table.condition.append(clause)
> +
>      def __clear(self):
>          changed = False
>  
> @@ -337,6 +391,13 @@ class Idl(object):
>                  self.is_lock_contended = True
>  
>      def __send_monitor_request(self):
> +        if self.state == self.IDL_S_INITIAL:
> +            self.state = self.IDL_S_MONITOR_COND_REQUESTED
> +            method = "monitor_cond"
> +        else:
> +            self.state = self.IDL_S_MONITOR_REQUESTED
> +            method = "monitor"
> +
>          monitor_requests = {}
>          for table in self.tables.itervalues():
>              columns = []
> @@ -346,23 +407,26 @@ class Idl(object):
>                      (column not in self.readonly[table.name])):
>                      columns.append(column)
>              monitor_requests[table.name] = {"columns": columns}
> +            if method == "monitor_cond" and table.condition:
> +                monitor_requests[table.name]["where"] = table.condition
> +                table.condition = None
> +
>          msg = ovs.jsonrpc.Message.create_request(
> -            "monitor", [self._db.name, None, monitor_requests])
> +            method, [self._db.name, str(self.uuid), monitor_requests])
>          self._monitor_request_id = msg.id
>          self._session.send(msg)
>  
> -    def __parse_update(self, update):
> +    def __parse_update(self, update, version):
>          try:
> -            self.__do_parse_update(update)
> -        except error.Error as e:
> +            self.__do_parse_update(update, version)
> +        except error.Error, e:
>              vlog.err("%s: error parsing update: %s"
>                       % (self._session.get_name(), e))
>  
> -    def __do_parse_update(self, table_updates):
> +    def __do_parse_update(self, table_updates, version):
>          if type(table_updates) != dict:
>              raise error.Error("<table-updates> is not an object",
>                                table_updates)
> -

This looks like an unrelated formatting change.

>          for table_name, table_update in table_updates.iteritems():
>              table = self.tables.get(table_name)
>              if not table:
> @@ -387,6 +451,11 @@ class Idl(object):
>                                        'is not an object'
>                                        % (table_name, uuid_string))
>  
> +                if version == Update_version.OVSDB_UPDATE2:
> +                    if self.__process_update2(table, uuid, row_update):
> +                        self.change_seqno += 1
> +                    continue
> +
>                  parser = ovs.db.parser.Parser(row_update, "row-update")
>                  old = parser.get_optional("old", [dict])
>                  new = parser.get_optional("new", [dict])
> @@ -399,6 +468,46 @@ class Idl(object):
>                  if self.__process_update(table, uuid, old, new):
>                      self.change_seqno += 1
>  
> +    def __process_update2(self, table, uuid, row_update):
> +        row = table.rows.get(uuid)
> +        changed = False
> +        if "delete" in row_update:
> +            if row:
> +                del table.rows[uuid]
> +                self.notify(ROW_DELETE, row)
> +                changed = True
> +            else:
> +                # XXX rate-limit
> +                vlog.warn("cannot delete missing row %s from table"
> +                          "%s" % (uuid, table.name))
> +        elif "insert" in row_update or "initial" in row_update:
> +            if row:
> +                vlog.warn("cannot add existing row %s from table"
> +                          " %s" % (uuid, table.name))
> +                del table.rows[uuid]
> +            row = self.__create_row(table, uuid)
> +            if "insert" in row_update:
> +                row_update = row_update['insert']
> +            else:
> +                row_update = row_update['initial']
> +            self.__add_default(table, row_update)
> +            if self.__row_update(table, row, row_update):
> +                changed = True
> +                self.notify(ROW_CREATE, row)
> +        elif "modify" in row_update:
> +            if not row:
> +                raise error.Error('Modify non-existing row')
> +
> +            self.__apply_diff(table, row, row_update['modify'])
> +            self.notify(ROW_UPDATE, row,
> +                        Row.from_json(self, table,
> +                                      uuid, row_update['modify']))
> +            changed = True
> +        else:
> +            raise error.Error('<row-update> unknown operation',
> +                              row_update)
> +        return changed
> +
>      def __process_update(self, table, uuid, old, new):
>          """Returns True if a column changed, False otherwise."""
>          row = table.rows.get(uuid)
> @@ -439,6 +548,42 @@ class Idl(object):
>                  self.notify(op, row, Row.from_json(self, table, uuid, old))
>          return changed
>  
> +    def __add_default(self, table, row_update):
> +        for column in table.columns.itervalues():
> +            if column.name not in row_update:
> +                if ((table.name not in self.readonly) or
> +                    (table.name in self.readonly) and
> +                    (column.name not in self.readonly[table.name])):
> +                    if column.type.n_min != 0 and not column.type.is_map():
> +                        if column.type.key.type == ovs.db.types.UuidType:
> +                            row_update[column.name] = ovs.ovsuuid.to_json(column.type.key.type.default)

Line length here is too long.  Please install 'flake8', re-run
configure, and then fix any issues it complains about, which should
include lines that are too long.


-- 
Russell Bryant



More information about the dev mailing list