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