Merge lp:~canonical-platform-qa/autopilot/fixing_pid_search_1569079 into lp:autopilot

Proposed by Christopher Lee on 2016-04-11
Status: Merged
Approved by: Christopher Lee on 2016-04-12
Approved revision: 579
Merged at revision: 577
Proposed branch: lp:~canonical-platform-qa/autopilot/fixing_pid_search_1569079
Merge into: lp:autopilot
Diff against target: 155 lines (+119/-1)
2 files modified
autopilot/introspection/_search.py (+42/-0)
autopilot/tests/unit/test_introspection_search.py (+77/-1)
To merge this branch: bzr merge lp:~canonical-platform-qa/autopilot/fixing_pid_search_1569079
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve on 2016-04-12
Thomi Richards (community) Approve on 2016-04-12
platform-qa-bot continuous-integration Approve on 2016-04-12
Autopilot Hackers 2016-04-11 Pending
Review via email: mp+291568@code.launchpad.net

Commit message

Add a final filter that ignores any children processes if the parent process matches and the supplied pid is the parents.

Description of the change

It's possible that a parent process spawns a child process that will match an attempt to get a proxy object for the parent.

Add a final filter that ignores any children processes if the parent process matches and the supplied pid is the parents.

To post a comment you must log in.
Max Brustkern (nuclearbob) wrote :

Seems reasonable to me, once we get a good test of it.

578. By Christopher Lee on 2016-04-11

Fix minor flake8 issue.

579. By Christopher Lee on 2016-04-12

Refactor to remove mock in test and potentially reduce calls to get pid details.

Thomi Richards (thomir) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'autopilot/introspection/_search.py'
2--- autopilot/introspection/_search.py 2016-01-22 17:21:36 +0000
3+++ autopilot/introspection/_search.py 2016-04-12 03:02:16 +0000
4@@ -171,6 +171,16 @@
5 process
6 )
7
8+ if pid is not None:
9+ # Due to the filtering including children parents, if there exists a
10+ # top-level pid, take that instead of any children that may have
11+ # matched.
12+ connections = _filter_parent_pids_from_children(
13+ pid,
14+ connections,
15+ dbus_bus
16+ )
17+
18 _raise_if_not_single_result(
19 connections,
20 _get_search_criteria_string_representation(**kwargs)
21@@ -184,6 +194,38 @@
22 )
23
24
25+def _map_connection_to_pid(connection, dbus_bus):
26+ try:
27+ return _get_bus_connections_pid(dbus_bus, connection)
28+ except dbus.DBusException as e:
29+ logger.info(
30+ "dbus.DBusException while attempting to get PID for %s: %r" %
31+ (connection, e))
32+
33+
34+def _filter_parent_pids_from_children(
35+ pid, connections, dbus_bus, _connection_pid_fn=_map_connection_to_pid):
36+ """Return any connections that have an actual pid matching the requested
37+ and aren't just a child of that requested pid.
38+
39+ :param pid: Pid passed in for matching
40+ :param connections: List of connections to filter
41+ :param dbus_bus: Dbus object that the connections are contained.
42+ :param _connection_pid_fn: Function that takes 2 args 'connection' 'dbus
43+ object' that returns the pid (or None if not found) of the connection on
44+ that dbus bus.
45+ (Note: Useful for testing.)
46+ :returns: List of suitable connections (e.g. returns what was passed if no
47+ connections match the pid (i.e. all matches are children)).
48+
49+ """
50+ for conn in connections:
51+ if pid == _connection_pid_fn(conn, dbus_bus):
52+ logger.info('Found the parent pid, ignoring any others.')
53+ return [conn]
54+ return connections
55+
56+
57 def _get_dbus_bus_from_string(dbus_string):
58 if dbus_string == 'session':
59 return dbus_handler.get_session_bus()
60
61=== modified file 'autopilot/tests/unit/test_introspection_search.py'
62--- autopilot/tests/unit/test_introspection_search.py 2015-06-16 02:39:23 +0000
63+++ autopilot/tests/unit/test_introspection_search.py 2016-04-12 03:02:16 +0000
64@@ -19,7 +19,7 @@
65
66 from dbus import DBusException
67 import os
68-from unittest.mock import patch, Mock
69+from unittest.mock import call, patch, Mock
70 from testtools import TestCase
71 from testtools.matchers import (
72 Contains,
73@@ -581,6 +581,82 @@
74 self.assertEqual(fake_process.pid, observed)
75
76
77+class FilterParentPidsFromChildrenTests(TestCase):
78+
79+ def test_returns_all_connections_with_no_parent_match(self):
80+ search_pid = 123
81+ connections = ['1:0', '1:3']
82+ dbus_bus = Mock()
83+ pid_mapping = Mock(side_effect=[111, 222])
84+ self.assertThat(
85+ _s._filter_parent_pids_from_children(
86+ search_pid,
87+ connections,
88+ dbus_bus,
89+ _connection_pid_fn=pid_mapping
90+ ),
91+ Equals(connections)
92+ )
93+
94+ def test_calls_connection_pid_fn_in_order(self):
95+ search_pid = 123
96+ connections = ['1:3', '1:0']
97+ dbus_bus = Mock()
98+ pid_mapping = Mock(side_effect=[222, 123])
99+ _s._filter_parent_pids_from_children(
100+ search_pid,
101+ connections,
102+ dbus_bus,
103+ _connection_pid_fn=pid_mapping
104+ )
105+
106+ self.assertTrue(
107+ pid_mapping.call_args_list == [
108+ call('1:3', dbus_bus),
109+ call('1:0', dbus_bus)
110+ ]
111+ )
112+
113+ def test_returns_just_parent_connection_with_pid_match(self):
114+ search_pid = 123
115+ # connection '1.0' has pid 123.
116+ connections = ['1:3', '1:0']
117+ dbus_bus = Mock()
118+ # Mapping returns parent pid on second call.
119+ pid_mapping = Mock(side_effect=[222, 123])
120+ self.assertThat(
121+ _s._filter_parent_pids_from_children(
122+ search_pid,
123+ connections,
124+ dbus_bus,
125+ _connection_pid_fn=pid_mapping
126+ ),
127+ Equals(['1:0'])
128+ )
129+
130+ self.assertTrue(
131+ pid_mapping.call_args_list == [
132+ call('1:3', dbus_bus),
133+ call('1:0', dbus_bus)
134+ ]
135+ )
136+
137+ def test_returns_all_connections_with_no_pids_returned_in_search(self):
138+ search_pid = 123
139+ connections = ['1:3', '1:0']
140+ dbus_bus = Mock()
141+ pid_mapping = Mock(side_effect=[None, None])
142+ self.assertThat(
143+ _s._filter_parent_pids_from_children(
144+ search_pid,
145+ connections,
146+ dbus_bus,
147+ _connection_pid_fn=pid_mapping
148+ ),
149+ Equals(connections)
150+ )
151+
152+
153 class ProcessSearchErrorStringRepTests(TestCase):
154
155 """Various tests for the _get_search_criteria_string_representation

Subscribers

People subscribed via source and target branches