Change in vdsm[ovirt-3.5]: net: run dhclient in its own process group.

ibarkan at redhat.com ibarkan at redhat.com
Tue Jul 21 11:35:13 UTC 2015


Hello Dan Kenigsberg,

I'd like you to do a code review.  Please visit

    https://gerrit.ovirt.org/43853

to review the following change.

Change subject: net: run dhclient in its own process group.
......................................................................

net: run dhclient in its own process group.

This is done to prevent systemd from killing supervdsm dhclient child
processes during supervdsm stop (or restart). Forking dhclient in
its own process group prevents systemd from doing this.
Since dhclient is deliberately killed either directly by supervdsm or
indirectly by executing ifdown on a device and since executing a
process in a different process group is harmless, the separate
cgroup is now the default behaviour.
This patch also fixes https://bugzilla.redhat.com/1187244 in a less
clumsy way than KillMode=process.

Change-Id: I82848a36b52cd8e9dec188d865ef86edc4bb7488
Signed-off-by: Ido Barkan <ibarkan at redhat.com>
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1228322
Reviewed-on: https://gerrit.ovirt.org/43005
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
Continuous-Integration: Jenkins CI
---
M init/systemd/vdsm-network.service.in
M vdsm/network/configurators/dhclient.py
M vdsm/network/configurators/ifcfg.py
3 files changed, 17 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/53/43853/1

diff --git a/init/systemd/vdsm-network.service.in b/init/systemd/vdsm-network.service.in
index 9680f06..c3fa44c 100644
--- a/init/systemd/vdsm-network.service.in
+++ b/init/systemd/vdsm-network.service.in
@@ -10,7 +10,6 @@
 ExecStartPre=@BINDIR@/vdsm-tool --vvverbose --append --logfile=@VDSMLOGDIR@/upgrade.log upgrade-unified-persistence
 ExecStartPre=@BINDIR@/vdsm-tool --vvverbose --append --logfile=@VDSMLOGDIR@/upgrade.log upgrade-3.0.0-networks
 ExecStart=@BINDIR@/vdsm-tool restore-nets
-KillMode=process
 RemainAfterExit=yes
 
 [Install]
diff --git a/vdsm/network/configurators/dhclient.py b/vdsm/network/configurators/dhclient.py
index 0b83b24..b2af4a0 100644
--- a/vdsm/network/configurators/dhclient.py
+++ b/vdsm/network/configurators/dhclient.py
@@ -23,12 +23,15 @@
 import signal
 import threading
 
+from vdsm import cmdutils
 from vdsm import ipwrapper
 from vdsm import netinfo
 from vdsm.utils import CommandPath
 from vdsm.utils import execCmd
 from vdsm.utils import pgrep
 from vdsm.utils import rmFile
+
+DHCLIENT_CGROUP = 'vdsm-dhclient'
 
 
 class DhcpClient(object):
@@ -37,20 +40,22 @@
     LEASE_FILE = LEASE_DIR + 'dhclient-%s.lease'
     DHCLIENT = CommandPath('dhclient', '/sbin/dhclient')
 
-    def __init__(self, iface):
+    def __init__(self, iface, cgroup=DHCLIENT_CGROUP):
         self.iface = iface
         self.pidFile = self.PID_FILE % self.iface
         if not os.path.exists(self.LEASE_DIR):
             os.mkdir(self.LEASE_DIR)
         self.leaseFile = (self.LEASE_FILE % self.iface)
+        self._cgroup = cgroup
 
     def _dhclient(self):
         # Ask dhclient to stop any dhclient running for the device
         if os.path.exists(os.path.join(netinfo.NET_PATH, self.iface)):
             kill_dhclient(self.iface)
-        rc, out, err = execCmd([self.DHCLIENT.cmd, '-1', '-pf',
-                                self.pidFile, '-lf', self.leaseFile,
-                                self.iface])
+        cmd = [self.DHCLIENT.cmd, '-1', '-pf', self.pidFile, '-lf',
+               self.leaseFile, self.iface]
+        cmd = cmdutils.systemd_run(cmd, scope=True, slice=self._cgroup)
+        rc, out, err = execCmd(cmd)
         return rc, out, err
 
     def start(self, async):
diff --git a/vdsm/network/configurators/ifcfg.py b/vdsm/network/configurators/ifcfg.py
index c3f9893..b988925 100644
--- a/vdsm/network/configurators/ifcfg.py
+++ b/vdsm/network/configurators/ifcfg.py
@@ -32,6 +32,7 @@
 from libvirt import libvirtError, VIR_ERR_NO_NETWORK
 
 from vdsm.config import config
+from vdsm import cmdutils
 from vdsm import constants
 from vdsm import ipwrapper
 from vdsm import netinfo
@@ -784,10 +785,15 @@
     return rc
 
 
-def ifup(iface, async=False):
+def ifup(iface, async=False, cgroup=dhclient.DHCLIENT_CGROUP):
     "Bring up an interface"
     def _ifup(netIf):
-        rc, out, err = utils.execCmd([constants.EXT_IFUP, netIf], raw=False)
+        cmd = [constants.EXT_IFUP, netIf]
+        
+        if cgroup is not None:
+            cmd = cmdutils.systemd_run(cmd, scope=True, slice=cgroup)
+            
+        rc, out, err = utils.execCmd(cmd, raw=False)
 
         if rc != 0:
             # In /etc/sysconfig/network-scripts/ifup* the last line usually


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I82848a36b52cd8e9dec188d865ef86edc4bb7488
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.5
Gerrit-Owner: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>


More information about the vdsm-patches mailing list