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
--
To view, visit
http://gerrit.ovirt.org/17004
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9095b528c2c910f12d5f170088a458bf11c71910
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Martin Sivák <msivak(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com>
Gerrit-Reviewer: Roy Golan <rgolan(a)redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes