Merge lp:~bcsaller/pyjuju/subordinate-ports into lp:pyjuju

Proposed by Benjamin Saller
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
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+111534@code.launchpad.net

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.

https://codereview.appspot.com/6324045/

To post a comment you must log in.
Revision history for this message
Benjamin Saller (bcsaller) wrote :

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/subordinate-ports/+merge/111534

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6324045/

Affected files:
   A [revision details]
   A examples/precise/recorder/hooks/install
   M juju/charm/tests/repository/series/logging/hooks/install
   M juju/hooks/protocol.py
   M juju/hooks/tests/test_communications.py
   M juju/hooks/tests/test_invoker.py
   M juju/state/tests/test_firewall.py
   M juju/state/tests/test_service.py

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

is it nesc/valuable to record the open port against both the subordinate and the primary

Revision history for this message
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://code.launchpad.net/~bcsaller/juju/subordinate-ports/+merge/111534
> You are the owner of lp:~bcsaller/juju/subordinate-ports.

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

while the implementation LGTM, the tests need fixing.

https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py#newcode37
juju/hooks/tests/test_invoker.py:37: self._invokers = {} # client_id ->
Invoker
pep8 need an extra space before comment

https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py#newcode1519
juju/hooks/tests/test_invoker.py:1519: (yield
unit_state.get_open_ports()),
This test never verifies the implementation, namely that the container
ports are open.

https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py#newcode1560
juju/hooks/tests/test_invoker.py:1560: "closed 80/tcp"])
pep8 need an extra blank line after this test

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_firewall.py
File juju/state/tests/test_firewall.py (right):

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_firewall.py#newcode192
juju/state/tests/test_firewall.py:192: """
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://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py
File juju/state/tests/test_service.py (right):

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py#newcode176
juju/state/tests/test_service.py:176: m1 = yield
self.machine_state_manager.add_machine_state(
machine creation and assignment is superflous

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py#newcode2771
juju/state/tests/test_service.py:2771: def
test_get_open_ports_subordiate(self):
typo in test name

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py#newcode2810
juju/state/tests/test_service.py:2810: self.assertEqual(
how is this entire test not entirely superfluous?

https://codereview.appspot.com/6324045/

Revision history for this message
Benjamin Saller (bcsaller) wrote :

https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py#newcode1519
juju/hooks/tests/test_invoker.py:1519: (yield
unit_state.get_open_ports()),
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://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_firewall.py
File juju/state/tests/test_firewall.py (right):

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_firewall.py#newcode192
juju/state/tests/test_firewall.py:192: """
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://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py
File juju/state/tests/test_service.py (right):

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py#newcode2810
juju/state/tests/test_service.py:2810: self.assertEqual(
On 2012/07/01 02:32:22, hazmat wrote:
> how is this entire test not entirely superfluous?

removed

https://codereview.appspot.com/6324045/

Revision history for this message
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://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py
> File juju/hooks/tests/test_invoker.py (right):
>
>
> https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py#newcode1519
> juju/hooks/tests/test_invoker.py:1519: (yield
> unit_state.get_open_ports()),
> 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://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_firewall.py
> File juju/state/tests/test_firewall.py (right):
>
>
> https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_firewall.py#newcode192
> juju/state/tests/test_firewall.py:192: """
> 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://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py
> File juju/state/tests/test_service.py (right):
>
>
> https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py#newcode2810
> juju/state/tests/test_service.py:2810: self.assertEqual(
> On 2012/07/01 02:32:22, hazmat wrote:
> > how is this entire test not entirely superfluous?
>
> removed
>
> https://codereview.appspot.com/6324045/
>
> --
> https://code.launchpad.net/~bcsaller/juju/subordinate-ports/+merge/111534
> You are subscribed to branch lp:juju.
>

548. By Benjamin Saller

merge trunk

Revision history for this message
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_and_close_ports

- 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_subordinate_defaults isn't used and should get yanked, additionally
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://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py
> > File juju/hooks/tests/test_invoker.py (right):
> >
> >
> >
> https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py#newcode1519
> > juju/hooks/tests/test_invoker.py:1519: (yield
> > unit_state.get_open_ports()),
> > 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://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_firewall.py
> > File juju/state/tests/test_firewall.py (right):
> >
> >
> >
> https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_firewall.py#newcode192
> > juju/state/tests/test_firewall.py:192: """
> > 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://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py
> > File juju/state/tests/test_service.py (right):
> >
> >
> >
> https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py#newcode2810
> > juju/state/tests/test_service.py:2810: self.assertEqual(
> > On 2012/07/01 02:32:22, hazmat wrote:
> > > how is this entire test not entirely superfluous?
> >
> > removed
> >
> > https://codereview.appspot.com/6324045/
> >
> > --
> >
> https://code.launchpad.net/~bcsaller/juju/subordinate-ports/+merge/111534
> > You are subscribed to branch lp:juju.
> >
>
> --
> https://code.launchpad.net/~bcsaller/juju/subordinate-ports/+merge/111534
> You are subscribed to branch lp:juju.
>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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

Subscribers

People subscribed via source and target branches

to status/vote changes: