Merge lp:~bcsaller/pyjuju/subordinate-ports into lp:pyjuju
- subordinate-ports
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Kapil Thangavelu | ||||
Approved revision: | 548 | ||||
Merged at revision: | 555 | ||||
Proposed branch: | lp:~bcsaller/pyjuju/subordinate-ports | ||||
Merge into: | lp:pyjuju | ||||
Diff against target: |
217 lines (+142/-1) 5 files modified
examples/precise/recorder/hooks/install (+3/-0) juju/hooks/protocol.py (+15/-0) juju/hooks/tests/test_communications.py (+3/-0) juju/hooks/tests/test_invoker.py (+84/-1) juju/state/tests/test_service.py (+37/-0) |
||||
To merge this branch: | bzr merge lp:~bcsaller/pyjuju/subordinate-ports | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email: mp+111534@code.launchpad.net |
Commit message
Description of the change
Subordinate port open/close support
Subordinates will open ports in their container. They will report open ports on the container and in the subordinate.
Benjamin Saller (bcsaller) wrote : | # |
Kapil Thangavelu (hazmat) wrote : | # |
is it nesc/valuable to record the open port against both the subordinate and the primary
Benjamin Saller (bcsaller) wrote : | # |
nesc, no, valuable, I think so. It has to be on the principal for this
to work the way its set up, recording it on the subordinate I think is
more clean that issuing commands to the subordinate and not seeing any
change in status for that service. The fact that the subordinate makes
a change somewhere else as well in status is more understandable than
not seeing it.
It might be possible to record the change in the sub and open it on
the principal but not record it there but that seemed like a recipe
for unintended consequences.
On Tue, Jun 26, 2012 at 9:35 AM, Kapil Thangavelu
<email address hidden> wrote:
> is it nesc/valuable to record the open port against both the subordinate and the primary
> --
> https:/
> You are the owner of lp:~bcsaller/juju/subordinate-ports.
Kapil Thangavelu (hazmat) wrote : | # |
while the implementation LGTM, the tests need fixing.
https:/
File juju/hooks/
https:/
juju/hooks/
Invoker
pep8 need an extra space before comment
https:/
juju/hooks/
unit_state.
This test never verifies the implementation, namely that the container
ports are open.
https:/
juju/hooks/
pep8 need an extra blank line after this test
https:/
File juju/state/
https:/
juju/state/
this test also seems superfluous. the only impl file being modified by
the branch is the invoker, yet we have tests in numerous state test
files verifying what has already been verified by other tests in those
files
https:/
File juju/state/
https:/
juju/state/
self.machine_
machine creation and assignment is superflous
https:/
juju/state/
test_get_
typo in test name
https:/
juju/state/
how is this entire test not entirely superfluous?
Benjamin Saller (bcsaller) wrote : | # |
https:/
File juju/hooks/
https:/
juju/hooks/
unit_state.
On 2012/07/01 02:32:22, hazmat wrote:
> This test never verifies the implementation, namely that the container
ports are
> open.
Right you are, thanks
https:/
File juju/state/
https:/
juju/state/
fair enough, removed
On 2012/07/01 02:32:22, hazmat wrote:
> this test also seems superfluous. the only impl file being modified by
the
> branch is the invoker, yet we have tests in numerous state test files
verifying
> what has already been verified by other tests in those files
https:/
File juju/state/
https:/
juju/state/
On 2012/07/01 02:32:22, hazmat wrote:
> how is this entire test not entirely superfluous?
removed
Kapil Thangavelu (hazmat) wrote : | # |
can you merge/resolve with trunk, there's a conflict.
thanks,
-k
On Mon, Jul 2, 2012 at 6:31 PM, Benjamin Saller <email address hidden> wrote:
>
>
> https:/
> File juju/hooks/
>
>
> https:/
> juju/hooks/
> unit_state.
> On 2012/07/01 02:32:22, hazmat wrote:
> > This test never verifies the implementation, namely that the container
> ports are
> > open.
>
> Right you are, thanks
>
>
> https:/
> File juju/state/
>
>
> https:/
> juju/state/
> fair enough, removed
>
> On 2012/07/01 02:32:22, hazmat wrote:
> > this test also seems superfluous. the only impl file being modified by
> the
> > branch is the invoker, yet we have tests in numerous state test files
> verifying
> > what has already been verified by other tests in those files
>
>
> https:/
> File juju/state/
>
>
> https:/
> juju/state/
> On 2012/07/01 02:32:22, hazmat wrote:
> > how is this entire test not entirely superfluous?
>
> removed
>
> https:/
>
> --
> https:/
> You are subscribed to branch lp:juju.
>
- 548. By Benjamin Saller
-
merge trunk
Kapil Thangavelu (hazmat) wrote : | # |
needs to be lbox reproposed when making a modification for inline changes..
just doing this one by hand...
nice catch re. test_invoker yield exe.ended
test_open_
- there are four open port calls and one close here, please ditch two of
the open ports, its repetitive, executing a process (slow), and not adding
any value.
setup_subordina
it still has all the unesc. machine stuff. these sort of methods (excessive
default setup) cause slow tests, ie. almost every slow unit test we have
uses the status base setup even then though most check only a fraction of
it.
LGTM, with the above minors please merge.
-k
On Mon, Jul 2, 2012 at 7:34 PM, Kapil Thangavelu <
<email address hidden>> wrote:
> can you merge/resolve with trunk, there's a conflict.
> thanks,
> -k
>
> On Mon, Jul 2, 2012 at 6:31 PM, Benjamin Saller <email address hidden>
> wrote:
>
> >
> >
> >
> https:/
> > File juju/hooks/
> >
> >
> >
> https:/
> > juju/hooks/
> > unit_state.
> > On 2012/07/01 02:32:22, hazmat wrote:
> > > This test never verifies the implementation, namely that the container
> > ports are
> > > open.
> >
> > Right you are, thanks
> >
> >
> >
> https:/
> > File juju/state/
> >
> >
> >
> https:/
> > juju/state/
> > fair enough, removed
> >
> > On 2012/07/01 02:32:22, hazmat wrote:
> > > this test also seems superfluous. the only impl file being modified by
> > the
> > > branch is the invoker, yet we have tests in numerous state test files
> > verifying
> > > what has already been verified by other tests in those files
> >
> >
> >
> https:/
> > File juju/state/
> >
> >
> >
> https:/
> > juju/state/
> > On 2012/07/01 02:32:22, hazmat wrote:
> > > how is this entire test not entirely superfluous?
> >
> > removed
> >
> > https:/
> >
> > --
> >
> https:/
> > You are subscribed to branch lp:juju.
> >
>
> --
> https:/
> You are subscribed to branch lp:juju.
>
Preview Diff
1 | === added file 'examples/precise/recorder/hooks/install' |
2 | --- examples/precise/recorder/hooks/install 1970-01-01 00:00:00 +0000 |
3 | +++ examples/precise/recorder/hooks/install 2012-07-03 00:41:21 +0000 |
4 | @@ -0,0 +1,3 @@ |
5 | +#!/bin/bash |
6 | +# for testing |
7 | +open-port 8080 |
8 | \ No newline at end of file |
9 | |
10 | === modified file 'juju/charm/tests/repository/series/logging/hooks/install' (properties changed: -x to +x) |
11 | === modified file 'juju/hooks/protocol.py' |
12 | --- juju/hooks/protocol.py 2012-06-14 14:33:22 +0000 |
13 | +++ juju/hooks/protocol.py 2012-07-03 00:41:21 +0000 |
14 | @@ -328,6 +328,14 @@ |
15 | """ |
16 | context = self.factory.get_context(client_id) |
17 | service_unit_state = yield context.get_local_unit_state() |
18 | + container = yield service_unit_state.get_container() |
19 | + if container: |
20 | + # also open the port on the container |
21 | + # we still do subordinate unit below to ease |
22 | + # status reporting |
23 | + yield container.open_port(port, proto) |
24 | + |
25 | + |
26 | yield service_unit_state.open_port(port, proto) |
27 | yield self.factory.log(logging.DEBUG, "opened %s/%s" % (port, proto)) |
28 | defer.returnValue({}) |
29 | @@ -348,6 +356,13 @@ |
30 | """ |
31 | context = self.factory.get_context(client_id) |
32 | service_unit_state = yield context.get_local_unit_state() |
33 | + container = yield service_unit_state.get_container() |
34 | + if container: |
35 | + # also close the port on the container |
36 | + # we still do subordinate unit below to ease |
37 | + # status reporting |
38 | + yield container.close_port(port, proto) |
39 | + |
40 | yield service_unit_state.close_port(port, proto) |
41 | yield self.factory.log(logging.DEBUG, "closed %s/%s" % (port, proto)) |
42 | defer.returnValue({}) |
43 | |
44 | === modified file 'juju/hooks/tests/test_communications.py' |
45 | --- juju/hooks/tests/test_communications.py 2012-06-06 23:00:51 +0000 |
46 | +++ juju/hooks/tests/test_communications.py 2012-07-03 00:41:21 +0000 |
47 | @@ -54,6 +54,9 @@ |
48 | self.ports = set() |
49 | self.config = {} |
50 | |
51 | + def get_container(self): |
52 | + return None |
53 | + |
54 | def open_port(self, port, proto): |
55 | self.ports.add((port, proto)) |
56 | |
57 | |
58 | === modified file 'juju/hooks/tests/test_invoker.py' |
59 | --- juju/hooks/tests/test_invoker.py 2012-07-01 18:38:54 +0000 |
60 | +++ juju/hooks/tests/test_invoker.py 2012-07-03 00:41:21 +0000 |
61 | @@ -1477,13 +1477,15 @@ |
62 | |
63 | mu1, mu2 = my_units |
64 | lu1, lu2 = log_units |
65 | + self.mysql_units = my_units |
66 | + self.log_units = log_units |
67 | |
68 | mystate = pick_attr(service_states, relation_role="server") |
69 | logstate = pick_attr(service_states, relation_role="client") |
70 | |
71 | yield mystate.add_unit_state(mu1) |
72 | self.relation = yield logstate.add_unit_state(lu1) |
73 | - # add the second container ( |
74 | + # add the second container |
75 | yield mystate.add_unit_state(mu2) |
76 | self.relation2 = yield logstate.add_unit_state(lu2) |
77 | |
78 | @@ -1509,11 +1511,92 @@ |
79 | "--format=smart")) |
80 | self.assertEqual(result, 0) |
81 | # verify that we see the proper unit |
82 | + yield exe.ended |
83 | self.assertIn("mysql/1", self.log.getvalue()) |
84 | # we don't see units in the other container |
85 | self.assertNotIn("mysql/0", self.log.getvalue()) |
86 | |
87 | |
88 | + @defer.inlineCallbacks |
89 | + def test_open_and_close_ports(self): |
90 | + """Verify that port hook commands run and changes are immediate.""" |
91 | + unit_state = self.log_units[0] |
92 | + container_state = self.mysql_units[0] |
93 | + |
94 | + self.assertEqual((yield unit_state.get_open_ports()), []) |
95 | + |
96 | + exe = yield self.ua.get_invoker( |
97 | + "database:42", "add", "logging/0", self.relation) |
98 | + result = yield exe(self.create_hook("open-port", "80")) |
99 | + self.assertEqual(result, 0) |
100 | + self.assertEqual( |
101 | + (yield unit_state.get_open_ports()), |
102 | + [{"port": 80, "proto": "tcp"}]) |
103 | + self.assertEqual( |
104 | + (yield container_state.get_open_ports()), |
105 | + [{"port": 80, "proto": "tcp"}]) |
106 | + |
107 | + result = yield exe(self.create_hook("open-port", "53/udp")) |
108 | + self.assertEqual(result, 0) |
109 | + self.assertEqual( |
110 | + (yield unit_state.get_open_ports()), |
111 | + [{"port": 80, "proto": "tcp"}, |
112 | + {"port": 53, "proto": "udp"}]) |
113 | + self.assertEqual( |
114 | + (yield container_state.get_open_ports()), |
115 | + [{"port": 80, "proto": "tcp"}, |
116 | + {"port": 53, "proto": "udp"}]) |
117 | + |
118 | + result = yield exe(self.create_hook("open-port", "53/tcp")) |
119 | + self.assertEqual(result, 0) |
120 | + self.assertEqual( |
121 | + (yield unit_state.get_open_ports()), |
122 | + [{"port": 80, "proto": "tcp"}, |
123 | + {"port": 53, "proto": "udp"}, |
124 | + {"port": 53, "proto": "tcp"}]) |
125 | + self.assertEqual( |
126 | + (yield container_state.get_open_ports()), |
127 | + [{"port": 80, "proto": "tcp"}, |
128 | + {"port": 53, "proto": "udp"}, |
129 | + {"port": 53, "proto": "tcp"}]) |
130 | + |
131 | + result = yield exe(self.create_hook("open-port", "443/tcp")) |
132 | + self.assertEqual(result, 0) |
133 | + self.assertEqual( |
134 | + (yield unit_state.get_open_ports()), |
135 | + [{"port": 80, "proto": "tcp"}, |
136 | + {"port": 53, "proto": "udp"}, |
137 | + {"port": 53, "proto": "tcp"}, |
138 | + {"port": 443, "proto": "tcp"}]) |
139 | + self.assertEqual( |
140 | + (yield container_state.get_open_ports()), |
141 | + [{"port": 80, "proto": "tcp"}, |
142 | + {"port": 53, "proto": "udp"}, |
143 | + {"port": 53, "proto": "tcp"}, |
144 | + {"port": 443, "proto": "tcp"}]) |
145 | + |
146 | + result = yield exe(self.create_hook("close-port", "80/tcp")) |
147 | + self.assertEqual(result, 0) |
148 | + self.assertEqual( |
149 | + (yield unit_state.get_open_ports()), |
150 | + [{"port": 53, "proto": "udp"}, |
151 | + {"port": 53, "proto": "tcp"}, |
152 | + {"port": 443, "proto": "tcp"}]) |
153 | + self.assertEqual( |
154 | + (yield container_state.get_open_ports()), |
155 | + [{"port": 53, "proto": "udp"}, |
156 | + {"port": 53, "proto": "tcp"}, |
157 | + {"port": 443, "proto": "tcp"}]) |
158 | + |
159 | + yield exe.ended |
160 | + self.assertLogLines( |
161 | + self.log.getvalue(), [ |
162 | + "opened 80/tcp", |
163 | + "opened 53/udp", |
164 | + "opened 443/tcp", |
165 | + "closed 80/tcp"]) |
166 | + |
167 | + |
168 | def capture_separate_log(name, level): |
169 | """Support the separate capture of logging at different log levels. |
170 | |
171 | |
172 | === modified file 'juju/state/tests/test_service.py' |
173 | --- juju/state/tests/test_service.py 2012-04-11 19:38:41 +0000 |
174 | +++ juju/state/tests/test_service.py 2012-07-03 00:41:21 +0000 |
175 | @@ -159,6 +159,43 @@ |
176 | unit_state = yield service_state.add_unit_state() |
177 | returnValue(unit_state) |
178 | |
179 | + @inlineCallbacks |
180 | + def setup_subordinate_defaults(self): |
181 | + mysql_ep = RelationEndpoint("mysql", "juju-info", "juju-info", |
182 | + "server", "global") |
183 | + logging_ep = RelationEndpoint("logging", "juju-info", "juju-info", |
184 | + "client", "container") |
185 | + |
186 | + logging_charm_state = yield self.get_subordinate_charm() |
187 | + self.assertTrue(logging_charm_state.is_subordinate()) |
188 | + log_state = yield self.service_state_manager.add_service_state( |
189 | + "logging", logging_charm_state, dummy_constraints) |
190 | + mysql_state = yield self.service_state_manager.add_service_state( |
191 | + "mysql", self.charm_state, dummy_constraints) |
192 | + |
193 | + m1 = yield self.machine_state_manager.add_machine_state( |
194 | + series_constraints) |
195 | + m2 = yield self.machine_state_manager.add_machine_state( |
196 | + series_constraints) |
197 | + |
198 | + relation_state, service_states = (yield |
199 | + self.relation_state_manager.add_relation_state( |
200 | + mysql_ep, logging_ep)) |
201 | + |
202 | + unit_state1 = yield mysql_state.add_unit_state() |
203 | + yield unit_state1.assign_to_machine(m1) |
204 | + unit_state0 = yield log_state.add_unit_state( |
205 | + container=unit_state1) |
206 | + |
207 | + unit_state3 = yield mysql_state.add_unit_state() |
208 | + yield unit_state3.assign_to_machine(m2) |
209 | + unit_state2 = yield log_state.add_unit_state( |
210 | + container=unit_state3) |
211 | + |
212 | + returnValue([service_states, relation_state, |
213 | + [(unit_state0, unit_state1), |
214 | + (unit_state2, unit_state3)]]) |
215 | + |
216 | |
217 | class ServiceStateManagerTest(ServiceStateManagerTestBase): |
218 |
Reviewers: mp+111534_ code.launchpad. net,
Message:
Please take a look.
Description:
Subordinate port open/close support
Subordinates will open ports in their container. They will report open
ports on the container and in the subordinate.
https:/ /code.launchpad .net/~bcsaller/ juju/subordinat e-ports/ +merge/ 111534
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/6324045/
Affected files: precise/ recorder/ hooks/install tests/repositor y/series/ logging/ hooks/install protocol. py tests/test_ communications. py tests/test_ invoker. py tests/test_ firewall. py tests/test_ service. py
A [revision details]
A examples/
M juju/charm/
M juju/hooks/
M juju/hooks/
M juju/hooks/
M juju/state/
M juju/state/