Antoni Segura Puimedon has posted comments on this change.
Change subject: Add IPv6 support to configNetwork
......................................................................
Patch Set 1: Code-Review-1
(17 comments)
And we should have unit tests for the new netmodels parts, probably also for
the configNetworks one.
....................................................
File vdsm/configNetwork.py
Line 47: bondingOptions=None, nics=None, mtu=None,
ipaddr=None,
Line 48: netmask=None, gateway=None, bootproto=None,
Line 49: ipv6addr=None, ipv6gateway=None,
ipv6autoconf=None,
Line 50: dhcpv6=None,
Line 51: _netinfo=None, configurator=None,
blockingdhcp=None,
Please, reformat this so the almost empty line ends up the last one.
Line 52: implicitBonding=None, defaultRoute=None,
**opts):
Line 53: """
Line 54: Constructs an object hierarchy that describes the network
configuration
Line 55: that is passed in the parameters.
Line 65: :param gateway: IPv4 address in dotted decimal format.
Line 66: :param bootproto: protocol for getting IP config for the net,
e.g., 'dhcp'
Line 67: :param ipv6addr: IPv6 address in format address[/prefixlen].
Line 68: :param ipv6gateway: IPv6 address in format address[/prefixlen].
Line 69: :param ipv6autoconf: whether to use stateless autoconfiguration.
s/whether to use stateless autoconfiguration/whether to use IPv6 's stateless
autoconfiguration/
Line 70: :param dhcpv6: whether to use DHCPv6.
Line 71: :param _netinfo: network information snapshot.
Line 72: :param configurator: instance to use to apply the network
configuration.
Line 73: :param blockingdhcp: whether to acquire dhcp IP config in a synced
manner.
Line 472: netmask="<ip>"
Line 473: gateway="<ip>"
Line 474: bootproto="..."
Line 475: delay="..."
Line 476: onboot="yes"|"no"
I'd probably add the new options here.
Line 477: (other options will be passed to the config
file AS-IS)
Line 478: -- OR --
Line 479: remove=True (other attributes can't be
specified)
Line 480:
....................................................
File vdsm/netconf/ifcfg.py
Line 470: cfg = cfg + 'MTU=%d\n' % mtu
Line 471: if defaultRoute:
Line 472: cfg = cfg + 'DEFROUTE=%s\n' % defaultRoute
Line 473: cfg += 'NM_CONTROLLED=no\n'
Line 474: if ipv6addr or ipv6gateway or ipv6autoconf or dhcpv6:
I think ipv6gateway is unnecessary. Even without gateway, if we have an
ipv6addr we need to set ipv6init to yes.
Line 475: cfg += 'IPV6INIT=yes\n'
Line 476: ipv6autoconf = 'no'
Line 477: if ipv6addr is not None:
Line 478: cfg += 'IPV6ADDR=%s\n' % pipes.quote(ipv6addr)
Line 472: cfg = cfg + 'DEFROUTE=%s\n' % defaultRoute
Line 473: cfg += 'NM_CONTROLLED=no\n'
Line 474: if ipv6addr or ipv6gateway or ipv6autoconf or dhcpv6:
Line 475: cfg += 'IPV6INIT=yes\n'
Line 476: ipv6autoconf = 'no'
I think this is unnecessary
Line 477: if ipv6addr is not None:
Line 478: cfg += 'IPV6ADDR=%s\n' % pipes.quote(ipv6addr)
Line 479: if ipv6gateway is not None:
Line 480: cfg += 'IPV6_DEFAULTGW=%s\n' %
pipes.quote(ipv6gateway)
Line 480: cfg += 'IPV6_DEFAULTGW=%s\n' %
pipes.quote(ipv6gateway)
Line 481: elif dhcpv6:
Line 482: cfg += 'DHCPV6C=yes\n'
Line 483: if ipv6autoconf:
Line 484: ipv6autoconf = 'yes'
You're always setting ipv6autoconf to 'yes' because you're always setting it to
'no' (which is a True value).
Line 485: cfg += 'IPV6_AUTOCONF=%s\n' % pipes.quote(ipv6autoconf)
Line 486: BLACKLIST = ['TYPE', 'NAME', 'DEVICE', 'bondingOptions',
Line 487: 'force', 'blockingdhcp',
Line 488: 'connectivityCheck', 'connectivityTimeout',
Line 481: elif dhcpv6:
Line 482: cfg += 'DHCPV6C=yes\n'
Line 483: if ipv6autoconf:
Line 484: ipv6autoconf = 'yes'
Line 485: cfg += 'IPV6_AUTOCONF=%s\n' % pipes.quote(ipv6autoconf)
Shouldn't this line just be:
cfg += 'IPV6_AUTOCONF=%s\n' % 'yes' if ipv6autoconf else 'no'
Line 486: BLACKLIST = ['TYPE', 'NAME', 'DEVICE', 'bondingOptions',
Line 487: 'force', 'blockingdhcp',
Line 488: 'connectivityCheck', 'connectivityTimeout',
Line 489: 'implicitBonding']
Line 502: self._createConfFile(conf, bridge.name, ipconfig.ipaddr,
Line 503: ipconfig.netmask, ipconfig.gateway,
Line 504: ipconfig.bootproto, bridge.mtu,
Line 505: self._toIfcfgFormat(self.defaultRoute),
Line 506: **opts)
s/self/ipconfig/
But in any case, you're not passing the ipv6addr and such. What about a patch
preceding this one that refactors _createconfFile so it takes an ipConfig
object?
same for all addX methods.
Line 507:
Line 508: def addVlan(self, vlan, **opts):
Line 509: """ Create ifcfg-* file with proper fields for VLAN """
Line 510: ipconfig = vlan.ipConfig
....................................................
File vdsm/netmodels.py
Line 27: from vdsm import netinfo
Line 28: import neterrors as ne
Line 29:
Line 30:
Line 31: class NetDevice(object):
Why are you undoing Mark's patch?
Line 32: def __init__(self, name, configurator, ipconfig=None, mtu=None):
Line 33: self.name = name
Line 34: self.ip = ipconfig
Line 35: self.mtu = mtu
Line 43: raise NotImplementedError
Line 44:
Line 45: @property
Line 46: def ipConfig(self):
Line 47: return self.ip.getConfig()
Same here
Line 48:
Line 49: @property
Line 50: def bridge(self):
Line 51: if isinstance(self.master, Bridge):
Line 330: self.defaultRoute)
Line 331:
Line 332: @classmethod
Line 333: def validateAddress(cls, address):
Line 334: addr = address.split('/', 1)
addr, prefix = address.split('/', 1)
Line 335: try:
Line 336: socket.inet_pton(socket.AF_INET6, addr[0])
Line 337: except socket.error:
Line 338: raise ConfigNetworkError(ne.ERR_BAD_ADDR, '%r is not a
valid IPv6 '
Line 332: @classmethod
Line 333: def validateAddress(cls, address):
Line 334: addr = address.split('/', 1)
Line 335: try:
Line 336: socket.inet_pton(socket.AF_INET6, addr[0])
socket.inet_pton(socket.AF_INET6, addr)
Line 337: except socket.error:
Line 338: raise ConfigNetworkError(ne.ERR_BAD_ADDR, '%r is not a
valid IPv6 '
Line 339: 'address.' % address)
Line 340: if len(addr) == 2:
Line 337: except socket.error:
Line 338: raise ConfigNetworkError(ne.ERR_BAD_ADDR, '%r is not a
valid IPv6 '
Line 339: 'address.' % address)
Line 340: if len(addr) == 2:
Line 341: cls.validatePrefixlen(addr[1])
cls.validatePrefix(prefix)
Line 342:
Line 343: @classmethod
Line 344: def validatePrefixlen(cls, prefixlen):
Line 345: try:
Line 340: if len(addr) == 2:
Line 341: cls.validatePrefixlen(addr[1])
Line 342:
Line 343: @classmethod
Line 344: def validatePrefixlen(cls, prefixlen):
s/validatePrefixlen/validatePrefix/
Line 345: try:
Line 346: prefixlen = int(prefixlen)
Line 347: if prefixlen < 0 or prefixlen > 127:
Line 348: raise ConfigNetworkError(ne.ERR_BAD_ADDR, '%r is not
valid '
Line 343: @classmethod
Line 344: def validatePrefixlen(cls, prefixlen):
Line 345: try:
Line 346: prefixlen = int(prefixlen)
Line 347: if prefixlen < 0 or prefixlen > 127:
I'd prefer the more readable:
if not(0 <= int(prefix) <= 127)
Line 348: raise ConfigNetworkError(ne.ERR_BAD_ADDR, '%r is not
valid '
Line 349: 'IPv6 prefixlen.' %
prefixlen)
Line 350: except ValueError:
Line 351: raise ConfigNetworkError(ne.ERR_BAD_ADDR, '%r is not
valid '
Line 345: try:
Line 346: prefixlen = int(prefixlen)
Line 347: if prefixlen < 0 or prefixlen > 127:
Line 348: raise ConfigNetworkError(ne.ERR_BAD_ADDR, '%r is not
valid '
Line 349: 'IPv6 prefixlen.' %
prefixlen)
s/prefixlen/prefix length/
same in line 351
Line 350: except ValueError:
Line 351: raise ConfigNetworkError(ne.ERR_BAD_ADDR, '%r is not
valid '
Line 352: 'IPv6 prefixlen.' % prefixlen)
Line 353:
Line 383: self.ipv6autoconf,
Line 384: self.dhcpv6)
Line 385:
Line 386: def getConfig(self):
Line 387: config = {}
This should just add the new elements to the return tuple instead of returning
a dictionary.
Line 388: if self.inet4:
Line 389: config['ipaddr'] = self.inet4.address
Line 390: config['netmask'] = self.inet4.netmask
Line 391: config['gateway'] = self.inet4.gateway
--
To view, visit http://gerrit.ovirt.org/18284
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8ed056b683f0cb893b2edcf1ae673c64ce5cd18c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Šebek <pse...@redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
