Change in vdsm[master]: Unified network persistence [1/3] - Save running config

wudxw at linux.vnet.ibm.com wudxw at linux.vnet.ibm.com
Tue Aug 27 06:08:38 UTC 2013


Mark Wu has posted comments on this change.

Change subject: Unified network persistence [1/3] - Save running config
......................................................................


Patch Set 18: Code-Review-1

(5 comments)

Besides the comments inline,  I would like to suggest implement the saving of running config in configurator's commit() function. For setupNetwork, we call commit() in the end of the call if all networks are setup successfully. for addNetwork and delNetwork,  we only call commit() when it completes successfully and it's a direct API call, not called from setupNetworks.  We can use the argument configurator to tell if it's called from setupNetworks.  

With this proposed change,  it's easier to implement the rollback from running config.

....................................................
File vdsm/configNetwork.py
Line 164:             attrs.update(dict(zip(spec.args, args)))
Line 165:         try:
Line 166:             ret = func(*args, **kwargs)
Line 167:         except:
Line 168:             raise
it looks the try .. except  statement is useless here.  The exception will be handled as before,  so we don't need consider it in the wrapper function.
Line 169:         if not saveRunningConf:
Line 170:             return ret
Line 171: 
Line 172:         runningConfig = RunningConfig()


Line 175:             bondAttrs = {'nics': attrs.pop('nics')}
Line 176:             bondingOptions = attrs.pop('bondingOptions', None)
Line 177:             if bondingOptions:
Line 178:                 bondAttrs['bondingOptions'] = bondingOptions
Line 179:             runningConfig.setBonding(bonding, bondAttrs)
I would like to suggest hide setBonding behind running.setNetwork.  We could let running.setNetwork accept the all arguments including those for bonding, and make setNetwork implement the logic above.
Line 180:         else:
Line 181:             nics = attrs.pop('nics', None)
Line 182:             if nics:
Line 183:                 attrs['nic'] = nics[0]


Line 183:                 attrs['nic'] = nics[0]
Line 184: 
Line 185:         netName = attrs.pop('network')
Line 186:         options = attrs.pop('options', None)
Line 187: 
how is 'options' included in attrs? And the keyword arguments included in options are already included in kwargs.  So I don't understand the code here.
Line 188:         # Clean netAttrs from fields that should not be serialized
Line 189:         netAttrs = dict((key, value) for key, value in attrs.iteritems() if
Line 190:                         value is not None and
Line 191:                         key not in ('configurator', '_netinfo', 'force',


Line 188:         # Clean netAttrs from fields that should not be serialized
Line 189:         netAttrs = dict((key, value) for key, value in attrs.iteritems() if
Line 190:                         value is not None and
Line 191:                         key not in ('configurator', '_netinfo', 'force',
Line 192:                                     'implicitBonding'))
probably this code should go to unifiedpersistence.py too.
Line 193: 
Line 194:         if options:
Line 195:             netAttrs.update((key, value) for key, value in options.iteritems()
Line 196:                             if value is not None)


Line 389:                                  'still exists' % network)
Line 390:     elif config.get('vars', 'persistence') == 'unified':
Line 391:         runningConfig = RunningConfig()
Line 392:         runningConfig.removeNetwork(network)
Line 393:         if bonding:
it could happen that the bonding still exists after the 'delNetwork' call because it's also used by other network. So we  need use netinfo.NetInfo().ifaceUsers(bonding) to check that.
Line 394:             runningConfig.removeBonding(bonding)
Line 395: 
Line 396: 
Line 397: def clientSeen(timeout):


-- 
To view, visit http://gerrit.ovirt.org/16699
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7137a96f84abd2c5e532c6c37737e36ef17567a9
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Petr Ĺ ebek <psebek at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list