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(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare(a)redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Petr Šebek <psebek(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes