Irit Goihman has uploaded a new change for review.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
sos: replace dumpStorageTable with dump_volume_chains
dumpStorageTable is old and uses vdscli and has been replaced with dump_volume_chains which uses jsonrpcvdscli
Change-Id: I73a85e6e720b61da1673af7161a21589ade79831 Signed-off-by: Irit Goihman igoihman@redhat.com --- M vdsm/sos/vdsm.py.in 1 file changed, 6 insertions(+), 6 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/28/62628/1
diff --git a/vdsm/sos/vdsm.py.in b/vdsm/sos/vdsm.py.in index 09ebb78..998c5a1 100644 --- a/vdsm/sos/vdsm.py.in +++ b/vdsm/sos/vdsm.py.in @@ -60,6 +60,7 @@
config = _importVdsmPylibModule("vdsm.config").config jsonrpcvdscli = _importVdsmPylibModule("vdsm.jsonrpcvdscli") +dump_volume_chains = _importVdsmPylibModule("vdsm.tool.dump_volume_chains")
class vdsm(Plugin, RedHatPlugin): @@ -151,12 +152,11 @@ self.addObjectAsFile( cli.getSpmStatus(pool), "getSpmStatus " + pool)
- self.collectExtOutput( - '/bin/su vdsm -s %s %s/dumpStorageTable.pyc' % ( - '@PYTHON@', - '@VDSMDIR@', - ) - ) + sd_uuid, = cli.getStorageDomainsList()["items"] + + self.addObjectAsFile( + dump_volume_chains.dump_chains("dump-volume-chains", sd_uuid), + "dump_volume_chains")
def _addVdsmRunDir(self): """Add everything under /var/run/vdsm except possibly confidential
gerrit-hooks has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 1:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 2:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Dan Kenigsberg has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/62628/2/vdsm/sos/vdsm.py.in File vdsm/sos/vdsm.py.in:
PS2, Line 169: StringIO( I think that this is an overkill - you can use collectExtOutput(vdsm-tool dump-volume-chains).
If you want to avoid the not-so-expensive execution of an external process, you can modify lib/vdsm/tool/dump_volume_chains.py to have a function that returns the needed string.
But I would opt for simple option one.
gerrit-hooks has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 3:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Dan Kenigsberg has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 3: Code-Review-1
(2 comments)
https://gerrit.ovirt.org/#/c/62628/3/vdsm/sos/vdsm.py.in File vdsm/sos/vdsm.py.in:
Line 58: requestQueue = requestQueues.split(",")[0] Line 59: return jsonrpcvdscli.connect(requestQueue=requestQueue) Line 60: Line 61: config = _importVdsmPylibModule("vdsm.config").config Line 62: dump_volume_chains = _importVdsmPylibModule("vdsm.tool.dump_volume_chains") do we still need this? Line 63: jsonrpcvdscli = _importVdsmPylibModule("vdsm.jsonrpcvdscli") Line 64: Line 65: Line 66: class vdsm(Plugin, RedHatPlugin):
Line 149: for pool in pools_list: Line 150: self.addObjectAsFile( Line 151: cli.getSpmStatus(pool), "getSpmStatus " + pool) Line 152: Line 153: sd_uuid, = cli.getStorageDomainsList()["items"] I think this would explode if you have multiple storage domains in the datacenter Line 154: Line 155: self.collectExtOutput("vdsm-tool dump-volume-chains %s" % sd_uuid) Line 156: Line 157: def _addVdsmRunDir(self):
Oved Ourfali has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 3:
Irit - can you answer Dan's comment?
gerrit-hooks has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 4:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Irit Goihman has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 4:
(2 comments)
sorry for the delayed response, I'm currently working on a fix to the jsonrpc command result representation, thus this patch isn't ready yet.
https://gerrit.ovirt.org/#/c/62628/3/vdsm/sos/vdsm.py.in File vdsm/sos/vdsm.py.in:
Line 58: Line 59: class vdsm(Plugin, RedHatPlugin): Line 60: """VDSM server related information Line 61: """ Line 62:
do we still need this?
Done Line 63: optionList = [("logsize", 'max size (MiB) to collect per log file', '', Line 64: False)] Line 65: Line 66: # Make compatible com sos version >= 3
Line 149: self.collectExtOutput("vdsm-tool dump-volume-chains %s" % sd_uuid) Line 150: Line 151: def _addVdsmRunDir(self): Line 152: """Add everything under /var/run/vdsm except possibly confidential Line 153: sysprep vfds and sockets"""
I think this would explode if you have multiple storage domains in the data
Done Line 154: Line 155: import glob Line 156: Line 157: for f in glob.glob("@VDSMRUNDIR@/*"):
gerrit-hooks has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 5:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Irit Goihman has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 5: Verified+1
Yaniv Bronhaim has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 5:
(2 comments)
https://gerrit.ovirt.org/#/c/62628/5/vdsm/sos/vdsm.py.in File vdsm/sos/vdsm.py.in:
Line 57 Line 58 Line 59 Line 60 Line 61 how is this removal related to the use of dump-volume-chains?
Line 126: self.collectExtOutput("/usr/bin/iostat") Line 127: self.collectExtOutput("/sbin/iscsiadm -m node") Line 128: self.collectExtOutput("/sbin/iscsiadm -m session") Line 129: Line 130: with closing(jsonrpcvdscli.connect()) as cli: this change can be done in different patch Line 131: self.addObjectAsFile(cli.getVdsCapabilities(), "getVdsCapabilities") Line 132: self.addObjectAsFile(cli.getVdsStats(), "getVdsStats") Line 133: self.addObjectAsFile(cli.getAllVmStats(), "getAllVmStats") Line 134: self.addObjectAsFile(cli.list(), "list")
gerrit-hooks has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 6:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Irit Goihman has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 6:
(2 comments)
https://gerrit.ovirt.org/#/c/62628/5/vdsm/sos/vdsm.py.in File vdsm/sos/vdsm.py.in:
Line 57 Line 58 Line 59 Line 60 Line 61
how is this removal related to the use of dump-volume-chains?
Done
Line 126: self.collectExtOutput("/usr/bin/iostat") Line 127: self.collectExtOutput("/sbin/iscsiadm -m node") Line 128: self.collectExtOutput("/sbin/iscsiadm -m session") Line 129: Line 130: with closing(jsonrpcvdscli.connect()) as cli:
this change can be done in different patch
Done Line 131: self.addObjectAsFile(cli.getVdsCapabilities(), "getVdsCapabilities") Line 132: self.addObjectAsFile(cli.getVdsStats(), "getVdsStats") Line 133: self.addObjectAsFile(cli.getAllVmStats(), "getAllVmStats") Line 134: self.addObjectAsFile(cli.list(), "list")
Irit Goihman has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 6: -Verified
pending jsonrpc cli fix
Yaniv Bronhaim has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 6: Code-Review+1
From Yaniv Bronhaim ybronhei@redhat.com:
Yaniv Bronhaim has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 9:
(1 comment)
https://gerrit.ovirt.org/#/c/62628/9/vdsm/sos/vdsm.py.in File vdsm/sos/vdsm.py.in:
Line 156: Line 157: sd_uuids = cli.Host.getStorageDomains() Line 158: Line 159: for sd_uuid in sd_uuids: Line 160: self.collectExtOutput("vdsm-tool dump-volume-chains %s" % sd_uuid) is this a new thing? the patch should be only like 157. this should be separated I guess Line 161: Line 162: def _addVdsmRunDir(self): Line 163: """Add everything under /var/run/vdsm except possibly confidential Line 164: sysprep vfds and sockets"""
From Yaniv Bronhaim ybronhei@redhat.com:
Yaniv Bronhaim has posted comments on this change.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
Patch Set 9: Code-Review+1
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has submitted this change and it was merged.
Change subject: sos: replace dumpStorageTable with dump_volume_chains ......................................................................
sos: replace dumpStorageTable with dump_volume_chains
dumpStorageTable is old and uses vdscli and has been replaced with dump_volume_chains which uses jsonrpc
Change-Id: I73a85e6e720b61da1673af7161a21589ade79831 Signed-off-by: Irit Goihman igoihman@redhat.com --- M vdsm/sos/vdsm.py.in 1 file changed, 4 insertions(+), 6 deletions(-)
Approvals: Piotr Kliczewski: Looks good to me, approved Yaniv Bronhaim: Looks good to me, but someone else must approve Jenkins CI: Passed CI tests Irit Goihman: Verified
vdsm-patches@lists.fedorahosted.org
-
automation＠ovirt.org -
Code Review -
Dan Kenigsberg -
igoihman＠redhat.com -
Oved Ourfali -
ybronhei＠redhat.com