Ido Barkan has posted comments on this change. Change subject: net: configurators: persist custom bond option ......................................................................
Patch Set 2: Code-Review-1 (7 comments) https://gerrit.ovirt.org/#/c/43891/2//COMMIT_MSG Commit Message: Line 11: passed to configurators and persisted). But this is not how Line 12: custom *network* property behaves. We want these two custom Line 13: properties to be symmetric - they have to be persisted Line 14: (if unified persistence is used) and must not be shown Line 15: in netinfo. I think you should drop a word here about the motivation for OVS. Line 16: Line 17: This patch moves removal of custom property down to Line 18: configurators. Thanks to it, custom property is persisted, Line 19: but not set and bond option. Line 15: in netinfo. Line 16: Line 17: This patch moves removal of custom property down to Line 18: configurators. Thanks to it, custom property is persisted, Line 19: but not set and bond option. as? Line 20: Line 21: Custom property is not dropped only in configurators, but in Line 22: models.py:Bond:areOptionsApplied as well - we don't want Line 23: configurator to reconfigure bond when there is a custom https://gerrit.ovirt.org/#/c/43891/2/vdsm/network/configurators/pyroute_two.py File vdsm/network/configurators/pyroute_two.py: Line 154: Line 155: def addBondOptions(self, bond): Line 156: logging.debug('Add bond options %s', bond.options) Line 157: options = (remove_custom_bond_option(bond.options) Line 158: if bond.options else '') see my comment in models.py Line 159: for option in options: Line 160: key, value = option.split('=') Line 161: with open(netinfo.BONDING_OPT % (bond.name, key), 'w') as f: Line 162: f.write(value) https://gerrit.ovirt.org/#/c/43891/2/vdsm/network/models.py File vdsm/network/models.py: Line 241: def areOptionsApplied(self): Line 242: # TODO: this method returns True iff self.options are a subset of the Line 243: # TODO: current bonding options. VDSM should probably compute if the Line 244: # TODO: non-default settings are equal to the non-default state. Line 245: options = (remove_custom_bond_option(self.options) can you drop a comment on why this is important? (same as in the commit message) Line 246: if self.options else '') Line 247: if options == '': Line 248: return True Line 249: confOpts = [option.split('=', 1) for option in options.split(' ')] Line 242: # TODO: this method returns True iff self.options are a subset of the Line 243: # TODO: current bonding options. VDSM should probably compute if the Line 244: # TODO: non-default settings are equal to the non-default state. Line 245: options = (remove_custom_bond_option(self.options) Line 246: if self.options else '') this is not needed because remove_custom_bond_option can eat an empty string without chocking. More over, since not long ago, bond options default to an empty string (see Bond __init__) and this just make things more confusing. So just: options = remove_custom_bond_option(self.options) Line 247: if options == '': Line 248: return True Line 249: confOpts = [option.split('=', 1) for option in options.split(' ')] Line 250: activeOpts = netinfo.bondOpts(self.name, https://gerrit.ovirt.org/#/c/43891/2/vdsm/network/utils.py File vdsm/network/utils.py: why do we need this file? currently we have configurators/__init__.py for that, and it is still not to full of garbage, so lets be consistent. When it becomes to crowded, we can extract a utility file. Line 1: # Copyright 2015 Red Hat, Inc. Line 2: # Line 3: # This program is free software; you can redistribute it and/or modify Line 4: # it under the terms of the GNU General Public License as published by Line 27: d_opts = dict(opt.split('=', 1) for opt in options.split()) Line 28: if d_opts.pop('custom', None) is not None: Line 29: options = ' '.join(['%s=%s' % (key, value) Line 30: for key, value in d_opts.items()]) Line 31: return options can also be written as: def remove_custom_bond_option(opts): return ' '.join((opt for opt in opts.split() if not opt.startswith('custom='))) -- To view, visit https://gerrit.ovirt.org/43891 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic4f1564e19904cc1d53bc6d1cf732ca35375332e Gerrit-PatchSet: 2 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: 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
