Change in vdsm[master]: logs: improve VDSM logs human-readability
Yoav Kleinberger has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logs: improve VDSM logs human-readability
oVirt Jenkins CI Server has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 2: Build Successful http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9512/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10296/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10452/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5378/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3536/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logs: improve VDSM logs human-readability
Vered Volansky has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logs: improve VDSM logs human-readability
Yoav Kleinberger has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 4: Verified+1 (1 comment) http://gerrit.ovirt.org/#/c/28869/4/vdsm/logger.conf.in File vdsm/logger.conf.in: Line 67 Line 68 Line 69 Line 70 Line 71 * Tidyness is essential for effective reading. That's the whole reason I started this - I find the logs hard to follow. Things with high variability should come after things that are relatively constant-width (e.g. timestamp). * I agree with Dan that message should come last. I now retain more of the old format. I still think the new one is an improvement. -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logs: improve VDSM logs human-readability
oVirt Jenkins CI Server has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 3: Build Successful http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9513/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10297/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10453/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5379/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3537/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logs: improve VDSM logs human-readability
oVirt Jenkins CI Server has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 4: Build Successful http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9514/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10298/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10454/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5380/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3538/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logs: improve VDSM logs human-readability
Vered Volansky has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 4: Code-Review+1 Although this patch is now reduced to de facto changing the delimeter from :: to space, I'm in favor. The current log is just a bulk of data, making the human reader actively search for the double-colons in order to read it. With spaces instead, the mind makes the field separation automatically. On the one hand, double colon was retained before the module name so it doesn't bother searches, and on the other, a space was added for clear human readability. -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logs: improve VDSM logs human-readability
Liron Aravot has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 4: can we get example of before/after in the commit message? -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vdsm-tool: Add logging configuration.
Yaniv Bronhaim has posted comments on this change. Change subject: vdsm-tool: Add logging configuration. .. Patch Set 10: (1 comment) http://gerrit.ovirt.org/#/c/27481/10/vdsm-tool/vdsm-tool File vdsm-tool/vdsm-tool: Line 135: "\t\tInclude information (and above) messages in log.", Line 136: " -vvv, --vvverbose", Line 137: "\t\tInclude debug (and above) messages in log.", Line 138: " -a, --append", Line 139: "\t\tAppend to logfile instead of truncating it", I thought verbose will mean to print the output also to stdout. for logging level you could just call the parameters --loglevel=DEBUG\WARNING\ERROR (as optional parameter to pick the basic level to start logging from).. its a bit odd to use type of verbose for that], what do you think? Line 140: "(if logging to a file)."])) Line 141: Line 142: for mod_name, mod_desc in tool_modules: Line 143: _usage_module(mod_name, mod_desc) -- To view, visit http://gerrit.ovirt.org/27481 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia495743f6e869f65843404e4d4c25c146ff14b43 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dima Kuznetsov Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Dima Kuznetsov Gerrit-Reviewer: Douglas Schilling Landgraf Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vdsm-tool: Change upgrade mechanism
Yaniv Bronhaim has posted comments on this change. Change subject: vdsm-tool: Change upgrade mechanism .. Patch Set 12: Code-Review+1 -- To view, visit http://gerrit.ovirt.org/27193 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6e1d28570dedfeff9fe60624b1db72d8cadf136a Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dima Kuznetsov Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Assaf Muller Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Dima Kuznetsov Gerrit-Reviewer: Douglas Schilling Landgraf Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logs: improve VDSM logs human-readability
Nir Soffer has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 3: (1 comment) http://gerrit.ovirt.org/#/c/28869/3/vdsm/logger.conf.in File vdsm/logger.conf.in: Line 66: [formatter_none] Line 67: format: %(message)s Line 68: Line 69: [formatter_long] Line 70: format: %(asctime)s %(levelname)s %(threadName)s (%(name)s ::%(module)s in function %(funcName)s %(pathname)s:%(lineno)d): %(message)s - Replacing :: with space is a good change. - Adding "in function" is a regression - loggername::module does not make sense, now that the delimiter is space Order should be: - time - level name - logger name - same logger may have multiple threads - thread name - multiple threads may run code in same module - pathname:lineo - standard notation e.g. grep - but only if pathname is module + ".py". If it contains the full path to the file, keep module:lineo. - function name - message If you look into engine.log, you can see that items are enclosed in [] or (), which make it easier to detect visually: 2014-06-10 13:46:57,716 ERROR [org.ovirt.engine.core.vdsbroker.vdsbroker.GetCapabilitiesVDSCommand] (org.ovirt.thread.pool-6-thread-5) [196f34d9] Command GetCapabilitiesVDSCommand(HostName = voodoo2, HostId = 871162b7-9200-482b-94da-97014551eea1, vds=Host[voodoo2]) execution failed. Exception: VDSNetworkException: java.net.ConnectException: Connection refused We should cosider grouping all the meta data or some parts of it, for example: timestamp level [who logged this] (where it was logged) message Translating to: timestamp levelname [loggername threadname] (module:lineno:function) message Please show us 30 seconds log from vdsm using the old and new format. The only way to evaluate this is to look at the log, not the format. Most vdsm developers are used to the current log format, and any change which is not significantly better will be hard to sell. Check also http://www.ovirt.org/Vdsm_Log_Files. If you change the log format, you will have to document both formats. Line 71: Line 72: [formatter_sysform] Line 73: format= vdsm %(name)s %(levelname)s %(message)s Line 74: datefmt= -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logs: improve VDSM logs human-readability
Nir Soffer has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 4: Adding Michal and Martin because this change affect every group. -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Martin Sivák Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Cleanup imports
Yoav Kleinberger has posted comments on this change. Change subject: domainMonitor: Cleanup imports .. Patch Set 1: Code-Review+1 Excellent -- To view, visit http://gerrit.ovirt.org/29015 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iff024da4c259e8976e6248b5b10e1bb55128312c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logs: improve VDSM logs human-readability
Yoav Kleinberger has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/28869/1/vdsm/logger.conf.in File vdsm/logger.conf.in: Line 68 Line 69 Line 70 Line 71 Line 72 * Tidiness is essential for effective reading. That's the whole reason I started this - I find the logs hard to follow. Things with high variability should come after things that are relatively constant-width (e.g. timestamp). * I agree with Dan that message should come last. * Please see the newest patchset. I now retain more of the old format. I still think the new one is an improvement. -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Martin Sivák Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe status handling
Yoav Kleinberger has posted comments on this change. Change subject: domainMonitor: Fix unsafe status handling .. Patch Set 1: Code-Review-1 (2 comments) just a small thing, see comment inside. http://gerrit.ovirt.org/#/c/29014/1//COMMIT_MSG Commit Message: Line 11: was written as if creating a new status object is expensive operation Line 12: that should be avoided. Line 13: Line 14: Instead of having long living status object in the monitor, I create new Line 15: forozen copy each monitor internal (10 seconds). The frozen copy ensure "interval", not "internal" Line 16: that careless developers trying to modify a status will get what they Line 17: deserve. Line 18: Line 19: Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773 http://gerrit.ovirt.org/#/c/29014/1/vdsm/storage/domainMonitor.py File vdsm/storage/domainMonitor.py: Line 59: self.isoPrefix = None Line 60: self.version = -1 Line 61: Line 62: Line 63: class FrozenStatus(DomainMonitorStatus): This class should be private to this module, _FrozenStatus Line 64: Line 65: def __init__(self, other): Line 66: for name in other.__slots__: Line 67: value = getattr(other, name) -- To view, visit http://gerrit.ovirt.org/29014 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: configurator.py: fix _removeFile to do as described in it's ...
Yaniv Bronhaim has posted comments on this change.
Change subject: configurator.py: fix _removeFile to do as described in it's doc
string.
..
Patch Set 5:
(1 comment)
http://gerrit.ovirt.org/#/c/28782/5/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 325: try:
Line 326: os.unlink(content['path'])
Line 327: except OSError as e:
Line 328: if e.errno != errno.ENOENT:
Line 329: raise InvalidRun("Removing file: %s failed",
content['path'])
> This is unexpected failure that you cannot handle. It should cause the oper
yep, not so good. you can still use the InvalidRun exception to avoid backtrace
print, but you need to add the original exception message
Line 330:
Line 331: def _unprefixAndRemoveSection(self, path):
Line 332: """
Line 333: undo changes done by _prefixAndPrepend.
--
To view, visit http://gerrit.ovirt.org/28782
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0436832ae63891c097038ef2b76606c30c40328a
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Douglas Schilling Landgraf
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: mooli tayer
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vdsm-upgrade: adds wrapper to ovirt-node-upgrade
Eyal Edri has posted comments on this change. Change subject: vdsm-upgrade: adds wrapper to ovirt-node-upgrade .. Patch Set 17: fabian - i don't see any ovirt-node rpm job in jenkins, so not sure if there is nightly repo ready for this. when do you plan to release a new version to be included in stable? -- To view, visit http://gerrit.ovirt.org/28244 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7b997d70a440545497246d1a19d9671b054a56a5 Gerrit-PatchSet: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf Gerrit-Reviewer: Alon Bar-Lev Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Douglas Schilling Landgraf Gerrit-Reviewer: Eyal Edri Gerrit-Reviewer: Fabian Deutsch Gerrit-Reviewer: Joey Boggs Gerrit-Reviewer: Sandro Bonazzola Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.4]: virt: sampling: make sure balloon values are str
Yaniv Bronhaim has submitted this change and it was merged. Change subject: virt: sampling: make sure balloon values are str .. virt: sampling: make sure balloon values are str This patch avoids XML-RPC integer overflows by ensuring all the balloon stats are string values. Bug-Url: https://bugzilla.redhat.com/284 Change-Id: Ia71ceab52c1247a94ab3a61b3bbafec76106d789 Signed-off-by: Francesco Romani Reviewed-on: http://gerrit.ovirt.org/28954 Reviewed-by: Martin Sivák Reviewed-by: Adam Litke Reviewed-by: Dan Kenigsberg Reviewed-on: http://gerrit.ovirt.org/28973 Reviewed-by: Michal Skrivanek Reviewed-by: Yaniv Bronhaim --- M vdsm/vm.py 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Yaniv Bronhaim: Looks good to me, approved Dan Kenigsberg: Looks good to me, but someone else must approve Francesco Romani: Verified Michal Skrivanek: Looks good to me, but someone else must approve -- To view, visit http://gerrit.ovirt.org/28973 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ia71ceab52c1247a94ab3a61b3bbafec76106d789 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.4 Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Martin Sivák Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: automat...@ovirt.org ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.4]: virt: sampling: make sure balloon values are str
Yaniv Bronhaim has posted comments on this change. Change subject: virt: sampling: make sure balloon values are str .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.ovirt.org/28973 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia71ceab52c1247a94ab3a61b3bbafec76106d789 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.4 Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Martin Sivák Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe iteration of domain monitor status
Yoav Kleinberger has posted comments on this change.
Change subject: domainMonitor: Fix unsafe iteration of domain monitor status
..
Patch Set 1: Code-Review-1
(2 comments)
http://gerrit.ovirt.org/#/c/29009/1/vdsm/storage/domainMonitor.py
File vdsm/storage/domainMonitor.py:
Line 119
Line 120
Line 121
Line 122
Line 123
as long as we're changing a public function, how about less name inflation?
getStatus
instead of
getMonitoredDomainsStatus
Line 118:
Line 119: del self._domains[sdUUID]
Line 120:
Line 121: def getMonitoredDomainsStatus(self):
Line 122: for sdUUID, monitor in self._domains.items():
how is this safer?
if _domains changes, the list you get from items is invalid, and you will yield
invalid values. What am I missing?
Line 123: yield sdUUID, monitor.getStatus()
Line 124:
Line 125: def close(self):
Line 126: self.log.info("Stopping domain monitors")
--
To view, visit http://gerrit.ovirt.org/29009
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I083c4b1925ee529c0993a291cad6076539d4ea66
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Xavi Francisco
Gerrit-Reviewer: Yoav Kleinberger
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: configurator.py: fix _removeFile to do as described in it's ...
mooli tayer has posted comments on this change.
Change subject: configurator.py: fix _removeFile to do as described in it's doc
string.
..
Patch Set 5:
(2 comments)
http://gerrit.ovirt.org/#/c/28782/5//COMMIT_MSG
Commit Message:
Line 6:
Line 7: configurator.py: fix _removeFile to do as described in it's doc string.
Line 8:
Line 9: Remove file in a non racy way without using the utils.rmFile()
Line 10: This method assumes the file exists.
> This -> which
Fixed
"... utils.rmFile() which assumes ..."
Line 11:
Line 12: Change-Id: I0436832ae63891c097038ef2b76606c30c40328a
Line 13: https://bugzilla.redhat.com/show_bug.cgi?id=1109569
http://gerrit.ovirt.org/#/c/28782/5/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 326: os.unlink(content['path'])
Line 327: except OSError as e:
Line 328: if e.errno != errno.ENOENT:
Line 329: raise InvalidRun("Removing file: %s failed",
content['path'])
Line 330:
So should I go with :
if e.errno != errno.ENOENT:
raise InvalidRun("Removing file: %s failed",content['path'],err=e)
Line 331: def _unprefixAndRemoveSection(self, path):
Line 332: """
Line 333: undo changes done by _prefixAndPrepend.
Line 334: """
--
To view, visit http://gerrit.ovirt.org/28782
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0436832ae63891c097038ef2b76606c30c40328a
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Douglas Schilling Landgraf
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: mooli tayer
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: configurator.py: Node unpersist must be called before removi...
mooli tayer has posted comments on this change. Change subject: configurator.py: Node unpersist must be called before removing a file. .. Patch Set 2: (1 comment) http://gerrit.ovirt.org/#/c/28821/2/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 318: """ Line 319: delete a file if it exists. Line 320: """ Line 321: if utils.isOvirtNode(): Line 322: NodeCfg().unpersist(content['path']) Looking at [1] It looks like NodeCfg().delete(filename) and not NodeCfg() unpersist should be called? http://gerrit.ovirt.org/#/c/28879/5/vdsm/network/configurators/ifcfg.py,cm Line 323: utils.rmFile(content['path']) Line 324: Line 325: def _unprefixAndRemoveSection(self, path): Line 326: """ -- To view, visit http://gerrit.ovirt.org/28821 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7226b792c904ff8ca35054e20b85be707c080b49 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: mooli tayer Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Douglas Schilling Landgraf Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe status handling
Nir Soffer has posted comments on this change. Change subject: domainMonitor: Fix unsafe status handling .. Patch Set 1: (2 comments) http://gerrit.ovirt.org/#/c/29014/1//COMMIT_MSG Commit Message: Line 11: was written as if creating a new status object is expensive operation Line 12: that should be avoided. Line 13: Line 14: Instead of having long living status object in the monitor, I create new Line 15: forozen copy each monitor internal (10 seconds). The frozen copy ensure > "interval", not "internal" will fix Line 16: that careless developers trying to modify a status will get what they Line 17: deserve. Line 18: Line 19: Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773 http://gerrit.ovirt.org/#/c/29014/1/vdsm/storage/domainMonitor.py File vdsm/storage/domainMonitor.py: Line 59: self.isoPrefix = None Line 60: self.version = -1 Line 61: Line 62: Line 63: class FrozenStatus(DomainMonitorStatus): > This class should be private to this module, _FrozenStatus It uses the same visibility as is superclass - why do you care about its privacy? If we go in this direction, we should make everything here private except the DomainMonitor class. I don't mind making these private, although I'm not happy work with all those _PrivateClass, but it is not related to this patch. Line 64: Line 65: def __init__(self, other): Line 66: for name in other.__slots__: Line 67: value = getattr(other, name) -- To view, visit http://gerrit.ovirt.org/29014 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe iteration of domain monitor status
Nir Soffer has posted comments on this change.
Change subject: domainMonitor: Fix unsafe iteration of domain monitor status
..
Patch Set 1:
(2 comments)
http://gerrit.ovirt.org/#/c/29009/1/vdsm/storage/domainMonitor.py
File vdsm/storage/domainMonitor.py:
Line 119
Line 120
Line 121
Line 122
Line 123
> as long as we're changing a public function, how about less name inflation?
getStatus was used to get one domain status, we are returning here all domains
status, and this is what the new name is trying to make clear.
Line 118:
Line 119: del self._domains[sdUUID]
Line 120:
Line 121: def getMonitoredDomainsStatus(self):
Line 122: for sdUUID, monitor in self._domains.items():
> how is this safer?
The list you get is valid for the time you ask about it, and this is the list
you should report to the engine back. If a domain was stopped 1 second or 1
millisecond after you asked it is not interesting.
This is like doing stat on a file - anything you return may be wrong because
the file may have changed after you checked its stats.
Line 123: yield sdUUID, monitor.getStatus()
Line 124:
Line 125: def close(self):
Line 126: self.log.info("Stopping domain monitors")
--
To view, visit http://gerrit.ovirt.org/29009
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I083c4b1925ee529c0993a291cad6076539d4ea66
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Xavi Francisco
Gerrit-Reviewer: Yoav Kleinberger
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: configurator.py: fix _removeFile to do as described in it's ...
Nir Soffer has posted comments on this change.
Change subject: configurator.py: fix _removeFile to do as described in it's doc
string.
..
Patch Set 5:
(2 comments)
http://gerrit.ovirt.org/#/c/28782/5/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 325: try:
Line 326: os.unlink(content['path'])
Line 327: except OSError as e:
Line 328: if e.errno != errno.ENOENT:
Line 329: raise InvalidRun("Removing file: %s failed",
content['path'])
> yep, not so good. you can still use the InvalidRun exception to avoid backt
No, we *want* backtrace in this case! The InvalidRun and other errors were
added to not show a backtrace in expected conditions, not to hide real errors
that we cannot handle.
Line 330:
Line 331: def _unprefixAndRemoveSection(self, path):
Line 332: """
Line 333: undo changes done by _prefixAndPrepend.
Line 326: os.unlink(content['path'])
Line 327: except OSError as e:
Line 328: if e.errno != errno.ENOENT:
Line 329: raise InvalidRun("Removing file: %s failed",
content['path'])
Line 330:
> So should I go with :
No, just raise - this is the case where the tool must crash. Something is very
wrong with the environment, and the only way is to crash loudly and let the
administrator fix the environment.
Line 331: def _unprefixAndRemoveSection(self, path):
Line 332: """
Line 333: undo changes done by _prefixAndPrepend.
Line 334: """
--
To view, visit http://gerrit.ovirt.org/28782
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0436832ae63891c097038ef2b76606c30c40328a
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Douglas Schilling Landgraf
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: mooli tayer
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe iteration of domain monitor status
Yoav Kleinberger has posted comments on this change.
Change subject: domainMonitor: Fix unsafe iteration of domain monitor status
..
Patch Set 1:
(2 comments)
http://gerrit.ovirt.org/#/c/29009/1/vdsm/storage/domainMonitor.py
File vdsm/storage/domainMonitor.py:
Line 119
Line 120
Line 121
Line 122
Line 123
> getStatus was used to get one domain status, we are returning here all doma
the object you call getStatus on gives you the proper context to understand the
meaning of "getStatus".
Look at the usage of getMonitoredDomainsStatus - the domainMonitor object
clarifies that you get the status for the monitored domains.
the same goes for this bit: it's clear that you get the status of a single
domain.
Line 118:
Line 119: del self._domains[sdUUID]
Line 120:
Line 121: def getMonitoredDomainsStatus(self):
Line 122: for sdUUID, monitor in self._domains.items():
> The list you get is valid for the time you ask about it, and this is the li
OK
Line 123: yield sdUUID, monitor.getStatus()
Line 124:
Line 125: def close(self):
Line 126: self.log.info("Stopping domain monitors")
--
To view, visit http://gerrit.ovirt.org/29009
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I083c4b1925ee529c0993a291cad6076539d4ea66
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Xavi Francisco
Gerrit-Reviewer: Yoav Kleinberger
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logs: improve VDSM logs human-readability
Yoav Kleinberger has posted comments on this change. Change subject: logs: improve VDSM logs human-readability .. Patch Set 4: Marina, the flowID is not part of the format of the log - it is generated inside VDSM's code, so it will be retained in the new format also, no worries. -- To view, visit http://gerrit.ovirt.org/28869 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65fcc843b4c2c6f1b0f438937a0702b80b714978 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yoav Kleinberger Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Liron Aravot Gerrit-Reviewer: Maor Lipchuk Gerrit-Reviewer: Marina Kalinin Gerrit-Reviewer: Martin Sivák Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Vered Volansky Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: configurator.py: fix _removeFile to do as described in it's ...
Yaniv Bronhaim has posted comments on this change.
Change subject: configurator.py: fix _removeFile to do as described in it's doc
string.
..
Patch Set 5:
(1 comment)
http://gerrit.ovirt.org/#/c/28782/5/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 326: os.unlink(content['path'])
Line 327: except OSError as e:
Line 328: if e.errno != errno.ENOENT:
Line 329: raise InvalidRun("Removing file: %s failed",
content['path'])
Line 330:
> No, just raise - this is the case where the tool must crash. Something is v
i dont' mind
Line 331: def _unprefixAndRemoveSection(self, path):
Line 332: """
Line 333: undo changes done by _prefixAndPrepend.
Line 334: """
--
To view, visit http://gerrit.ovirt.org/28782
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0436832ae63891c097038ef2b76606c30c40328a
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Douglas Schilling Landgraf
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: mooli tayer
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe status handling
Yoav Kleinberger has posted comments on this change. Change subject: domainMonitor: Fix unsafe status handling .. Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/29014/1/vdsm/storage/domainMonitor.py File vdsm/storage/domainMonitor.py: Line 59: self.isoPrefix = None Line 60: self.version = -1 Line 61: Line 62: Line 63: class FrozenStatus(DomainMonitorStatus): > It uses the same visibility as is superclass - why do you care about its pr It should be private, so when I look at it, I know that I can change it without looking for who uses it. Since this is a new class, and we know for sure no one uses it outside this module, it should be private to the module. Perhaps this is not observed in most of VDSM's code - it is no reason to continue bad practices. Line 64: Line 65: def __init__(self, other): Line 66: for name in other.__slots__: Line 67: value = getattr(other, name) -- To view, visit http://gerrit.ovirt.org/29014 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe iteration
Yoav Kleinberger has posted comments on this change. Change subject: domainMonitor: Fix unsafe iteration .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.ovirt.org/29007 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5313d25c7148e4a0362d19fe31d2e78f94c26e39 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe iteration of domain monitor status
Nir Soffer has posted comments on this change. Change subject: domainMonitor: Fix unsafe iteration of domain monitor status .. Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/29009/1/vdsm/storage/domainMonitor.py File vdsm/storage/domainMonitor.py: Line 119 Line 120 Line 121 Line 122 Line 123 > the object you call getStatus on gives you the proper context to understand When you getStatus, you expect it ot return one value. We cannot use this to return a generator. Note that we have: domainMonitor.monitoredDomains domainMonitor.poolMonitoredDomains So the new method use the same convention: domainMonitor.getMonitoredDomainsStatus If we go with your suggestion we have to rename monitoredDomains to domains and poolDomains. This may be better I don't want to fix the names in this patch, only fix the issue with the iteration. I think we can fix the names in a following patch, including the privacy changes you asked in the other patch. -- To view, visit http://gerrit.ovirt.org/29009 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I083c4b1925ee529c0993a291cad6076539d4ea66 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe status handling
Nir Soffer has posted comments on this change. Change subject: domainMonitor: Fix unsafe status handling .. Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/29014/1/vdsm/storage/domainMonitor.py File vdsm/storage/domainMonitor.py: Line 59: self.isoPrefix = None Line 60: self.version = -1 Line 61: Line 62: Line 63: class FrozenStatus(DomainMonitorStatus): > It should be private, so when I look at it, I know that I can change it wit Makes sense - but DomainMonitorStatus and FrozenStatus should be public - they are what you get from getStatus, and other modules use them. So you cannot treat them as private. If we want to fix privacy, we should make DomainMonitorThread private, since you should never access it outside of this module. Line 64: Line 65: def __init__(self, other): Line 66: for name in other.__slots__: Line 67: value = getattr(other, name) -- To view, visit http://gerrit.ovirt.org/29014 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: ioprocess implementation
oVirt Jenkins CI Server has posted comments on this change. Change subject: ioprocess implementation .. Patch Set 25: Code-Review-1 Verified-1 Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9516/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10301/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10457/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5383/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3541/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/26967 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I132129315c73e880d998a13f84e822a9d4fec2a6 Gerrit-PatchSet: 25 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan Gerrit-Reviewer: Barak Azulay Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: oop: Add an option to configure oop implementation
oVirt Jenkins CI Server has posted comments on this change. Change subject: oop: Add an option to configure oop implementation .. Patch Set 26: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9515/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/776/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10299/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10455/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5381/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3539/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc19_created/7/ : FAILURE -- To view, visit http://gerrit.ovirt.org/26576 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibd756afd43d23631dc7ed4bac64bec9a81b358b4 Gerrit-PatchSet: 26 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan Gerrit-Reviewer: Barak Azulay Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Additional oop functionality
oVirt Jenkins CI Server has posted comments on this change. Change subject: Additional oop functionality .. Patch Set 19: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9517/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10300/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10456/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5382/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3540/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/27641 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I438f01236767e807f010be7531678ee5b1a05056 Gerrit-PatchSet: 19 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Dima Kuznetsov Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fencing: Introduce getHostLeaseStatus API
Itamar Heim has posted comments on this change. Change subject: fencing: Introduce getHostLeaseStatus API .. Patch Set 5: reading the commit message - what is the difference between unknown, fail and dead? -- To view, visit http://gerrit.ovirt.org/28873 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iccd62e58a194aa0ceb0f5e2503b8ec7e4349971b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Barak Azulay Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Final separation of IOProcess and RFH
oVirt Jenkins CI Server has posted comments on this change. Change subject: Final separation of IOProcess and RFH .. Patch Set 16: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9518/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10302/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10458/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5384/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3542/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/28088 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ief85d2dca2d22058c4ed2504e49dc3dd62547532 Gerrit-PatchSet: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe status handling
Yoav Kleinberger has posted comments on this change. Change subject: domainMonitor: Fix unsafe status handling .. Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/29014/1/vdsm/storage/domainMonitor.py File vdsm/storage/domainMonitor.py: Line 59: self.isoPrefix = None Line 60: self.version = -1 Line 61: Line 62: Line 63: class FrozenStatus(DomainMonitorStatus): > Makes sense - but DomainMonitorStatus and FrozenStatus should be public - t I don't agree that simply because you return some object, its class should be public. The class should be public only it is explicitly called upon from outside, not implicitly. regarding DomainMonitorThread - I agree. Line 64: Line 65: def __init__(self, other): Line 66: for name in other.__slots__: Line 67: value = getattr(other, name) -- To view, visit http://gerrit.ovirt.org/29014 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe iteration of domain monitor status
Yoav Kleinberger has posted comments on this change. Change subject: domainMonitor: Fix unsafe iteration of domain monitor status .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.ovirt.org/#/c/29009/1/vdsm/storage/domainMonitor.py File vdsm/storage/domainMonitor.py: Line 119 Line 120 Line 121 Line 122 Line 123 > When you getStatus, you expect it ot return one value. We cannot use this t OK -- To view, visit http://gerrit.ovirt.org/29009 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I083c4b1925ee529c0993a291cad6076539d4ea66 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fencing: Introduce getHostLeaseStatus API
Nir Soffer has posted comments on this change. Change subject: fencing: Introduce getHostLeaseStatus API .. Patch Set 5: From https://git.fedorahosted.org/cgit/sanlock.git/tree/src/lockspace.c#n937 /* * After the lockspace starts, there is a limited amount of * time that we've been watching the other hosts. This means * we can't make an accurate assessment of their state, because * the state is based on monitoring the hosts for host_fail_seconds * and host_dead_seconds, or seeing a renewal. When none of * those are true (not enough time monitoring and not seeing a * renewal), we return UNKNOWN. * * (Example number of seconds below are based on hosts using the * default 10 second io timeout.) * * * For hosts that are alive when we start, we return: * UNKNOWN then LIVE * * UNKNOWN would typically last for 10-20 seconds, but it's possible that * UNKNOWN could persist for up to 80 seconds before LIVE is returned. * LIVE is returned after we see the timestamp change once. * * * For hosts that are dead when we start, we'd return: * UNKNOWN then FAIL then DEAD * * UNKNOWN would last for 80 seconds before we return FAIL. * FAIL would last for 60 more seconds before we return DEAD. * * * Hosts that are failing and don't recover would be the same as prev. * * * For hosts thet are failing but recover, we'd return: * UNKNOWN then FAIL then LIVE * * * For another host that is alive when we start, * the sequence of values is: * * 0: we have not yet called check_other_leases() * first_check = 0, last_check = 0, last_live = 0 * * other host renews its lease * * 10: we call check_other_leases() for the first time, * first_check = 10, last_check = 10, last_live = 10 * * other host renews its lease * * 20: we call check_other_leases() for the second time, * first_check = 10, last_check = 20, last_live = 20 * * At 10, we have not yet seen a renewal from the other host, i.e. we have * not seen its timestamp change (we only have one sample). The host could * be dead or alive, so we set the state to UNKNOWN. The way we know * that we have not yet observed the timestamp change is that * first_check == last_live, (10 == 10). * * At 20, we have seen a renewal, i.e. the timestamp changed between checks, * so we return LIVE. * * In the other case, if the host was actually dead, not alive, it would not * have renewed between 10 and 20. So at 20 we would continue to see * first_check == last_live, and would return UNKNOWN. If the host remains * dead, we'd continue to report UNKNOWN for the first 80 seconds. * After 80 seconds, we'd return FAIL. After 140 seconds we'd return DEAD. */ I think we need to take couple of samples, starting when we suspect that a host is not healthy, and finishing at least 80 seconds later. Then when we fence a host, we can look at the samples and apply various policies. The simplest policy would be to avoid fencing if a host is live on at least one domain after waiting 80 seconds or so. Maybe we should move this discussion to the mailing list? -- To view, visit http://gerrit.ovirt.org/28873 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iccd62e58a194aa0ceb0f5e2503b8ec7e4349971b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Barak Azulay Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Itamar Heim Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe status handling
Nir Soffer has posted comments on this change. Change subject: domainMonitor: Fix unsafe status handling .. Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/29014/1/vdsm/storage/domainMonitor.py File vdsm/storage/domainMonitor.py: Line 59: self.isoPrefix = None Line 60: self.version = -1 Line 61: Line 62: Line 63: class FrozenStatus(DomainMonitorStatus): > I don't agree that simply because you return some object, its class should But if we treat DomainMonitorStatus as private, people may assume that it is not accessed outside this module, and remove an instance variable used from another module. This is how it work in languages enforcing privacy like java. Since we don't have interfaces, what left is have public like names for such classes. Line 64: Line 65: def __init__(self, other): Line 66: for name in other.__slots__: Line 67: value = getattr(other, name) -- To view, visit http://gerrit.ovirt.org/29014 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.4]: virt: sampling: make sure balloon values are str
oVirt Jenkins CI Server has posted comments on this change. Change subject: virt: sampling: make sure balloon values are str .. Patch Set 2: Build Failed http://jenkins.ovirt.org/job/vdsm_3.4_create-rpms_merged/232/ : FAILURE -- To view, visit http://gerrit.ovirt.org/28973 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia71ceab52c1247a94ab3a61b3bbafec76106d789 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.4 Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Martin Sivák Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fencing: Introduce getHostLeaseStatus API
Yoav Kleinberger has posted comments on this change.
Change subject: fencing: Introduce getHostLeaseStatus API
..
Patch Set 5: Code-Review-1
(3 comments)
http://gerrit.ovirt.org/#/c/28873/5/tests/testrunner.py
File tests/testrunner.py:
Line 53
Line 54
Line 55
Line 56
Line 57
I don't see any test code referring to this. What's the use of this mock?
http://gerrit.ovirt.org/#/c/28873/5/vdsm/storage/clusterlock.py
File vdsm/storage/clusterlock.py:
Line 41: SDM_LEASE_NAME = 'SDM'
Line 42: SDM_LEASE_OFFSET = 512 * 2048
Line 43:
Line 44: # Host status codes
Line 45: HOST_UNKNOWN = sanlock.HOST_UNKNOWN
I suggest using strings instead of numbers. Translate the sanlock constants to
names
STATES = { sanlock.HOST_FREE: 'free', sanlock.HOST_LIVE: 'live', ... }
whenever such a string appears in a log, the human trying to figure it out will
understand it immediately.
Line 46: HOST_FREE = sanlock.HOST_FREE
Line 47: HOST_LIVE = sanlock.HOST_LIVE
Line 48: HOST_FAIL = sanlock.HOST_FAIL
Line 49: HOST_DEAD = sanlock.HOST_DEAD
Line 56: class HostStatusNotAvailable(Exception):
Line 57: """
Line 58: Raised when host status is not available because the cluster lock
does not
Line 59: support this operation, or raised an exception.
Line 60: """
supposedly, you are trying to hide sanlock from outside users by replacing a
SanlockException with this. Is this really needed? YAGNI
suppose shutil wrapped all os.OSError etc with ShutilException. What good would
it do the user?
I'm not convinced this does any real good. Delete this, and you can get rid of
lots of code - always a good thing.
Line 61:
Line 62:
Line 63: class SafeLease(object):
Line 64: log = logging.getLogger("Storage.SafeLease")
--
To view, visit http://gerrit.ovirt.org/28873
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iccd62e58a194aa0ceb0f5e2503b8ec7e4349971b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Barak Azulay
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Itamar Heim
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Xavi Francisco
Gerrit-Reviewer: Yoav Kleinberger
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Fix unsafe status handling
Yoav Kleinberger has posted comments on this change. Change subject: domainMonitor: Fix unsafe status handling .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.ovirt.org/#/c/29014/1/vdsm/storage/domainMonitor.py File vdsm/storage/domainMonitor.py: Line 59: self.isoPrefix = None Line 60: self.version = -1 Line 61: Line 62: Line 63: class FrozenStatus(DomainMonitorStatus): > But if we treat DomainMonitorStatus as private, people may assume that it i OK Line 64: Line 65: def __init__(self, other): Line 66: for name in other.__slots__: Line 67: value = getattr(other, name) -- To view, visit http://gerrit.ovirt.org/29014 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If4affb67b6e65382dc4e55ebad9443e2722f2773 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Xavi Francisco Gerrit-Reviewer: Yoav Kleinberger Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fencing: Introduce getHostLeaseStatus API
Nir Soffer has posted comments on this change.
Change subject: fencing: Introduce getHostLeaseStatus API
..
Patch Set 5:
(3 comments)
http://gerrit.ovirt.org/#/c/28873/5/tests/testrunner.py
File tests/testrunner.py:
Line 53
Line 54
Line 55
Line 56
Line 57
> I don't see any test code referring to this. What's the use of this mock?
The clustlerlock module define constants using sanlock constants, so using
object() breaks this code during the tests.
http://gerrit.ovirt.org/#/c/28873/5/vdsm/storage/clusterlock.py
File vdsm/storage/clusterlock.py:
Line 41: SDM_LEASE_NAME = 'SDM'
Line 42: SDM_LEASE_OFFSET = 512 * 2048
Line 43:
Line 44: # Host status codes
Line 45: HOST_UNKNOWN = sanlock.HOST_UNKNOWN
> I suggest using strings instead of numbers. Translate the sanlock constants
This is indeed nicer for the log and for the engine, but for code inside vdsm
that uses these statuses, it is better that code will use a constant instead of
string literals.
So we can do:
HOST_STATUS = {sanlock.HOST_UNKNOWN: HOST_UNKNOWN, ...}
HOST_UNKNOWN = "unknown"
And in getHostStatus:
return HOST_STATUS[sanlock_status]
Waiting for others review with this.
Line 46: HOST_FREE = sanlock.HOST_FREE
Line 47: HOST_LIVE = sanlock.HOST_LIVE
Line 48: HOST_FAIL = sanlock.HOST_FAIL
Line 49: HOST_DEAD = sanlock.HOST_DEAD
Line 56: class HostStatusNotAvailable(Exception):
Line 57: """
Line 58: Raised when host status is not available because the cluster lock
does not
Line 59: support this operation, or raised an exception.
Line 60: """
> supposedly, you are trying to hide sanlock from outside users by replacing
Users of this class should not depend on sanlock, unlike os module. Think about
the client code using this:
import sanlock
import foolock
import barlock
try:
return dommain.getHostStatus()
except (sanlock.SanlockException, foolock.FooLockException,
barlock.BarLockException):
...
We don't want to go there.
clusterlock.py is like os.py - it returns common errors for multiple
clusterlocks and os return common errors for multiple os (e.g. posix, nt).
Line 61:
Line 62:
Line 63: class SafeLease(object):
Line 64: log = logging.getLogger("Storage.SafeLease")
--
To view, visit http://gerrit.ovirt.org/28873
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iccd62e58a194aa0ceb0f5e2503b8ec7e4349971b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Barak Azulay
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Itamar Heim
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Xavi Francisco
Gerrit-Reviewer: Yoav Kleinberger
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vdsm-upgrade: adds wrapper to ovirt-node-upgrade
Sandro Bonazzola has posted comments on this change.
Change subject: vdsm-upgrade: adds wrapper to ovirt-node-upgrade
..
Patch Set 17:
(1 comment)
http://gerrit.ovirt.org/#/c/28244/17/vdsm_reg/vdsm-upgrade
File vdsm_reg/vdsm-upgrade:
Line 15:
Line 16: from xml.sax import saxutils
Line 17:
Line 18: _log_file = '/var/log/vdsm-reg/vds_bootstrap_upgrade.' + \
Line 19: time.strftime("%Y%m%d_%H%M%S")+'.log'
missing space around operator +
also, why not just use "/path/%s.log" % time ?
Line 20:
Line 21: logging.basicConfig(
Line 22: level=logging.DEBUG,
Line 23: format='%(asctime)s %(levelname)-8s %(message)s',
--
To view, visit http://gerrit.ovirt.org/28244
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b997d70a440545497246d1a19d9671b054a56a5
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf
Gerrit-Reviewer: Alon Bar-Lev
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Douglas Schilling Landgraf
Gerrit-Reviewer: Eyal Edri
Gerrit-Reviewer: Fabian Deutsch
Gerrit-Reviewer: Joey Boggs
Gerrit-Reviewer: Sandro Bonazzola
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fencing: Introduce getHostLeaseStatus API
Yoav Kleinberger has posted comments on this change.
Change subject: fencing: Introduce getHostLeaseStatus API
..
Patch Set 5:
(2 comments)
http://gerrit.ovirt.org/#/c/28873/5/vdsm/storage/clusterlock.py
File vdsm/storage/clusterlock.py:
Line 41: SDM_LEASE_NAME = 'SDM'
Line 42: SDM_LEASE_OFFSET = 512 * 2048
Line 43:
Line 44: # Host status codes
Line 45: HOST_UNKNOWN = sanlock.HOST_UNKNOWN
> This is indeed nicer for the log and for the engine, but for code inside vd
I'm OK with your suggestion regarding this issue.
Line 46: HOST_FREE = sanlock.HOST_FREE
Line 47: HOST_LIVE = sanlock.HOST_LIVE
Line 48: HOST_FAIL = sanlock.HOST_FAIL
Line 49: HOST_DEAD = sanlock.HOST_DEAD
Line 56: class HostStatusNotAvailable(Exception):
Line 57: """
Line 58: Raised when host status is not available because the cluster lock
does not
Line 59: support this operation, or raised an exception.
Line 60: """
> Users of this class should not depend on sanlock, unlike os module. Think a
if this future arrives, we can deal with it then. YAGNI
Line 61:
Line 62:
Line 63: class SafeLease(object):
Line 64: log = logging.getLogger("Storage.SafeLease")
--
To view, visit http://gerrit.ovirt.org/28873
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iccd62e58a194aa0ceb0f5e2503b8ec7e4349971b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Barak Azulay
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Itamar Heim
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Xavi Francisco
Gerrit-Reviewer: Yoav Kleinberger
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fencing: Introduce getHostLeaseStatus API
Nir Soffer has posted comments on this change.
Change subject: fencing: Introduce getHostLeaseStatus API
..
Patch Set 5:
(1 comment)
http://gerrit.ovirt.org/#/c/28873/5/vdsm/storage/clusterlock.py
File vdsm/storage/clusterlock.py:
Line 56: class HostStatusNotAvailable(Exception):
Line 57: """
Line 58: Raised when host status is not available because the cluster lock
does not
Line 59: support this operation, or raised an exception.
Line 60: """
> if this future arrives, we can deal with it then. YAGNI
The future is here - this module already has 3 types of clusterlocks, raising
sanlock specific error is not acceptable now.
Line 61:
Line 62:
Line 63: class SafeLease(object):
Line 64: log = logging.getLogger("Storage.SafeLease")
--
To view, visit http://gerrit.ovirt.org/28873
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iccd62e58a194aa0ceb0f5e2503b8ec7e4349971b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Barak Azulay
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Itamar Heim
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Xavi Francisco
Gerrit-Reviewer: Yoav Kleinberger
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Introduce cancellation points
Yoav Kleinberger has posted comments on this change.
Change subject: domainMonitor: Introduce cancellation points
..
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/28721/4/vdsm/storage/domainMonitor.py
File vdsm/storage/domainMonitor.py:
Line 207: try:
Line 208: self._monitorDomain()
Line 209: except utils.Canceled:
Line 210: self.log.debug("Canceled domain monitor for %s",
self.sdUUID)
Line 211: break
I prefer
return
instead of "break". No real difference, but takes about 10s less brain time to
understand, i.e. more readable.
Line 212: except:
Line 213: self.log.error("The domain monitor for %s failed
unexpectedly",
Line 214:self.sdUUID, exc_info=True)
Line 215: self.stopEvent.wait(self.interval)
--
To view, visit http://gerrit.ovirt.org/28721
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0a965758736a0b9fa7ac7c2e105bcbbfb04163b8
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yoav Kleinberger
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: fencing: Introduce getHostLeaseStatus API
Yoav Kleinberger has posted comments on this change.
Change subject: fencing: Introduce getHostLeaseStatus API
..
Patch Set 5: Code-Review+1
(1 comment)
http://gerrit.ovirt.org/#/c/28873/5/vdsm/storage/clusterlock.py
File vdsm/storage/clusterlock.py:
Line 56: class HostStatusNotAvailable(Exception):
Line 57: """
Line 58: Raised when host status is not available because the cluster lock
does not
Line 59: support this operation, or raised an exception.
Line 60: """
> The future is here - this module already has 3 types of clusterlocks, raisi
OK
Line 61:
Line 62:
Line 63: class SafeLease(object):
Line 64: log = logging.getLogger("Storage.SafeLease")
--
To view, visit http://gerrit.ovirt.org/28873
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iccd62e58a194aa0ceb0f5e2503b8ec7e4349971b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Barak Azulay
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Itamar Heim
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Xavi Francisco
Gerrit-Reviewer: Yoav Kleinberger
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: domainMonitor: Introduce cancellation points
Nir Soffer has posted comments on this change.
Change subject: domainMonitor: Introduce cancellation points
..
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/28721/4/vdsm/storage/domainMonitor.py
File vdsm/storage/domainMonitor.py:
Line 207: try:
Line 208: self._monitorDomain()
Line 209: except utils.Canceled:
Line 210: self.log.debug("Canceled domain monitor for %s",
self.sdUUID)
Line 211: break
> I prefer
ok
Line 212: except:
Line 213: self.log.error("The domain monitor for %s failed
unexpectedly",
Line 214:self.sdUUID, exc_info=True)
Line 215: self.stopEvent.wait(self.interval)
--
To view, visit http://gerrit.ovirt.org/28721
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0a965758736a0b9fa7ac7c2e105bcbbfb04163b8
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Allon Mureinik
Gerrit-Reviewer: Antoni Segura Puimedon
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yoav Kleinberger
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: stats: get rid of _getStatsInternal
Dan Kenigsberg has submitted this change and it was merged. Change subject: virt: stats: get rid of _getStatsInternal .. virt: stats: get rid of _getStatsInternal it was used on just one place anyway. Change-Id: I469759b86115d80bad62c3449fe9084d5f2e550b Signed-off-by: Francesco Romani Reviewed-on: http://gerrit.ovirt.org/26557 Reviewed-by: Dan Kenigsberg --- M vdsm/virt/vm.py 1 file changed, 3 insertions(+), 7 deletions(-) Approvals: Dan Kenigsberg: Looks good to me, approved Francesco Romani: Verified -- To view, visit http://gerrit.ovirt.org/26557 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I469759b86115d80bad62c3449fe9084d5f2e550b Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: stats: get rid of _getStatsInternal
Dan Kenigsberg has posted comments on this change. Change subject: virt: stats: get rid of _getStatsInternal .. Patch Set 14: Code-Review+2 -- To view, visit http://gerrit.ovirt.org/26557 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I469759b86115d80bad62c3449fe9084d5f2e550b Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sampling: allow window=1 in AdvancedStatsFunction
Dan Kenigsberg has posted comments on this change. Change subject: sampling: allow window=1 in AdvancedStatsFunction .. Patch Set 1: Code-Review-1 -1 for visibility -- To view, visit http://gerrit.ovirt.org/28992 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ce1fed5a211ed58b7df4728d5bfc3c17fccfbd8 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: janitorial: move isVdsmImage into utils
Dan Kenigsberg has posted comments on this change. Change subject: janitorial: move isVdsmImage into utils .. Patch Set 5: Code-Review-1 (2 comments) http://gerrit.ovirt.org/#/c/28477/5//COMMIT_MSG Commit Message: Line 7: janitorial: move isVdsmImage into utils Line 8: Line 9: this patch moves vdsm/virt/vm.isVdsmImage into utils without code Line 10: changes. Line 11: The move has little oif any benefit on its own, but it is a preliminary oif->if Line 12: step to the move of VmSamplingThread from vm.py to sampling.py. Line 13: Line 14: Change-Id: I9cb288eef41b567da36849e00f848e1ba20a62af Line 8: Line 9: this patch moves vdsm/virt/vm.isVdsmImage into utils without code Line 10: changes. Line 11: The move has little oif any benefit on its own, but it is a preliminary Line 12: step to the move of VmSamplingThread from vm.py to sampling.py. But why take the code out of the virt package, where it naturally belongs? Can we avoid that? Line 13: Line 14: Change-Id: I9cb288eef41b567da36849e00f848e1ba20a62af -- To view, visit http://gerrit.ovirt.org/28477 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9cb288eef41b567da36849e00f848e1ba20a62af Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: stats: get rid of _getStatsInternal
oVirt Jenkins CI Server has posted comments on this change. Change subject: virt: stats: get rid of _getStatsInternal .. Patch Set 15: Build Successful http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged/1500/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/26557 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I469759b86115d80bad62c3449fe9084d5f2e550b Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Antoni Segura Puimedon Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: tests: Add symlink mount test
Nir Soffer has posted comments on this change. Change subject: tests: Add symlink mount test .. Patch Set 5: Enrico, as you can see I cheated and I don't test gfs2, but my test does reproduce the issue in a simpler way. -- To view, visit http://gerrit.ovirt.org/27514 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f3d6333921505846f345d015907e5f6174af4f8 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Enrico Tagliavini Gerrit-Reviewer: Enrico Tagliavini Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: tests: Add symlink mount test
oVirt Jenkins CI Server has posted comments on this change. Change subject: tests: Add symlink mount test .. Patch Set 5: Code-Review-1 Verified-1 Build Unstable http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9519/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10303/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10459/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5385/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3543/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/27514 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f3d6333921505846f345d015907e5f6174af4f8 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Enrico Tagliavini Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Enrico Tagliavini Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: tests: Add symlink mount test
Nir Soffer has posted comments on this change. Change subject: tests: Add symlink mount test .. Patch Set 5: Verified+1 Test fail if I disable the fix for this issue in mount.py. Succeeds with current code. -- To view, visit http://gerrit.ovirt.org/27514 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f3d6333921505846f345d015907e5f6174af4f8 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Enrico Tagliavini Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Enrico Tagliavini Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: hsm: [WIP] Support force detach on orphaned Storage Domains.
Maor Lipchuk has uploaded a new change for review.
Change subject: hsm: [WIP] Support force detach on orphaned Storage Domains.
..
hsm: [WIP] Support force detach on orphaned Storage Domains.
Force detach of storage domain only being supported on storage domains which are
part of pool.
For supporting import Data Storage Domain feature, we should also
support force detach of Storage Domain which are not connected to any
pool.
The force detach will clear the SP_UUID meta data of the imported
Storage Domain and will enable the engine to attach it to other DC in
different setups.
Signed-off-by: Maor Lipchuk
Change-Id: I4a6fb01775f7c834a3466e45b9b7ed9b4bd5c3be
---
M vdsm/storage/hsm.py
M vdsm/storage/sp.py
2 files changed, 8 insertions(+), 4 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/42/29042/1
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index cc2d8a0..c26fc0a 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -755,7 +755,14 @@
pool = self.getPool(spUUID)
if sdUUID == pool.masterDomain.sdUUID:
raise se.CannotDetachMasterStorageDomain(sdUUID)
-pool.forcedDetachSD(sdUUID)
+domains = pool.getDomains()
+if sdUUID in domains:
+pool.forcedDetachSD(sdUUID)
+else:
+dom = sdCache.produce(sdUUID=sdUUID)
+self.log.info("Preparing to delete sdUUID=%s", sdUUID)
+dom.detach(dom.getPools()[0])
+self.log.info("delete sdUUID=%s", sdUUID)
@public
def detachStorageDomain(self, sdUUID, spUUID, msdUUID=None,
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index 2b4f2ce..c6f2bee 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -927,9 +927,6 @@
self.log.warn("Force detaching domain `%s`", sdUUID)
domains = self.getDomains()
-if sdUUID not in domains:
-return True
-
del domains[sdUUID]
self._backend.setDomainsMap(domains)
--
To view, visit http://gerrit.ovirt.org/29042
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4a6fb01775f7c834a3466e45b9b7ed9b4bd5c3be
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Maor Lipchuk
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: hsm: [WIP] Support force detach on orphaned Storage Domains.
oVirt Jenkins CI Server has posted comments on this change. Change subject: hsm: [WIP] Support force detach on orphaned Storage Domains. .. Patch Set 1: Build Successful http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9520/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10304/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/10460/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5386/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3544/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/29042 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4a6fb01775f7c834a3466e45b9b7ed9b4bd5c3be Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: misc: Simplify DynamicBarrier implemenation
Yaniv Bronhaim has posted comments on this change. Change subject: misc: Simplify DynamicBarrier implemenation .. Patch Set 4: (1 comment) http://gerrit.ovirt.org/#/c/29003/4/vdsm/storage/misc.py File vdsm/storage/misc.py: Line 652: Line 653: class DynamicBarrier(object): Line 654: def __init__(self): Line 655: self._cond = threading.Condition() Line 656: self._busy = False i don't understand the reason for the lock, why wasn't it a boolean at first, how nonblocking lock could help here even if condition can return? Line 657: Line 658: def enter(self): Line 659: """ Line 660: Enter the dynamic barrier. Returns True if you should be -- To view, visit http://gerrit.ovirt.org/29003 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia403cd9f734eaafdce95ba5123e54ec4e4939172 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Allon Mureinik Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: LiveMerge: Add liveMerge capability to vdsCaps
Francesco Romani has posted comments on this change. Change subject: LiveMerge: Add liveMerge capability to vdsCaps .. Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/28998/1/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 1155: # @kdumpStatus: The current status of kdump configuration for the host: Line 1156: # enabled (1), disabled(0), unknown(-1) Line 1157: # (new in version 4.15.0) Line 1158: # Line 1159: # @liveMerge: #optional Indicates if live merge is supported on this > Thanks for your review! I never thought of this semantic! But I definitely see your point, and I'm fine with it. Let'a see what the maintainer (Dan) thinks about this. Line 1160: # host. Line 1161: # (new in version 4.15.0) Line 1162: # Line 1163: # Since: 4.15.0 -- To view, visit http://gerrit.ovirt.org/28998 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iac66c679166b5687ed3940e517fe6827fe10e258 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Greg Padgett Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: janitorial: move isVdsmImage into utils
Francesco Romani has posted comments on this change. Change subject: janitorial: move isVdsmImage into utils .. Patch Set 5: (2 comments) http://gerrit.ovirt.org/#/c/28477/5//COMMIT_MSG Commit Message: Line 7: janitorial: move isVdsmImage into utils Line 8: Line 9: this patch moves vdsm/virt/vm.isVdsmImage into utils without code Line 10: changes. Line 11: The move has little oif any benefit on its own, but it is a preliminary > oif->if Done Line 12: step to the move of VmSamplingThread from vm.py to sampling.py. Line 13: Line 14: Change-Id: I9cb288eef41b567da36849e00f848e1ba20a62af Line 8: Line 9: this patch moves vdsm/virt/vm.isVdsmImage into utils without code Line 10: changes. Line 11: The move has little oif any benefit on its own, but it is a preliminary Line 12: step to the move of VmSamplingThread from vm.py to sampling.py. > But why take the code out of the virt package, where it naturally belongs? I can begin a new utils-like module inside the virt package. Would that be ok? Line 13: Line 14: Change-Id: I9cb288eef41b567da36849e00f848e1ba20a62af -- To view, visit http://gerrit.ovirt.org/28477 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9cb288eef41b567da36849e00f848e1ba20a62af Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
