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