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

[Benjamin Saller]

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/

[Benjamin Saller (bcsaller)]

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

[Kapil Thangavelu (hazmat)]

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

[Benjamin Saller (bcsaller)]

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.

[Kapil Thangavelu (hazmat)]

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/

[Benjamin Saller (bcsaller)]

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/

[Kapil Thangavelu (hazmat)]

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.
>

[lp:~bcsaller/pyjuju/subordinate-ports]

548. By Benjamin Saller on 2012-07-03

merge trunk

[Kapil Thangavelu (hazmat)]

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.
>

=== added file 'examples/precise/recorder/hooks/install'
--- examples/precise/recorder/hooks/install	1970-01-01 00:00:00 +0000
+++ examples/precise/recorder/hooks/install	2012-07-03 00:41:21 +0000
@@ -0,0 +1,3 @@
+#!/bin/bash
+# for testing
+open-port 8080
 \ No newline at end of file
 
=== modified file 'juju/charm/tests/repository/series/logging/hooks/install' (properties changed: -x to +x)
=== modified file 'juju/hooks/protocol.py'
--- juju/hooks/protocol.py	2012-06-14 14:33:22 +0000
+++ juju/hooks/protocol.py	2012-07-03 00:41:21 +0000
@@ -328,6 +328,14 @@
         """
         context = self.factory.get_context(client_id)
         service_unit_state = yield context.get_local_unit_state()
+        container = yield service_unit_state.get_container()
+        if container:
+            # also open the port on the container
+            # we still do subordinate unit below to ease
+            # status reporting
+            yield container.open_port(port, proto)
+
+
         yield service_unit_state.open_port(port, proto)
         yield self.factory.log(logging.DEBUG, "opened %s/%s" % (port, proto))
         defer.returnValue({})
@@ -348,6 +356,13 @@
         """
         context = self.factory.get_context(client_id)
         service_unit_state = yield context.get_local_unit_state()
+        container = yield service_unit_state.get_container()
+        if container:
+            # also close the port on the container
+            # we still do subordinate unit below to ease
+            # status reporting
+            yield container.close_port(port, proto)
+
         yield service_unit_state.close_port(port, proto)
         yield self.factory.log(logging.DEBUG, "closed %s/%s" % (port, proto))
         defer.returnValue({})
 
=== modified file 'juju/hooks/tests/test_communications.py'
--- juju/hooks/tests/test_communications.py	2012-06-06 23:00:51 +0000
+++ juju/hooks/tests/test_communications.py	2012-07-03 00:41:21 +0000
@@ -54,6 +54,9 @@
         self.ports = set()
         self.config = {}
 
+    def get_container(self):
+        return None
+
     def open_port(self, port, proto):
         self.ports.add((port, proto))
 
 
=== modified file 'juju/hooks/tests/test_invoker.py'
--- juju/hooks/tests/test_invoker.py	2012-07-01 18:38:54 +0000
+++ juju/hooks/tests/test_invoker.py	2012-07-03 00:41:21 +0000
@@ -1477,13 +1477,15 @@
 
         mu1, mu2 = my_units
         lu1, lu2 = log_units
+        self.mysql_units = my_units
+        self.log_units = log_units
 
         mystate = pick_attr(service_states, relation_role="server")
         logstate = pick_attr(service_states, relation_role="client")
 
         yield mystate.add_unit_state(mu1)
         self.relation = yield logstate.add_unit_state(lu1)
-        # add the second container (
+        # add the second container
         yield mystate.add_unit_state(mu2)
         self.relation2 = yield logstate.add_unit_state(lu2)
 
@@ -1509,11 +1511,92 @@
                                             "--format=smart"))
         self.assertEqual(result, 0)
         # verify that we see the proper unit
+        yield exe.ended
         self.assertIn("mysql/1", self.log.getvalue())
         # we don't see units in the other container
         self.assertNotIn("mysql/0", self.log.getvalue())
 
 
+    @defer.inlineCallbacks
+    def test_open_and_close_ports(self):
+        """Verify that port hook commands run and changes are immediate."""
+        unit_state = self.log_units[0]
+        container_state = self.mysql_units[0]
+
+        self.assertEqual((yield unit_state.get_open_ports()), [])
+
+        exe = yield self.ua.get_invoker(
+            "database:42", "add", "logging/0", self.relation)
+        result = yield exe(self.create_hook("open-port", "80"))
+        self.assertEqual(result, 0)
+        self.assertEqual(
+            (yield unit_state.get_open_ports()),
+            [{"port": 80, "proto": "tcp"}])
+        self.assertEqual(
+            (yield container_state.get_open_ports()),
+            [{"port": 80, "proto": "tcp"}])
+
+        result = yield exe(self.create_hook("open-port", "53/udp"))
+        self.assertEqual(result, 0)
+        self.assertEqual(
+            (yield unit_state.get_open_ports()),
+            [{"port": 80, "proto": "tcp"},
+             {"port": 53, "proto": "udp"}])
+        self.assertEqual(
+            (yield container_state.get_open_ports()),
+            [{"port": 80, "proto": "tcp"},
+             {"port": 53, "proto": "udp"}])
+
+        result = yield exe(self.create_hook("open-port", "53/tcp"))
+        self.assertEqual(result, 0)
+        self.assertEqual(
+            (yield unit_state.get_open_ports()),
+            [{"port": 80, "proto": "tcp"},
+             {"port": 53, "proto": "udp"},
+             {"port": 53, "proto": "tcp"}])
+        self.assertEqual(
+            (yield container_state.get_open_ports()),
+            [{"port": 80, "proto": "tcp"},
+             {"port": 53, "proto": "udp"},
+             {"port": 53, "proto": "tcp"}])
+
+        result = yield exe(self.create_hook("open-port", "443/tcp"))
+        self.assertEqual(result, 0)
+        self.assertEqual(
+            (yield unit_state.get_open_ports()),
+            [{"port": 80, "proto": "tcp"},
+             {"port": 53, "proto": "udp"},
+             {"port": 53, "proto": "tcp"},
+             {"port": 443, "proto": "tcp"}])
+        self.assertEqual(
+            (yield container_state.get_open_ports()),
+            [{"port": 80, "proto": "tcp"},
+             {"port": 53, "proto": "udp"},
+             {"port": 53, "proto": "tcp"},
+             {"port": 443, "proto": "tcp"}])
+
+        result = yield exe(self.create_hook("close-port", "80/tcp"))
+        self.assertEqual(result, 0)
+        self.assertEqual(
+            (yield unit_state.get_open_ports()),
+            [{"port": 53, "proto": "udp"},
+             {"port": 53, "proto": "tcp"},
+             {"port": 443, "proto": "tcp"}])
+        self.assertEqual(
+            (yield container_state.get_open_ports()),
+            [{"port": 53, "proto": "udp"},
+             {"port": 53, "proto": "tcp"},
+             {"port": 443, "proto": "tcp"}])
+
+        yield exe.ended
+        self.assertLogLines(
+            self.log.getvalue(), [
+                "opened 80/tcp",
+                "opened 53/udp",
+                "opened 443/tcp",
+                "closed 80/tcp"])
+
+
 def capture_separate_log(name, level):
     """Support the separate capture of logging at different log levels.
 
 
=== modified file 'juju/state/tests/test_service.py'
--- juju/state/tests/test_service.py	2012-04-11 19:38:41 +0000
+++ juju/state/tests/test_service.py	2012-07-03 00:41:21 +0000
@@ -159,6 +159,43 @@
         unit_state = yield service_state.add_unit_state()
         returnValue(unit_state)
 
+    @inlineCallbacks
+    def setup_subordinate_defaults(self):
+        mysql_ep = RelationEndpoint("mysql", "juju-info", "juju-info",
+                                    "server", "global")
+        logging_ep = RelationEndpoint("logging", "juju-info", "juju-info",
+                                      "client", "container")
+
+        logging_charm_state = yield self.get_subordinate_charm()
+        self.assertTrue(logging_charm_state.is_subordinate())
+        log_state = yield self.service_state_manager.add_service_state(
+                "logging", logging_charm_state, dummy_constraints)
+        mysql_state = yield self.service_state_manager.add_service_state(
+                "mysql", self.charm_state, dummy_constraints)
+
+        m1 = yield self.machine_state_manager.add_machine_state(
+            series_constraints)
+        m2 = yield self.machine_state_manager.add_machine_state(
+            series_constraints)
+
+        relation_state, service_states = (yield
+                self.relation_state_manager.add_relation_state(
+                        mysql_ep, logging_ep))
+
+        unit_state1 = yield mysql_state.add_unit_state()
+        yield unit_state1.assign_to_machine(m1)
+        unit_state0 = yield log_state.add_unit_state(
+            container=unit_state1)
+
+        unit_state3 = yield mysql_state.add_unit_state()
+        yield unit_state3.assign_to_machine(m2)
+        unit_state2 = yield log_state.add_unit_state(
+            container=unit_state3)
+
+        returnValue([service_states, relation_state,
+                     [(unit_state0, unit_state1),
+                      (unit_state2, unit_state3)]])
+
 
 class ServiceStateManagerTest(ServiceStateManagerTestBase):