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

Tuesday, 21 July 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(a)redhat.com&gt;
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1228322
Reviewed-on: https://gerrit.ovirt.org/43005
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
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(a)/upgrade.log
upgrade-unified-persistence
 ExecStartPre=@BINDIR@/vdsm-tool --vvverbose --append --logfile=@VDSMLOGDIR(a)/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(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011