Mark Wu has posted comments on this change.
Change subject: [WIP] Unified persistence.
......................................................................
Patch Set 8: I would prefer that you didn't submit this
(8 inline comments)
If you like my comments, I am happy to update the patch on behalf of you.
....................................................
File vdsm/configNetwork.py
Line 210: return ret
Line 211: return wrapped
Line 212:
Line 213:
Line 214: @_addRunningConfig
I don't like the approach of using decorator. It's too complex for me.
What kind of advantadge does it have than the way as what you did on delNetwork
Line 215: def addNetwork(network, vlan=None, bonding=None, nics=None,
ipaddr=None,
Line 216: netmask=None, prefix=None, mtu=None, gateway=None,
force=False,
Line 217: configWriter=None, bondingOptions=None, bridged=True,
Line 218: _netinfo=None, **options):
Line 632: for netName in networksAdded:
Line 633: logger.info('Adding network %s to running configuration',
Line 634: netName)
Line 635: _saveRunningConfig(networks[netName],
Line 636: netinfo.NET_CONF_RUN_DIR + netName)
the edited network can't be saved. I feel we don't need maintain the set of
added network and deleted network. we can just use the 'revome' attr to tell
how we should update the the running conf.
for netName, attr in networks.iteritems():
if 'remove' in attr:
logger.info('Removing network %s from running configuration',
netName)
_removeRunningConfig(netinfo.NET_CONF_RUN_DIR + netName)
else:
logger.info('Adding network %s to running configuration',
netName)
_saveRunningConfig(networks[netName],
netinfo.NET_CONF_RUN_DIR + netName)
Line 637: existingBonds = netinfo.bondings()
Line 638: for bondName, attr in bondings.iteritems():
Line 639: if bondName in existingBonds:
Line 640: logger.info('Removing bond %s from running
configuration',
Line 635: _saveRunningConfig(networks[netName],
Line 636: netinfo.NET_CONF_RUN_DIR + netName)
Line 637: existingBonds = netinfo.bondings()
Line 638: for bondName, attr in bondings.iteritems():
Line 639: if bondName in existingBonds:
I don't understand the logic here. I think it could be done in a simliar way as
the networks:
for bondName, attr in bondings.iteritems():
if 'remove' in attr:
logger.info('Removing bond %s from running configuration',
bondName)
_removeRunningConfig(netinfo.BOND_CONF_RUN_DIR + bondName)
else:
logger.info('Adding bond %s to running configuration',
bondName)
_saveRunningConfig(bondings[bondName],
netinfo.BOND_CONF_RUN_DIR + bondName)
then we have the same handling of nets and bonds, we can extract a function to
handle that.
Line 640: logger.info('Removing bond %s from running
configuration',
Line 641: bondName)
Line 642: _removeRunningConfig(netinfo.BOND_CONF_RUN_DIR +
bondName)
Line 643: else:
Line 643: else:
Line 644: logger.info('Adding bond %s to running configuration',
Line 645: bondName)
Line 646: _saveRunningConfig(bondings[bondName],
Line 647: netinfo.NET_CONF_RUN_DIR +
bondName)
should be netinfo.BOND_CONF_RUN_DIR, right?
Line 648:
Line 649:
Line 650: def setSafeNetworkConfig():
Line 651: """Declare current network configuration as 'safe'"""
Line 649:
Line 650: def setSafeNetworkConfig():
Line 651: """Declare current network configuration as 'safe'"""
Line 652: execCmd([constants.EXT_VDSM_STORE_NET_CONFIG,
Line 653: config.get('vars', 'persistency')])
should be 'persistence'
Line 654:
Line 655:
Line 656: def usage():
Line 657: print """Usage:
....................................................
File vdsm/vdsm-restore-net-config
Line 24: import glob
Line 25: import json
Line 26: import os
Line 27:
Line 28: >>>>>>> 2fda785... [WIP] Unified persistence.
the confict need be resolved
Line 29: from netconf import ifcfg
Line 30: from vdsm.config import config
Line 31: from vdsm import netinfo
Line 32: from configNetwork import setupNetworks
....................................................
File vdsm/vdsm-store-net-config.in
Line 52: PERS_CONF_SYMLINK=$PERS_NET_CONF_PATH
Line 53: PERS_CONF_DIR_ROOTNAME="$PERS_CONF_SYMLINK."
Line 54: PERS_CONF_NEW_DIR="$PERS_CONF_DIR_ROOTNAME$TIMESTAMP"
Line 55: PERS_CONF_NEW_SYMLINK="$PERS_CONF_SYMLINK.link.$TIMESTAMP"
Line 56:
/var/lib/vdsm/persistence doesn't exist. it causes the following code fail.
we could creat it dynamically:
[ ! -d "$PERS_CONF_PATH" ] && mkdir "$PERS_CONF_PATH"
or delegate it to rpm spec, as what we handle dir '/var/lib/vdsm/netconfback'
Line 57: cp -r "$RUN_CONF_DIR" "$PERS_CONF_NEW_DIR"
Line 58: ln -s "$PERS_CONF_NEW_DIR" "$PERS_CONF_NEW_SYMLINK"
Line 59: mv -fT "$PERS_CONF_NEW_SYMLINK" "$PERS_CONF_SYMLINK"
Line 60: find "$PERS_CONF_PATH" -type d -path "$PERS_CONF_DIR_ROOTNAME*" | \
Line 57: cp -r "$RUN_CONF_DIR" "$PERS_CONF_NEW_DIR"
Line 58: ln -s "$PERS_CONF_NEW_DIR" "$PERS_CONF_NEW_SYMLINK"
Line 59: mv -fT "$PERS_CONF_NEW_SYMLINK" "$PERS_CONF_SYMLINK"
Line 60: find "$PERS_CONF_PATH" -type d -path "$PERS_CONF_DIR_ROOTNAME*" | \
Line 61: grep -v "$PERS_CONF_NEW_DIR" | xargs rm -fr
we just save the networks or bonds changed by API in running dir. it could be
just part of all networks. So we should update PERS_NET_CONF_PATH with
RUN_CONF_DIR instead of removing the last PERS_NET_CONF_PATH.
Otherwise, we could choose always save all vdsm networks params in the running
conf dir. For the networks not be touched by the API call, we could construct
them from netinfo.
Line 62: }
Line 63:
Line 64:
Line 65: if isOvirt
--
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Assaf Muller <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Petr Šebek <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
