Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator ......................................................................
Patch Set 7: Code-Review-1 (6 comments) Need to update for new Configurator interface http://gerrit.ovirt.org/#/c/30909/7//COMMIT_MSG Commit Message: Line 14: Line 15: Now it will be reconfigured only on user demand. Line 16: Line 17: Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da Line 18: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1076531 You can shorten this to: https://bugzilla.redhat.com/1076531 http://gerrit.ovirt.org/#/c/30909/7/lib/vdsm/tool/configurators/multipath.py File lib/vdsm/tool/configurators/multipath.py: Line 1: # Copyright 2014 Red Hat, Inc. This is not new code, mostly copied from storage/multipath.py, so maybe use 2009-2014? Line 2: # Line 3: # This program is free software; you can redistribute it and/or modify Line 4: # it under the terms of the GNU General Public License as published by Line 5: # the Free Software Foundation; either version 2 of the License, or Line 87: "/lib/udev/scsi_id", # Ubuntu Line 88: ) Line 89: Line 90: def getName(self): Line 91: return 'multipath' Configurators do not have such method now. See the ModuleConfigure for the proper way to define this. Line 92: Line 93: def getServices(self): Line 94: ''' Line 95: If multipathd is up, it will be reloaded after configuration, Line 89: Line 90: def getName(self): Line 91: return 'multipath' Line 92: Line 93: def getServices(self): Configurators do not have such method now. See the ModuleConfigure for the proper way to define this. Line 94: ''' Line 95: If multipathd is up, it will be reloaded after configuration, Line 96: or started before vdsm starts. Line 97: ''' Line 93: def getServices(self): Line 94: ''' Line 95: If multipathd is up, it will be reloaded after configuration, Line 96: or started before vdsm starts. Line 97: ''' This is correct but it does not explain why we return an empty services list. The comment should be clear about that. If you don't see a need to explain this, you can simplify a little bit by not implement at all. Line 98: return [] Line 99: Line 100: def configure(self): Line 101: """ Line 173: sys.stdout.write("multipath requires configuration\n") Line 174: return NOT_CONFIGURED Line 175: Line 176: def validate(self): Line 177: return True The only reason to implement this is to explain why we are not implementing validate, but since you do not explain anything, this adds no value to the reader. -- To view, visit http://gerrit.ovirt.org/30909 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
