Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator

nsoffer at redhat.com nsoffer at redhat.com
Mon Sep 8 09:36:26 UTC 2014


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 <ykaplan at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list