Merge ~smoser/cloud-init:fix/fix-dhclient-hook-down into cloud-init:master

Proposed by Scott Moser
Status: Merged
Approved by: Chad Smith
Approved revision: 4fdd42ed0a1a10eb213b041517ae715f93e6a7c7
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~smoser/cloud-init:fix/fix-dhclient-hook-down
Merge into: cloud-init:master
Diff against target: 330 lines (+193/-63)
5 files modified
bash_completion/cloud-init (+4/-1)
cloudinit/cmd/main.py (+4/-16)
cloudinit/dhclient_hook.py (+72/-38)
cloudinit/tests/test_dhclient_hook.py (+105/-0)
tests/unittests/test_cli.py (+8/-8)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve
Chad Smith Approve
Review via email: mp+356428@code.launchpad.net

Commit message

dhclient-hook: cleanups, tests and fix a bug on 'down' event.

I noticed a bug in dhclient_hook on the 'down' event, using 'is'
operator rather than '==' (if self.net_action is 'down').

This refactors/simplifies the code a bit for easier testing and adds
tests. The reason for the rename of 'action' to 'event' is to just
be internally consistent. The word and Namespace 'action' is used
by cloud-init main, so it was not really usable here.

Also adds a main which can easily be debugged with:
  CI_DHCP_HOOK_DATA_D=./my.d python -m cloudinit.dhclient_hook up eth0

Description of the change

see commit message

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

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

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

review: Approve (continuous-integration)
Revision history for this message
Chad Smith (chad.smith) wrote :

Can we add some bash_completion/cloud-init goodness for dhclient-hook.

Also, do we know of any public consumers that might be hurt by changing these cmdline parameter names from
(net_action, net_interface) TO (event, interface) negative?

review: Needs Information
Revision history for this message
Chad Smith (chad.smith) :
Revision history for this message
Scott Moser (smoser) wrote :

> Can we add some bash_completion/cloud-init goodness for dhclient-hook.
>
> Also, do we know of any public consumers that might be hurt by changing these
> cmdline parameter names from
> (net_action, net_interface) TO (event, interface) negative?

I dont think there would be any external consumers.

7c6cc7d... by Scott Moser

address Chad's feedback. ACTIONS -> EVENTS

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

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

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

review: Approve (continuous-integration)
4fdd42e... by Scott Moser

per chad feedback update dhclient-hook bash completion

Revision history for this message
Chad Smith (chad.smith) wrote :

Thanks for the update on tab-completion. LGTM!

review: Approve
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

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

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

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/bash_completion/cloud-init b/bash_completion/cloud-init
2index 8c25032..a9577e9 100644
3--- a/bash_completion/cloud-init
4+++ b/bash_completion/cloud-init
5@@ -30,7 +30,10 @@ _cloudinit_complete()
6 devel)
7 COMPREPLY=($(compgen -W "--help schema net-convert" -- $cur_word))
8 ;;
9- dhclient-hook|features)
10+ dhclient-hook)
11+ COMPREPLY=($(compgen -W "--help up down" -- $cur_word))
12+ ;;
13+ features)
14 COMPREPLY=($(compgen -W "--help" -- $cur_word))
15 ;;
16 init)
17diff --git a/cloudinit/cmd/main.py b/cloudinit/cmd/main.py
18index 5a43702..933c019 100644
19--- a/cloudinit/cmd/main.py
20+++ b/cloudinit/cmd/main.py
21@@ -41,7 +41,7 @@ from cloudinit.settings import (PER_INSTANCE, PER_ALWAYS, PER_ONCE,
22 from cloudinit import atomic_helper
23
24 from cloudinit.config import cc_set_hostname
25-from cloudinit.dhclient_hook import LogDhclient
26+from cloudinit import dhclient_hook
27
28
29 # Welcome message template
30@@ -586,12 +586,6 @@ def main_single(name, args):
31 return 0
32
33
34-def dhclient_hook(name, args):
35- record = LogDhclient(args)
36- record.check_hooks_dir()
37- record.record()
38-
39-
40 def status_wrapper(name, args, data_d=None, link_d=None):
41 if data_d is None:
42 data_d = os.path.normpath("/var/lib/cloud/data")
43@@ -795,15 +789,9 @@ def main(sysv_args=None):
44 'query',
45 help='Query standardized instance metadata from the command line.')
46
47- parser_dhclient = subparsers.add_parser('dhclient-hook',
48- help=('run the dhclient hook'
49- 'to record network info'))
50- parser_dhclient.add_argument("net_action",
51- help=('action taken on the interface'))
52- parser_dhclient.add_argument("net_interface",
53- help=('the network interface being acted'
54- ' upon'))
55- parser_dhclient.set_defaults(action=('dhclient_hook', dhclient_hook))
56+ parser_dhclient = subparsers.add_parser(
57+ dhclient_hook.NAME, help=dhclient_hook.__doc__)
58+ dhclient_hook.get_parser(parser_dhclient)
59
60 parser_features = subparsers.add_parser('features',
61 help=('list defined features'))
62diff --git a/cloudinit/dhclient_hook.py b/cloudinit/dhclient_hook.py
63index 7f02d7f..72b51b6 100644
64--- a/cloudinit/dhclient_hook.py
65+++ b/cloudinit/dhclient_hook.py
66@@ -1,5 +1,8 @@
67 # This file is part of cloud-init. See LICENSE file for license information.
68
69+"""Run the dhclient hook to record network info."""
70+
71+import argparse
72 import os
73
74 from cloudinit import atomic_helper
75@@ -8,44 +11,75 @@ from cloudinit import stages
76
77 LOG = logging.getLogger(__name__)
78
79+NAME = "dhclient-hook"
80+UP = "up"
81+DOWN = "down"
82+EVENTS = (UP, DOWN)
83+
84+
85+def _get_hooks_dir():
86+ i = stages.Init()
87+ return os.path.join(i.paths.get_runpath(), 'dhclient.hooks')
88+
89+
90+def _filter_env_vals(info):
91+ """Given info (os.environ), return a dictionary with
92+ lower case keys for each entry starting with DHCP4_ or new_."""
93+ new_info = {}
94+ for k, v in info.items():
95+ if k.startswith("DHCP4_") or k.startswith("new_"):
96+ key = (k.replace('DHCP4_', '').replace('new_', '')).lower()
97+ new_info[key] = v
98+ return new_info
99+
100+
101+def run_hook(interface, event, data_d=None, env=None):
102+ if event not in EVENTS:
103+ raise ValueError("Unexpected event '%s'. Expected one of: %s" %
104+ (event, EVENTS))
105+ if data_d is None:
106+ data_d = _get_hooks_dir()
107+ if env is None:
108+ env = os.environ
109+ hook_file = os.path.join(data_d, interface + ".json")
110+
111+ if event == UP:
112+ if not os.path.exists(data_d):
113+ os.makedirs(data_d)
114+ atomic_helper.write_json(hook_file, _filter_env_vals(env))
115+ LOG.debug("Wrote dhclient options in %s", hook_file)
116+ elif event == DOWN:
117+ if os.path.exists(hook_file):
118+ os.remove(hook_file)
119+ LOG.debug("Removed dhclient options file %s", hook_file)
120+
121+
122+def get_parser(parser=None):
123+ if parser is None:
124+ parser = argparse.ArgumentParser(prog=NAME, description=__doc__)
125+ parser.add_argument(
126+ "event", help='event taken on the interface', choices=EVENTS)
127+ parser.add_argument(
128+ "interface", help='the network interface being acted upon')
129+ # cloud-init main uses 'action'
130+ parser.set_defaults(action=(NAME, handle_args))
131+ return parser
132+
133+
134+def handle_args(name, args, data_d=None):
135+ """Handle the Namespace args.
136+ Takes 'name' as passed by cloud-init main. not used here."""
137+ return run_hook(interface=args.interface, event=args.event, data_d=data_d)
138+
139+
140+if __name__ == '__main__':
141+ import sys
142+ parser = get_parser()
143+ args = parser.parse_args(args=sys.argv[1:])
144+ return_value = handle_args(
145+ NAME, args, data_d=os.environ.get('_CI_DHCP_HOOK_DATA_D'))
146+ if return_value:
147+ sys.exit(return_value)
148
149-class LogDhclient(object):
150-
151- def __init__(self, cli_args):
152- self.hooks_dir = self._get_hooks_dir()
153- self.net_interface = cli_args.net_interface
154- self.net_action = cli_args.net_action
155- self.hook_file = os.path.join(self.hooks_dir,
156- self.net_interface + ".json")
157-
158- @staticmethod
159- def _get_hooks_dir():
160- i = stages.Init()
161- return os.path.join(i.paths.get_runpath(), 'dhclient.hooks')
162-
163- def check_hooks_dir(self):
164- if not os.path.exists(self.hooks_dir):
165- os.makedirs(self.hooks_dir)
166- else:
167- # If the action is down and the json file exists, we need to
168- # delete the file
169- if self.net_action is 'down' and os.path.exists(self.hook_file):
170- os.remove(self.hook_file)
171-
172- @staticmethod
173- def get_vals(info):
174- new_info = {}
175- for k, v in info.items():
176- if k.startswith("DHCP4_") or k.startswith("new_"):
177- key = (k.replace('DHCP4_', '').replace('new_', '')).lower()
178- new_info[key] = v
179- return new_info
180-
181- def record(self):
182- envs = os.environ
183- if self.hook_file is None:
184- return
185- atomic_helper.write_json(self.hook_file, self.get_vals(envs))
186- LOG.debug("Wrote dhclient options in %s", self.hook_file)
187
188 # vi: ts=4 expandtab
189diff --git a/cloudinit/tests/test_dhclient_hook.py b/cloudinit/tests/test_dhclient_hook.py
190new file mode 100644
191index 0000000..7aab8dd
192--- /dev/null
193+++ b/cloudinit/tests/test_dhclient_hook.py
194@@ -0,0 +1,105 @@
195+# This file is part of cloud-init. See LICENSE file for license information.
196+
197+"""Tests for cloudinit.dhclient_hook."""
198+
199+from cloudinit import dhclient_hook as dhc
200+from cloudinit.tests.helpers import CiTestCase, dir2dict, populate_dir
201+
202+import argparse
203+import json
204+import mock
205+import os
206+
207+
208+class TestDhclientHook(CiTestCase):
209+
210+ ex_env = {
211+ 'interface': 'eth0',
212+ 'new_dhcp_lease_time': '3600',
213+ 'new_host_name': 'x1',
214+ 'new_ip_address': '10.145.210.163',
215+ 'new_subnet_mask': '255.255.255.0',
216+ 'old_host_name': 'x1',
217+ 'PATH': '/usr/sbin:/usr/bin:/sbin:/bin',
218+ 'pid': '614',
219+ 'reason': 'BOUND',
220+ }
221+
222+ # some older versions of dhclient put the same content,
223+ # but in upper case with DHCP4_ instead of new_
224+ ex_env_dhcp4 = {
225+ 'REASON': 'BOUND',
226+ 'DHCP4_dhcp_lease_time': '3600',
227+ 'DHCP4_host_name': 'x1',
228+ 'DHCP4_ip_address': '10.145.210.163',
229+ 'DHCP4_subnet_mask': '255.255.255.0',
230+ 'INTERFACE': 'eth0',
231+ 'PATH': '/usr/sbin:/usr/bin:/sbin:/bin',
232+ 'pid': '614',
233+ }
234+
235+ expected = {
236+ 'dhcp_lease_time': '3600',
237+ 'host_name': 'x1',
238+ 'ip_address': '10.145.210.163',
239+ 'subnet_mask': '255.255.255.0'}
240+
241+ def setUp(self):
242+ super(TestDhclientHook, self).setUp()
243+ self.tmp = self.tmp_dir()
244+
245+ def test_handle_args(self):
246+ """quick test of call to handle_args."""
247+ nic = 'eth0'
248+ args = argparse.Namespace(event=dhc.UP, interface=nic)
249+ with mock.patch.dict("os.environ", clear=True, values=self.ex_env):
250+ dhc.handle_args(dhc.NAME, args, data_d=self.tmp)
251+ found = dir2dict(self.tmp + os.path.sep)
252+ self.assertEqual([nic + ".json"], list(found.keys()))
253+ self.assertEqual(self.expected, json.loads(found[nic + ".json"]))
254+
255+ def test_run_hook_up_creates_dir(self):
256+ """If dir does not exist, run_hook should create it."""
257+ subd = self.tmp_path("subdir", self.tmp)
258+ nic = 'eth1'
259+ dhc.run_hook(nic, 'up', data_d=subd, env=self.ex_env)
260+ self.assertEqual(
261+ set([nic + ".json"]), set(dir2dict(subd + os.path.sep)))
262+
263+ def test_run_hook_up(self):
264+ """Test expected use of run_hook_up."""
265+ nic = 'eth0'
266+ dhc.run_hook(nic, 'up', data_d=self.tmp, env=self.ex_env)
267+ found = dir2dict(self.tmp + os.path.sep)
268+ self.assertEqual([nic + ".json"], list(found.keys()))
269+ self.assertEqual(self.expected, json.loads(found[nic + ".json"]))
270+
271+ def test_run_hook_up_dhcp4_prefix(self):
272+ """Test run_hook filters correctly with older DHCP4_ data."""
273+ nic = 'eth0'
274+ dhc.run_hook(nic, 'up', data_d=self.tmp, env=self.ex_env_dhcp4)
275+ found = dir2dict(self.tmp + os.path.sep)
276+ self.assertEqual([nic + ".json"], list(found.keys()))
277+ self.assertEqual(self.expected, json.loads(found[nic + ".json"]))
278+
279+ def test_run_hook_down_deletes(self):
280+ """down should delete the created json file."""
281+ nic = 'eth1'
282+ populate_dir(
283+ self.tmp, {nic + ".json": "{'abcd'}", 'myfile.txt': 'text'})
284+ dhc.run_hook(nic, 'down', data_d=self.tmp, env={'old_host_name': 'x1'})
285+ self.assertEqual(
286+ set(['myfile.txt']),
287+ set(dir2dict(self.tmp + os.path.sep)))
288+
289+ def test_get_parser(self):
290+ """Smoke test creation of get_parser."""
291+ # cloud-init main uses 'action'.
292+ event, interface = (dhc.UP, 'mynic0')
293+ self.assertEqual(
294+ argparse.Namespace(event=event, interface=interface,
295+ action=(dhc.NAME, dhc.handle_args)),
296+ dhc.get_parser().parse_args([event, interface]))
297+
298+
299+# vi: ts=4 expandtab
300diff --git a/tests/unittests/test_cli.py b/tests/unittests/test_cli.py
301index 199d69b..d283f13 100644
302--- a/tests/unittests/test_cli.py
303+++ b/tests/unittests/test_cli.py
304@@ -246,18 +246,18 @@ class TestCLI(test_helpers.FilesystemMockingTestCase):
305 self.assertEqual('cc_ntp', parseargs.name)
306 self.assertFalse(parseargs.report)
307
308- @mock.patch('cloudinit.cmd.main.dhclient_hook')
309- def test_dhclient_hook_subcommand(self, m_dhclient_hook):
310+ @mock.patch('cloudinit.cmd.main.dhclient_hook.handle_args')
311+ def test_dhclient_hook_subcommand(self, m_handle_args):
312 """The subcommand 'dhclient-hook' calls dhclient_hook with args."""
313- self._call_main(['cloud-init', 'dhclient-hook', 'net_action', 'eth0'])
314- (name, parseargs) = m_dhclient_hook.call_args_list[0][0]
315- self.assertEqual('dhclient_hook', name)
316+ self._call_main(['cloud-init', 'dhclient-hook', 'up', 'eth0'])
317+ (name, parseargs) = m_handle_args.call_args_list[0][0]
318+ self.assertEqual('dhclient-hook', name)
319 self.assertEqual('dhclient-hook', parseargs.subcommand)
320- self.assertEqual('dhclient_hook', parseargs.action[0])
321+ self.assertEqual('dhclient-hook', parseargs.action[0])
322 self.assertFalse(parseargs.debug)
323 self.assertFalse(parseargs.force)
324- self.assertEqual('net_action', parseargs.net_action)
325- self.assertEqual('eth0', parseargs.net_interface)
326+ self.assertEqual('up', parseargs.event)
327+ self.assertEqual('eth0', parseargs.interface)
328
329 @mock.patch('cloudinit.cmd.main.main_features')
330 def test_features_hook_subcommand(self, m_features):

Subscribers

People subscribed via source and target branches