Petr Horáček has uploaded a new change for review.
Change subject: net: ovs: draft of better rollback ......................................................................
net: ovs: draft of better rollback
now it's in a piggy state but working!
Change-Id: I8f6b63d03bb9579e260bfad1686047a431f69543 Signed-off-by: Petr Horáček phoracek@redhat.com --- M debian/vdsm-hook-ovs.install M tests/functional/networkTestsOVS.py M vdsm.spec.in M vdsm_hooks/ovs/Makefile.am M vdsm_hooks/ovs/README A vdsm_hooks/ovs/ovs_after_network_setup.py A vdsm_hooks/ovs/ovs_after_network_setup_failture.py M vdsm_hooks/ovs/ovs_before_network_setup.py M vdsm_hooks/ovs/ovs_utils.py 9 files changed, 238 insertions(+), 60 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/07/46907/1
diff --git a/debian/vdsm-hook-ovs.install b/debian/vdsm-hook-ovs.install index 36e1f0f..c7b6692 100644 --- a/debian/vdsm-hook-ovs.install +++ b/debian/vdsm-hook-ovs.install @@ -10,3 +10,7 @@ usr/libexec/vdsm/hooks/before_device_create/ovs_utils.py usr/libexec/vdsm/hooks/before_network_setup/50_ovs usr/libexec/vdsm/hooks/before_network_setup/ovs_utils.py +usr/libexec/vdsm/hooks/after_network_setup/50_ovs +usr/libexec/vdsm/hooks/after_network_setup/ovs_utils.py +usr/libexec/vdsm/hooks/after_network_setup_failture/50_ovs +usr/libexec/vdsm/hooks/after_network_setup_failture/ovs_utils.py diff --git a/tests/functional/networkTestsOVS.py b/tests/functional/networkTestsOVS.py index b26105d..d1fe95e 100644 --- a/tests/functional/networkTestsOVS.py +++ b/tests/functional/networkTestsOVS.py @@ -41,6 +41,7 @@ NetworkTest.__test__ = False
BRIDGE_NAME = 'ovsbr0' +ERR_HOOK_ERROR = 78
# Tests which are not supported by OVS hook (because of OVS hook or because of # tests themselves). Some of these tests should be inherited and 'repaired' diff --git a/vdsm.spec.in b/vdsm.spec.in index fe70e24..bdbd704 100644 --- a/vdsm.spec.in +++ b/vdsm.spec.in @@ -1180,6 +1180,10 @@ %{_libexecdir}/%{vdsm_name}/hooks/before_network_setup/ovs_setup_mtu.py* %{_libexecdir}/%{vdsm_name}/hooks/before_network_setup/ovs_setup_ovs.py* %{_libexecdir}/%{vdsm_name}/hooks/before_network_setup/ovs_utils.py* +%{_libexecdir}/%{vdsm_name}/hooks/after_network_setup/50_ovs +%{_libexecdir}/%{vdsm_name}/hooks/after_network_setup/ovs_utils.py* +%{_libexecdir}/%{vdsm_name}/hooks/after_network_setup_failture/50_ovs +%{_libexecdir}/%{vdsm_name}/hooks/after_network_setup_failture/ovs_utils.py*
%files hook-macspoof %defattr(-, root, root, -) diff --git a/vdsm_hooks/ovs/Makefile.am b/vdsm_hooks/ovs/Makefile.am index 4699ef5..48f1820 100644 --- a/vdsm_hooks/ovs/Makefile.am +++ b/vdsm_hooks/ovs/Makefile.am @@ -36,6 +36,8 @@ ovs_before_network_setup_ovs.py \ ovs_before_network_setup_ip.py \ ovs_before_network_setup_mtu.py \ + ovs_after_network_setup.py \ + ovs_after_network_setup_failture.py \ $(utilsfile) \ sudoers.in
@@ -59,6 +61,12 @@ $(DESTDIR)$(vdsmhooksdir)/before_network_setup/ovs_setup_ip.py $(INSTALL_SCRIPT) $(srcdir)/ovs_before_network_setup_mtu.py \ $(DESTDIR)$(vdsmhooksdir)/before_network_setup/ovs_setup_mtu.py + $(MKDIR_P) $(DESTDIR)$(vdsmhooksdir)/after_network_setup + $(INSTALL_SCRIPT) $(srcdir)/ovs_after_network_setup.py \ + $(DESTDIR)$(vdsmhooksdir)/after_network_setup/50_ovs + $(MKDIR_P) $(DESTDIR)$(vdsmhooksdir)/after_network_setup_failture + $(INSTALL_SCRIPT) $(srcdir)/ovs_after_network_setup_failture.py \ + $(DESTDIR)$(vdsmhooksdir)/after_network_setup_failture/50_ovs
uninstall-local: uninstall-data-utils uninstall-data-sudoers $(RM) $(DESTDIR)$(vdsmhooksdir)/after_get_caps/50_ovs @@ -68,6 +76,8 @@ $(RM) $(DESTDIR)$(vdsmhooksdir)/before_network_setup/ovs_setup_ovs.py $(RM) $(DESTDIR)$(vdsmhooksdir)/before_network_setup/ovs_setup_ip.py $(RM) $(DESTDIR)$(vdsmhooksdir)/before_network_setup/ovs_setup_mtu.py + $(RM) $(DESTDIR)$(vdsmhooksdir)/after_network_setup/50_ovs + $(RM) $(DESTDIR)$(vdsmhooksdir)/after_network_setup_failture/50_ovs
install-data-utils: $(MKDIR_P) $(DESTDIR)$(vdsmhooksdir)/after_get_caps @@ -82,12 +92,20 @@ $(MKDIR_P) $(DESTDIR)$(vdsmhooksdir)/before_network_setup $(INSTALL_SCRIPT) $(srcdir)/$(utilsfile) \ $(DESTDIR)$(vdsmhooksdir)/before_network_setup/$(utilsfile) + $(MKDIR_P) $(DESTDIR)$(vdsmhooksdir)/after_network_setup + $(INSTALL_SCRIPT) $(srcdir)/$(utilsfile) \ + $(DESTDIR)$(vdsmhooksdir)/after_network_setup/$(utilsfile) + $(MKDIR_P) $(DESTDIR)$(vdsmhooksdir)/after_network_setup_failture + $(INSTALL_SCRIPT) $(srcdir)/$(utilsfile) \ + $(DESTDIR)$(vdsmhooksdir)/after_network_setup_failture/$(utilsfile)
uninstall-data-utils: $(RM) $(DESTDIR)$(vdsmhooksdir)/after_get_caps/$(utilsfile) $(RM) $(DESTDIR)$(vdsmhooksdir)/after_get_stats/$(utilsfile) $(RM) $(DESTDIR)$(vdsmhooksdir)/before_device_create/$(utilsfile) $(RM) $(DESTDIR)$(vdsmhooksdir)/before_network_setup/$(utilsfile) + $(RM) $(DESTDIR)$(vdsmhooksdir)/after_network_setup/$(utilsfile) + $(RM) $(DESTDIR)$(vdsmhooksdir)/after_network_setup_failture/$(utilsfile)
install-data-sudoers: $(MKDIR_P) $(DESTDIR)$(sysconfdir)/sudoers.d diff --git a/vdsm_hooks/ovs/README b/vdsm_hooks/ovs/README index 78e4fcb..8b7bba0 100644 --- a/vdsm_hooks/ovs/README +++ b/vdsm_hooks/ovs/README @@ -53,6 +53,22 @@ - All networks are bridged
+Rollback +-------- + +OVS rollback is a bit complex and *zaslouzisi* some explanation: + +1) After ovs_before_network_setup.py:configure() calls prepare_ovs() (which + is the last safe moment before we touch any system configuration) we save + initial network configuration into a temporary file. +2) If an error occurs during setupNetworks(), + ovs_after_network_setup_failture.py is executed. If there is an temporary + file containing initial network configuration, ovs_utils.py:rollback() + is called. +3) If there was no error, ovs_after_network_setup.py is executed and just + removes temporary initial config file. + + Backporting -----------
diff --git a/vdsm_hooks/ovs/ovs_after_network_setup.py b/vdsm_hooks/ovs/ovs_after_network_setup.py new file mode 100644 index 0000000..7f99db1 --- /dev/null +++ b/vdsm_hooks/ovs/ovs_after_network_setup.py @@ -0,0 +1,49 @@ +#!/usr/bin/env python +# Copyright 2015 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# +# Refer to the README and COPYING files for full details of the license +# +import traceback + +import hooking + +from ovs_utils import remove_init_config, log + + +def main(): + setup_nets_config = hooking.read_json() + + inRollback = setup_nets_config['request']['options'].get('_inRollback') + inOVSRollback = setup_nets_config['request']['options'].get( + '_inOVSRollback') + + if inRollback and not inOVSRollback: + log('Non-OVS rollback is done. Leaving OVS init_config for OVS ' + 'rollback.') + elif inRollback and inOVSRollback: + log('OVS rollback is done. Removing OVS init_config backup.') + remove_init_config() + else: + log('Network setup was successfull. Removing OVS init_config backup.') + remove_init_config() + + +if __name__ == '__main__': + try: + main() + except: + hooking.exit_hook(traceback.format_exc()) diff --git a/vdsm_hooks/ovs/ovs_after_network_setup_failture.py b/vdsm_hooks/ovs/ovs_after_network_setup_failture.py new file mode 100755 index 0000000..9935a13 --- /dev/null +++ b/vdsm_hooks/ovs/ovs_after_network_setup_failture.py @@ -0,0 +1,47 @@ +#!/usr/bin/env python +# Copyright 2015 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# +# Refer to the README and COPYING files for full details of the license +# +import traceback + +from vdsm import supervdsm + +import hooking + +from ovs_utils import log + + +def main(): + setup_nets_config = hooking.read_json() + in_rollback = setup_nets_config['request']['options'].get('_inRollback', + False) + if in_rollback: + log('Failed while trying to rollback.') + else: + log('Configuration failed. In this point, non-OVS rollback should be ' + 'done. Executing OVS rollback.') + supervdsm.getProxy().setupNetworks( + {}, {}, {'connectivityCheck': False, '_inRollback': True, + '_inOVSRollback': True}) + + +if __name__ == '__main__': + try: + main() + except: + hooking.exit_hook(traceback.format_exc()) diff --git a/vdsm_hooks/ovs/ovs_before_network_setup.py b/vdsm_hooks/ovs/ovs_before_network_setup.py index 54a941e..64a52c1 100755 --- a/vdsm_hooks/ovs/ovs_before_network_setup.py +++ b/vdsm_hooks/ovs/ovs_before_network_setup.py @@ -17,21 +17,27 @@ # # Refer to the README and COPYING files for full details of the license # -from contextlib import contextmanager from copy import deepcopy import sys import traceback + +from libvirt import libvirtError
from vdsm.netconfpersistence import RunningConfig
from hooking import execCmd import hooking
-from ovs_utils import (is_ovs_network, is_ovs_bond, rollback, log, EXT_IP, - EXT_OVS_VSCTL) +from ovs_utils import (is_ovs_network, is_ovs_bond, load_init_config, log, + save_init_config, iter_ovs_nets, suppress, + destroy_ovs_bridge, EXT_IP, EXT_OVS_VSCTL) from ovs_setup_ovs import configure_ovs, prepare_ovs from ovs_setup_ip import configure_ip from ovs_setup_mtu import configure_mtu + +# TODO: move required modules into vdsm/lib +sys.path.append('/usr/share/vdsm') +from network.configurators import libvirt
def _separate_ovs_nets_bonds(nets, bonds, running_config): @@ -59,29 +65,50 @@ return ovs_nets, non_ovs_nets, ovs_bonds, non_ovs_bonds
-@contextmanager -def _rollback(running_config, initial_config, in_rollback): - try: - yield - except: - if in_rollback: - log('Failed while trying to rollback:') - else: - log('Configuration failed. Entering rollback.') - rollback(running_config, initial_config) - log('Rollback finished. Initial error:') - raise +def _remove_ovs_nets(initial_config, running_config): + log('Remove OVS networks: %s %s' % (initial_config, running_config)) + for libvirt_ovs_nets in (iter_ovs_nets(running_config.networks), + iter_ovs_nets(initial_config.networks)): + for net, attrs in libvirt_ovs_nets: + with suppress(libvirtError): # network not found + libvirt.removeNetwork(net) + + destroy_ovs_bridge() + for net, attrs in running_config.networks.items(): + if is_ovs_network(attrs): + running_config.networks.pop(net) + for bond, attrs in running_config.bonds.items(): + if is_ovs_bond(attrs): + running_config.bonds.pop(bond) + running_config.save()
-def configure(nets, bonds, running_config, in_rollback): +def _rollback(running_config): + initial_config = load_init_config() + if initial_config is None: + log('No needed OVS changes to be done.') + else: + log('Remove OVS networks.') + _remove_ovs_nets(initial_config, running_config) + log('Reconfigure OVS networks according to initial_config.') + _configure(initial_config.networks, initial_config.bonds, + running_config, save_init=False) + + +def _configure(nets, bonds, running_config, save_init=True): initial_config = deepcopy(running_config)
commands, libvirt_create, libvirt_remove = prepare_ovs( nets, bonds, running_config) - with _rollback(running_config, initial_config, in_rollback): - configure_ovs(commands, libvirt_create, libvirt_remove, running_config) - configure_mtu(running_config) - configure_ip(nets, initial_config.networks) + + if save_init: + log('Saving initial configuration for optional rollback: %s' % + initial_config) + save_init_config(initial_config) + + configure_ovs(commands, libvirt_create, libvirt_remove, running_config) + configure_mtu(running_config) + configure_ip(nets, initial_config.networks)
log('Saving running configuration: %s %s' % (running_config.networks, running_config.bonds)) @@ -95,16 +122,29 @@ running_config = RunningConfig() networks = setup_nets_config['request']['networks'] bondings = setup_nets_config['request']['bondings'] - inRollback = setup_nets_config['request']['options'].get('_inRollback', - False) - ovs_nets, non_ovs_nets, ovs_bonds, non_ovs_bonds = \ - _separate_ovs_nets_bonds(networks, bondings, running_config) - configure(ovs_nets, ovs_bonds, running_config, inRollback)
- setup_nets_config['request']['bondings'] = non_ovs_bonds - setup_nets_config['request']['networks'] = non_ovs_nets - log('Hook finished, returning non-OVS networks and bondings back ' - 'to VDSM: %s' % setup_nets_config) + inRollback = setup_nets_config['request']['options'].get('_inRollback') + inOVSRollback = setup_nets_config['request']['options'].get( + '_inOVSRollback') + + if inRollback and not inOVSRollback: + log('Non-OVS rollback is to be done. Returning nets_config unchanged') + elif inOVSRollback: + log('OVS rollback is to be done.') + _rollback(running_config) + setup_nets_config['request']['bondings'] = {} + setup_nets_config['request']['networks'] = {} + log('OVS rollback finished, returning empty networks and bondings ' + 'configuration back to VDSM.') + else: + ovs_nets, non_ovs_nets, ovs_bonds, non_ovs_bonds = \ + _separate_ovs_nets_bonds(networks, bondings, running_config) + _configure(ovs_nets, ovs_bonds, running_config) + setup_nets_config['request']['bondings'] = non_ovs_bonds + setup_nets_config['request']['networks'] = non_ovs_nets + log('Hook finished, returning non-OVS networks and bondings back to ' + 'VDSM: %s' % setup_nets_config) + hooking.write_json(setup_nets_config)
diff --git a/vdsm_hooks/ovs/ovs_utils.py b/vdsm_hooks/ovs/ovs_utils.py index 7375d20..d592b8f 100644 --- a/vdsm_hooks/ovs/ovs_utils.py +++ b/vdsm_hooks/ovs/ovs_utils.py @@ -18,19 +18,14 @@ # Refer to the README and COPYING files for full details of the license # from contextlib import contextmanager -import sys - -from libvirt import libvirtError +import errno +import os +import pickle
from hooking import execCmd import hooking
from vdsm.utils import CommandPath -from vdsm import supervdsm - -# TODO: move required modules into vdsm/lib -sys.path.append('/usr/share/vdsm') -from network.configurators import libvirt
EXT_IP = CommandPath('ip', '/sbin/ip').cmd EXT_OVS_VSCTL = CommandPath('ovs-vsctl', @@ -40,6 +35,8 @@ '/usr/sbin/ovs-appctl', '/usr/bin/ovs-appctl').cmd BRIDGE_NAME = 'ovsbr0' + +INIT_CONFIG_FILE = '/tmp/ovs_init_config' # TODO: VDSM tmp folder
def rget(dict, keys, default=None): @@ -123,28 +120,30 @@ raise Exception('\n'.join(err))
-def rollback(running_config, initial_config): - diff = running_config.diffFrom(initial_config) - if diff: - for libvirt_ovs_nets in (iter_ovs_nets(running_config.networks), - iter_ovs_nets(initial_config.networks)): - for net, attrs in libvirt_ovs_nets: - with suppress(libvirtError): # network not found - libvirt.removeNetwork(net) - - destroy_ovs_bridge() - for net, attrs in running_config.networks.items(): - if is_ovs_network(attrs): - running_config.networks.pop(net) - for bond, attrs in running_config.bonds.items(): - if is_ovs_bond(attrs): - running_config.bonds.pop(bond) - running_config.save() - - supervdsm.getProxy().setupNetworks( - initial_config.networks, initial_config.bonds, - {'connectivityCheck': False, '_inRollback': True}) - - def log(message): hooking.log('OVS: %s' % message) + + +def load_init_config(): + try: + with open(INIT_CONFIG_FILE) as f: + init_config = pickle.load(f) + except IOError as e: + if e.errno != errno.ENOENT: + raise + return None + else: + return init_config + + +def save_init_config(init_config): + with open(INIT_CONFIG_FILE, 'w') as f: + pickle.dump(init_config, f) + + +def remove_init_config(): + try: + os.remove(INIT_CONFIG_FILE) + except OSError as e: + if e.errno != errno.ENOENT: + raise
automation@ovirt.org has posted comments on this change.
Change subject: net: ovs: draft of better rollback ......................................................................
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: net: ovs: draft of better rollback ......................................................................
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'])
automation@ovirt.org has posted comments on this change.
Change subject: net: ovs: draft of better rollback ......................................................................
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'])
automation@ovirt.org has posted comments on this change.
Change subject: net: ovs: draft of better rollback ......................................................................
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: net: ovs: better rollback ......................................................................
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'])
Petr Horáček has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
Patch Set 5: Verified+1
Passed functional/networkTestsOVS.py without a regression. ConnectivityCheck is now working.
automation@ovirt.org has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
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: net: ovs: better rollback ......................................................................
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'])
Petr Horáček has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
Patch Set 7: Verified+1
Just changed readme, copying v+1
Petr Horáček has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
Patch Set 7: -Verified
Found a problem while writing docs.
automation@ovirt.org has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
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'])
Ondřej Svoboda has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
Patch Set 8: Code-Review-1
(18 comments)
I find modification of dictionaries while iterating them dangerous.
Some hook scripts don't have proper executable permissions. With assumed RPM magic not universally present, I would like to see +x permissions set explicitly :-)
https://gerrit.ovirt.org/#/c/46907/8//COMMIT_MSG Commit Message:
Line 9: Untill Until
Line 13: failtures failures
Line 19: witch with a
Line 20: OVS only an OVS-only (the hyphen helps, don't forget the next occurence as well)
https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/README File vdsm_hooks/ovs/README:
Line 65: finishes finishes,
Line 118: happen happens
Line 136: trailing space
https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_after_get_stats.py File vdsm_hooks/ovs/ovs_after_get_stats.py:
Line 1: #!/usr/bin/env python The file has a hashbang, but its mode changed to non-executable in this commit.
https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_after_network_setup.... File vdsm_hooks/ovs/ovs_after_network_setup.py:
Line 1: #!/usr/bin/env python Also, it looks like it could be run standalone, but executable permissions are not set.
Line 41: successfull successful
https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_after_network_setup_... File vdsm_hooks/ovs/ovs_after_network_setup_fail.py:
Line 1: #!/usr/bin/env python hashbang/non-executable mismatch, as with other files
Line 29: , : False) dict.get() would just return None.
Line 34: In At
https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_before_network_setup... File vdsm_hooks/ovs/ovs_before_network_setup.py:
Line 82: running_config.networks.pop(net) Doesn't Python complain when you modify a dictionary while iterating it? You can use dict() to create a temporary copy on line 80.
Line 85: running_config.bonds.pop(bond) Likewise.
Line 94: Remove Removing (to keep uniformity)
Line 96: Reconfigure Reconfiguring
Line 135: inRollback No need to mimic camelCase :-)
Petr Horáček has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
Patch Set 8:
(13 comments)
https://gerrit.ovirt.org/#/c/46907/8//COMMIT_MSG Commit Message:
Line 5: CommitDate: 2015-10-22 18:31:05 +0200 Line 6: Line 7: net: ovs: better rollback Line 8: Line 9: Untill now, OVS hook was not able to rollback after failed
Until
Done Line 10: setup of non-OVS networks. Line 11: Line 12: Now it uses after_network_setup and after_network_setup_fail Line 13: hook points to handle general rollback for all failtures
Line 9: Untill now, OVS hook was not able to rollback after failed Line 10: setup of non-OVS networks. Line 11: Line 12: Now it uses after_network_setup and after_network_setup_fail Line 13: hook points to handle general rollback for all failtures
failures
Done Line 14: (both OVS and non-OVS). Line 15: Line 16: Before OVS setup we save initial OVS configuration to a temporary Line 17: file via pickle. If an exception occurs during setupNetworks,
Line 15: Line 16: Before OVS setup we save initial OVS configuration to a temporary Line 17: file via pickle. If an exception occurs during setupNetworks, Line 18: API.py does the standard rollback. Then after_network_setup_fail Line 19: is executed and runs setupNetworks witch special option _inOVSRollback
with a
Done Line 20: which triggers OVS only rollback. OVS only rollback removes all Line 21: OVS networks and recreates them according to configuration saved in Line 22: the temprorary file. When everything is done, temporary file is Line 23: removed.
https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/README File vdsm_hooks/ovs/README:
Line 61: 1) After ovs_before_network_setup.py:configure() calls prepare_libvirt() (which Line 62: is the last safe moment before we touch any system configuration) we save Line 63: initial network configuration into a temporary file. Line 64: 2) If an error occurs during setupNetworks(), non-OVS rolls back to the state Line 65: between OVS and non-OVS configuration. When this rollback finishes
finishes,
Done Line 66: ovs_after_network_setup_fail.py is executed. This scripts just executes Line 67: setupNetworks command with special option _inOVSRollback which tells OVS Line 68: hook to do a rollback. Line 69: 2.1) If there is a temporary file containing initial network configuration,
Line 114: | Line 115: V Line 116: +-----------------------------------------------------------------------+ Line 117: | *API.setupNetworks()* | Line 118: | | If anything bad happen, it |
happens
Done Line 119: | network setup : will trigger non-OVS rollback (which | Line 120: | | will be ignored by OVS). when it's done | Line 121: | | it will continue with | Line 122: | | after_network_setup_fail hook point. |
Line 132: |||| | Receives _inRollback=True and |||| Line 133: |||| *ovs_before_network_setup* : _inOVSRollback=True. If there |||| Line 134: |||| | is no OVS init config, we |||| Line 135: |||| | just log. If the file exists |||| Line 136: |||| | we remove all OVS networks, ||||
trailing space
Done Line 137: |||| | and recreate networks |||| Line 138: |||| | according to saved config. |||| Line 139: |||| | Then we send empty configuration |||| Line 140: |||| | back to VDSM, because of VDSM ||||
https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_after_get_stats.py File vdsm_hooks/ovs/ovs_after_get_stats.py:
Line 1: #!/usr/bin/env python
The file has a hashbang, but its mode changed to non-executable in this com
Makefile turns it into a executable. This test does not have embedded test, to it does not need x flag. But according to your patch comment, I'm changing it. Line 2: # Copyright 2015 Red Hat, Inc. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by
https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_after_network_setup.... File vdsm_hooks/ovs/ovs_after_network_setup.py:
Line 37: elif inRollback and inOVSRollback: Line 38: hooking.log('OVS rollback is done. Removing OVS init_config backup.') Line 39: remove_init_config() Line 40: else: Line 41: hooking.log('Network setup was successfull. Removing OVS init_config '
successful
Done Line 42: 'backup.') Line 43: remove_init_config() Line 44: Line 45:
https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_after_network_setup_... File vdsm_hooks/ovs/ovs_after_network_setup_fail.py:
Line 1: #!/usr/bin/env python
hashbang/non-executable mismatch, as with other files
Makefile turns it into executable. We have x flag only on hook files with simple tests (well ovs_after_get_stats.py is executable and has not test, but this is fixed in one of following patches). Line 2: # Copyright 2015 Red Hat, Inc. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by
Line 26: Line 27: def main(): Line 28: setup_nets_config = hooking.read_json() Line 29: in_rollback = setup_nets_config['request']['options'].get('_inRollback', Line 30: False)
dict.get() would just return None.
Done Line 31: if in_rollback: Line 32: hooking.log('Failed while trying to rollback.') Line 33: else: Line 34: hooking.log('Configuration failed. In this point, non-OVS rollback '
Line 30: False) Line 31: if in_rollback: Line 32: hooking.log('Failed while trying to rollback.') Line 33: else: Line 34: hooking.log('Configuration failed. In this point, non-OVS rollback '
At
Done Line 35: 'should be done. Executing OVS rollback.') Line 36: supervdsm.getProxy().setupNetworks( Line 37: {}, {}, {'connectivityCheck': False, '_inRollback': True, Line 38: '_inOVSRollback': True})
https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_before_network_setup... File vdsm_hooks/ovs/ovs_before_network_setup.py:
Line 78: Line 79: destroy_ovs_bridge() Line 80: for net, attrs in running_config.networks.items(): Line 81: if is_ovs_network(attrs): Line 82: running_config.networks.pop(net)
Doesn't Python complain when you modify a dictionary while iterating it? Yo
I'm not iterating that dict, .items() created a copy of it. Anyways, in next patch I use six everywhere. Line 83: for bond, attrs in running_config.bonds.items(): Line 84: if is_ovs_bond(attrs): Line 85: running_config.bonds.pop(bond) Line 86: running_config.save()
Line 92: hooking.log('No needed OVS changes to be done.') Line 93: else: Line 94: hooking.log('Remove OVS networks.') Line 95: _remove_ovs_nets(initial_config, running_config) Line 96: hooking.log('Reconfigure OVS networks according to initial_config.')
Reconfiguring
Done Line 97: _configure(initial_config.networks, initial_config.bonds, Line 98: running_config, save_init=False) Line 99: Line 100:
automation@ovirt.org has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
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'])
automation@ovirt.org has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
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'])
Petr Horáček has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
Patch Set 10: Verified+1
Passed without a regression. Is able to rollback OVS after non-OVS fail. Connectivity check is working.
automation@ovirt.org has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
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'])
Ido Barkan has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
Patch Set 11: Code-Review-1
(10 comments)
https://gerrit.ovirt.org/#/c/46907/11/vdsm_hooks/ovs/ovs_after_network_setup... File vdsm_hooks/ovs/ovs_after_network_setup.py:
Line 24: remove_init_config this is used only here, so can be declared only here and be private
Line 35: remove_init_config() so remove_init_config() can get out of the if-else block
https://gerrit.ovirt.org/#/c/46907/11/vdsm_hooks/ovs/ovs_before_network_setu... File vdsm_hooks/ovs/ovs_before_network_setup.py:
Line 80: for net, attrs in running_config.networks.items(): : if is_ovs_network(attrs): : running_config.networks.pop(net) : for bond, attrs in running_config.bonds.items(): : if is_ovs_bond(attrs): : running_config.bonds.pop(bond) : running_config.save() this should be s separate function and be called after _remove_ovs_nets
Line 141: setup_nets_config['request']['bondings'] = {} : setup_nets_config['request']['networks'] = {} see next comment
Line 149: tup_nets_config['request']['bondings'] = non_ovs_bonds : setup_nets_config['request']['networks'] = non_ovs_nets this 'filtering' action should have a name (although it is simplistic). so the reader can understand what you are trying to do ("_filter_network_config"?)
https://gerrit.ovirt.org/#/c/46907/11/vdsm_hooks/ovs/ovs_utils.py File vdsm_hooks/ovs/ovs_utils.py:
Line 23: pickle https://docs.python.org/2/library/pickle.html#module-cPickle
should be faster.
Line 122: def load_init_config(): : try: : with open(INIT_CONFIG_FILE) as f: : init_config = pickle.load(f) : except IOError as e: : if e.errno != errno.ENOENT: : raise : return None : else: : return init_config the only user of this is ovs_before_network_setup.py so it can be moved there. if you won't be strict about those, you'll end up with a utils_garbage_can.py
Line 134: ef save_init_config(init_config): : with open(INIT_CONFIG_FILE, 'w') as f: : pickle.dump(init_config, f) see comment above
Line 142: except OSError as e: why are you expecting it to be missing? is there a specific flow?
Line 139: def remove_init_config(): : try: : os.remove(INIT_CONFIG_FILE) : except OSError as e: : if e.errno != errno.ENOENT: : raise see comments above about users of this functions
Petr Horáček has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
Patch Set 11:
(9 comments)
https://gerrit.ovirt.org/#/c/46907/11/vdsm_hooks/ovs/ovs_after_network_setup... File vdsm_hooks/ovs/ovs_after_network_setup.py:
Line 20: import traceback Line 21: Line 22: import hooking Line 23: Line 24: from ovs_utils import remove_init_config
this is used only here, so can be declared only here and be private
Done Line 25: Line 26: Line 27: def main(): Line 28: setup_nets_config = hooking.read_json()
Line 31: '_inOVSRollback') Line 32: Line 33: if in_ovs_rollback: Line 34: hooking.log('Rollback is done. Removing OVS init_config backup.') Line 35: remove_init_config()
so remove_init_config() can get out of the if-else block
Done Line 36: else: Line 37: hooking.log('Network setup was successful. Removing OVS init_config ' Line 38: 'backup.') Line 39: remove_init_config()
https://gerrit.ovirt.org/#/c/46907/11/vdsm_hooks/ovs/ovs_before_network_setu... File vdsm_hooks/ovs/ovs_before_network_setup.py:
Line 82: running_config.networks.pop(net) Line 83: for bond, attrs in running_config.bonds.items(): Line 84: if is_ovs_bond(attrs): Line 85: running_config.bonds.pop(bond) Line 86: running_config.save()
this should be s separate function and be called after _remove_ovs_nets
Done Line 87: Line 88: Line 89: def _rollback(running_config): Line 90: initial_config = load_init_config()
Line 146: ovs_nets, non_ovs_nets, ovs_bonds, non_ovs_bonds = \ Line 147: _separate_ovs_nets_bonds(networks, bondings, running_config) Line 148: _configure(ovs_nets, ovs_bonds, running_config) Line 149: setup_nets_config['request']['bondings'] = non_ovs_bonds Line 150: setup_nets_config['request']['networks'] = non_ovs_nets
this 'filtering' action should have a name (although it is simplistic). so
I dont understand. Do you mean to separate _seaparate_ovs_nets_bonds function and returning of non_ovs_bonds/nets back to dictionary? are not explicit variables names and log 'comment' enough? Line 151: hooking.log('Hook finished, returning non-OVS networks and bondings ' Line 152: 'back to VDSM: %s' % setup_nets_config) Line 153: Line 154: hooking.write_json(setup_nets_config)
https://gerrit.ovirt.org/#/c/46907/11/vdsm_hooks/ovs/ovs_utils.py File vdsm_hooks/ovs/ovs_utils.py:
Line 19: # Line 20: from contextlib import contextmanager Line 21: import errno Line 22: import os Line 23: import pickle
https://docs.python.org/2/library/pickle.html#module-cPickle
Done Line 24: Line 25: from hooking import execCmd Line 26: Line 27: from vdsm.utils import CommandPath
Line 127: if e.errno != errno.ENOENT: Line 128: raise Line 129: return None Line 130: else: Line 131: return init_config
the only user of this is ovs_before_network_setup.py so it can be moved the
Done Line 132: Line 133: Line 134: def save_init_config(init_config): Line 135: with open(INIT_CONFIG_FILE, 'w') as f:
Line 132: Line 133: Line 134: def save_init_config(init_config): Line 135: with open(INIT_CONFIG_FILE, 'w') as f: Line 136: pickle.dump(init_config, f)
see comment above
Done Line 137: Line 138: Line 139: def remove_init_config(): Line 140: try:
Line 138: Line 139: def remove_init_config(): Line 140: try: Line 141: os.remove(INIT_CONFIG_FILE) Line 142: except OSError as e:
why are you expecting it to be missing? is there a specific flow?
we do not save this file until we do changes in system. if it failed before this point, there will be no such file saved. i added a comment here Line 143: if e.errno != errno.ENOENT:
Line 140: try: Line 141: os.remove(INIT_CONFIG_FILE) Line 142: except OSError as e: Line 143: if e.errno != errno.ENOENT: Line 144: raise
see comments above about users of this functions
Done
automation@ovirt.org has posted comments on this change.
Change subject: net: ovs: better rollback ......................................................................
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'])
automation@ovirt.org has posted comments on this change.
Change subject: hooks: ovs: better rollback ......................................................................
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'])
automation@ovirt.org has posted comments on this change.
Change subject: hooks: ovs: better rollback ......................................................................
Patch Set 14:
* 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'])
Ido Barkan has posted comments on this change.
Change subject: hooks: ovs: better rollback ......................................................................
Patch Set 11:
(1 comment)
https://gerrit.ovirt.org/#/c/46907/11/vdsm_hooks/ovs/ovs_before_network_setu... File vdsm_hooks/ovs/ovs_before_network_setup.py:
Line 146: ovs_nets, non_ovs_nets, ovs_bonds, non_ovs_bonds = \ Line 147: _separate_ovs_nets_bonds(networks, bondings, running_config) Line 148: _configure(ovs_nets, ovs_bonds, running_config) Line 149: setup_nets_config['request']['bondings'] = non_ovs_bonds Line 150: setup_nets_config['request']['networks'] = non_ovs_nets
I dont understand. Do you mean to separate _seaparate_ovs_nets_bonds functi
I meant doing this dictionary update (which you do twice) in a function with a meaningful name. Line 151: hooking.log('Hook finished, returning non-OVS networks and bondings ' Line 152: 'back to VDSM: %s' % setup_nets_config) Line 153: Line 154: hooking.write_json(setup_nets_config)
Petr Horáček has posted comments on this change.
Change subject: hooks: ovs: better rollback ......................................................................
Patch Set 11:
(1 comment)
https://gerrit.ovirt.org/#/c/46907/11/vdsm_hooks/ovs/ovs_before_network_setu... File vdsm_hooks/ovs/ovs_before_network_setup.py:
Line 146: ovs_nets, non_ovs_nets, ovs_bonds, non_ovs_bonds = \ Line 147: _separate_ovs_nets_bonds(networks, bondings, running_config) Line 148: _configure(ovs_nets, ovs_bonds, running_config) Line 149: setup_nets_config['request']['bondings'] = non_ovs_bonds Line 150: setup_nets_config['request']['networks'] = non_ovs_nets
I meant doing this dictionary update (which you do twice) in a function wit
Done Line 151: hooking.log('Hook finished, returning non-OVS networks and bondings ' Line 152: 'back to VDSM: %s' % setup_nets_config) Line 153: Line 154: hooking.write_json(setup_nets_config)
Ido Barkan has posted comments on this change.
Change subject: hooks: ovs: better rollback ......................................................................
Patch Set 14: Code-Review+1
(1 comment)
just a small nit on a function name. +1 otherwise.
https://gerrit.ovirt.org/#/c/46907/14/vdsm_hooks/ovs/ovs_before_network_setu... File vdsm_hooks/ovs/ovs_before_network_setup.py:
Line 77: _destroy_ovs_net maybe add 'libvirt' to the function name? (and log msg)
vdsm-patches@lists.fedorahosted.org
-
automation＠ovirt.org -
ibarkan＠redhat.com -
osvoboda＠redhat.com -
phoracek＠redhat.com