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.py 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_setup.py 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 -- To view, visit https://gerrit.ovirt.org/46907 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8f6b63d03bb9579e260bfad1686047a431f69543 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: Petr Horáček <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
