Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Dan Kenigsberg has submitted this change and it was merged. Change subject: Move multipath configuration to vdsm-tool configurator .. Move multipath configuration to vdsm-tool configurator Previously multipath is reconfigured on each vdsm service restart. multipath.conf should not be changed since important changes done by the sysadmin are needed and might be overwritten. vdsm will fail to start if multipath configuration is required. Multipath will be reconfigured only on user demand using vdsm-tool configure --module multipath. as part of the move to using vdsm tool configurator for multipath, we want to consolidate the configuration of multipath with other configurators, such as libvirt. Rotating the configuration file will end up deleting the original multipath conf file for the user, - pre vdsm installation. So we'll use backup files with timestamp instead of rotation. Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1076531 Signed-off-by: Yeela Kaplan ykap...@redhat.com Reviewed-on: http://gerrit.ovirt.org/30909 Reviewed-by: Dan Kenigsberg dan...@redhat.com --- M lib/vdsm/tool/configurator.py M lib/vdsm/tool/configurators/Makefile.am A lib/vdsm/tool/configurators/multipath.py M vdsm.spec.in M vdsm/storage/hsm.py M vdsm/storage/multipath.py M vdsm/supervdsmServer 7 files changed, 198 insertions(+), 140 deletions(-) Approvals: Yeela Kaplan: Verified Dan Kenigsberg: Looks good to me, approved -- To view, visit http://gerrit.ovirt.org/30909 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da Gerrit-PatchSet: 22 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 22: Build Failed http://jenkins.ovirt.org/job/vdsm_master-libfapi_create-rpms-el6-x86_64_merged/75/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc21-x86_64_merged/292/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master-libfapi_create-rpms-fc20-x86_64_merged/65/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged_test_debug/509/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master-libfapi_create-rpms-el7-x86_64_merged/68/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc20-x86_64_merged/308/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/4302/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el6-x86_64_merged/314/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el7-x86_64_merged/316/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master-libfapi_create-rpms-fc21-x86_64_merged/65/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/6139/ : SUCCESS -- 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: 22 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Dan Kenigsberg has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 21: Code-Review+2 -- 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: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Saggi Mizrahi has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 18: Code-Review+1 -- 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: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Saggi Mizrahi has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 20: Code-Review+1 -- 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: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 20: Verified+1 -- 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: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 21: Verified+1 -- 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: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 19: Build Failed http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/132/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/698/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13862/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/679/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/137/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13073/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14025/ : FAILURE -- 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: 19 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 20: Build Failed http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/134/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/700/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13864/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/681/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/139/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13075/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14027/ : FAILURE -- 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: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 21: Build Failed http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/136/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/702/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13866/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/683/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/141/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13077/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14029/ : FAILURE -- 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: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 18: Build Failed http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/109/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/675/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13787/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/655/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12998/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13950/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/114/ : FAILURE -- 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: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 17:
(1 comment)
http://gerrit.ovirt.org/#/c/30909/17/lib/vdsm/tool/configurators/multipath.py
File lib/vdsm/tool/configurators/multipath.py:
Line 135: {'scsi_id_path': self._scsi_id.cmd})
Line 136: f.flush()
Line 137: cmd = [constants.EXT_CP, f.name,
Line 138:self._MPATH_CONF]
Line 139: rc, out, err = utils.execCmd(cmd)
Since we run as root now, we can use shutil.copyfile() instead of running cp.
If you want to make it easier to review and understand later, you can replace
this on the next patch.
Line 140:
Line 141: if rc != 0:
Line 142: raise RuntimeError(Failed to perform Multipath
config.)
Line 143: utils.persist(self._MPATH_CONF)
--
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: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 16: (4 comments) http://gerrit.ovirt.org/#/c/30909/16/lib/vdsm/tool/configurators/multipath.py File lib/vdsm/tool/configurators/multipath.py: Line 73: # RHEV REVISION 0.4, # RHEV REVISION 0.5, Line 74: # RHEV REVISION 0.6, # RHEV REVISION 0.7, Line 75: # RHEV REVISION 0.8, # RHEV REVISION 0.9] Line 76: Line 77: _MPATH_CONF_TAG = # RHEV REVISION 1.0 You need to rebase - see http://gerrit.ovirt.org/#/c/35072/3/vdsm/storage/m Done Line 78: Line 79: # Having the PRIVATE_TAG in the conf file means Line 80: # vdsm-tool should never change the conf file Line 81: # even when using the --force flag Line 107: supported state. The original configuration, if any, is saved Line 108: Line 109: Line 110: if os.path.exists(self._MPATH_CONF): Line 111: backup = MPATH_CONF + '.' + datetime.now().strftime(%Y%m%d%H%M) self._MPATH_CONF? Done Line 112: try: Line 113: shutil.copyfile(self._MPATH_CONF, backup) Line 114: except IOError: Line 115: raise RuntimeError(Failed to copy conf to backup) Line 111: backup = MPATH_CONF + '.' + datetime.now().strftime(%Y%m%d%H%M) Line 112: try: Line 113: shutil.copyfile(self._MPATH_CONF, backup) Line 114: except IOError: Line 115: raise RuntimeError(Failed to copy conf to backup) You are hiding the original error, which will make it impossible to debug. Done Line 116: else: Line 117: utils.persist(backup) Line 118: Line 119: with tempfile.NamedTemporaryFile() as f: Line 162: sys.stdout.write(This manual override for multipath.conf Line 163: was based on downrevved template. Line 164: You are strongly advised to Line 165: contact your support representatives\n) Line 166: return CONFIGURED You need to rebase, the constants are different now (YES, NO, MAYBE) Done Line 167: Line 168: if self._MPATH_CONF_TAG in first: Line 169: sys.stdout.write(Current revision of multipath.conf detected, Line 170: preserving\n) -- 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: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 17: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12779/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/82/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13731/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/78/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/643/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/624/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13568/ : SUCCESS -- 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: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 16: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12772/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el7_created/77/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13724/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc21_created/73/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/638/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/619/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13561/ : SUCCESS -- 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: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 16: (4 comments) http://gerrit.ovirt.org/#/c/30909/16/lib/vdsm/tool/configurators/multipath.py File lib/vdsm/tool/configurators/multipath.py: Line 73: # RHEV REVISION 0.4, # RHEV REVISION 0.5, Line 74: # RHEV REVISION 0.6, # RHEV REVISION 0.7, Line 75: # RHEV REVISION 0.8, # RHEV REVISION 0.9] Line 76: Line 77: _MPATH_CONF_TAG = # RHEV REVISION 1.0 You need to rebase - see http://gerrit.ovirt.org/#/c/35072/3/vdsm/storage/multipath.py Line 78: Line 79: # Having the PRIVATE_TAG in the conf file means Line 80: # vdsm-tool should never change the conf file Line 81: # even when using the --force flag Line 107: supported state. The original configuration, if any, is saved Line 108: Line 109: Line 110: if os.path.exists(self._MPATH_CONF): Line 111: backup = MPATH_CONF + '.' + datetime.now().strftime(%Y%m%d%H%M) self._MPATH_CONF? Any reason to use datetime, when the simpler time.strftime() give the same results? datetime.now().strftime(%Y%m%d%H%M) '201411232052' time.strftime(%Y%m%d%H%M) '201411232052' Line 112: try: Line 113: shutil.copyfile(self._MPATH_CONF, backup) Line 114: except IOError: Line 115: raise RuntimeError(Failed to copy conf to backup) Line 111: backup = MPATH_CONF + '.' + datetime.now().strftime(%Y%m%d%H%M) Line 112: try: Line 113: shutil.copyfile(self._MPATH_CONF, backup) Line 114: except IOError: Line 115: raise RuntimeError(Failed to copy conf to backup) You are hiding the original error, which will make it impossible to debug. Since this error is not expected (root cannot copy file in /etc?!), the simplest way to handle it would be to *not* handle it and let the tool fail with clear traceback. Line 116: else: Line 117: utils.persist(backup) Line 118: Line 119: with tempfile.NamedTemporaryFile() as f: Line 162: sys.stdout.write(This manual override for multipath.conf Line 163: was based on downrevved template. Line 164: You are strongly advised to Line 165: contact your support representatives\n) Line 166: return CONFIGURED You need to rebase, the constants are different now (YES, NO, MAYBE) Line 167: Line 168: if self._MPATH_CONF_TAG in first: Line 169: sys.stdout.write(Current revision of multipath.conf detected, Line 170: preserving\n) -- 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: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 15: Code-Review+1 (1 comment) this part looks good imo http://gerrit.ovirt.org/#/c/30909/15/lib/vdsm/tool/configurators/multipath.py File lib/vdsm/tool/configurators/multipath.py: Line 31: from ... import constants Line 32: Line 33: Line 34: class Configurator(ModuleConfigure): Line 35: what about removeConf? (in another patch) Line 36: _MPATH_CONF = /etc/multipath.conf Line 37: Line 38: _CONF_BACKUP = /etc/multipath.conf.bak Line 39: -- 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: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 15: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13099/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/503/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/485/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12941/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12151/ : FAILURE -- 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: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: (3 comments) Separating into two steps / patches: 1. move to backup file from rotate files 2. move to vdsm tool configurator http://gerrit.ovirt.org/#/c/30909/13/lib/vdsm/tool/configurators/multipath.py File lib/vdsm/tool/configurators/multipath.py: Line 101: during configuration. Line 102: ''' Line 103: return [] Line 104: Line 105: def _parseConf(self): It would be nicer if the private function is at the end of the class, after Done Line 106: first = second = '' Line 107: with open(self._MPATH_CONF) as f: Line 108: mpathconf = [x.strip(\n) for x in f.readlines()] Line 109: try: Line 124: first, second = self._parseConf() Line 125: if not os.path.exists(self._CONF_BACKUP) and \ Line 126: self._MPATH_CONF_TAG not in first: Line 127: cmd = [constants.EXT_CP, self._MPATH_CONF, self._CONF_BACKUP] Line 128: rc, out, err = utils.execCmd(cmd) Right, and there's no reason to use execCmd imo - shutil.copy() should be e Done Line 129: utils.persistFile(self._CONF_BACKUP) Line 130: Line 131: with tempfile.NamedTemporaryFile() as f: Line 132: f.write(self._MPATH_CONF_TEMPLATE % Line 138: Line 139: if rc != 0: Line 140: raise RuntimeError(Failed to perform Multipath config.) Line 141: if utils.isOvirtNode(): Line 142: utils.persistFile(self._MPATH_CONF) You don't need the ovirt node check, utils.presistFile does nothing now if Done Line 143: Line 144: # Flush all unused multipath device maps Line 145: utils.execCmd([constants.EXT_MULTIPATH, -F]) Line 146: -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 14: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12911/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/465/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/448/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12753/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11962/ : FAILURE -- 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: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: moving to use backup instead of rotateFiles is not because 'Dan doesn't like it'. The cheange is because it does not make sense and there is no need to rotate the conf file. as part of doing things the same way as all configurators, we are using a single backup file just like the backup of the libvirt conf by the configurator. The user only needs the backup of his original conf file. not of the vdsm conf file. and that's what we want to preserve. That is the way I would explain it to the user. -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Dan Kenigsberg has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: Just to be clear, I refused to see rotateFiles in utils in its current fragile form (ignoring errors, funny-named args). As discussed and agreed in an email thread long time ago (Yeela, please add a link to the commit message), we should consolidate the multipath config semantics with that of other configurators. In my opinion, this can be done in one blow - no need to move the rotateFile code only to delete it in a following patch. However, the commit message should be clearer about it: it makes TWO changes. * multipath.conf is not longer edited by the vdsm daemon, only by vdsm-tool * only the original multipath.conf is backed up. That's particularly reasonable given the former point, as vdsm does not do auto-rotation by its own volition. -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Dan Kenigsberg has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: (1 comment) http://gerrit.ovirt.org/#/c/30909/13/lib/vdsm/tool/configurators/multipath.py File lib/vdsm/tool/configurators/multipath.py: Line 124: first, second = self._parseConf() Line 125: if not os.path.exists(self._CONF_BACKUP) and \ Line 126: self._MPATH_CONF_TAG not in first: Line 127: cmd = [constants.EXT_CP, self._MPATH_CONF, self._CONF_BACKUP] Line 128: rc, out, err = utils.execCmd(cmd) If we cannot write multipath configuration file running as root, something Right, and there's no reason to use execCmd imo - shutil.copy() should be enough. Line 129: utils.persistFile(self._CONF_BACKUP) Line 130: Line 131: with tempfile.NamedTemporaryFile() as f: Line 132: f.write(self._MPATH_CONF_TEMPLATE % -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: Code-Review-1 This patch does now two unrelated changes: 1. Keep one backup of multipath.conf instead of rotating files 2. Move multipath configuration to vdsm-tool The first change looks unneeded to me. There is no need to tie these changes and make one of them depend on the other. The second change is a must for fixing this bug: https://bugzilla.redhat.com/1108711 So the best way is to fix rotateFiles so Dan will not object to have it in utils, and postpone the other change for later. Please separated these changes. -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Dan Kenigsberg has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: Back in http://lists.ovirt.org/pipermail/devel/2014-June/007897.html we agreed to take Fedorico's points 1 an 3, meaning to drop usage of rotateFiles in this context. I do not see how exposing rotateFiles in utils should be a requirement when this is what we aim for. -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: We may aim for different backup solution, but it is not related to this patch. -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: It doesn't make sense to change the use of rotateFiles to copy in a different following patch, because it means that we will need to move rotateFiles to utils from storage.misc, and then in the following patch move it back. and Dan is against exposing rotateFilles to utils... so this is the only way to do it. -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: This is not the only way to go. If Dan has a problem with rotateFiles, we can fix it. We should not change the implementation because someone (even if the maintainer) does not like having some function in some module. How do you explain this change to a user? We stopped rotating multipath.conf backups because Dan did not like having utils.rotateFiles? If we really need this change, it should be done *before* this patch, so we can easily compare the changes later. -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: I intend to agree with nir about the code changes. most of the comments I had were about documentation to add or return values to check, which seems easy to spot while reviewing and I don't mind if you'll add those later on, but it requires some arrangement. I think that only dan's last comment about the backup files is relevant for that saying, reverting it and adding that in following patch will be accepted by all of us i guess -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 12: Code-Review+1 (1 comment) my comment there although its as previous implementation in multipath.py. i prefer to see the rc checked http://gerrit.ovirt.org/#/c/30909/12/lib/vdsm/tool/configurators/multipath.py File lib/vdsm/tool/configurators/multipath.py: Line 141: if utils.isOvirtNode(): Line 142: utils.persistFile(self._MPATH_CONF) Line 143: Line 144: # Flush all unused multipath device maps Line 145: utils.execCmd([constants.EXT_MULTIPATH, -F]) don't you want to check the return value ? Line 146: Line 147: rc = service.service_reload(multipathd) Line 148: if rc != 0: Line 149: status = service.service_status(multipathd, False) -- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: Build Failed http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/418/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/435/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11784/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12730/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12573/ : SUCCESS -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 13: (3 comments) I think we are going in the wrong direction. This patch should be simple move of multipath code as is to vdsm-tool. Any change in this code will be very hard to review later, because of the code moves. We can always improve the details later. Regarding the backup change - it is the worst idea in this patch - mix code move an behavior change, and the behavior change is questionable. http://gerrit.ovirt.org/#/c/30909/13/lib/vdsm/tool/configurators/multipath.py File lib/vdsm/tool/configurators/multipath.py: Line 101: during configuration. Line 102: ''' Line 103: return [] Line 104: Line 105: def _parseConf(self): It would be nicer if the private function is at the end of the class, after the public module configure functions. Line 106: first = second = '' Line 107: with open(self._MPATH_CONF) as f: Line 108: mpathconf = [x.strip(\n) for x in f.readlines()] Line 109: try: Line 124: first, second = self._parseConf() Line 125: if not os.path.exists(self._CONF_BACKUP) and \ Line 126: self._MPATH_CONF_TAG not in first: Line 127: cmd = [constants.EXT_CP, self._MPATH_CONF, self._CONF_BACKUP] Line 128: rc, out, err = utils.execCmd(cmd) If we cannot write multipath configuration file running as root, something is very wrong in this host, and we should fail hard. Line 129: utils.persistFile(self._CONF_BACKUP) Line 130: Line 131: with tempfile.NamedTemporaryFile() as f: Line 132: f.write(self._MPATH_CONF_TEMPLATE % Line 138: Line 139: if rc != 0: Line 140: raise RuntimeError(Failed to perform Multipath config.) Line 141: if utils.isOvirtNode(): Line 142: utils.persistFile(self._MPATH_CONF) You don't need the ovirt node check, utils.presistFile does nothing now if we are not on ovirt node. Line 143: Line 144: # Flush all unused multipath device maps Line 145: utils.execCmd([constants.EXT_MULTIPATH, -F]) Line 146: -- 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: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 11: Code-Review+1 -- 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: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 12: Build Failed http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/396/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/413/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11693/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12637/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12482/ : SUCCESS -- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 10: Verified+1 -- 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: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 11: Build Failed http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-fc20_created/369/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_install-rpm-sanity-el6_created/386/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11609/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12553/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12398/ : SUCCESS -- 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: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 11: Verified+1 -- 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: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 11: Code-Review+1 -- 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: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 9: (4 comments) http://gerrit.ovirt.org/#/c/30909/9//COMMIT_MSG Commit Message: Line 11: Line 12: multipath.conf should not be changed since important changes Line 13: done by the sysadmin are needed and might be overwritten. Line 14: Line 15: Now it will be reconfigured only on user demand. While all this is correct, it does not emphasize the important change: Done Line 16: Line 17: Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da Line 18: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1076531 http://gerrit.ovirt.org/#/c/30909/9/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 41: _CONFIGURATORS = dict((m.name, m) for m in ( Line 42: certificates.Certificates(), Line 43: libvirt.Libvirt(), Line 44: sanlock.Sanlock(), Line 45: multipath.Multipath(), you need to rebase again (http://gerrit.ovirt.org/31775) Done Line 46: )) Line 47: Line 48: Line 49: @expose(configure) http://gerrit.ovirt.org/#/c/30909/9/lib/vdsm/tool/configurators/multipath.py File lib/vdsm/tool/configurators/multipath.py: Line 62: }\n Line 63: } Line 64: ) Line 65: Line 66: _MAX_CONF_COPIES = 5 Lets not change this in this patch. I don't understand why this is better a OK Line 67: Line 68: # conf file configured by vdsm should contain a tag Line 69: # in form of RHEV REVISION X.Y Line 70: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3, Line 169: Line 170: for tag in self._OLD_TAGS: Line 171: if tag in first: Line 172: sys.stdout.write(Downrev multipath.conf detected, Line 173: upgrade required\n) This means that multipath.conf was configured by old vdsm version, and we n OK Line 174: return NOT_CONFIGURED Line 175: Line 176: sys.stdout.write(multipath requires configuration\n) -- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 10: Build Successful http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11598/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12542/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12387/ : SUCCESS -- 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: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 10: Code-Review+1 -- 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: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 9: (5 comments) http://gerrit.ovirt.org/#/c/30909/9/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 41: _CONFIGURATORS = dict((m.name, m) for m in ( Line 42: certificates.Certificates(), Line 43: libvirt.Libvirt(), Line 44: sanlock.Sanlock(), Line 45: multipath.Multipath(), you need to rebase again (http://gerrit.ovirt.org/31775) Line 46: )) Line 47: Line 48: Line 49: @expose(configure) http://gerrit.ovirt.org/#/c/30909/9/lib/vdsm/tool/configurators/multipath.py File lib/vdsm/tool/configurators/multipath.py: Line 62: }\n Line 63: } Line 64: ) Line 65: Line 66: _MAX_CONF_COPIES = 5 MAX_OLD_CONF_FILES_COPY Line 67: Line 68: # conf file configured by vdsm should contain a tag Line 69: # in form of RHEV REVISION X.Y Line 70: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3, Line 96: ''' : If multipathd is up, it will be reloaded after configuration, : or started before vdsm starts, so service should not be stopped : during configuration. : ''' but if vdsm is up and we update the multipath version only, than we run vdsm-tool configure which will restart multipathd, don't we need to restart also vdsm? Line 125: if utils.isOvirtNode(): Line 126: utils.persistFile(self._MPATH_CONF) Line 127: Line 128: # Flush all unused multipath device maps Line 129: utils.execCmd([constants.EXT_MULTIPATH, -F]) don't you need to check the rc of this command? Line 130: Line 131: rc = service.service_reload(multipathd) Line 132: if rc != 0: Line 133: status = service.service_status(multipathd, False) Line 169: Line 170: for tag in self._OLD_TAGS: Line 171: if tag in first: Line 172: sys.stdout.write(Downrev multipath.conf detected, Line 173: upgrade required\n) Downrev? do you mean: old conf file is detected, please run yum upgrade multipth ? Line 174: return NOT_CONFIGURED Line 175: Line 176: sys.stdout.write(multipath requires configuration\n) -- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 9: (4 comments) http://gerrit.ovirt.org/#/c/30909/9/lib/vdsm/tool/configurators/multipath.py File lib/vdsm/tool/configurators/multipath.py: Line 62: }\n Line 63: } Line 64: ) Line 65: Line 66: _MAX_CONF_COPIES = 5 MAX_OLD_CONF_FILES_COPY Lets not change this in this patch. I don't understand why this is better anyway. Line 67: Line 68: # conf file configured by vdsm should contain a tag Line 69: # in form of RHEV REVISION X.Y Line 70: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3, Line 96: ''' Line 97: If multipathd is up, it will be reloaded after configuration, Line 98: or started before vdsm starts, so service should not be stopped Line 99: during configuration. Line 100: ''' but if vdsm is up and we update the multipath version only, than we run vds We do not restart multipathd, only reload it. If it is not running, we do nothing, as it will run when vdsm starts because vdsm requires multipathd (systemd) or starts it (sysv). Line 101: return [] Line 102: Line 103: def configure(self): Line 104: Line 125: if utils.isOvirtNode(): Line 126: utils.persistFile(self._MPATH_CONF) Line 127: Line 128: # Flush all unused multipath device maps Line 129: utils.execCmd([constants.EXT_MULTIPATH, -F]) don't you need to check the rc of this command? We need, but this is how this code worked for years. Lets keep it in this patch, and check later if we should ignore the return value here or not. Remember that this patch is trying to move the same logic from vdsm to vdsm-tool, not fix the world. Lets focus on this goal. Line 130: Line 131: rc = service.service_reload(multipathd) Line 132: if rc != 0: Line 133: status = service.service_status(multipathd, False) Line 169: Line 170: for tag in self._OLD_TAGS: Line 171: if tag in first: Line 172: sys.stdout.write(Downrev multipath.conf detected, Line 173: upgrade required\n) Downrev? do you mean: old conf file is detected, please run yum upgrade mu This means that multipath.conf was configured by old vdsm version, and we need to run vdsm-tool configure to update multipath.conf. I agree that this is not clear, but this is not related to Yeela changes. Lets get this merged first and then improve the texts. Line 174: return NOT_CONFIGURED Line 175: Line 176: sys.stdout.write(multipath requires configuration\n) -- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 9: Build Successful http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11510/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12454/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12299/ : SUCCESS -- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 9: (1 comment) The patch is in great shape now. Unfortunately a rebase is required after http://gerrit.ovirt.org/31775 was merged. http://gerrit.ovirt.org/#/c/30909/9//COMMIT_MSG Commit Message: Line 11: Line 12: multipath.conf should not be changed since important changes Line 13: done by the sysadmin are needed and might be overwritten. Line 14: Line 15: Now it will be reconfigured only on user demand. While all this is correct, it does not emphasize the important change: Previously multipath configuration was overwritten silently if configuration was required when vdsm started. Now vdsm will fail to start if multipath configuration is required, and the user will have to run vdsm-tool configure --module multipath so she has more control over the configuration, and the behavior is consistent with other modules (e.g. libvirt). Line 16: Line 17: Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da Line 18: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1076531 -- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 8: Build Successful http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12356/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11412/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12201/ : SUCCESS -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 7: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11356/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12300/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12145/ : SUCCESS -- 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 ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
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 ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 6: now it requires big rebase... i warned you -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 6: I think the rebase should be easy - just move the new class to its own module. -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 6:
(2 comments)
http://gerrit.ovirt.org/#/c/30909/6/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 745: utils.rotateFiles(
Line 746: os.path.dirname(self._MPATH_CONF),
Line 747: os.path.basename(self._MPATH_CONF),
Line 748: self._MAX_CONF_COPIES,
Line 749: cp=True, persist=True)
I think this shold be rebased on I think you should rebase this on
http://gerrit.ovirt.org/31583.
Otherwise I don't see how do you have utils.rotateFiles here - it was moved by
that patch from misc to utils.
Line 750: with tempfile.NamedTemporaryFile() as f:
Line 751: f.write(self._MPATH_CONF_TEMPLATE %
Line 752: {'scsi_id_path': self._scsi_id.cmd})
Line 753: f.flush()
Line 777: If the tag above is followed by tag RHEV PRIVATE the
configuration
Line 778: should be preserved at all cost.
Line 779:
Line 780: if os.getuid() != 0:
Line 781: raise NotRootError()
This may be unneeded because of http://gerrit.ovirt.org/31293
Lets wait until that one is merged.
Line 782:
Line 783: if os.path.exists(self._MPATH_CONF):
Line 784: first = second = ''
Line 785: with open(self._MPATH_CONF) as f:
--
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 5: (5 comments) http://gerrit.ovirt.org/#/c/30909/5/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 700: ) Line 701: Line 702: _MAX_CONF_COPIES = 5 Line 703: Line 704: # conf file configured by vdsm should contain private tag This is not private tag - this comment is confusing. Should be called re Done Line 705: # in form of RHEV REVISION X.Y Line 706: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3, Line 707: # RHEV REVISION 0.4, # RHEV REVISION 0.5, Line 708: # RHEV REVISION 0.6, # RHEV REVISION 0.7, Line 710: Line 711: _MPATH_CONF_TAG = # RHEV REVISION 1.0 Line 712: Line 713: # Having the PRIVATE_TAG in the conf file means Line 714: # vdsm-tool should never change the conf file Lets be more clear, since Yaniv was confused about it, and other will be as Done Line 715: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE Line 716: Line 717: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF Line 718: Line 725: def getName(self): Line 726: return 'multipath' Line 727: Line 728: def getServices(self): Line 729: return [] There is no need to implement this, because we inherit this from the super Done Line 730: Line 731: def configure(self): Line 732: Line 733: Set up the multipath daemon configuration to the known and Line 760: Line 761: rc = service.service_reload(multipathd) Line 762: if rc != 0: Line 763: status = service.service_status(multipathd, False) Line 764: if status == 0: nice catch .. Not such a nice catch. if the service is not running, it will actually return 1. See service.py line 410-413. Line 765: raise RuntimeError(Failed to reload Multipath.) Line 766: Line 767: def isconfigured(self, *args): Line 768: Line 766: Line 767: def isconfigured(self, *args): Line 768: Line 769: Check the multipath daemon configuration. The configuration file Line 770: /etc/multipath.conf should contain private tag in form Same as above, private is confusing, remove it. Done Line 771: RHEV REVISION X.Y for this check to succeed. Line 772: If the tag above is followed by tag RHEV PRIVATE the configuration Line 773: should be preserved at all cost. Line 774: -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 5: (1 comment) http://gerrit.ovirt.org/#/c/30909/5/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 760: Line 761: rc = service.service_reload(multipathd) Line 762: if rc != 0: Line 763: status = service.service_status(multipathd, False) Line 764: if status == 0: Not such a nice catch. Yeela is right, I missed that check. Line 765: raise RuntimeError(Failed to reload Multipath.) Line 766: Line 767: def isconfigured(self, *args): Line 768: -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 6: Build Successful http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10965/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11907/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11750/ : SUCCESS -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 5: -Code-Review (1 comment) so still need to improve the comments. not only me complaining http://gerrit.ovirt.org/#/c/30909/5/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 760: Line 761: rc = service.service_reload(multipathd) Line 762: if rc != 0: Line 763: status = service.service_status(multipathd, False) Line 764: if status == 0: If the service is not running, this will raise ServiceOperationError, not r nice catch .. Line 765: raise RuntimeError(Failed to reload Multipath.) Line 766: Line 767: def isconfigured(self, *args): Line 768: -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 5: Code-Review+1 oh well .. at least I know now what the tags and the 2 empty lines mean -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 5: Code-Review-1 (5 comments) Usage of service_status looks wrong, and some minor documentation improvements can be nice. http://gerrit.ovirt.org/#/c/30909/5/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 700: ) Line 701: Line 702: _MAX_CONF_COPIES = 5 Line 703: Line 704: # conf file configured by vdsm should contain private tag This is not private tag - this comment is confusing. Should be called revision tag or just tag. Line 705: # in form of RHEV REVISION X.Y Line 706: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3, Line 707: # RHEV REVISION 0.4, # RHEV REVISION 0.5, Line 708: # RHEV REVISION 0.6, # RHEV REVISION 0.7, Line 710: Line 711: _MPATH_CONF_TAG = # RHEV REVISION 1.0 Line 712: Line 713: # Having the PRIVATE_TAG in the conf file means Line 714: # vdsm-tool should never change the conf file Lets be more clear, since Yaniv was confused about it, and other will be as well. should never change the conf file - should never change the conf file even when using the --force flag. Line 715: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE Line 716: Line 717: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF Line 718: Line 725: def getName(self): Line 726: return 'multipath' Line 727: Line 728: def getServices(self): Line 729: return [] There is no need to implement this, because we inherit this from the super class. However having this method returning empty string give us nice place for a docstring, explaining why we must return empty string. Without documentation, the next developer will be confused why this is different from other configuators. Unlike other configurators, we do not stop multipathd during configuration but reload it. Returning empty list means that we don't depend on any service. We can also put this info in configure() instead. Line 730: Line 731: def configure(self): Line 732: Line 733: Set up the multipath daemon configuration to the known and Line 760: Line 761: rc = service.service_reload(multipathd) Line 762: if rc != 0: Line 763: status = service.service_status(multipathd, False) Line 764: if status == 0: If the service is not running, this will raise ServiceOperationError, not return status != 0. This should be: try: service.service_status(multipathd, False) except ServiceOperationError: pass # multipath was not running else: raise RuntimeError(Failed to reload Multipath.) See service.py line 349. Line 765: raise RuntimeError(Failed to reload Multipath.) Line 766: Line 767: def isconfigured(self, *args): Line 768: Line 766: Line 767: def isconfigured(self, *args): Line 768: Line 769: Check the multipath daemon configuration. The configuration file Line 770: /etc/multipath.conf should contain private tag in form Same as above, private is confusing, remove it. Line 771: RHEV REVISION X.Y for this check to succeed. Line 772: If the tag above is followed by tag RHEV PRIVATE the configuration Line 773: should be preserved at all cost. Line 774: -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 4: Code-Review-1
(3 comments)
I'm sorry, i don't understand how the tags work. I know this patch here just to
move things around, but I want to do it right and understand the content of the
conf file if you do. please also verify that the behavior with vdsm-tool
configure --module multipath , and with --force flag, is as you expected,
because I really not sure about it. and explain with the desired behavior about
overriding the conf and all .. the semantic very important. keep in mind that
on upgrade and host deploy we call configure with --force for all modules, make
sure it won't disturb your logic
http://gerrit.ovirt.org/#/c/30909/4/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 668:
Line 669: class MultipathModuleConfigure(_ModuleConfigure):
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
what with my comment :( to me its not redundant . I don't understand why you
put 2 empty lines before the conf itself and I prefer to understand. otherwise
I might post a patch that removes and waste time without knowing the reason.
why not to agree..? its only 2 line for you, and great pleasure for me . please
add it..
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 701:
Line 702: _MAX_CONF_COPIES = 5
Line 703:
Line 704: # conf file configured by vdsm should contain private tag
Line 705: # in form of RHEV REVISION X.Y
what is those private tags? if you understand, just explain.. that's all im
asking
Line 706: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3,
Line 707: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 708: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 709: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 710:
Line 711: _MPATH_CONF_TAG = # RHEV REVISION 1.0
Line 712:
Line 713: # Having the PRIVATE_TAG in the conf file means
Line 714: # we should never change the conf file
who is we? vdsm-tool won't touch it ? manually I can't modify it? I really
don't understand ...
as far as I understand, when putting # RHEV REVISION 1.0 before the
configuration, vdsm-tool configure call will assume that the following conf is
valid and won't override it even with --force flag. is that true?
Line 715: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 716:
Line 717: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 718:
--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 5: Build Successful http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10691/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11633/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11476/ : SUCCESS -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 4:
(3 comments)
force should not override existing configuration if it contains PRIVATE_TAG
http://gerrit.ovirt.org/#/c/30909/4/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 668:
Line 669: class MultipathModuleConfigure(_ModuleConfigure):
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
what with my comment :( to me its not redundant . I don't understand why yo
I really think it's unnecessary, I wouldn't want adding it to the code.
If you insist that this comment is necessary I'd prefer it in a different
patch, because it is irrelevant to the context of the patch.
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 701:
Line 702: _MAX_CONF_COPIES = 5
Line 703:
Line 704: # conf file configured by vdsm should contain private tag
Line 705: # in form of RHEV REVISION X.Y
what is those private tags? if you understand, just explain.. that's all im
These are tags that represent the version of the configuration.
When we configure multipath we check the version, and if there's no PRIVATE_TAG
and the version is not the latest we upgrade the conf file.
Line 706: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3,
Line 707: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 708: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 709: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 710:
Line 711: _MPATH_CONF_TAG = # RHEV REVISION 1.0
Line 712:
Line 713: # Having the PRIVATE_TAG in the conf file means
Line 714: # we should never change the conf file
who is we? vdsm-tool won't touch it ? manually I can't modify it? I really
we is vdsm-tool, I'll change it in the next patchset so it'll be clear.
Indeed, on force we won't change the multipath.conf file.
Line 715: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 716:
Line 717: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 718:
--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 2:
(7 comments)
http://gerrit.ovirt.org/#/c/30909/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
These newlines adds an empty line between the tags and the contents of the
thanks! :) why couldn't you say it in my first comment
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 677: getuid_callout \%(scsi_id_path)s --whitelisted
Line 678: --replace-whitespace --device=/dev/%%n\\n
Line 705: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
The usage of this tag is explained in the docstring of isconfigured().
move the explanation here please
Line 710: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 711:
Line 712: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 713:
Line 720: def getName(self):
Line 721: return 'multipath'
Line 722:
Line 723: def getServices(self):
Line 724: return [multipathd]
If we return [multipathd], then with systemd, vdsm will also stop, becaus
whatever you know is better than I know. just saying what this method means
because I wasn't sure you understand.
if you test that returning empty list works as we expected, please do that.
Line 725:
Line 726: def configure(self):
Line 727:
Line 728: Set up the multipath daemon configuration to the known and
Line 755:
Line 756: cmd = [constants.EXT_VDSM_TOOL, service-reload,
multipathd]
Line 757: rc, out, err = utils.execCmd(cmd)
Line 758: if rc != 0:
Line 759: sys.stdout.write(Failed to reload Multipath.\n)
anyway line 724 will cause multipathd restart (it will stop the service if
this will be required if we remove multipathd from getServices. just import
service and use the function directly instead of execCmd
Line 760:
Line 761: def isconfigured(self, *args):
Line 762:
Line 763: Check the multipath daemon configuration. The configuration
file
Line 785: sys.stdout.write(This manual override for
multipath.conf
Line 786: was based on downrevved
template.
Line 787: You are strongly advised to
Line 788: contact your support
representatives\n)
Line 789: return CONFIGURED
If the administrator added # RHEV PRIVATE tag to multipath.conf, we are n
that exactly why i asked for comments near the titles! just explain the meaning
of them..
Line 790:
Line 791: if self._MPATH_CONF_TAG in first:
Line 792: sys.stdout.write(Current revision of multipath.conf
detected,
Line 793: preserving\n)
Line 802: sys.stdout.write(multipath Defaulting to False\n)
Line 803: return NOT_CONFIGURED
Line 804:
Line 805: def validate(self):
Line 806: return True
We don't have code for this. Implementing this requires parsing multipath c
np. just asking
Line 807:
Line 808:
Line 809: __configurers = (
Line 810: LibvirtModuleConfigure(),
Line 837:
Line 838: services = []
Line 839: for c in configurer_to_trigger:
Line 840: for s in c.getServices():
Line 841: if service.service_status(s, False) == 0:
Stopping multipath is not required to configure it, and we are not doing it
right. so change it
Line 842: if not args.force:
Line 843: raise InvalidRun(
Line 844: \n\nCannot configure while service '%s' is
Line 845: running.\n Stop the service manually or use
the
--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 2:
(7 comments)
http://gerrit.ovirt.org/#/c/30909/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
excuse me to waste your time.. I think it worth a bit effort and you'll ne
Yaniv, I am strongly against changing anything about the way the file is
configured.
Even if it's just a blank line.
The goal of this patch is to move the configuration outside of vdsm and that's
the only thing that should be verified now.
any other change should be done in a different patch.
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 677: getuid_callout \%(scsi_id_path)s --whitelisted
Line 678: --replace-whitespace --device=/dev/%%n\\n
Line 705: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
move the explanation here please
Done
Line 710: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 711:
Line 712: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 713:
Line 720: def getName(self):
Line 721: return 'multipath'
Line 722:
Line 723: def getServices(self):
Line 724: return [multipathd]
correct. and btw, it won't start multipathd if it wasn't up when you call t
Done
Line 725:
Line 726: def configure(self):
Line 727:
Line 728: Set up the multipath daemon configuration to the known and
Line 755:
Line 756: cmd = [constants.EXT_VDSM_TOOL, service-reload,
multipathd]
Line 757: rc, out, err = utils.execCmd(cmd)
Line 758: if rc != 0:
Line 759: sys.stdout.write(Failed to reload Multipath.\n)
this will be required if we remove multipathd from getServices. just import
Done
Line 760:
Line 761: def isconfigured(self, *args):
Line 762:
Line 763: Check the multipath daemon configuration. The configuration
file
Line 766: If the tag above is followed by tag RHEV PRIVATE the
configuration
Line 767: should be preserved at all cost.
Line 768:
Line 769: if os.getuid() != 0:
Line 770: raise NotRootError()
do we really need to be root for that?
yes. reading multipath was previously done by supervdsmserver
Line 771:
Line 772: if os.path.exists(self._MPATH_CONF):
Line 773: first = second = ''
Line 774: with open(self._MPATH_CONF) as f:
Line 785: sys.stdout.write(This manual override for
multipath.conf
Line 786: was based on downrevved
template.
Line 787: You are strongly advised to
Line 788: contact your support
representatives\n)
Line 789: return CONFIGURED
that exactly why i asked for comments near the titles! just explain the mea
Done
Line 790:
Line 791: if self._MPATH_CONF_TAG in first:
Line 792: sys.stdout.write(Current revision of multipath.conf
detected,
Line 793: preserving\n)
Line 798: sys.stdout.write(Downrev multipath.conf
detected,
Line 799: upgrade required\n)
Line 800: return NOT_CONFIGURED
Line 801:
Line 802: sys.stdout.write(multipath Defaulting to False\n)
I think this will be more clear if we change the code to this:
Done
Line 803: return NOT_CONFIGURED
Line 804:
Line 805: def validate(self):
Line 806: return True
--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 3: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10662/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11604/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11447/ : SUCCESS -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 3:
(3 comments)
http://gerrit.ovirt.org/#/c/30909/3/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 668:
Line 669: class MultipathModuleConfigure(_ModuleConfigure):
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
add a comment - start with 2 empty lines between the tags and the contents of
the file
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 704: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3,
Line 705: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
where is my explanation :(
just copy it from isconfigured() doc to here
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
Line 710: # Do not override conf file when private tag added by admin
Line 711: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 712:
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
Line 710: # Do not override conf file when private tag added by admin
this tag\having # RHEV PRIVATE means that the configure won't override
Line 711: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 712:
Line 713: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 714:
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 3:
(3 comments)
http://gerrit.ovirt.org/#/c/30909/3/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 668:
Line 669: class MultipathModuleConfigure(_ModuleConfigure):
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
add a comment - start with 2 empty lines between the tags and the contents
I really disagree with this kind of comment. It feels redundant to me.
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 704: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3,
Line 705: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
where is my explanation :(
Done
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
Line 710: # Do not override conf file when private tag added by admin
Line 711: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 712:
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
Line 710: # Do not override conf file when private tag added by admin
this tag\having # RHEV PRIVATE means that the configure won't override ..
Done
Line 711: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 712:
Line 713: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 714:
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 4: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10674/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11616/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11459/ : SUCCESS -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 2:
(7 comments)
http://gerrit.ovirt.org/#/c/30909/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
excuse me to waste your time.. I think it worth a bit effort and you'll ne
These newlines adds an empty line between the tags and the contents of the
file. See line 712.
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 677: getuid_callout \%(scsi_id_path)s --whitelisted
Line 678: --replace-whitespace --device=/dev/%%n\\n
Line 705: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
again, matters to me. I didn't ask for any logic here, only a comment that
The usage of this tag is explained in the docstring of isconfigured().
Line 710: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 711:
Line 712: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 713:
Line 720: def getName(self):
Line 721: return 'multipath'
Line 722:
Line 723: def getServices(self):
Line 724: return [multipathd]
correct. and btw, it won't start multipathd if it wasn't up when you call t
If we return [multipathd], then with systemd, vdsm will also stop, because
multipathd is a required service for vdsmd. So after configuration, vdsm will
be stopped even if it was running before.
sanlock has same issue - it will leave vdsm stopped when using systemd.
If we go in this way, we should return here [vdsmd, supervdsmd,
multipathd], like LibvirtModuleConfigure.
On the other hand, modifying the way we configure multipathd because we do it
in vdsm-tool is the wrong reason for change. This tool should serve the code
configuring modules, not the other way around.
So I suggest to remove this method, and keep code reloading multipathd in
configure().
Line 725:
Line 726: def configure(self):
Line 727:
Line 728: Set up the multipath daemon configuration to the known and
Line 785: sys.stdout.write(This manual override for
multipath.conf
Line 786: was based on downrevved
template.
Line 787: You are strongly advised to
Line 788: contact your support
representatives\n)
Line 789: return CONFIGURED
I don't understand those cases, but NOT_SURE means that with --force flag (
If the administrator added # RHEV PRIVATE tag to multipath.conf, we are not
allowed to touch the file.
Our options are:
- Fail the operation because we cannot upgrade the file
- Warn that we cannot upgrade the file - this is the current policy when vdsm
configure multipath during startup.
If returning NOT_SURE will override the file, I'm sure we should not return
that.
See https://bugzilla.redhat.com/1076531
Line 790:
Line 791: if self._MPATH_CONF_TAG in first:
Line 792: sys.stdout.write(Current revision of multipath.conf
detected,
Line 793: preserving\n)
Line 798: sys.stdout.write(Downrev multipath.conf
detected,
Line 799: upgrade required\n)
Line 800: return NOT_CONFIGURED
Line 801:
Line 802: sys.stdout.write(multipath Defaulting to False\n)
what does this print mean ?
I think this will be more clear if we change the code to this:
if not os.path.exists(self._MPATH_CONF):
sys.stdout.write(multipath configuration not found)
return NOT_CONFIGURED
Check configuration...
To keep minimal changes, I would only change the message in this patch.
Line 803: return NOT_CONFIGURED
Line 804:
Line 805: def validate(self):
Line 806: return True
Line 802: sys.stdout.write(multipath Defaulting to False\n)
Line 803: return NOT_CONFIGURED
Line 804:
Line 805: def validate(self):
Line 806: return True
validate checks for mistake in the conf file. don't we have any validation
We don't have code for this. Implementing this requires parsing multipath
configuration format, not something that we like do.
We can add a warning here that multipathd conifguration validation is not
implemented. Otherwise admin may believe that the configuration is valid when
it is not.
Line 807:
Line 808:
Line 809: __configurers = (
Line 810: LibvirtModuleConfigure(),
Line 837:
Line 838: services = []
Line 839: for c in
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 2: Code-Review-1
(8 comments)
http://gerrit.ovirt.org/#/c/30909/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
do we need those 2 blank lines?
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 677: getuid_callout \%(scsi_id_path)s --whitelisted
Line 678: --replace-whitespace --device=/dev/%%n\\n
Line 705: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
any reason for the version number? maybe add comment that explains it as in
libvirt conf (line 345)
Line 710: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 711:
Line 712: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 713:
Line 720: def getName(self):
Line 721: return 'multipath'
Line 722:
Line 723: def getServices(self):
Line 724: return [multipathd]
this means what services need to restart after configuring multipath. not sure,
but do we require libvirt restart or vdsm restart after configuring multipathd?
Line 725:
Line 726: def configure(self):
Line 727:
Line 728: Set up the multipath daemon configuration to the known and
Line 755:
Line 756: cmd = [constants.EXT_VDSM_TOOL, service-reload,
multipathd]
Line 757: rc, out, err = utils.execCmd(cmd)
Line 758: if rc != 0:
Line 759: sys.stdout.write(Failed to reload Multipath.\n)
first we use service.py for those actions, second def configure() should take
of restarting the service if required. what service-reload does in that case?
Line 760:
Line 761: def isconfigured(self, *args):
Line 762:
Line 763: Check the multipath daemon configuration. The configuration
file
Line 766: If the tag above is followed by tag RHEV PRIVATE the
configuration
Line 767: should be preserved at all cost.
Line 768:
Line 769: if os.getuid() != 0:
Line 770: raise NotRootError()
do we really need to be root for that?
Line 771:
Line 772: if os.path.exists(self._MPATH_CONF):
Line 773: first = second = ''
Line 774: with open(self._MPATH_CONF) as f:
Line 785: sys.stdout.write(This manual override for
multipath.conf
Line 786: was based on downrevved
template.
Line 787: You are strongly advised to
Line 788: contact your support
representatives\n)
Line 789: return CONFIGURED
I don't understand those cases, but NOT_SURE means that with --force flag
(which we pass during deploy) we will override manual changes in the conf file.
I assume that we want here to return NOT_SURE. and return CONFIGURED only if
we're sure that the manual changes are correct.
Line 790:
Line 791: if self._MPATH_CONF_TAG in first:
Line 792: sys.stdout.write(Current revision of multipath.conf
detected,
Line 793: preserving\n)
Line 798: sys.stdout.write(Downrev multipath.conf
detected,
Line 799: upgrade required\n)
Line 800: return NOT_CONFIGURED
Line 801:
Line 802: sys.stdout.write(multipath Defaulting to False\n)
what does this print mean ?
Line 803: return NOT_CONFIGURED
Line 804:
Line 805: def validate(self):
Line 806: return True
Line 802: sys.stdout.write(multipath Defaulting to False\n)
Line 803: return NOT_CONFIGURED
Line 804:
Line 805: def validate(self):
Line 806: return True
validate checks for mistake in the conf file. don't we have any validation
here? conflicts that we can point on ?
Line 807:
Line 808:
Line 809: __configurers = (
Line 810: LibvirtModuleConfigure(),
--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 2: Code-Review-1
(7 comments)
There are some issues in the old code from multipath that must be changed in
this patch, otherwise this patch is not correct.
http://gerrit.ovirt.org/#/c/30909/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
do we need those 2 blank lines?
Yaniv, this is code moved from storage/multipath.py - we don't want to change
it in this patch.
There is lot of cleanup that we can do, but in a separate future patch. We want
to get this patch in 3.5 as soon as possible.
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 677: getuid_callout \%(scsi_id_path)s --whitelisted
Line 678: --replace-whitespace --device=/dev/%%n\\n
Line 705: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
any reason for the version number? maybe add comment that explains it as in
Again, code moved from multipath, don't touch it in this patch.
Line 710: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 711:
Line 712: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 713:
Line 720: def getName(self):
Line 721: return 'multipath'
Line 722:
Line 723: def getServices(self):
Line 724: return [multipathd]
this means what services need to restart after configuring multipath. not s
In systemd, vdsm depends on multipath, so it will be restarted when you restart
multipath. Do we have this logic in the vdsm-tool or we rstart services
manually?
But we don't need to restart multipath since we reload it in configure()
Line 725:
Line 726: def configure(self):
Line 727:
Line 728: Set up the multipath daemon configuration to the known and
Line 750: if utils.isOvirtNode():
Line 751: NodeCfg().persist(self._MPATH_CONF)
Line 752:
Line 753: # Flush all unused multipath device maps
Line 754: utils.execCmd([constants.EXT_MULTIPATH, -F])
In a later patch, we should check if this is really needed, and fix error
handling. As Dan say, if you don't care that it failed, why do you run it?
When return value is not checked, the code must explain why.
Line 755:
Line 756: cmd = [constants.EXT_VDSM_TOOL, service-reload,
multipathd]
Line 757: rc, out, err = utils.execCmd(cmd)
Line 758: if rc != 0:
Line 752:
Line 753: # Flush all unused multipath device maps
Line 754: utils.execCmd([constants.EXT_MULTIPATH, -F])
Line 755:
Line 756: cmd = [constants.EXT_VDSM_TOOL, service-reload,
multipathd]
We are running in vdsm tool :-)
Line 757: rc, out, err = utils.execCmd(cmd)
Line 758: if rc != 0:
Line 759: sys.stdout.write(Failed to reload Multipath.\n)
Line 760:
Line 755:
Line 756: cmd = [constants.EXT_VDSM_TOOL, service-reload,
multipathd]
Line 757: rc, out, err = utils.execCmd(cmd)
Line 758: if rc != 0:
Line 759: sys.stdout.write(Failed to reload Multipath.\n)
Yaniv:
we don't need to restart multipathd after configuration, reload is enough for
multipathd.
Yeela:
This will fail on the first vdsm install, because multipathd is not running,
logging bogus error to stdout, while nothing was failed.
If this fail later, when multipathd *is* running, we are hidding a fatal error
that must cause the tool to fail loudly.
The correct logic for reloading multipath is:
reload multipathd
if failed, check if multipathd is running
if running, fail loudly
I think I already commented about this in an older patch.
This must be fixed in *this* patch, as the code moved form multipath.py is not
correct, assuming that multipath is always running.
Line 760:
Line 761: def isconfigured(self, *args):
Line 762:
Line 763: Check the multipath daemon configuration. The configuration
file
Line 837:
Line 838: services = []
Line 839: for c in configurer_to_trigger:
Line 840: for s in c.getServices():
Line 841: if service.service_status(s, False) == 0:
Yaniv: does this mean that we cannot configure multiapthd while it is running?
There is no problem in doing that from the multipathd point of view. Not sure
if we like to change multipathd configuration while vdsm is running.
Line 842: if not args.force:
Line 843: raise InvalidRun(
Line 844: \n\nCannot configure while service '%s' is
Line 845:
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 2:
(4 comments)
http://gerrit.ovirt.org/#/c/30909/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
Yaniv, this is code moved from storage/multipath.py - we don't want to chan
please..if without those 2 blank lines all stay the same, why do you so care?
just check that
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 677: getuid_callout \%(scsi_id_path)s --whitelisted
Line 678: --replace-whitespace --device=/dev/%%n\\n
Line 705: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
Again, code moved from multipath, don't touch it in this patch.
comment? seriously ? please add a comment and explain your investigation of the
old code about this version.
Line 710: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 711:
Line 712: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 713:
Line 720: def getName(self):
Line 721: return 'multipath'
Line 722:
Line 723: def getServices(self):
Line 724: return [multipathd]
In systemd, vdsm depends on multipath, so it will be restarted when you res
check the configure code please. we manually stop those services before
configure all the related service that might be effected during this service
conf changes. so vdsm is one of them if I understand correctly.. and not
multipathd of course. I just explain what this function is being used to
(please check and don't believe me)
Line 725:
Line 726: def configure(self):
Line 727:
Line 728: Set up the multipath daemon configuration to the known and
Line 837:
Line 838: services = []
Line 839: for c in configurer_to_trigger:
Line 840: for s in c.getServices():
Line 841: if service.service_status(s, False) == 0:
Yaniv: does this mean that we cannot configure multiapthd while it is runni
we don't. we stop all services that returns from getServices. I explained that
above. and why we should care if we stop multipathd and start it after the
configure instead of calling the reload? please convince me that its better..
otherwise I don't see why to change this logic, it sounds right to stop
something before changing its behavior
Line 842: if not args.force:
Line 843: raise InvalidRun(
Line 844: \n\nCannot configure while service '%s' is
Line 845: running.\n Stop the service manually or use
the
--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 2:
(3 comments)
http://gerrit.ovirt.org/#/c/30909/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
please..if without those 2 blank lines all stay the same, why do you so car
Changing now these lines is a waste of time and make the review harder. We
should touch only stuff that matter now. We can improve other stuff later.
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 677: getuid_callout \%(scsi_id_path)s --whitelisted
Line 678: --replace-whitespace --device=/dev/%%n\\n
Line 705: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
comment? seriously ? please add a comment and explain your investigation of
It does not matter now, we don't want to change the logic of the multipath
configuration in this patch, just to move it into vdsm-tool.
Line 710: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 711:
Line 712: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 713:
Line 720: def getName(self):
Line 721: return 'multipath'
Line 722:
Line 723: def getServices(self):
Line 724: return [multipathd]
check the configure code please. we manually stop those services before con
I think that stopping multipath while we configuring should be the safest way,
and if all other services work like this, then this should be the solution at
this point.
The nice thing using this logic is simplifying configure(), no need to reload
and handle inactive multipathd.
Line 725:
Line 726: def configure(self):
Line 727:
Line 728: Set up the multipath daemon configuration to the known and
--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 2:
(4 comments)
http://gerrit.ovirt.org/#/c/30909/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 670:
Line 671: _MPATH_CONF = /etc/multipath.conf
Line 672:
Line 673: _STRG_MPATH_CONF = (
Line 674: \n\n
Changing now these lines is a waste of time and make the review harder. We
excuse me to waste your time.. I think it worth a bit effort and you'll never
touch it later (as you didn't touch the old way till now)
Line 675: defaults {\n
Line 676: polling_interval5\n
Line 677: getuid_callout \%(scsi_id_path)s --whitelisted
Line 678: --replace-whitespace --device=/dev/%%n\\n
Line 705: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 706: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 707: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
Line 708:
Line 709: _MPATH_CONF_TAG = # RHEV REVISION 1.0
It does not matter now, we don't want to change the logic of the multipath
again, matters to me. I didn't ask for any logic here, only a comment that
explain your investigation about why we use this tag in the configuration.
please try understand what I ask first.. I think its reasonable to ask for
explanation and that it won't take you so long if you already understand this
part (as I wish you do)
Line 710: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 711:
Line 712: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 713:
Line 720: def getName(self):
Line 721: return 'multipath'
Line 722:
Line 723: def getServices(self):
Line 724: return [multipathd]
I think that stopping multipath while we configuring should be the safest w
correct. and btw, it won't start multipathd if it wasn't up when you call to
configure. but are you sure vdsmd shouldn't restart? if not, this part is well
done.
Line 725:
Line 726: def configure(self):
Line 727:
Line 728: Set up the multipath daemon configuration to the known and
Line 755:
Line 756: cmd = [constants.EXT_VDSM_TOOL, service-reload,
multipathd]
Line 757: rc, out, err = utils.execCmd(cmd)
Line 758: if rc != 0:
Line 759: sys.stdout.write(Failed to reload Multipath.\n)
Yaniv:
anyway line 724 will cause multipathd restart (it will stop the service if up,
configure it, and will start it afterwards. if the service was down, only
configure it.
this part is not needed, and we do anything as this will be called when
multipathd is down
Line 760:
Line 761: def isconfigured(self, *args):
Line 762:
Line 763: Check the multipath daemon configuration. The configuration
file
--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: Allon Mureinik amure...@redhat.com
Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Nir Soffer nsof...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com
Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 1: (3 comments) Nir: Thank you for the review... some answers to your comments: 1. as far as I could tell, there is no special check when vdsm starts, it just uses vdsm tool isconfigured. do you know any other place for this? 2.Thank you, reverted in following patch. 3. maybe we should close the bug as duplicate of the current bug-url attached to this patch? http://gerrit.ovirt.org/#/c/30909/1//COMMIT_MSG Commit Message: Line 7: Move multipath configuration to vdsm-tool configurator Line 8: Line 9: Previously multipath is reconfigured on each vdsm Line 10: service restart. Line 11: Now it will be reconfigured only on user demand. This does not describe why this change is needed. This is the most importan Done Line 12: Line 13: Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da Line 8: Line 9: Previously multipath is reconfigured on each vdsm Line 10: service restart. Line 11: Now it will be reconfigured only on user demand. Line 12: We have a bug url for this Done Line 13: Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da http://gerrit.ovirt.org/#/c/30909/1/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 759: rc, out, err = utils.execCmd(cmd) Line 760: if rc != 0: Line 761: sys.stdout.write(Failed to reload Multipath.) Line 762: Line 763: def isconfigured(self, *args): This should return CONFIGURED, NOT_CONFIGURED, or NOT_SURE, not a boolean. Done Line 764: Line 765: Check the multipath daemon configuration. The configuration file Line 766: /etc/multipath.conf should contain private tag in form Line 767: RHEV REVISION X.Y for this check to succeed. -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 1: 1. as far as I could tell, there is no special check when vdsm starts, it just uses vdsm tool isconfigured. do you know any other place for this? I did not check this yet, but if the isconfigured call fails with clear error message when multipath is not configured, the this is fine. -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 2: Verified+1 -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 1: 3. maybe we should close the bug as duplicate of the current bug-url attached to this patch? bug 1120209 are not a duplicate, and this change fix it a side effect. -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 2: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10601/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11543/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11386/ : SUCCESS -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 3: (10 comments) http://gerrit.ovirt.org/#/c/26123/3/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 23 Line 24 Line 25 Line 26 Line 27 This change is good, but not related to this patch. Please separate to anot It's related because I'm using constants. Done. Line 219: product \Compellent Vol\\n Line 220: no_path_retry fail\n Line 221: }\n Line 222: } Line 223: ) Please use multi-line string literals for stuff like this: This is an unrelated change to this patch. It should be done in a different patch if needed. Line 224: Line 225: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3, Line 226: # RHEV REVISION 0.4, # RHEV REVISION 0.5, Line 227: # RHEV REVISION 0.6, # RHEV REVISION 0.7, Line 224: Line 225: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3, Line 226: # RHEV REVISION 0.4, # RHEV REVISION 0.5, Line 227: # RHEV REVISION 0.6, # RHEV REVISION 0.7, Line 228: # RHEV REVISION 0.8, # RHEV REVISION 0.9] This is too much duplication here. Please replace with something like: This is an unrelated change to this patch. It should be done in a different patch if needed. Line 229: _MPATH_CONF_TAG = # RHEV REVISION 1.0 Line 230: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE Line 231: Line 232: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF Line 240: ) Line 241: Line 242: Line 243: def __init__(self): Line 244: super(MultipathModuleConfigure, self).__init__() This does nothing, please remove. Not implementing this will call the super Done Line 245: Line 246: def getName(self): Line 247: return 'multipath' Line 248: Line 257: if os.getuid() != 0: Line 258: raise UserWarning(Must run as root) Line 259: if self.isconfigured(): Line 260: return Line 261: if os.path.exists(MultipathModuleConfigure._MPATH_CONF): Use self. Done Line 262: utils.rotateFiles( Line 263: os.path.dirname(MultipathModuleConfigure._MPATH_CONF), Line 264: os.path.basename(MultipathModuleConfigure._MPATH_CONF), Line 265: MultipathModuleConfigure._MAX_CONF_COPIES, Line 276: raise RuntimeError(Failed to perform Multipath config.) Line 277: utils.persistFile(MultipathModuleConfigure._MPATH_CONF) Line 278: Line 279: # Flush all unused multipath device maps Line 280: utils.execCmd([constants.EXT_MULTIPATH, -F]) Not sure this is needed, since multipath should do this when reloading conf This is an unrelated change to this patch. It should be done in a different patch if needed. Line 281: Line 282: cmd = [constants.EXT_VDSM_TOOL, service-reload, multipathd] Line 283: rc, out, err = utils.execCmd(cmd) Line 284: if rc != 0: Line 289: Check the multipath daemon configuration. The configuration file Line 290: /etc/multipath.conf should contain private tag in form Line 291: RHEV REVISION X.Y for this check to succeed. Line 292: If the tag above is followed by tag RHEV PRIVATE the configuration Line 293: should be preserved at all cost. This information must be in a header in the configuration file. The user sh I don't see why the user needs to read vdsm code Line 294: Line 295: Line 296: if os.getuid() != 0: Line 297: raise UserWarning(Must run as root) Line 295: Line 296: if os.getuid() != 0: Line 297: raise UserWarning(Must run as root) Line 298: Line 299: if os.path.exists(MultipathModuleConfigure.MPATH_CONF): Use self instead of the class name. Otherwise you will have to modify this Done Line 300: first = second = '' Line 301: with open(MultipathModuleConfigure.MPATH_CONF) as f: Line 302: mpathconf = [x.strip(\n) for x in f.readlines()] Line 303: try: http://gerrit.ovirt.org/#/c/26123/3/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 1038: else: Line 1039: return OSName.UNKNOWN Line 1040: Line 1041: Line 1042: def isOvirtNode(): please rebase on top on current master (which already has this function) Done Line 1043: return getos() in (OSName.RHEVH, OSName.OVIRT) Line 1044: Line 1045: Line 1046: def persistFile(name): Line 1042: def isOvirtNode(): Line 1043: return getos() in (OSName.RHEVH, OSName.OVIRT) Line 1044: Line 1045: Line 1046: def persistFile(name): please introduce this function in its own patch. it should not ignore error Done Line 1047: if isOvirtNode(): Line 1048:
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has abandoned this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Abandoned replaced by a new patch -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has uploaded a new change for review.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Move multipath configuration to vdsm-tool configurator
Previously multipath is reconfigured on each vdsm
service restart.
Now it will be reconfigured only on user demand.
Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da
Signed-off-by: Yeela Kaplan ykap...@redhat.com
---
M lib/vdsm/tool/configurator.py
M vdsm/storage/hsm.py
M vdsm/storage/multipath.py
M vdsm/supervdsmServer
4 files changed, 147 insertions(+), 121 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/09/30909/1
diff --git a/lib/vdsm/tool/configurator.py b/lib/vdsm/tool/configurator.py
index 1e446a6..57923e8 100644
--- a/lib/vdsm/tool/configurator.py
+++ b/lib/vdsm/tool/configurator.py
@@ -26,6 +26,7 @@
import rpm
import shutil
import sys
+import tempfile
import traceback
import uuid
@@ -665,9 +666,155 @@
return configured
+class MultipathModuleConfigure(_ModuleConfigure):
+
+_MPATH_CONF = /etc/multipath.conf
+
+_STRG_MPATH_CONF = (
+\n\n
+defaults {\n
+polling_interval5\n
+getuid_callout \%(scsi_id_path)s --whitelisted
+--replace-whitespace --device=/dev/%%n\\n
+no_path_retry fail\n
+user_friendly_names no\n
+flush_on_last_del yes\n
+fast_io_fail_tmo5\n
+dev_loss_tmo30\n
+max_fds 4096\n
+}\n
+\n
+devices {\n
+device {\n
+vendor \HITACHI\\n
+product \DF.*\\n
+getuid_callout \%(scsi_id_path)s --whitelisted
+--replace-whitespace --device=/dev/%%n\\n
+}\n
+device {\n
+vendor \COMPELNT\\n
+product \Compellent Vol\\n
+no_path_retry fail\n
+}\n
+}
+)
+
+_MAX_CONF_COPIES = 5
+
+_OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3,
+ # RHEV REVISION 0.4, # RHEV REVISION 0.5,
+ # RHEV REVISION 0.6, # RHEV REVISION 0.7,
+ # RHEV REVISION 0.8, # RHEV REVISION 0.9]
+
+_MPATH_CONF_TAG = # RHEV REVISION 1.0
+_MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
+
+_MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
+
+_scsi_id = utils.CommandPath(scsi_id,
+ /sbin/scsi_id, # EL6
+ /usr/lib/udev/scsi_id, # Fedora
+ /lib/udev/scsi_id, # Ubuntu
+ )
+
+def getName(self):
+return 'multipath'
+
+def getServices(self):
+return [multipathd]
+
+def configure(self):
+
+Set up the multipath daemon configuration to the known and
+supported state. The original configuration, if any, is saved
+
+if os.getuid() != 0:
+raise UserWarning(Must run as root)
+
+if self.isconfigured():
+return
+
+if os.path.exists(self._MPATH_CONF):
+utils.rotateFiles(
+os.path.dirname(self._MPATH_CONF),
+os.path.basename(self._MPATH_CONF),
+self._MAX_CONF_COPIES,
+cp=True, persist=True)
+with tempfile.NamedTemporaryFile() as f:
+f.write(self._MPATH_CONF_TEMPLATE %
+{'scsi_id_path': self._scsi_id.cmd})
+f.flush()
+cmd = [constants.EXT_CP, f.name,
+ self._MPATH_CONF]
+rc, out, err = utils.execCmd(cmd)
+
+if rc != 0:
+raise RuntimeError(Failed to perform Multipath config.)
+utils.persistFile(self._MPATH_CONF)
+
+# Flush all unused multipath device maps
+utils.execCmd([constants.EXT_MULTIPATH, -F])
+
+cmd = [constants.EXT_VDSM_TOOL, service-reload, multipathd]
+rc, out, err = utils.execCmd(cmd)
+if rc != 0:
+sys.stdout.write(Failed to reload Multipath.)
+
+def isconfigured(self, *args):
+
+Check the multipath daemon configuration. The configuration file
+/etc/multipath.conf should contain private tag in form
+RHEV REVISION X.Y for this check to succeed.
+If the tag above is followed by tag RHEV PRIVATE the configuration
+should be preserved at all cost.
+
+if os.getuid() != 0:
+raise UserWarning(Must run as root)
+
+if os.path.exists(self._MPATH_CONF):
+first = second = ''
+with open(self._MPATH_CONF) as f:
+mpathconf = [x.strip(\n) for x in f.readlines()]
+try:
+first = mpathconf[0]
+second = mpathconf[1]
+
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 1: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10561/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11503/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11346/ : SUCCESS -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 1: (2 comments) So much nicer when we have several simple patches! Whats missing: 1. checking that multipath is configured when starting vdsm, and failing if multipath is not configured. This should be implemented in the same way we do it for libvirt and sanlock. This addition is a must - we want to move the configuration step to vdsm tool, giving more control to the administrator, but we cannot remove the validation when vdsm is started. 2. Revert http://gerrit.ovirt.org/30704 - When this patch is merged, that patch is not needed any more. 3. Add the bug url from http://gerrit.ovirt.org/30704 to the commit message, since this fix also that bug. The code moved from multipath need some cleanup, but not in this patch. http://gerrit.ovirt.org/#/c/30909/1//COMMIT_MSG Commit Message: Line 8: Line 9: Previously multipath is reconfigured on each vdsm Line 10: service restart. Line 11: Now it will be reconfigured only on user demand. Line 12: We have a bug url for this Line 13: Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da http://gerrit.ovirt.org/#/c/30909/1/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 759: rc, out, err = utils.execCmd(cmd) Line 760: if rc != 0: Line 761: sys.stdout.write(Failed to reload Multipath.) Line 762: Line 763: def isconfigured(self, *args): This should return CONFIGURED, NOT_CONFIGURED, or NOT_SURE, not a boolean. Line 764: Line 765: Check the multipath daemon configuration. The configuration file Line 766: /etc/multipath.conf should contain private tag in form Line 767: RHEV REVISION X.Y for this check to succeed. -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/30909/1//COMMIT_MSG Commit Message: Line 7: Move multipath configuration to vdsm-tool configurator Line 8: Line 9: Previously multipath is reconfigured on each vdsm Line 10: service restart. Line 11: Now it will be reconfigured only on user demand. This does not describe why this change is needed. This is the most important info in a commit message. Line 12: Line 13: Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegu...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 3: This patch changes very old code - please start by rebasing on master. Please break this to few smaller patches. For example, moving rotateFiles from misc to utils is a good change even if we don't move multipath configuration to vdsm-tool. Having all changes in one patch make it harder and slower to review. -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Patch Set 3:
(10 comments)
To make it easier to review, please separate patches where you remove code
without any change (except changes required by the movement), and patches were
you modify the code.
http://gerrit.ovirt.org/#/c/26123/3/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 23
Line 24
Line 25
Line 26
Line 27
This change is good, but not related to this patch. Please separate to another
patch before or after this patch.
Line 219: product \Compellent Vol\\n
Line 220: no_path_retry fail\n
Line 221: }\n
Line 222: }
Line 223: )
Please use multi-line string literals for stuff like this:
_STRG_MPATH_CONF =
defaults {
blah yes
}
...
Line 224:
Line 225: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3,
Line 226: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 227: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 224:
Line 225: _OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3,
Line 226: # RHEV REVISION 0.4, # RHEV REVISION 0.5,
Line 227: # RHEV REVISION 0.6, # RHEV REVISION 0.7,
Line 228: # RHEV REVISION 0.8, # RHEV REVISION 0.9]
This is too much duplication here. Please replace with something like:
_OLD_TAGS = tuple(# RHAT REVISION 0.%d % n for n in range(2, 9))
This will also fix the pep8 issues in the current code.
Line 229: _MPATH_CONF_TAG = # RHEV REVISION 1.0
Line 230: _MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
Line 231:
Line 232: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 240: )
Line 241:
Line 242:
Line 243: def __init__(self):
Line 244: super(MultipathModuleConfigure, self).__init__()
This does nothing, please remove. Not implementing this will call the super
implementation. I know this error is common in this file but this is not a
reason to repeat it.
Line 245:
Line 246: def getName(self):
Line 247: return 'multipath'
Line 248:
Line 257: if os.getuid() != 0:
Line 258: raise UserWarning(Must run as root)
Line 259: if self.isconfigured():
Line 260: return
Line 261: if os.path.exists(MultipathModuleConfigure._MPATH_CONF):
Use self.
Line 262: utils.rotateFiles(
Line 263: os.path.dirname(MultipathModuleConfigure._MPATH_CONF),
Line 264:
os.path.basename(MultipathModuleConfigure._MPATH_CONF),
Line 265: MultipathModuleConfigure._MAX_CONF_COPIES,
Line 276: raise RuntimeError(Failed to perform Multipath
config.)
Line 277: utils.persistFile(MultipathModuleConfigure._MPATH_CONF)
Line 278:
Line 279: # Flush all unused multipath device maps
Line 280: utils.execCmd([constants.EXT_MULTIPATH, -F])
Not sure this is needed, since multipath should do this when reloading
configuration. For another patch.
Line 281:
Line 282: cmd = [constants.EXT_VDSM_TOOL, service-reload,
multipathd]
Line 283: rc, out, err = utils.execCmd(cmd)
Line 284: if rc != 0:
Line 281:
Line 282: cmd = [constants.EXT_VDSM_TOOL, service-reload,
multipathd]
Line 283: rc, out, err = utils.execCmd(cmd)
Line 284: if rc != 0:
Line 285: raise RuntimeError(Failed to reload Multipath.)
This assumes that multipathd is running, but this is not the case when using
vdsm-tool configure before vdsm is started in the first time.
If is not running there is no need to reload, because it will pick the new
configuration when it is started by vdsm later.
So we should reload only if multipathd is running.
Line 286:
Line 287: def isconfigured(self, *args):
Line 288:
Line 289: Check the multipath daemon configuration. The configuration
file
Line 289: Check the multipath daemon configuration. The configuration
file
Line 290: /etc/multipath.conf should contain private tag in form
Line 291: RHEV REVISION X.Y for this check to succeed.
Line 292: If the tag above is followed by tag RHEV PRIVATE the
configuration
Line 293: should be preserved at all cost.
This information must be in a header in the configuration file. The user should
not read vdsm code to understand this mechanism.
Line 294:
Line 295:
Line 296: if os.getuid() != 0:
Line 297: raise UserWarning(Must run as root)
Line 295:
Line 296: if os.getuid() != 0:
Line 297: raise UserWarning(Must run as root)
Line 298:
Line 299: if os.path.exists(MultipathModuleConfigure.MPATH_CONF):
Use self instead of the class name. Otherwise you
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Dan Kenigsberg has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 2: (2 comments) http://gerrit.ovirt.org/#/c/26123/2/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 154: else: Line 155: raise Line 156: Line 157: Line 158: def rotateFiles(directory, prefixName, gen, cp=False, persist=False): good to have rotate to old conf files. +1 for use it in configurator (maybe The benefit of commenting out is that we: * don't need this awfully-implemented function out of storage * already have the mechanism to uncomment the file and return it to original content upon uninstall and during upgrade. Line 159: logging.debug(dir: %s, prefixName: %s, versions: %s % Line 160: (directory, prefixName, gen)) Line 161: gen = int(gen) Line 162: files = os.listdir(directory) Line 906: else: Line 907: return OSName.UNKNOWN Line 908: Line 909: Line 910: def isOvirtNode(): please rebase on top of http://gerrit.ovirt.org/28486 ; in any case, such changes belong to a separate patch. Line 911: return getos() in (OSName.RHEVH, OSName.OVIRT) Line 912: Line 913: Line 914: def persistFile(name): -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Itamar Heim ih...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Dan Kenigsberg has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 3: Code-Review-1 (2 comments) very partial review http://gerrit.ovirt.org/#/c/26123/3/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 1038: else: Line 1039: return OSName.UNKNOWN Line 1040: Line 1041: Line 1042: def isOvirtNode(): please rebase on top on current master (which already has this function) Line 1043: return getos() in (OSName.RHEVH, OSName.OVIRT) Line 1044: Line 1045: Line 1046: def persistFile(name): Line 1042: def isOvirtNode(): Line 1043: return getos() in (OSName.RHEVH, OSName.OVIRT) Line 1044: Line 1045: Line 1046: def persistFile(name): please introduce this function in its own patch. it should not ignore error emanating of the persist script. Line 1047: if isOvirtNode(): Line 1048: execCmd([constants.EXT_PERSIST, name], sudo=True) Line 1049: Line 1050: -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Itamar Heim ih...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 3: Code-Review-1 and don't forget to rebase and add toolTests for that -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Itamar Heim ih...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 3: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9779/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8841/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9626/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_master_storage_functional_tests_localfs_gerrit/1136/ : There was an infra issue, please contact in...@ovirt.org -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Itamar Heim ih...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 3: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8841/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9626/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_master_storage_functional_tests_localfs_gerrit/1136/ : There was an infra issue, please contact in...@ovirt.org http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9815/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Itamar Heim ih...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 3: Code-Review-1 Please fix pep8 violations: lib/vdsm/tool/configurator.py:226:17: E128 continuation line under-indented for visual indent lib/vdsm/tool/configurator.py:243:5: E303 too many blank lines (2) -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Itamar Heim ih...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 3: Code-Review-1 Verified-1 Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9779/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8841/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9626/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_master_storage_functional_tests_localfs_gerrit/1122/ : There was an infra issue, please contact in...@ovirt.org -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Itamar Heim ih...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Itamar Heim has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 2: ping -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Itamar Heim ih...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
mooli tayer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 2: Regarding yaniv previous comment the tool tests were merged: http://gerrit.ovirt.org/#/c/25263/ -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yaniv Bronhaim has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 2: Code-Review-1 (5 comments) hope that during next week http://gerrit.ovirt.org/#/c/25263/ will be merged. and mooli will be able to show you how to test tools stuff. the verified ack should be only if tool tests exist as part of the additional configure class patch http://gerrit.ovirt.org/#/c/26123/2/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 22: import grp Line 23: import argparse Line 24: import tempfile Line 25: Line 26: from .. import utils, constants why? try to stick with your goal in this patch scope. you can propose this change in separate patch and ill be glad to discuss about it Line 27: from . import service, expose Line 28: Line 29: Line 30: class _ModuleConfigure(object): Line 216: _scsi_id = utils.CommandPath(scsi_id, Line 217: /sbin/scsi_id, # EL6 Line 218: /usr/lib/udev/scsi_id, # Fedora Line 219: /lib/udev/scsi_id, # Ubuntu Line 220: ) put all multipath related parameters as class globals Line 221: Line 222: Line 223: class MultipathModuleConfigure(_ModuleConfigure): Line 224: Line 306: upgrade required) Line 307: return False Line 308: Line 309: sys.stdout.write(multipath Defaulting to False) Line 310: return False check the rest of the interface, what about validate.. might be also useful. and keep in mind that reconfigureOnForce is very meaningful if you want to override old configuration on --force flag . if False (not), state it specifically Line 311: Line 312: Line 313: __configurers = ( Line 314: LibvirtModuleConfigure(), http://gerrit.ovirt.org/#/c/26123/2/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 154: else: Line 155: raise Line 156: Line 157: Line 158: def rotateFiles(directory, prefixName, gen, cp=False, persist=False): this function is so old and dusty. would you break this big patch to smalle good to have rotate to old conf files. +1 for use it in configurator (maybe even put this function there) and use it also for old libvirt related conf files (but in separate patch) Line 159: logging.debug(dir: %s, prefixName: %s, versions: %s % Line 160: (directory, prefixName, gen)) Line 161: gen = int(gen) Line 162: files = os.listdir(directory) Line 206: try: Line 207: execCmd([constants.EXT_PERSIST, newName], logErr=False, Line 208: sudo=True) Line 209: except: Line 210: pass omg :) Line 211: Line 212: Line 213: IPXMLRPCRequestHandler = SimpleXMLRPCRequestHandler Line 214: -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsl...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has uploaded a new change for review.
Change subject: Move multipath configuration to vdsm-tool configurator
..
Move multipath configuration to vdsm-tool configurator
Previously multipathe is recofigured on each vdsm
service restart.
Now it will be reconfigured only on user demand.
Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8
Signed-off-by: Yeela Kaplan ykap...@redhat.com
---
M lib/vdsm/tool/configurator.py
M lib/vdsm/utils.py
M tests/miscTests.py
M tests/utilsTests.py
M vdsm/caps.py
M vdsm/storage/hsm.py
M vdsm/storage/misc.py
M vdsm/storage/multipath.py
M vdsm/storage/sd.py
M vdsm/supervdsmServer
10 files changed, 301 insertions(+), 281 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/23/26123/1
diff --git a/lib/vdsm/tool/configurator.py b/lib/vdsm/tool/configurator.py
index 869d774..d97af61 100644
--- a/lib/vdsm/tool/configurator.py
+++ b/lib/vdsm/tool/configurator.py
@@ -21,11 +21,10 @@
import sys
import grp
import argparse
+import tempfile
-from .. import utils
+from .. import utils, constants
from . import service, expose
-from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP
-from ..constants import QEMU_PROCESS_GROUP, VDSM_GROUP
class _ModuleConfigure(object):
@@ -68,7 +67,7 @@
rc, out, err = utils.execCmd(
(
os.path.join(
-P_VDSM_EXEC,
+constants.P_VDSM_EXEC,
'libvirt_configure.sh'
),
action,
@@ -126,7 +125,7 @@
'/usr/sbin/usermod',
'-a',
'-G',
-'%s,%s' % (QEMU_PROCESS_GROUP, VDSM_GROUP),
+'%s,%s' % (constants.QEMU_PROCESS_GROUP, constants.VDSM_GROUP),
'sanlock'
),
raw=True,
@@ -156,7 +155,7 @@
break
else:
raise RuntimeError(Unable to find sanlock service groups)
-ret = grp.getgrnam(DISKIMAGE_GROUP)[2] in groups
+ret = grp.getgrnam(constants.DISKIMAGE_GROUP)[2] in groups
except IOError as e:
if e.errno == os.errno.ENOENT:
sys.stdout.write(sanlock service is not running\n)
@@ -172,9 +171,149 @@
return ret
+MPATH_CONF = /etc/multipath.conf
+
+STRG_MPATH_CONF = (
+\n\n
+defaults {\n
+polling_interval5\n
+getuid_callout \%(scsi_id_path)s --whitelisted
+--replace-whitespace --device=/dev/%%n\\n
+no_path_retry fail\n
+user_friendly_names no\n
+flush_on_last_del yes\n
+fast_io_fail_tmo5\n
+dev_loss_tmo30\n
+max_fds 4096\n
+}\n
+\n
+devices {\n
+device {\n
+vendor \HITACHI\\n
+product \DF.*\\n
+getuid_callout \%(scsi_id_path)s --whitelisted
+--replace-whitespace --device=/dev/%%n\\n
+}\n
+device {\n
+vendor \COMPELNT\\n
+product \Compellent Vol\\n
+no_path_retry fail\n
+}\n
+}
+)
+
+OLD_TAGS = [# RHAT REVISION 0.2, # RHEV REVISION 0.3,
+# RHEV REVISION 0.4, # RHEV REVISION 0.5,
+# RHEV REVISION 0.6, # RHEV REVISION 0.7,
+# RHEV REVISION 0.8, # RHEV REVISION 0.9]
+MPATH_CONF_TAG = # RHEV REVISION 1.0
+MPATH_CONF_PRIVATE_TAG = # RHEV PRIVATE
+
+MPATH_CONF_TEMPLATE = MPATH_CONF_TAG + STRG_MPATH_CONF
+
+MAX_CONF_COPIES = 5
+
+_scsi_id = utils.CommandPath(scsi_id,
+ /sbin/scsi_id, # EL6
+ /usr/lib/udev/scsi_id, # Fedora
+ /lib/udev/scsi_id, # Ubuntu
+ )
+
+
+class MultipathModuleConfigure(_ModuleConfigure):
+
+def __init__(self):
+super(MultipathModuleConfigure, self).__init__()
+
+def getName(self):
+return 'multipath'
+
+def getServices(self):
+return [multipathd]
+
+def configure(self):
+
+Set up the multipath daemon configuration to the known and
+supported state. The original configuration, if any, is saved
+
+if os.getuid() != 0:
+raise UserWarning(Must run as root)
+if self.isconfigured():
+return
+if os.path.exists(MPATH_CONF):
+utils.rotateFiles(
+os.path.dirname(MPATH_CONF),
+os.path.basename(MPATH_CONF), MAX_CONF_COPIES,
+cp=True, persist=True)
+with tempfile.NamedTemporaryFile() as f:
+f.write(MPATH_CONF_TEMPLATE % {'scsi_id_path': _scsi_id.cmd})
+f.flush()
+cmd = [constants.EXT_CP, f.name, MPATH_CONF]
+rc, out, err = utils.execCmd(cmd)
+
+if rc != 0:
+raise
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 1: Code-Review-1 Verified-1 Build Unstable http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6872/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7662/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7772/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 2: Verified+1 multipath is configurated when using vdsm-tool, under the same conditions as before (will not change existing ovirt configuration). -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
oVirt Jenkins CI Server has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 2: Build Unstable http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6874/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7664/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7774/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
Dan Kenigsberg has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator .. Patch Set 2: Code-Review-1 (3 comments) Very very partial review. Note the persistent pep8 errors lib/vdsm/tool/configurator.py:217:26: E128 continuation line under-indented for visual indent lib/vdsm/tool/configurator.py:220:26: E124 closing bracket does not match visual indentation lib/vdsm/tool/configurator.py:267:5: E303 too many blank lines (2) http://gerrit.ovirt.org/#/c/26123/2/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 154: else: Line 155: raise Line 156: Line 157: Line 158: def rotateFiles(directory, prefixName, gen, cp=False, persist=False): this function is so old and dusty. would you break this big patch to smaller ones, and have a separate patch re-implementing this function? I, for one, do not recall why we care about cp=True. I certainly resent the silent swallowing of exceptions. Luckily, when used by an external vdsm-tool, there is no excuse for doing that. Line 159: logging.debug(dir: %s, prefixName: %s, versions: %s % Line 160: (directory, prefixName, gen)) Line 161: gen = int(gen) Line 162: files = os.listdir(directory) Line 203: except: Line 204: pass Line 205: if isOvirtNode() and persist and not cp: Line 206: try: Line 207: execCmd([constants.EXT_PERSIST, newName], logErr=False, we already have a function wrapper for this (persistFile). Let's use it. Line 208: sudo=True) Line 209: except: Line 210: pass Line 211: Line 206: try: Line 207: execCmd([constants.EXT_PERSIST, newName], logErr=False, Line 208: sudo=True) Line 209: except: Line 210: pass omg Line 211: Line 212: Line 213: IPXMLRPCRequestHandler = SimpleXMLRPCRequestHandler Line 214: -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybron...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer mta...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
