Change in vdsm[master]: network: bridge inherits DHCP unique identifier from its DHC...

danken at redhat.com danken at redhat.com
Wed Aug 19 09:32:14 UTC 2015


Dan Kenigsberg has posted comments on this change.

Change subject: network: bridge inherits DHCP unique identifier from its DHCP-enabled port
......................................................................


Patch Set 4: Code-Review-1

(2 comments)

I have minor comments to the code.

However, please handle - in a separate patch - Petr's comment. I think that we have a similar problem when we define a bridge on top of a bond on top of several nics, where one of them had dhclient running.

https://gerrit.ovirt.org/#/c/44539/4/vdsm/network/configurators/dhclient.py
File vdsm/network/configurators/dhclient.py:

Line 46:                  cgroup=DHCLIENT_CGROUP):
Line 47:         self.iface = iface
Line 48:         self.family = family
Line 49:         self.default_route = default_route
Line 50:         self.duid_source = None if duid_source is None else (
it is confusing to use the same name (duid_source) for both the source device name, and the source device lease file. would you please differentiate between the two?
Line 51:             self.LEASE_FILE.format('' if family == 4 else '6', duid_source))
Line 52:         self.pidFile = self.PID_FILE % (family, self.iface)
Line 53:         if not os.path.exists(self.LEASE_DIR):
Line 54:             os.mkdir(self.LEASE_DIR)


https://gerrit.ovirt.org/#/c/44539/4/vdsm/network/configurators/ifcfg.py
File vdsm/network/configurators/ifcfg.py:

Line 552:             conf += 'STP=%s\n' % ('on' if bridge.stp else 'off')
Line 553:         conf += 'ONBOOT=yes\n'
Line 554:         if bridge.duid_source:
Line 555:             conf += ('DHCLIENTARGS="-df /var/lib/dhclient/'
Line 556:                      'dhclient--%s.lease"\n' % bridge.duid_source)
it is a bit odd that here you do not use the LEASE_DIR constant
Line 557: 
Line 558:         if 'custom' in opts and 'bridge_opts' in opts['custom']:
Line 559:             opts['bridging_opts'] = opts['custom']['bridge_opts']
Line 560:         self._createConfFile(conf, bridge.name, bridge.ipv4, bridge.ipv6,


-- 
To view, visit https://gerrit.ovirt.org/44539
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If91b46657be8a5a42a118b23155d059df379727e
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkaplan at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list