Change in vdsm[master]: vdsm: Adding guest agent API versioning support

[Vinzenz Feenstra]

Vinzenz Feenstra has uploaded a new change for review.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

vdsm: Adding guest agent API versioning support

With the increasing complexity on different version of the guest agent
and vdsm we're now introducing API versioning, so only supported messages
are exchanged between guest agent and vdsm.

Change-Id: I9095b528c2c910f12d5f170088a458bf11c71910
Signed-off-by: Vinzenz Feenstra 
---
M vdsm/guestIF.py
1 file changed, 27 insertions(+), 1 deletion(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/04/17004/1

diff --git a/vdsm/guestIF.py b/vdsm/guestIF.py
index dae4c3b..02e44d9 100644
--- a/vdsm/guestIF.py
+++ b/vdsm/guestIF.py
@@ -26,6 +26,8 @@
 import supervdsm
 import unicodedata

+_MAX_SUPPORTED_API_VERSION = 0
+
 __REPLACEMENT_CHAR = u'\ufffd'
 __RESTRICTED_CHARS = set(range(8 + 1)). \
     union(set(range(0xB, 0xC + 1))). \
@@ -97,6 +99,7 @@

def __init__(self, socketName, channelListener, log, user='Unknown',
                  ips='', connect=True):
+        self.apiVersion = 0
         self.log = log
         self._socketName = socketName
         self._stopped = True
@@ -126,6 +129,26 @@
                     self._onChannelTimeout,
                     self)

+    def _setAPIVersion(self, version):
+        try:
+            version = int(version)
+        except ValueError:
+            self.log.error("Received invalid version value: %s", version)
+
+        if version > _MAX_SUPPORTED_API_VERSION:
+            # This actually is not supposed to happen, as the guest agent
+            # is not supposed to send a higher version than requested.
+            # We'll just for our highest version.
+            version = _MAX_SUPPORTED_API_VERSION
+
+        if version < 0:
+            version = 0
+
+        if self.apiVersion != version:
+            self.log.info("Guest API version changed from %d to %d",
+                          self.apiVersion, version)
+            self.apiVersion = version
+
     def _prepare_socket(self):
         supervdsm.getProxy().prepareVmChannel(self._socketName)

@@ -148,7 +171,8 @@
                 self.log.debug("Connected to %s", self._socketName)
                 self._messageState = MessageState.NORMAL
                 self._clearReadBuffer()
-                self._forward('refresh')
+                self._forward('refresh',
+                              args={'apiVersion': _MAX_SUPPORTED_API_VERSION})
                 self._stopped = False
                 ret = True
             else:
@@ -183,6 +207,8 @@
             self.guestInfo['guestName'] = args['name']
         elif message == 'os-version':
             self.guestInfo['guestOs'] = args['version']
+        elif message == 'api-version':
+            self._setAPIVersion(args['apiVersion'])
         elif message == 'network-interfaces':
             interfaces = []
             old_ips = ''

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2545/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3352/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3435/ : SUCCESS

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 1:

The Guest Agent Change related to this fix is here: http://gerrit.ovirt.org/#/c/16995/

[Martin Sivak]

Martin Sivák has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

(1 inline comment)

Looks good, but please document the behaviour at least a tiny bit.

....................................................
File vdsm/guestIF.py
Line 98:     MAX_MESSAGE_SIZE = 2 ** 20  # 1 MiB for now
Line 99: 
Line 100:     def __init__(self, socketName, channelListener, log, user='Unknown',
Line 101:                  ips='', connect=True):
Line 102:         self.apiVersion = 0
Can you please describe the function and logic of this attribute here?

I would like to see at least the information that it is negotiated with the guest agent and ends up the common lowest version that both vdsm and guest-agent support.
Line 103:         self.log = log
Line 104:         self._socketName = socketName
Line 105:         self._stopped = True
Line 106:         self.guestStatus = None

Michal Skrivanek has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

(1 inline comment)

....................................................
File vdsm/guestIF.py
Line 98:     MAX_MESSAGE_SIZE = 2 ** 20  # 1 MiB for now
Line 99: 
Line 100:     def __init__(self, socketName, channelListener, log, user='Unknown',
Line 101:                  ips='', connect=True):
Line 102:         self.apiVersion = 0
vdsm proposes the highest it supports, guest-agent replies with the lower value of the highest it supports or vdsm.
Both sides support at least version 0.
Line 103:         self.log = log
Line 104:         self._socketName = socketName
Line 105:         self._stopped = True
Line 106:         self.guestStatus = None

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6622/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5729/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6535/ : SUCCESS

Michal Skrivanek has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 2:

(3 comments)

....................................................
File vdsm/guestIF.py
Line 144:             version = 0
Line 145: 
Line 146:         return version
Line 147: 
Line 148:     def _setAPIVersion(self, version):
since this is only rarely happening on renegotiation I'd log the request as well for debugging purposes
Line 149:         version = self._normalizeAPIVersion(version)
Line 150:         if self.apiVersion != version:
Line 151:             self.log.info("Guest API version changed from %d to %d",
Line 152:                           self.apiVersion, version)

Line 173:                 self.log.debug("Connected to %s", self._socketName)
Line 174:                 self._messageState = MessageState.NORMAL
Line 175:                 self._clearReadBuffer()
Line 176:                 self._forward('refresh',
Line 177:                               args={'apiVersion': _MAX_SUPPORTED_API_VERSION})
log also here (for the sake of guest agent debugging once we have more versions)
Line 178:                 self._stopped = False
Line 179:                 ret = True
Line 180:             else:
Line 181:                 self.log.debug("Failed to connect to %s with %d",

Line 204:                 for (k, v) in args['memory-stat'].iteritems():
Line 205:                     # Convert the value to string since 64-bit integer is not
Line 206:                     # supported in XMLRPC
Line 207:                     self.guestInfo['memoryStats'][k] = str(v)
Line 208:             # Trigger API version renegotiation when the API version changed on
I'm not sure if we need to send it all the time. What about for any api version >0 the guest agent would send it here only until acknowledged by vdsm calling "api-version" or "refresh without apiVersion"?

I wonder if instead of dragging api-version along forever we would not be better off with assuming the previously negotiated API level and renegotiate on any exception
Line 209:             # the guest side. (e.g. VM rebooted, service was restarted etc)
Line 210:             try:
Line 211:                 apiVersion = int(args.get('apiVersion', self.apiVersion))
Line 212:             except ValueError:

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6685/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6598/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5792/ : SUCCESS

Francesco Romani has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 3:

(1 comment)

....................................................
File vdsm/guestIF.py
Line 135:         try:
Line 136:             return int(version)
Line 137:         except ValueError:
Line 138:             self.log.error("Received invalid version value: %s", version)
Line 139:             return 0
If an invalid version should be treated like a missing version, maybe this should be -1 or _MAX_SUPPORTED_API_VERSION
Line 140: 
Line 141:     def _setAPIVersion(self, version):
Line 142:         version = self._validateAPIVersion(version)
Line 143:         if version > _MAX_SUPPORTED_API_VERSION:

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 3: Verified+1

(1 comment)

http://gerrit.ovirt.org/#/c/17004/3/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 135:         try:
Line 136:             return int(version)
Line 137:         except ValueError:
Line 138:             self.log.error("Received invalid version value: %s", version)
Line 139:             return 0
> If an invalid version should be treated like a missing version, maybe this

0 is supposed to be the lowest possible version, reporting the apiVersion at all shows it knows about this. Therefore the most possible version in that case can be only 0. -1 on the other hand means that it is disabled completely which of course can be triggered again, however this will result in some ping pong I try to avoid.
Line 140: 
Line 141:     def _setAPIVersion(self, version):
Line 142:         version = self._validateAPIVersion(version)
Line 143:         if version > _MAX_SUPPORTED_API_VERSION:

Michal Skrivanek has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.ovirt.org/#/c/17004/3/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 135:         try:
Line 136:             return int(version)
Line 137:         except ValueError:
Line 138:             self.log.error("Received invalid version value: %s", version)
Line 139:             return 0
> 0 is supposed to be the lowest possible version, reporting the apiVersion a

worth a comment that you're resetting to the lowest
Line 140: 
Line 141:     def _setAPIVersion(self, version):
Line 142:         version = self._validateAPIVersion(version)
Line 143:         if version > _MAX_SUPPORTED_API_VERSION:

Francesco Romani has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/17004/3/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 135:         try:
Line 136:             return int(version)
Line 137:         except ValueError:
Line 138:             self.log.error("Received invalid version value: %s", version)
Line 139:             return 0
> 0 is supposed to be the lowest possible version, reporting the apiVersion a

Got it. Fine for me. Probably worth a comment as Michal pointed out.
Line 140: 
Line 141:     def _setAPIVersion(self, version):
Line 142:         version = self._validateAPIVersion(version)
Line 143:         if version > _MAX_SUPPORTED_API_VERSION:

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/17004/3/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 135:         try:
Line 136:             return int(version)
Line 137:         except ValueError:
Line 138:             self.log.error("Received invalid version value: %s", version)
Line 139:             return 0
> Got it. Fine for me. Probably worth a comment as Michal pointed out.

Done
Line 140: 
Line 141:     def _setAPIVersion(self, version):
Line 142:         version = self._validateAPIVersion(version)
Line 143:         if version > _MAX_SUPPORTED_API_VERSION:

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 4: Verified+1

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/17004/3/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 134:     def _validateAPIVersion(self, version):
Line 135:         try:
Line 136:             return int(version)
Line 137:         except ValueError:
Line 138:             self.log.error("Received invalid version value: %s", version)
If I can make a suggestion based on the code and comments.

def _validateAPIVersion(self, version):
   """ Validate API version 
    Args:
    version - - the api version to validate

Returns:
    A validate api version or 0 which means the lowest possible version of API.

Note:
    We do not return -1 since it means it's disabled and will keep trying to get
    such info.
    """
    _LOWEST_POSSIBLE_API_VERSION = 0
    try:
        api_version = int(version)
    except ValueError:
        self.log.error("Received invalid version value: %s", version)
        api_version =  _LOWEST_POSSIBLE_API_VERSION

return api_version
Line 139:             return 0
Line 140: 
Line 141:     def _setAPIVersion(self, version):
Line 142:         version = self._validateAPIVersion(version)

Francesco Romani has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 4: Code-Review+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 5:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6709/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6622/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5816/ : SUCCESS

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/17004/3/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 134:     def _validateAPIVersion(self, version):
Line 135:         try:
Line 136:             return int(version)
Line 137:         except ValueError:
Line 138:             self.log.error("Received invalid version value: %s", version)
> If I can make a suggestion based on the code and comments.

forgot to mention: I used api_version variable to have multiple points of return.
Line 139:             return 0
Line 140: 
Line 141:     def _setAPIVersion(self, version):
Line 142:         version = self._validateAPIVersion(version)

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/17004/3/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 134:     def _validateAPIVersion(self, version):
Line 135:         try:
Line 136:             return int(version)
Line 137:         except ValueError:
Line 138:             self.log.error("Received invalid version value: %s", version)
> forgot to mention: I used api_version variable to have multiple points of r

*avoid
Line 139:             return 0
Line 140: 
Line 141:     def _setAPIVersion(self, version):
Line 142:         version = self._validateAPIVersion(version)

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 5:

(5 comments)

http://gerrit.ovirt.org/#/c/17004/5/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 98:     MAX_MESSAGE_SIZE = 2 ** 20  # 1 MiB for now
Line 99: 
Line 100:     def __init__(self, socketName, channelListener, log, user='Unknown',
Line 101:                  ips='', connect=True):
Line 102:         self.apiVersion = -1
-1 means disable right? Maybe a constant here too.
Line 103:         self.log = log
Line 104:         self._socketName = socketName
Line 105:         self._sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
Line 106:         self._stopped = True

Line 142: 
Line 143:         Note:
Line 144:         We do not return -1 since it means it's disabled and would be an
Line 145:         unexpected value for the protocol. (The value returned here is used
Line 146:         in the API)
Re-reading the code this "Note" shouldn't be in _setAPIVersion? Since int(version) might return -1.
Line 147:         """
Line 148:         _LOWEST_POSSIBLE_API_VERSION = 0
Line 149:         try:
Line 150:             return int(version)

Line 155:     def _setAPIVersion(self, version):
Line 156:         version = self._validateAPIVersion(version)
Line 157:         if version > _MAX_SUPPORTED_API_VERSION:
Line 158:             version = _MAX_SUPPORTED_API_VERSION
Line 159:         elif version < 0:
Maybe we should use _LOWEST_POSSIBLE_API_VERSION constant here?

Another question, _MAX_SUPPORTED_API_VERSION = 0 shouldn't be _MIN_SUPPORED_API_VERSION? If yes, we could even remove  _LOWEST_POSSIBLE_API_VERSION and use it too. Please note, I am not very familiar with this part of code, just trying to help.
Line 160:             version = 0
Line 161:         if self.apiVersion != version:
Line 162:             self.log.info("Guest API version changed from %d to %d",
Line 163:                           self.apiVersion, version)

Line 224:                     self.log.debug("Common API version changed from %d to %d",
Line 225:                                    self.apiVersion, commonVersion)
Line 226:                     self._forward('api-version',
Line 227:                                   {'apiVersion': commonVersion})
Line 228:             elif self.apiVersion != -1:
if we are going to use constant, could replace it here too.
Line 229:                 self.log.debug("API versioning no longer reported by guest.")
Line 230:                 self.apiVersion = -1
Line 231:         elif message == 'host-name':
Line 232:             self.guestInfo['guestName'] = args['name']

Line 226:                     self._forward('api-version',
Line 227:                                   {'apiVersion': commonVersion})
Line 228:             elif self.apiVersion != -1:
Line 229:                 self.log.debug("API versioning no longer reported by guest.")
Line 230:                 self.apiVersion = -1
here too
Line 231:         elif message == 'host-name':
Line 232:             self.guestInfo['guestName'] = args['name']
Line 233:         elif message == 'os-version':
Line 234:             self.guestInfo['guestOs'] = args['version']

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 5:

(4 comments)

http://gerrit.ovirt.org/#/c/17004/5/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 142: 
Line 143:         Note:
Line 144:         We do not return -1 since it means it's disabled and would be an
Line 145:         unexpected value for the protocol. (The value returned here is used
Line 146:         in the API)
> Re-reading the code this "Note" shouldn't be in _setAPIVersion? Since int(v

Yeah it should also go for min(apiVersion, _MIN_SUPPORTED_API_VERSION)
Line 147:         """
Line 148:         _LOWEST_POSSIBLE_API_VERSION = 0
Line 149:         try:
Line 150:             return int(version)

Line 155:     def _setAPIVersion(self, version):
Line 156:         version = self._validateAPIVersion(version)
Line 157:         if version > _MAX_SUPPORTED_API_VERSION:
Line 158:             version = _MAX_SUPPORTED_API_VERSION
Line 159:         elif version < 0:
> Maybe we should use _LOWEST_POSSIBLE_API_VERSION constant here?

_MAX_SUPPORTED_API_VERSION  is currently 0 because we did not yet extend the API since this was introduced.
I decided to change _LOWEST_POSSIBLE_API_VERSION to _MIN_SUPPORTED_API_VERSION as a module global constant, this seems to be better
Line 160:             version = 0
Line 161:         if self.apiVersion != version:
Line 162:             self.log.info("Guest API version changed from %d to %d",
Line 163:                           self.apiVersion, version)

Line 224:                     self.log.debug("Common API version changed from %d to %d",
Line 225:                                    self.apiVersion, commonVersion)
Line 226:                     self._forward('api-version',
Line 227:                                   {'apiVersion': commonVersion})
Line 228:             elif self.apiVersion != -1:
> if we are going to use constant, could replace it here too.

Done
Line 229:                 self.log.debug("API versioning no longer reported by guest.")
Line 230:                 self.apiVersion = -1
Line 231:         elif message == 'host-name':
Line 232:             self.guestInfo['guestName'] = args['name']

Line 226:                     self._forward('api-version',
Line 227:                                   {'apiVersion': commonVersion})
Line 228:             elif self.apiVersion != -1:
Line 229:                 self.log.debug("API versioning no longer reported by guest.")
Line 230:                 self.apiVersion = -1
> here too

Done
Line 231:         elif message == 'host-name':
Line 232:             self.guestInfo['guestName'] = args['name']
Line 233:         elif message == 'os-version':
Line 234:             self.guestInfo['guestOs'] = args['version']

Francesco Romani has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 6: Code-Review+1

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 6:

(5 comments)

http://gerrit.ovirt.org/#/c/17004/6/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 26: import supervdsm
Line 27: import unicodedata
Line 28: 
Line 29: _MAX_SUPPORTED_API_VERSION = 0
Line 30: _MIN_SUPPORTED_API_VERSION = 0
Hi,

Don't understand why _MIN and _MAX as 0? Could you please clarify?
Line 31: _API_DISABLED_VALUE = -1
Line 32: 
Line 33: __REPLACEMENT_CHAR = u'\ufffd'
Line 34: __RESTRICTED_CHARS = set(range(8 + 1)). \

Line 27: import unicodedata
Line 28: 
Line 29: _MAX_SUPPORTED_API_VERSION = 0
Line 30: _MIN_SUPPORTED_API_VERSION = 0
Line 31: _API_DISABLED_VALUE = -1
I would go _API_DISABLE or other name without VALUE but it's personal opinion.
Line 32: 
Line 33: __REPLACEMENT_CHAR = u'\ufffd'
Line 34: __RESTRICTED_CHARS = set(range(8 + 1)). \
Line 35:     union(set(range(0xB, 0xC + 1))). \

Line 138:         Args:
Line 139:         version - the api version value to validate
Line 140: 
Line 141:         Returns:
Line 142:         A valid api version or 0 which means the lowest possible version of
Can return -1 (disable) at this stage too right?
Line 143:         the API.
Line 144:         """
Line 145:         try:
Line 146:             apiVersion = int(version)

Line 146:             apiVersion = int(version)
Line 147:         except ValueError:
Line 148:             self.log.warning("Received invalid version value: %s", version)
Line 149:             apiVersion = _MIN_SUPPORTED_API_VERSION
Line 150:         return min(apiVersion, _MIN_SUPPORTED_API_VERSION)
Question: apiversion could be a value > 0? If yes, not sure if we should use min().
Line 151: 
Line 152:     def _setAPIVersion(self, version):
Line 153:         """ Sets the API version if it was changed
Line 154:         Args:

Line 154:         Args:
Line 155:         version - the api version to set
Line 156: 
Line 157:         Note: If the value of `version` is not in between
Line 158:         _MIN_SUPPORTED_API_VERSION and _MAX_SUPPORTED_API_VERSION the value
between? but both init as 0, not sure if I understood it.
Line 159:         will be adjusted to have the highest possible API version supported.
Line 160: 
Line 161:         This should not be required and the protocol does not rely on this,
Line 162:         this is simply a data santization fallback.

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 6:

(4 comments)

http://gerrit.ovirt.org/#/c/17004/6/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 26: import supervdsm
Line 27: import unicodedata
Line 28: 
Line 29: _MAX_SUPPORTED_API_VERSION = 0
Line 30: _MIN_SUPPORTED_API_VERSION = 0
> Hi,

Because with this patchset there's no new version, 0 is the baseline for now. There's a new patch in the pipe which will increase MAX to 1. Simply 0 means all what was there before. Since there's no versioning so far, this is simply version 0.

See here: http://gerrit.ovirt.org/#/c/23258/1/vdsm/guestIF.py
Line 31: _API_DISABLED_VALUE = -1
Line 32: 
Line 33: __REPLACEMENT_CHAR = u'\ufffd'
Line 34: __RESTRICTED_CHARS = set(range(8 + 1)). \

Line 138:         Args:
Line 139:         version - the api version value to validate
Line 140: 
Line 141:         Returns:
Line 142:         A valid api version or 0 which means the lowest possible version of
> Can return -1 (disable) at this stage too right?

no, it should not
Line 143:         the API.
Line 144:         """
Line 145:         try:
Line 146:             apiVersion = int(version)

Line 146:             apiVersion = int(version)
Line 147:         except ValueError:
Line 148:             self.log.warning("Received invalid version value: %s", version)
Line 149:             apiVersion = _MIN_SUPPORTED_API_VERSION
Line 150:         return min(apiVersion, _MIN_SUPPORTED_API_VERSION)
> Question: apiversion could be a value > 0? If yes, not sure if we should us

you're right, It actually MUST be a value >= 0.  This should have been max and not min so that at a very minimum it is _MIN_SUPPORTED_API_VERSION
Line 151: 
Line 152:     def _setAPIVersion(self, version):
Line 153:         """ Sets the API version if it was changed
Line 154:         Args:

Line 154:         Args:
Line 155:         version - the api version to set
Line 156: 
Line 157:         Note: If the value of `version` is not in between
Line 158:         _MIN_SUPPORTED_API_VERSION and _MAX_SUPPORTED_API_VERSION the value
> between? but both init as 0, not sure if I understood it.

Well that's the documentation, it doesn't matter that there's no version 1 yet, however this will come with the above mentioned patchset.
Well this is the documentation, the constants can change over time.
Line 159:         will be adjusted to have the highest possible API version supported.
Line 160: 
Line 161:         This should not be required and the protocol does not rely on this,
Line 162:         this is simply a data santization fallback.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: Adding guest agent API versioning support
......................................................................

Patch Set 7:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6748/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6661/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5856/ : SUCCESS

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 8:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6750/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6662/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5857/ : SUCCESS

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 8: Verified+1

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

Francesco Romani has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 8:

(2 comments)

http://gerrit.ovirt.org/#/c/17004/8/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 162:         this is simply a data santization fallback.
Line 163:         """
Line 164:         version = self._validateAPIVersion(version)
Line 165:         version = min(version, _MAX_SUPPORTED_API_VERSION)
Line 166:         version = max(version, _MIN_SUPPORTED_API_VERSION)
Minor thing (because the logic looks good): isn't this redundant? If we can't get here without returning from _validateAPIVersion, where we have the exact same check.
Perhaps it could be worth move the version clamping inside _validateAPIVersion, it could be seen as validation after all.
Line 167:         if self.apiVersion != version:
Line 168:             self.log.info("Guest API version changed from %d to %d",
Line 169:                           self.apiVersion, version)
Line 170:             self.apiVersion = version

Line 227: 
Line 228:             if 'apiVersion' in args:
Line 229:                 # The guest agent supports API Versioning
Line 230:                 apiVersion = self._validateAPIVersion(args['apiVersion'])
Line 231:                 commonVersion = min(_MAX_SUPPORTED_API_VERSION, apiVersion)
Minor thing (because it can stay this way): as matter of personal taste I'd swap the arguments of min to make them look consistent with _setAPIVersion (same operation but different ordering of parameters may look like a code smell).
Line 232:                 if commonVersion != self.apiVersion:
Line 233:                     self.log.debug("Common API version changed from %d to %d",
Line 234:                                    self.apiVersion, commonVersion)
Line 235:                     self._forward('api-version',

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 9:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6764/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6677/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5872/ : SUCCESS

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 9: Code-Review+1

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 9: Verified+1

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

Francesco Romani has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 9: Code-Review+1

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 10: Verified+1

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 11: Verified+1

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

Francesco Romani has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 11:

(3 comments)

Seems good excepts for a few minor things/typos.

http://gerrit.ovirt.org/#/c/17004/11//COMMIT_MSG
Commit Message:

Line 21:   - If it does NOT exist it and the apiVersion is not set to -1 it will
Line 22:     set it to -1, with the meaning that it is not supported at all by the
Line 23:     guest agent on the other end.
Line 24: - The guest agent on receiving this message makes it own check and sets
Line 25:   the `apiVersion` to the commonVersion
s/to the commonVersion/to the common value/ I think
Line 26: 
Line 27: - If VDSM sends the `refresh` command it also sends its maximum supported
Line 28:   version however if it does not, capable guest agents are disabling the
Line 29:   versioning support and will know it in pre-supported state as well.

http://gerrit.ovirt.org/#/c/17004/11/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 142: 0
I'd drop the '0 which means the' because we have the constants to encapsulate the values.

Line 160:         _MIN_SUPPORTED_API_VERSION and _MAX_SUPPORTED_API_VERSION the value
Line 161:         will be adjusted to have the highest possible API version supported.
Line 162: 
Line 163:         This should not be required and the protocol does not rely on this,
Line 164:         this is simply a data santization fallback.
typo: sanitization
Line 165:         """
Line 166:         if self.apiVersion != version:
Line 167:             self.log.info("Guest API version changed from %d to %d",
Line 168:                           self.apiVersion, version)

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 10: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5933/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6826/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6721/ : SUCCESS

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 11: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5935/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6828/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6723/ : SUCCESS

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 11: -Verified

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5972/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6760/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6866/ : SUCCESS

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 11: Code-Review+1

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 11:

(3 comments)

http://gerrit.ovirt.org/#/c/17004/11//COMMIT_MSG
Commit Message:

Line 21:   - If it does NOT exist it and the apiVersion is not set to -1 it will
Line 22:     set it to -1, with the meaning that it is not supported at all by the
Line 23:     guest agent on the other end.
Line 24: - The guest agent on receiving this message makes it own check and sets
Line 25:   the `apiVersion` to the commonVersion
> s/to the commonVersion/to the common value/ I think

Done
Line 26: 
Line 27: - If VDSM sends the `refresh` command it also sends its maximum supported
Line 28:   version however if it does not, capable guest agents are disabling the
Line 29:   versioning support and will know it in pre-supported state as well.

http://gerrit.ovirt.org/#/c/17004/11/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 142: 0
> I'd drop the '0 which means the' because we have the constants to encapsula

Done

Line 160:         _MIN_SUPPORTED_API_VERSION and _MAX_SUPPORTED_API_VERSION the value
Line 161:         will be adjusted to have the highest possible API version supported.
Line 162: 
Line 163:         This should not be required and the protocol does not rely on this,
Line 164:         this is simply a data santization fallback.
> typo: sanitization

Done
Line 165:         """
Line 166:         if self.apiVersion != version:
Line 167:             self.log.info("Guest API version changed from %d to %d",
Line 168:                           self.apiVersion, version)

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 12: Verified+1

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 12:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5975/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6763/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6869/ : SUCCESS

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 12: Code-Review-1

(10 comments)

http://gerrit.ovirt.org/#/c/17004/12//COMMIT_MSG
Commit Message:

Line 15:   unsupported
Line 16: - The guest agent sends its max supported version with every `heartbeat`
Line 17:   message
Line 18: - VDSM checks that `apiVersion` exists in the `heartbeat` message arguments
Line 19:   - If it exists it retrieves the minimum common value and sends the
The English is not clear for me. Please review my suggestion:

If apiVersion exists, Vdsm finds the *maximum* common value between itself
 and the guest. Vdsm then sends ...

I still wonder why a version resolution is really needed: wouldn't it be simpler for Vdsm to have its own version, and the guest - its possibly different version.

What is hampered if Vdsm simply sends its own version to the agent? Let the agent do whatever it wants to do with this information, as long as it does not use incompatible messages.
Line 20:     `api-version` message with the common value as `apiVersion` argument
Line 21:   - If it does NOT exist it and the apiVersion is not set to -1 it will
Line 22:     set it to -1, with the meaning that it is not supported at all by the
Line 23:     guest agent on the other end.

Line 17:   message
Line 18: - VDSM checks that `apiVersion` exists in the `heartbeat` message arguments
Line 19:   - If it exists it retrieves the minimum common value and sends the
Line 20:     `api-version` message with the common value as `apiVersion` argument
Line 21:   - If it does NOT exist it and the apiVersion is not set to -1 it will
again, I find the English syntax too complex:

If  apiVersion does NOT exists in heartbeat, (and apiVersion is not already set to -1,)  apiVersion is set

/me is not sure why the parenthesized subsentence is needed.
Line 22:     set it to -1, with the meaning that it is not supported at all by the
Line 23:     guest agent on the other end.
Line 24: - The guest agent on receiving this message makes it own check and sets
Line 25:   the `apiVersion` to the common value

Line 20:     `api-version` message with the common value as `apiVersion` argument
Line 21:   - If it does NOT exist it and the apiVersion is not set to -1 it will
Line 22:     set it to -1, with the meaning that it is not supported at all by the
Line 23:     guest agent on the other end.
Line 24: - The guest agent on receiving this message makes it own check and sets
makes it -> makes its
Line 25:   the `apiVersion` to the common value
Line 26: 
Line 27: - If VDSM sends the `refresh` command it also sends its maximum supported
Line 28:   version however if it does not, capable guest agents are disabling the

http://gerrit.ovirt.org/#/c/17004/12/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 100:     MAX_MESSAGE_SIZE = 2 ** 20  # 1 MiB for now
Line 101: 
Line 102:     def __init__(self, socketName, channelListener, log, user='Unknown',
Line 103:                  ips='', connect=True):
Line 104:         self.apiVersion = _API_DISABLED_VALUE
How about renaming this to effectiveApiVersion? We have 3 versions: the agent's, Vdsm's, and the one chosen to be used.
Line 105:         self.log = log
Line 106:         self._socketName = socketName
Line 107:         self._sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
Line 108:         self._stopped = True

Line 132:                     self._onChannelRead,
Line 133:                     self._onChannelTimeout,
Line 134:                     self)
Line 135: 
Line 136:     def _validateAPIVersion(self, version):
can be a @staticmethod. Or a module-level function.
Line 137:         """ Validate API version
Line 138:         Args:
Line 139:         version - the api version value to validate
Line 140:

Line 146:             apiVersion = int(version)
Line 147:         except ValueError:
Line 148:             self.log.warning("Received invalid version value: %s", version)
Line 149:             apiVersion = _MIN_SUPPORTED_API_VERSION
Line 150:         apiVersion = max(apiVersion, _MIN_SUPPORTED_API_VERSION)
I do not understand. If a buggy agent sends a "gfagfdsag" version, why do we assume that it supports _MIN_SUPPORTED_API_VERSION ? To me, such a behavior is a hint not to trust this agent at all.
Line 151:         apiVersion = min(apiVersion, _MAX_SUPPORTED_API_VERSION)
Line 152:         return apiVersion
Line 153: 
Line 154:     def _setAPIVersion(self, version):

Line 191:                 self._clearReadBuffer()
Line 192:                 # Report the _MAX_SUPPORTED_API_VERSION on refresh to enable
Line 193:                 # the other side to see that we support API versioning
Line 194:                 self._forward('refresh',
Line 195:                               {'apiVersion': _MAX_SUPPORTED_API_VERSION})
If we plan to have deprecation built into the protocol, we should expose _MIN_SUPPORTED_API_VERSION, too. Otherwise, old agent would keep sending deprecated messages.
Line 196:                 self._stopped = False
Line 197:                 ret = True
Line 198:             else:
Line 199:                 self.log.debug("Failed to connect to %s with %d",

Line 212:         self.log.log(logging.TRACE, "Guest's message %s: %s", message, args)
Line 213:         if self.guestStatus is None:
Line 214:             self.guestStatus = 'Up'
Line 215:         if message == 'heartbeat':
Line 216:             self.guestStatus = 'Up'
time to factor this handling to a helper method (in a different patch).
Line 217:             self.guestInfo['memUsage'] = int(args['free-ram'])
Line 218:             # ovirt-guest-agent reports the following fields in 'memory-stat':
Line 219:             # 'mem_total', 'mem_free', 'mem_unused', 'swap_in', 'swap_out',
Line 220:             # 'pageflt' and 'majflt'

Line 225:                     self.guestInfo['memoryStats'][k] = str(v)
Line 226: 
Line 227:             if 'apiVersion' in args:
Line 228:                 # The guest agent supports API Versioning
Line 229:                 commonVersion = self._validateAPIVersion(args['apiVersion'])
_validateAPIVersion() is misleading here: it also does a conversion.
Line 230:                 if commonVersion != self.apiVersion:
Line 231:                     self.log.debug("Common API version changed from %d to %d",
Line 232:                                    self.apiVersion, commonVersion)
Line 233:                     self._setAPIVersion(commonVersion)

Line 233:                     self._setAPIVersion(commonVersion)
Line 234:                     self._forward('api-version',
Line 235:                                   {'apiVersion': commonVersion})
Line 236:             elif self.apiVersion != _API_DISABLED_VALUE:
Line 237:                 # Older versions of the guest agent do not report this field
older versions - older than what?
Line 238:                 # Disable the API if not already disabled (e.g. after
Line 239:                 # downgrade)
Line 240:                 self.log.debug("API versioning no longer reported by guest.")
Line 241:                 self.apiVersion = _API_DISABLED_VALUE

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 12:

(2 comments)

http://gerrit.ovirt.org/#/c/17004/12/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 146:             apiVersion = int(version)
Line 147:         except ValueError:
Line 148:             self.log.warning("Received invalid version value: %s", version)
Line 149:             apiVersion = _MIN_SUPPORTED_API_VERSION
Line 150:         apiVersion = max(apiVersion, _MIN_SUPPORTED_API_VERSION)
> I do not understand. If a buggy agent sends a "gfagfdsag" version, why do w

Done
Line 151:         apiVersion = min(apiVersion, _MAX_SUPPORTED_API_VERSION)
Line 152:         return apiVersion
Line 153: 
Line 154:     def _setAPIVersion(self, version):

Line 191:                 self._clearReadBuffer()
Line 192:                 # Report the _MAX_SUPPORTED_API_VERSION on refresh to enable
Line 193:                 # the other side to see that we support API versioning
Line 194:                 self._forward('refresh',
Line 195:                               {'apiVersion': _MAX_SUPPORTED_API_VERSION})
> If we plan to have deprecation built into the protocol, we should expose _M

The _MIN_SUPPORTED_API_VERSION should stay always like this, because of backwards compatibility we cannot remove the support for the older messages. However with a negotiated higher supported version we can make it report other things instead of older messages, real deprecation unfortunately is not an option.
Line 196:                 self._stopped = False
Line 197:                 ret = True
Line 198:             else:
Line 199:                 self.log.debug("Failed to connect to %s with %d",

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 13:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6187/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7080/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/6966/ : SUCCESS

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 14:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6189/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7082/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/6968/ : SUCCESS

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

Martin Polednik has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 14: Code-Review+1

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 14: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/17004/14/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 237:                     self.guestInfo['memoryStats'][k] = str(v)
Line 238: 
Line 239:             if 'apiVersion' in args:
Line 240:                 # The guest agent supports API Versioning
Line 241:                 self._handleApiVersion(args['apiVersion'])
Python is case-sensitive, and we need unit tests to catch such errors!
Line 242:             elif self.effectiveApiVersion != _API_DISABLED_VALUE:
Line 243:                 # Older versions of the guest agent (before the introduction
Line 244:                 # API versioning) do not report this field
Line 245:                 # Disable the API if not already disabled (e.g. after

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 15:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6192/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7085/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/6971/ : SUCCESS

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 16: Verified+1

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

Francesco Romani has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 16: Code-Review+1

looks good

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 16: Code-Review+1

(1 comment)

you o

http://gerrit.ovirt.org/#/c/17004/16/vdsm/guestIF.py
File vdsm/guestIF.py:

Line 26: import supervdsm
Line 27: import unicodedata
Line 28: 
Line 29: _MAX_SUPPORTED_API_VERSION = 0
Line 30: _API_DISABLED_VALUE = 0
sorry to note this so late, but the name of this constant is not so clear. unfortunately, I cannot think of a much better name, so I give my +1..

How about IMPLICIT_API_VERSION_ZERO ?
Line 31: 
Line 32: _MESSAGE_API_VERSION_LOOKUP = {}
Line 33: 
Line 34: __REPLACEMENT_CHAR = u'\ufffd'

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 17:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6238/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7127/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7016/ : SUCCESS

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 17: Code-Review+2

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

Patch Set 17: Verified+1

Change in vdsm[master]: guestIF: Adding guest agent API versioning support

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: guestIF: Adding guest agent API versioning support
......................................................................

guestIF: Adding guest agent API versioning support

With the increasing complexity on different version of the guest agent
and vdsm we're now introducing API versioning, so only supported messages
are exchanged between guest agent and vdsm.

VDSM:
  - Adds a field to the 'refresh' message reporting the highest API version
    supported by VDSM

- Upon receiving the 'heartbeat' message check for the `apiVersion` field
    to know if the guest agent supports api versioning.
    - If the fields is not present:
      The guest agent won't support api versioning. And it needs to be
      disabled on the VDSM side and the version 0 has to be assumed. That
      simply means only messages can be sent which were supported before the
      API versioning was introduced.
    - If the field is present:
      The value of the field is supposed to represent the maximum version
      supported by the guest agent.

VDSM then makes a `min(vdsmMaxApiVersion, guestAgentMaxApiVersion)`
      to determine the highest common value and uses this value as supported
      API version.
      When the value has changed since the last time a heartbeat was received.
      VDSM will send a message 'api-version' to update the guest agent about
      the determined value. And sets the internally stored effectiveApiVersion
      value to this new value.

Guest Agent:
    - Adds the field `apiVersion` to the heartbeat containing the highest API
    version supported by the guest agent

- Upon receiving the `refresh` message without a `apiVersion` field, the
    guest agent immediately will revert immediately fall back to version 0
    To avoid sending any messages which are not yet supported.

- Upon receiving the `api-version` message, the guest agent will verify the
    value received and sets the value to min(received, maxAgentApiVersion)
    which should, of course, result in setting the received value (this is
    just an additional step to ensure it does not send more than supported by
    VDSM.

Change-Id: I9095b528c2c910f12d5f170088a458bf11c71910
Signed-off-by: Vinzenz Feenstra 
Reviewed-on: http://gerrit.ovirt.org/17004
Reviewed-by: Dan Kenigsberg 
---
M vdsm/guestIF.py
1 file changed, 65 insertions(+), 1 deletion(-)

Approvals:
  Vinzenz Feenstra: Verified
  Dan Kenigsberg: Looks good to me, approved