Russell Bryant <russ...@ovn.org> wrote on 25/01/2016 10:06:35 PM:
> 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 <lir...@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):
>
Will fix it.
> > + 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?
>
Right. No need for a class here.
> > 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().
>
Will use uuid.uuid64() instead.
> > + 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.
>
Will fix this.
> > 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()
?
>
Same as above, will use uuid.uuid64().
> > + 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.
>
Right.
> > 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
>
Thanks,
- Liran
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
