Mike Kolesnik has uploaded a new change for review.
Change subject: hooks: Extract function for command execution ......................................................................
hooks: Extract function for command execution
This function will be used by the openstacknet hook to execute commands in a more convenient way, instead of repeating the logic to exit the hook everywhere.
Change-Id: Id0bcc3be324d1afd9855f9d624cbe4efbf3cf6dd Signed-off-by: Mike Kolesnik mkolesni@redhat.com --- M vdsm_hooks/openstacknet/after_device_create.py M vdsm_hooks/openstacknet/openstacknet_utils.py 2 files changed, 11 insertions(+), 6 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/83/22583/1
diff --git a/vdsm_hooks/openstacknet/after_device_create.py b/vdsm_hooks/openstacknet/after_device_create.py index be23a32..bdd1c22 100755 --- a/vdsm_hooks/openstacknet/after_device_create.py +++ b/vdsm_hooks/openstacknet/after_device_create.py @@ -33,17 +33,13 @@ from openstacknet_utils import PROVIDER_TYPE_KEY from openstacknet_utils import PT_BRIDGE from openstacknet_utils import VNIC_ID_KEY +from openstacknet_utils import executeOrExit from vdsm.constants import EXT_BRCTL
def disconnectVnic(portId): tapName = ('tap' + portId)[:DEV_MAX_LENGTH] - command = [EXT_BRCTL, 'delif', DUMMY_BRIDGE, tapName] - retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) - - if retcode != 0: - hooking.exit_hook("Can't disconnect %s from %s, due to: %s" - % (tapName, DUMMY_BRIDGE, err)) + executeOrExit([EXT_BRCTL, 'delif', DUMMY_BRIDGE, tapName])
def main(): diff --git a/vdsm_hooks/openstacknet/openstacknet_utils.py b/vdsm_hooks/openstacknet/openstacknet_utils.py index d6c769b..36f6e0f 100644 --- a/vdsm_hooks/openstacknet/openstacknet_utils.py +++ b/vdsm_hooks/openstacknet/openstacknet_utils.py @@ -1,5 +1,6 @@ #!/usr/bin/python
+import hooking from vdsm.netinfo import DUMMY_BRIDGE
# Constants for hook's API @@ -15,3 +16,11 @@
# Make pyflakes happy DUMMY_BRIDGE + + +def executeOrExit(command, expectSuccess=True): + retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) + commandFailed = retcode != 0 if expectSuccess else retcode == 0 + if commandFailed: + hooking.exit_hook("Failed to execute %s, due to: %s" % + (str(command), err))
Mike Kolesnik has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 1: Verified+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6207/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5421/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6312/ : SUCCESS
Antoni Segura Puimedon has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
.................................................... File vdsm_hooks/openstacknet/openstacknet_utils.py Line 17: # Make pyflakes happy Line 18: DUMMY_BRIDGE Line 19: Line 20: Line 21: def executeOrExit(command, expectSuccess=True): Do you foresee ever expectSuccess to be False? Otherwise I'd drop this extra parameter and logic. Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: commandFailed = retcode != 0 if expectSuccess else retcode == 0 Line 24: if commandFailed: Line 25: hooking.exit_hook("Failed to execute %s, due to: %s" %
Dan Kenigsberg has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 1:
(2 comments)
.................................................... File vdsm_hooks/openstacknet/openstacknet_utils.py Line 21: def executeOrExit(command, expectSuccess=True): Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: commandFailed = retcode != 0 if expectSuccess else retcode == 0 Line 24: if commandFailed: Line 25: hooking.exit_hook("Failed to execute %s, due to: %s" % Usually it's not nice for a module to explicitly shut down its controlling application. From the usage in a future patch it seems that this could be replaced by raising an exception.
Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: commandFailed = retcode != 0 if expectSuccess else retcode == 0 Line 24: if commandFailed: Line 25: hooking.exit_hook("Failed to execute %s, due to: %s" % Line 26: (str(command), err)) no need to call str(). %s does this automatically, and better.
Mike Kolesnik has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 1:
(3 comments)
.................................................... File vdsm_hooks/openstacknet/openstacknet_utils.py Line 17: # Make pyflakes happy Line 18: DUMMY_BRIDGE Line 19: Line 20: Line 21: def executeOrExit(command, expectSuccess=True): Yes it is used in later patches, otherwise I wouldn't have added this. Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: commandFailed = retcode != 0 if expectSuccess else retcode == 0 Line 24: if commandFailed: Line 25: hooking.exit_hook("Failed to execute %s, due to: %s" %
Line 21: def executeOrExit(command, expectSuccess=True): Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: commandFailed = retcode != 0 if expectSuccess else retcode == 0 Line 24: if commandFailed: Line 25: hooking.exit_hook("Failed to execute %s, due to: %s" % I don't see any hooks code directly raising any kind of exception, and from what I understood the way to exit a hook is by calling hooking.exit_hook().
Can you please suggest how this can be achieved with exception, as this ould be the first of it's kind?
Simply "raise Exception(...)" would do the trick, and be correct for hooking code?
Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: commandFailed = retcode != 0 if expectSuccess else retcode == 0 Line 24: if commandFailed: Line 25: hooking.exit_hook("Failed to execute %s, due to: %s" % Line 26: (str(command), err)) Done
Dan Kenigsberg has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 1:
(2 comments)
.................................................... File vdsm_hooks/openstacknet/openstacknet_utils.py Line 17: # Make pyflakes happy Line 18: DUMMY_BRIDGE Line 19: Line 20: Line 21: def executeOrExit(command, expectSuccess=True): Could you point me to a such usage? searching for executeOrExit.*False found nothing. If this arg is indeed a must, "ignoreErrors" would be a clearer name imo. Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: commandFailed = retcode != 0 if expectSuccess else retcode == 0 Line 24: if commandFailed: Line 25: hooking.exit_hook("Failed to execute %s, due to: %s" %
Line 21: def executeOrExit(command, expectSuccess=True): Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: commandFailed = retcode != 0 if expectSuccess else retcode == 0 Line 24: if commandFailed: Line 25: hooking.exit_hook("Failed to execute %s, due to: %s" % It's a genrally good practice not to exit() an application from within a "utils" library - the main script should take the decisions.
Yes, a simple
if commandFailed: raise RuntimeError("exec failed")
would do.
Mike Kolesnik has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 1:
(2 comments)
.................................................... File vdsm_hooks/openstacknet/openstacknet_utils.py Line 17: # Make pyflakes happy Line 18: DUMMY_BRIDGE Line 19: Line 20: Line 21: def executeOrExit(command, expectSuccess=True): Sorry the implementation using it was apparently only in my head.. I'll remove it. Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: commandFailed = retcode != 0 if expectSuccess else retcode == 0 Line 24: if commandFailed: Line 25: hooking.exit_hook("Failed to execute %s, due to: %s" %
Line 21: def executeOrExit(command, expectSuccess=True): Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: commandFailed = retcode != 0 if expectSuccess else retcode == 0 Line 24: if commandFailed: Line 25: hooking.exit_hook("Failed to execute %s, due to: %s" % Done
oVirt Jenkins CI Server has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 2: Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6535/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5642/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6448/ : SUCCESS
Dan Kenigsberg has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
.................................................... File vdsm_hooks/openstacknet/openstacknet_utils.py Line 19: Line 20: Line 21: def executeOrExit(command): Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: if retCode != 0: Python is case sensitive... Line 24: raise RuntimeError("Failed to execute %s, due to: %s" %
Mike Kolesnik has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 2:
(1 comment)
.................................................... File vdsm_hooks/openstacknet/openstacknet_utils.py Line 19: Line 20: Line 21: def executeOrExit(command): Line 22: retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) Line 23: if retCode != 0: Done Line 24: raise RuntimeError("Failed to execute %s, due to: %s" %
oVirt Jenkins CI Server has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 3:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6538/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5645/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6451/ : SUCCESS
Assaf Muller has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 3: Code-Review+1
Dan Kenigsberg has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 3: Code-Review+2
Mike Kolesnik has posted comments on this change.
Change subject: hooks: Extract function for command execution ......................................................................
Patch Set 3: Verified+1
Dan Kenigsberg has submitted this change and it was merged.
Change subject: hooks: Extract function for command execution ......................................................................
hooks: Extract function for command execution
This function will be used by the openstacknet hook to execute commands in a more convenient way, instead of repeating the logic to exit the hook everywhere.
Change-Id: Id0bcc3be324d1afd9855f9d624cbe4efbf3cf6dd Signed-off-by: Mike Kolesnik mkolesni@redhat.com Reviewed-on: http://gerrit.ovirt.org/22583 Reviewed-by: Assaf Muller amuller@redhat.com Reviewed-by: Dan Kenigsberg danken@redhat.com --- M vdsm_hooks/openstacknet/after_device_create.py M vdsm_hooks/openstacknet/openstacknet_utils.py 2 files changed, 10 insertions(+), 6 deletions(-)
Approvals: Mike Kolesnik: Verified Assaf Muller: Looks good to me, but someone else must approve Dan Kenigsberg: Looks good to me, approved
vdsm-patches@lists.fedorahosted.org
-
amuller＠redhat.com -
asegurap＠redhat.com -
Dan Kenigsberg -
Jenkins CI RO -
mkolesni＠redhat.com