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.py 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_setup.py 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 -- 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
