Change in vdsm[master]: storage: Add multipath-resize command line tool

nsoffer at redhat.com nsoffer at redhat.com
Mon Mar 23 07:45:31 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: storage: Add multipath-resize command line tool
......................................................................


Patch Set 8:

(8 comments)

https://gerrit.ovirt.org/#/c/38467/8//COMMIT_MSG
Commit Message:

Line 5: CommitDate: 2015-03-21 21:24:21 +0200
Line 6: 
Line 7: storage: Add multipath-resize command line tool
Line 8: 
Line 9: multipath-resize resizes online multipath device after a LUN was resized
> either "a multipath device" or "multipath devices"
Done
Line 10: on the storage server.
Line 11: 
Line 12: Since this operation requires SCSI scanning all device paths, and SCSI
Line 13: scan may block for long time when a storage server is inaccessible, this


Line 9: multipath-resize resizes online multipath device after a LUN was resized
Line 10: on the storage server.
Line 11: 
Line 12: Since this operation requires SCSI scanning all device paths, and SCSI
Line 13: scan may block for long time when a storage server is inaccessible, this
> s/long/a long/
Done
Line 14: must be performed out of Vdsm process.
Line 15: 
Line 16: Having a separate command line tool is also useful for debugging and
Line 17: administration.


Line 10: on the storage server.
Line 11: 
Line 12: Since this operation requires SCSI scanning all device paths, and SCSI
Line 13: scan may block for long time when a storage server is inaccessible, this
Line 14: must be performed out of Vdsm process.
> s/Vdsm/VDSM/, no?
Dan (and me) like "Vdsm" - we already discussed this few month ago.
Line 15: 
Line 16: Having a separate command line tool is also useful for debugging and
Line 17: administration.
Line 18: 


Line 15: 
Line 16: Having a separate command line tool is also useful for debugging and
Line 17: administration.
Line 18: 
Line 19: Vdsm is expected to use this tool without options, logging stderr on
> Here too
Same
Line 20: errors. For debugging, it is useful to use the -v option to get verbose
Line 21: logging.
Line 22: 
Line 23: Change-Id: I229ef55555fa757989329939a9267041785f2c0f


Line 18: 
Line 19: Vdsm is expected to use this tool without options, logging stderr on
Line 20: errors. For debugging, it is useful to use the -v option to get verbose
Line 21: logging.
Line 22: 
> Consider adding Relates-To
Done
Line 23: Change-Id: I229ef55555fa757989329939a9267041785f2c0f


https://gerrit.ovirt.org/#/c/38467/8/vdsm/storage/multipath-resize
File vdsm/storage/multipath-resize:

Line 79:     if "-v" in args:
Line 80:         loglevel = logging.DEBUG
Line 81:         args.remove("-v")
Line 82:     else:
Line 83:         loglevel = logging.INFO
> I wonder if it's worth the hassle to use argparse
If this was a tool for users, yes, but these option are for infrequent debugging, so I tried to keep it minimal.
Line 84: 
Line 85:     logging.basicConfig(level=loglevel, format="%(name)s: %(message)s")
Line 86: 
Line 87:     if not args:


Line 83:         loglevel = logging.INFO
Line 84: 
Line 85:     logging.basicConfig(level=loglevel, format="%(name)s: %(message)s")
Line 86: 
Line 87:     if not args:
> What if you get more than one argument? E.g.:
Right, should be fixed.
Line 88:         log.error("No device name specified")
Line 89:         return 2
Line 90: 
Line 91:     guid = args[0]


Line 121: def list_slaves(name):
Line 122:     return os.listdir("/sys/block/%s/slaves" % name)
Line 123: 
Line 124: 
Line 125: def rescan_path(name):
> I may very well be over engineering things, but this looks awefully like fc
Yes, this is a variant of scsi scan, but I don't think this can be reusable library code.

- Logging is different, and belong to application, not library
- Error handling assume calling through tmap(), not generic
- Data written is different

The generic parts that can be reusable:

- unbuffered write to path

We only have 2 of these, and both are temporary inefficient versions of what should be written in C*. So I would not invest time in this.
Line 126:     try:
Line 127:         log.debug("Rescanning path %s", name)
Line 128:         path = "/sys/block/%s/device/rescan" % name
Line 129:         start = utils.monotonic_time()


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I229ef55555fa757989329939a9267041785f2c0f
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Daniel Erez <derez at redhat.com>
Gerrit-Reviewer: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Fred Rolland <frolland at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer 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