Change in vdsm[master]: storage: Add resize map support in multipath.py

frolland at redhat.com frolland at redhat.com
Thu May 7 07:47:20 UTC 2015


Freddy Rolland has posted comments on this change.

Change subject: storage: Add resize map support in multipath.py
......................................................................


Patch Set 5:

(17 comments)

https://gerrit.ovirt.org/#/c/40468/5//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2015-05-03 11:38:08 +0300
Line 4: Commit:     Fred Rolland <frolland at redhat.com>
Line 5: CommitDate: 2015-05-03 17:25:20 +0300
Line 6: 
Line 7: storage: Add resize_if_needed method in multipath.py
> You added also resize_devices_if_needed. Why are you describing only this f
Done
Line 8: 
Line 9: This method checks if a multipath device should be resized.
Line 10: It will check the slaves sizes and if it is bigger than the multipath
Line 11: device it will call "multipathd resize".


Line 5: CommitDate: 2015-05-03 17:25:20 +0300
Line 6: 
Line 7: storage: Add resize_if_needed method in multipath.py
Line 8: 
Line 9: This method checks if a multipath device should be resized.
> method -> function
Done
Line 10: It will check the slaves sizes and if it is bigger than the multipath
Line 11: device it will call "multipathd resize".
Line 12: 
Line 13: Change-Id: Ib1020667baeb5976fb1c78daedcc88727648f13d


Line 7: storage: Add resize_if_needed method in multipath.py
Line 8: 
Line 9: This method checks if a multipath device should be resized.
Line 10: It will check the slaves sizes and if it is bigger than the multipath
Line 11: device it will call "multipathd resize".
> It does not call "multipathd resize" - lets not show incomplete example, an
Changed the commit message.
Not sure about the scenario that will cause the slaves not to be the same size
Line 12: 
Line 13: Change-Id: Ib1020667baeb5976fb1c78daedcc88727648f13d
Line 14: Bug-Url:https://bugzilla.redhat.com/609689


https://gerrit.ovirt.org/#/c/40468/5/vdsm/storage/multipath.py
File vdsm/storage/multipath.py:

Line 52:                              "/usr/lib/udev/scsi_id",  # Fedora
Line 53:                              "/lib/udev/scsi_id",  # EL6, Ubuntu
Line 54:                              )
Line 55: 
Line 56: MULTIPATHD = utils.CommandPath("multipathd",
> Should be private - _MULTIPATHD
Done
Line 57:                                "/usr/sbin/multipathd",
Line 58:                                "/sbin/multipathd")
Line 59: 
Line 60: 


Line 54:                              )
Line 55: 
Line 56: MULTIPATHD = utils.CommandPath("multipathd",
Line 57:                                "/usr/sbin/multipathd",
Line 58:                                "/sbin/multipathd")
> Is /sbin/multipathd for rhel6? we don't support it any more.
Done
Line 59: 
Line 60: 
Line 61: def rescan():
Line 62:     """


Line 312: 
Line 313:         yield dmInfoDir.split("/")[3], guid
Line 314: 
Line 315: 
Line 316: def resize_devices_if_needed(filterGuids=None):
> I don't think we need or want this.  We don't want to make it easy to resiz
Done
Line 317:     filteringOn = filterGuids is not None
Line 318:     filterLen = len(filterGuids) if filteringOn else -1
Line 319:     devsFound = 0
Line 320: 


Line 328:         devsFound += 1
Line 329:         resize_if_needed(guid)
Line 330: 
Line 331: 
Line 332: def resize_if_needed(guid):
> Please move up under rescan
Done
Line 333:         log.debug("Check if device %r needs resize", guid)
Line 334:         name = devicemapper.getDmId(guid)
Line 335:         deviceSize = getDeviceSize(name)
Line 336:         svdsm = supervdsm.getProxy()


Line 329:         resize_if_needed(guid)
Line 330: 
Line 331: 
Line 332: def resize_if_needed(guid):
Line 333:         log.debug("Check if device %r needs resize", guid)
> Extra indentation
Done
Line 334:         name = devicemapper.getDmId(guid)
Line 335:         deviceSize = getDeviceSize(name)
Line 336:         svdsm = supervdsm.getProxy()
Line 337:         for slave in devicemapper.getSlaves(name):


Line 335:         deviceSize = getDeviceSize(name)
Line 336:         svdsm = supervdsm.getProxy()
Line 337:         for slave in devicemapper.getSlaves(name):
Line 338:             if getDeviceSize(slave) > deviceSize:
Line 339:                 log.debug("Slave size is larger, %r needs resize", guid)
> Not enough details.
Removed it, too much log, called every time on getdevicelist for easch device
Line 340:                 try:
Line 341:                     svdsm.resize_map(name)
Line 342:                 except Exception as e:
Line 343:                     log.error("Error: %s while trying to resize device: "


Line 337:         for slave in devicemapper.getSlaves(name):
Line 338:             if getDeviceSize(slave) > deviceSize:
Line 339:                 log.debug("Slave size is larger, %r needs resize", guid)
Line 340:                 try:
Line 341:                     svdsm.resize_map(name)
> Why do you call this multiple times?
Done
Line 342:                 except Exception as e:
Line 343:                     log.error("Error: %s while trying to resize device: "
Line 344:                               "%s", str(e.message), guid)
Line 345:                 break


Line 338:             if getDeviceSize(slave) > deviceSize:
Line 339:                 log.debug("Slave size is larger, %r needs resize", guid)
Line 340:                 try:
Line 341:                     svdsm.resize_map(name)
Line 342:                 except Exception as e:
> Since we should call multipathd once, we don't need to handle errors. The c
Done
Line 343:                     log.error("Error: %s while trying to resize device: "
Line 344:                               "%s", str(e.message), guid)
Line 345:                 break
Line 346: 


Line 340:                 try:
Line 341:                     svdsm.resize_map(name)
Line 342:                 except Exception as e:
Line 343:                     log.error("Error: %s while trying to resize device: "
Line 344:                               "%s", str(e.message), guid)
> You don't need to convert e.message to str, and you don't need to access e.
Done
Line 345:                 break
Line 346: 
Line 347: 
Line 348: def resize_map(name):


Line 342:                 except Exception as e:
Line 343:                     log.error("Error: %s while trying to resize device: "
Line 344:                               "%s", str(e.message), guid)
Line 345:                 break
Line 346: 
> This should be something like this:
Done
Line 347: 
Line 348: def resize_map(name):
Line 349:     log.debug("Resizing map %r", name)
Line 350:     cmd = [MULTIPATHD.cmd, "-kresize map %s" % name]


Line 344:                               "%s", str(e.message), guid)
Line 345:                 break
Line 346: 
Line 347: 
Line 348: def resize_map(name):
> This requires root permissions, and can run only from supervdsm, so lets ma
Done
Line 349:     log.debug("Resizing map %r", name)
Line 350:     cmd = [MULTIPATHD.cmd, "-kresize map %s" % name]
Line 351:     start = utils.monotonic_time()
Line 352:     rc, out, err = utils.execCmd(cmd, raw=True, execCmdLogger=log)


Line 353:     # multipathd reports some errors using non-zero exit code and stderr (need
Line 354:     # to be root), but -k commands always return 0, and the result is reported
Line 355:     # using stdout.
Line 356:     if rc != 0 or out != "ok\n":
Line 357:         raise Exception("Resizing map %r failed: out=%r err=%r"
> Use multipath.Error
Done
Line 358:                         % (name, out, err))
Line 359:     elapsed = utils.monotonic_time() - start
Line 360:     log.debug("Resized map %r in %.2f seconds", name, elapsed)
Line 361: 


https://gerrit.ovirt.org/#/c/40468/5/vdsm/supervdsmServer
File vdsm/supervdsmServer:

Line 63:     changeNumvfs
Line 64: 
Line 65: from network.tc import setPortMirroring, unsetPortMirroring
Line 66: from storage.multipath import getScsiSerial as _getScsiSerial
Line 67: from storage.multipath import resize_map as _resize_map
> You are using same code as the exiting code, but in this case we should not
Done
Line 68: from storage.iscsi import getDevIscsiInfo as _getdeviSCSIinfo
Line 69: from storage.iscsi import readSessionInfo as _readSessionInfo
Line 70: from supervdsm import _SuperVdsmManager
Line 71: from storage import hba


Line 147:         return _getScsiSerial(*args, **kwargs)
Line 148: 
Line 149:     @logDecorator
Line 150:     def resize_map(selfself, devName):
Line 151:         return _resize_map(devName)
> Should be:
Done
Line 152: 
Line 153:     @logDecorator
Line 154:     def removeDeviceMapping(self, devName):
Line 155:         return _removeMapping(devName)


-- 
To view, visit https://gerrit.ovirt.org/40468
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1020667baeb5976fb1c78daedcc88727648f13d
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Fred Rolland <frolland at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list