Martin Polednik has uploaded a new change for review.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
supervdsm: move udevadm methods to udevadm module
udevadm module already exists, so there is no reason to keep the method logic in supervdsm.
Change-Id: Ie2b72b580e77872068ae6357410d940a83626b05 Signed-off-by: Martin Polednik mpolednik@redhat.com --- M lib/vdsm/udevadm.py M vdsm/supervdsmServer 2 files changed, 52 insertions(+), 42 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/08/44808/1
diff --git a/lib/vdsm/udevadm.py b/lib/vdsm/udevadm.py index 2e9bdc2..9c58075 100644 --- a/lib/vdsm/udevadm.py +++ b/lib/vdsm/udevadm.py @@ -19,10 +19,14 @@ #
from __future__ import absolute_import +import errno import logging from . import utils
+from vdsm.constants import EXT_UDEVADM + _UDEVADM = utils.CommandPath("udevadm", "/sbin/udevadm", "/usr/sbin/udevadm") +_UDEV_WITH_RELOAD_VERSION = 181
class Error(Exception): @@ -61,6 +65,50 @@ logging.error("%s", e)
+def trigger(attr_matches=(), property_matches=()): + _reload() + cmd = [EXT_UDEVADM, 'trigger', '--verbose', '--action', 'change'] + + for name, value in property_matches: + cmd.append('--property-match={}={}'.format(name, value)) + + for name, value in attr_matches: + cmd.append('--property-match={}={}'.format(name, value)) + + rc, out, err = utils.execCmd(cmd) + if rc: + raise OSError(errno.EINVAL, 'Could not trigger change ' + 'out %s\nerr %s' % (out, err)) + + +def _reload(): + if _has_reload(): + reload = "--reload" + else: + reload = "--reload-rules" + cmd = [EXT_UDEVADM, 'control', reload] + rc, out, err = utils.execCmd(cmd) + if rc: + logging.error("Udevadm reload-rules command failed rc=%s, " + "out="%s", err="%s"", rc, out, err) + raise OSError(errno.EINVAL, "Could not reload rules") + + +@utils.memoized +def _version(): + cmd = [EXT_UDEVADM, '--version'] + rc, out, err = utils.execCmd(cmd) + if rc: + logging.error("Udevadm version command failed rc=%s, " + "out="%s", err="%s"", rc, out, err) + raise RuntimeError("Could not get udev version number") + return int(out[0]) + + +def _has_reload(): + return _version() > _UDEV_WITH_RELOAD_VERSION + + def _run_command(args): cmd = [_UDEVADM.cmd] cmd.extend(args) diff --git a/vdsm/supervdsmServer b/vdsm/supervdsmServer index 57b7b4d..17f12f3 100755 --- a/vdsm/supervdsmServer +++ b/vdsm/supervdsmServer @@ -35,6 +35,7 @@ from vdsm.infra import zombiereaper
from caps import Architecture +from vdsm import udevadm import numaUtils
LOG_CONF_PATH = "/etc/vdsm/svdsm.logger.conf" @@ -73,7 +74,7 @@ from storage import multipath from storage.fileUtils import chown, resolveGid, resolveUid from storage.fileUtils import validateAccess as _validateAccess -from vdsm.constants import METADATA_GROUP, EXT_CHOWN, EXT_UDEVADM, \ +from vdsm.constants import METADATA_GROUP, EXT_CHOWN, \ DISKIMAGE_USER, DISKIMAGE_GROUP, \ P_LIBVIRT_VMCHANNELS, P_OVIRT_VMCONSOLES, \ VDSM_USER, QEMU_PROCESS_USER, QEMU_PROCESS_GROUP @@ -119,8 +120,6 @@
class _SuperVdsm(object): - - UDEV_WITH_RELOAD_VERSION = 181
log = logging.getLogger("SuperVdsm.ServerCallback")
@@ -283,20 +282,8 @@ return setSafeNetworkConfig()
@logDecorator - def udevTrigger(self, attr_matches=(), property_matches=()): - self.__udevReloadRules() - cmd = [EXT_UDEVADM, 'trigger', '--verbose', '--action', 'change'] - - for name, value in property_matches: - cmd.append('--property-match={}={}'.format(name, value)) - - for name, value in attr_matches: - cmd.append('--property-match={}={}'.format(name, value)) - - rc, out, err = utils.execCmd(cmd) - if rc: - raise OSError(errno.EINVAL, 'Could not trigger change ' - 'out %s\nerr %s' % (out, err)) + def udevTrigger(self, *args, **kwargs): + udevadm.trigger(*args, **kwargs)
@logDecorator def appropriateDevice(self, guid, thiefId): @@ -453,31 +440,6 @@ @logDecorator def set_rp_filter_strict(self, dev): sysctl.set_rp_filter_strict(dev) - - def __udevReloadRules(self): - if self.__udevOperationReload(): - reload = "--reload" - else: - reload = "--reload-rules" - cmd = [EXT_UDEVADM, 'control', reload] - rc, out, err = utils.execCmd(cmd) - if rc: - self.log.error("Udevadm reload-rules command failed rc=%s, " - "out="%s", err="%s"", rc, out, err) - raise OSError(errno.EINVAL, "Could not reload rules") - - @utils.memoized - def __udevVersion(self): - cmd = [EXT_UDEVADM, '--version'] - rc, out, err = utils.execCmd(cmd) - if rc: - self.log.error("Udevadm version command failed rc=%s, " - " out="%s", err="%s"", rc, out, err) - raise RuntimeError("Could not get udev version number") - return int(out[0]) - - def __udevOperationReload(self): - return self.__udevVersion() > self.UDEV_WITH_RELOAD_VERSION
def terminate(signo, frame):
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 1:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 2:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Francesco Romani has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 2:
(1 comment)
looks nicer, and patch looks like code move + renaming. Will doublecheck move later.
https://gerrit.ovirt.org/#/c/44808/2/lib/vdsm/udevadm.py File lib/vdsm/udevadm.py:
Line 81: 'out %s\nerr %s' % (out, err)) Line 82: Line 83: Line 84: def _reload(): Line 85: if _has_reload(): to me looks simpler and nicer if we just inline this method. Drop this comment if _has_reload has 2+ callers, but it seems it is used just here. Line 86: reload = "--reload" Line 87: else: Line 88: reload = "--reload-rules" Line 89: cmd = [EXT_UDEVADM, 'control', reload]
Nir Soffer has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 2: Code-Review-1
Please see my comments in https://gerrit.ovirt.org/44684
I don't want to repeat all 10 again.
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 3:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Francesco Romani has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 3: Code-Review-1
(2 comments)
seems bad rebase. -1 for visibility.
https://gerrit.ovirt.org/#/c/44808/3/lib/vdsm/udevadm.py File lib/vdsm/udevadm.py:
Line 60: except Error as e: Line 61: logging.error("%s", e) Line 62: Line 63: Line 64: def udevTrigger(self, attr_matches=(), property_matches=()): seems bad rebase, IIUC should be
def trigger(attr_matches=(), property_matches=()): Line 65: ''' Line 66: Request device events from the kernel. Line 67: Line 68: Arguments:
Line 84: Line 85: and causes only events from devices that match Line 86: given property to be triggered. Line 87: ''' Line 88: self.__udevReloadRules() this should be renamed or dropped, propably Line 89: cmd = ['trigger', '--verbose', '--action', 'change'] Line 90: Line 91: for name, value in property_matches: Line 92: cmd.append('--property-match={}={}'.format(name, value))
Nir Soffer has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 3: Code-Review-1
(2 comments)
https://gerrit.ovirt.org/#/c/44808/3/lib/vdsm/udevadm.py File lib/vdsm/udevadm.py:
Line 84: Line 85: and causes only events from devices that match Line 86: given property to be triggered. Line 87: ''' Line 88: self.__udevReloadRules()
this should be renamed or dropped, propably
__udevReloadRules should move here. Line 89: cmd = ['trigger', '--verbose', '--action', 'change'] Line 90: Line 91: for name, value in property_matches: Line 92: cmd.append('--property-match={}={}'.format(name, value))
https://gerrit.ovirt.org/#/c/44808/3/vdsm/supervdsmServer File vdsm/supervdsmServer:
Line 281: def setSafeNetworkConfig(self): Line 282: return setSafeNetworkConfig() Line 283: Line 284: @logDecorator Line 285: def udevTrigger(self, *args, **kwargs): Using *args, **kwargs make it hard to understand the code. Now you have to drill down to udevadm.trigger to understand what are the arguments.
Should be:
def udevTrigger(self, attr_matches=(), property_matches=()): Line 286: try: Line 287: udevadm.trigger(*args, **kwargs) Line 288: except udevadm.Error as e: Line 289: raise OSError(errno.EINVAL, 'Could not trigger change '
Nir Soffer has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 1:
This should include removal of the EXT_UDEVADM constant from both constats.py, configure.ac and supervdsm.
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 4:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 5:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 6:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 7:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Nir Soffer has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 7: Code-Review+2
Martin Polednik has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 7: Verified+1
Martin Polednik has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 7: -Verified
sneaky rebase/bug found
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 8:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 9:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Nir Soffer has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 9:
(1 comment)
Need another rebase.
https://gerrit.ovirt.org/#/c/44808/9/lib/vdsm/udevadm.py File lib/vdsm/udevadm.py:
Line 60: except Error as e: Line 61: logging.error("%s", e) Line 62: Line 63: Line 64: def trigger(attr_matches=(), property_matches=()): :-) Line 65: ''' Line 66: Request device events from the kernel. Line 67: Line 68: Arguments:
Nir Soffer has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 9: Code-Review-1
For visibility.
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 10:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Nir Soffer has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 10: Code-Review+2
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 11:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Nir Soffer has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 11: Code-Review+2
Martin Polednik has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 11: Verified+1
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 12:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Dan Kenigsberg has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 12: Code-Review-1
please rebase
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 13:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Martin Polednik has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 13: Verified+1
Dan Kenigsberg has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 13: Code-Review+2
Dan Kenigsberg has submitted this change and it was merged.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
supervdsm: move udevadm methods to udevadm module
udevadm module already exists, so there is no reason to keep the method logic in supervdsm. udevadm also provides us with a means to execute a command, it is therefore used instead of EXT_UDEVADM. To keep the behavior same, Error raised by trigger is caught in supervdsm and re-raised as IOError (same as before the patch).
Change-Id: Ie2b72b580e77872068ae6357410d940a83626b05 Signed-off-by: Martin Polednik mpolednik@redhat.com Reviewed-on: https://gerrit.ovirt.org/44808 Continuous-Integration: Jenkins CI Reviewed-by: Dan Kenigsberg danken@redhat.com --- M configure.ac M lib/vdsm/constants.py.in M lib/vdsm/udevadm.py M vdsm/supervdsmServer 4 files changed, 45 insertions(+), 53 deletions(-)
Approvals: Jenkins CI: Passed CI tests Dan Kenigsberg: Looks good to me, approved Martin Polednik: Verified
automation@ovirt.org has posted comments on this change.
Change subject: supervdsm: move udevadm methods to udevadm module ......................................................................
Patch Set 14:
* Update tracker::IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found.
vdsm-patches@lists.fedorahosted.org