Merge ~smoser/cloud-init:bug/1732964-kill-dhclient-on-ec2 into cloud-init:master

[Scott Moser]

EC2: Kill dhclient process used in sandbox dhclient.

dhclient runs, obtains a address and then backgrounds itself.
cloud-init did not take care to kill it after it was done with it.
After it has run and created the leases, we can kill it.

LP: #1732964

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:b383afc9f88adba046f862bb62ff90c4a6d22775
https://jenkins.ubuntu.com/server/job/cloud-init-ci/509/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/509/rebuild

review: Needs Fixing (continuous-integration)

c0a23f6... by Scott Moser on 2017-11-17

update test

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:c0a23f6efe8002370abebdfff8607b36a711df19
https://jenkins.ubuntu.com/server/job/cloud-init-ci/511/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/511/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

This is good work, thanks for the fix. Tested EC2 across upgrade path and watched the pid read and kill of dhcp process.

review: Approve

diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py
index 0cba703..f3a412a 100644
--- a/cloudinit/net/dhcp.py
+++ b/cloudinit/net/dhcp.py
@@ -8,6 +8,7 @@ import configobj
 import logging
 import os
 import re
+import signal
 
 from cloudinit.net import find_fallback_nic, get_devicelist
 from cloudinit import temp_utils
@@ -119,7 +120,13 @@ def dhcp_discovery(dhclient_cmd_path, interface, cleandir):
     cmd = [sandbox_dhclient_cmd, '-1', '-v', '-lf', lease_file,
            '-pf', pid_file, interface, '-sf', '/bin/true']
     util.subp(cmd, capture=True)
-    return parse_dhcp_lease_file(lease_file)
+    pid = None
+    try:
+        pid = int(util.load_file(pid_file).strip())
+        return parse_dhcp_lease_file(lease_file)
+    finally:
+        if pid:
+            os.kill(pid, signal.SIGKILL)
 
 
 def networkd_parse_lease(content):
diff --git a/cloudinit/net/tests/test_dhcp.py b/cloudinit/net/tests/test_dhcp.py
index 1c1f504..3d8e15c 100644
--- a/cloudinit/net/tests/test_dhcp.py
+++ b/cloudinit/net/tests/test_dhcp.py
@@ -2,6 +2,7 @@
 
 import mock
 import os
+import signal
 from textwrap import dedent
 
 from cloudinit.net.dhcp import (
@@ -114,8 +115,9 @@ class TestDHCPDiscoveryClean(CiTestCase):
         self.assertEqual('eth9', call[0][1])
         self.assertIn('/var/tmp/cloud-init/cloud-init-dhcp-', call[0][2])
 
+    @mock.patch('cloudinit.net.dhcp.os.kill')
     @mock.patch('cloudinit.net.dhcp.util.subp')
-    def test_dhcp_discovery_run_in_sandbox(self, m_subp):
+    def test_dhcp_discovery_run_in_sandbox(self, m_subp, m_kill):
         """dhcp_discovery brings up the interface and runs dhclient.
 
         It also returns the parsed dhcp.leases file generated in the sandbox.
@@ -134,6 +136,10 @@ class TestDHCPDiscoveryClean(CiTestCase):
         """)
         lease_file = os.path.join(tmpdir, 'dhcp.leases')
         write_file(lease_file, lease_content)
+        pid_file = os.path.join(tmpdir, 'dhclient.pid')
+        my_pid = 1
+        write_file(pid_file, "%d\n" % my_pid)
+
         self.assertItemsEqual(
             [{'interface': 'eth9', 'fixed-address': '192.168.2.74',
               'subnet-mask': '255.255.255.0', 'routers': '192.168.2.1'}],
@@ -149,6 +155,7 @@ class TestDHCPDiscoveryClean(CiTestCase):
                 [os.path.join(tmpdir, 'dhclient'), '-1', '-v', '-lf',
                  lease_file, '-pf', os.path.join(tmpdir, 'dhclient.pid'),
                  'eth9', '-sf', '/bin/true'], capture=True)])
+        m_kill.assert_has_calls([mock.call(my_pid, signal.SIGKILL)])
 
 
 class TestSystemdParseLeases(CiTestCase):