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): > + 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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev