Merge lp:~axwalk/juju-core/lp1173093-add-opened-ports-status into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

Add open-ports to unit status

Also, update instance.Port's String method to
format as number/protocol, so we get the same
format as in pyjuju.

e.g.:
        mongodb/0:
          agent-state: error
          agent-state-info: 'hook failed: "start"'
          agent-version: 1.11.0
          machine: "1"
          open-ports:
          - 27017/tcp
          - 27019/tcp
          - 27021/tcp
          - 28017/tcp

https://codereview.appspot.com/11679044/

Add open-ports to unit status

Also, update instance.Port's String method to
format as number/protocol, so we get the same
format as in pyjuju.

e.g.:
        mongodb/0:
          agent-state: error
          agent-state-info: 'hook failed: "start"'
          agent-version: 1.11.0
          machine: "1"
          open-ports:
          - 27017/tcp
          - 27019/tcp
          - 27021/tcp
          - 28017/tcp

https://codereview.appspot.com/11679044/

[Andrew Wilkins (axwalk)]

Reviewers: mp+176327_code.launchpad.net,

Message:
Please take a look.

Description:
Add open-ports to unit status

e.g.:
         mongodb/0:
           agent-state: error
           agent-state-info: 'hook failed: "start"'
           agent-version: 1.11.0
           machine: "1"
           open-ports:
           - tcp:27017
           - tcp:27019
           - tcp:27021
           - tcp:28017

https://code.launchpad.net/~axwalk/juju-core/lp1173093-add-opened-ports-status/+merge/176327

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/11679044/
Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130723031013-rutwe89s8u7anok1
+New revision: <email address hidden>

Index: cmd/juju/status.go
=== modified file 'cmd/juju/status.go'
--- cmd/juju/status.go 2013-06-30 23:42:39 +0000
+++ cmd/juju/status.go 2013-07-23 06:33:51 +0000
@@ -255,6 +255,9 @@

  func (context *statusContext) processUnit(unit *state.Unit) (status
unitStatus) {
   status.PublicAddress, _ = unit.PublicAddress()
+ for _, port := range unit.OpenedPorts() {
+ status.OpenedPorts = append(status.OpenedPorts, port.String())
+ }
   if unit.IsPrincipal() {
    status.Machine, _ = unit.AssignedMachineId()
   }
@@ -439,6 +442,7 @@
   AgentVersion string `json:"agent-version,omitempty"
yaml:"agent-version,omitempty"`
   Life string `json:"life,omitempty"
yaml:"life,omitempty"`
   Machine string `json:"machine,omitempty"
yaml:"machine,omitempty"`
+ OpenedPorts []string `json:"open-ports,omitempty"
yaml:"open-ports,omitempty"`
   PublicAddress string `json:"public-address,omitempty"
yaml:"public-address,omitempty"`
   Subordinates map[string]unitStatus `json:"subordinates,omitempty"
yaml:"subordinates,omitempty"`
  }

Index: cmd/juju/status_test.go
=== modified file 'cmd/juju/status_test.go'
--- cmd/juju/status_test.go 2013-07-17 10:30:12 +0000
+++ cmd/juju/status_test.go 2013-07-23 06:33:51 +0000
@@ -368,6 +368,12 @@
    addUnit{"dummy-service", "1"},
    addAliveUnit{"exposed-service", "2"},
    setUnitStatus{"exposed-service/0", params.StatusError, "You Require More
Vespene Gas"},
+ // Open multiple ports with different protocols,
+ // ensure they're sorted on protocol, then number.
+ openUnitPort{"exposed-service/0", "udp", 10},
+ openUnitPort{"exposed-service/0", "udp", 2},
+ openUnitPort{"exposed-service/0", "tcp", 3},
+ openUnitPort{"exposed-service/0", "tcp", 2},
    // Simulate some status with no info, while the agent is down.
    setUnitStatus{"dummy-service/0", params.StatusStarted, ""},
    expect{
@@ -387,6 +393,9 @@
          "machine": "2",
          "agent-state": "error",
          "agent-state-info": "You Require More Vespene Gas",
+ "open-ports": L{
+ "tcp:2", "tcp:3", "udp:2", "udp:10",
+ },
         },
        },
       },
@@ -453,6 +462,9 @@
          "machine": "2",
   ...

[Martin Packman (gz)]

The reitveld diff is borked, so going by the launchpad one.

Addition to status looks good overall, and the added testing is fine.

There does seem to be a minor format change from python (which I thought
I'd seen discussed somewhere?) in that colon replaces slash as the
seperator. If we want to keep that, we should at least document it as a
known change, as it has the potential to break scripts.

https://codereview.appspot.com/11679044/

[Roger Peppe (rogpeppe)]

On 2013/07/23 10:50:40, gz wrote:
> The reitveld diff is borked, so going by the launchpad one.

for the record, you can usually un-bork it by re-proposing.

> There does seem to be a minor format change from python (which I
thought I'd
> seen discussed somewhere?) in that colon replaces slash as the
seperator. If we
> want to keep that, we should at least document it as a known change,
as it has
> the potential to break scripts.

i think we should keep the python format.
there's ancient precedent for tcp/23 (/etc/services) and no
particular reason to change the format AFAICS.

i'd be tempted to change state.Port.String to print the slash;
i suspect that nothing else will need to change.

https://codereview.appspot.com/11679044/

[Roger Peppe (rogpeppe)]

On 2013/07/23 11:07:08, rog wrote:
> On 2013/07/23 10:50:40, gz wrote:
> > The reitveld diff is borked, so going by the launchpad one.

> for the record, you can usually un-bork it by re-proposing.

> > There does seem to be a minor format change from python (which I
thought I'd
> > seen discussed somewhere?) in that colon replaces slash as the
seperator. If
> we
> > want to keep that, we should at least document it as a known change,
as it has
> > the potential to break scripts.

> i think we should keep the python format.
> there's ancient precedent for tcp/23 (/etc/services) and no
> particular reason to change the format AFAICS.

> i'd be tempted to change state.Port.String to print the slash;
> i suspect that nothing else will need to change.

oh yes, LGTM with the format fixed.

https://codereview.appspot.com/11679044/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/11679044/

[Andrew Wilkins (axwalk)]

On 2013/07/24 03:07:49, axw wrote:
> Please take a look.

As the updated description says, I've modified instance.Port to format
ports as suggested. I pored over all uses of instance.Port, and couldn't
find any assumptions about its Stringer format. Tests pass.

https://codereview.appspot.com/11679044/

[Tim Penhey (thumper)]

LGTM

https://codereview.appspot.com/11679044/

=== modified file 'cmd/juju/status.go'
--- cmd/juju/status.go	2013-07-22 04:04:34 +0000
+++ cmd/juju/status.go	2013-07-24 03:06:27 +0000
@@ -258,6 +258,9 @@
 
 func (context *statusContext) processUnit(unit *state.Unit) (status unitStatus) {
 	status.PublicAddress, _ = unit.PublicAddress()
+	for _, port := range unit.OpenedPorts() {
+		status.OpenedPorts = append(status.OpenedPorts, port.String())
+	}
 	if unit.IsPrincipal() {
 		status.Machine, _ = unit.AssignedMachineId()
 	}
@@ -442,6 +445,7 @@
 	AgentVersion   string                `json:"agent-version,omitempty" yaml:"agent-version,omitempty"`
 	Life           string                `json:"life,omitempty" yaml:"life,omitempty"`
 	Machine        string                `json:"machine,omitempty" yaml:"machine,omitempty"`
+	OpenedPorts    []string              `json:"open-ports,omitempty" yaml:"open-ports,omitempty"`
 	PublicAddress  string                `json:"public-address,omitempty" yaml:"public-address,omitempty"`
 	Subordinates   map[string]unitStatus `json:"subordinates,omitempty" yaml:"subordinates,omitempty"`
 }

=== modified file 'cmd/juju/status_test.go'
--- cmd/juju/status_test.go	2013-07-22 04:23:33 +0000
+++ cmd/juju/status_test.go	2013-07-24 03:06:27 +0000
@@ -369,6 +369,12 @@
 		addUnit{"dummy-service", "1"},
 		addAliveUnit{"exposed-service", "2"},
 		setUnitStatus{"exposed-service/0", params.StatusError, "You Require More Vespene Gas"},
+		// Open multiple ports with different protocols,
+		// ensure they're sorted on protocol, then number.
+		openUnitPort{"exposed-service/0", "udp", 10},
+		openUnitPort{"exposed-service/0", "udp", 2},
+		openUnitPort{"exposed-service/0", "tcp", 3},
+		openUnitPort{"exposed-service/0", "tcp", 2},
 		// Simulate some status with no info, while the agent is down.
 		setUnitStatus{"dummy-service/0", params.StatusStarted, ""},
 		expect{
@@ -388,6 +394,9 @@
 								"machine":          "2",
 								"agent-state":      "error",
 								"agent-state-info": "You Require More Vespene Gas",
+								"open-ports": L{
+									"2/tcp", "3/tcp", "2/udp", "10/udp",
+								},
 							},
 						},
 					},
@@ -454,6 +463,9 @@
 								"machine":          "2",
 								"agent-state":      "error",
 								"agent-state-info": "You Require More Vespene Gas",
+								"open-ports": L{
+									"2/tcp", "3/tcp", "2/udp", "10/udp",
+								},
 							},
 						},
 					},
@@ -1052,6 +1064,19 @@
 	c.Assert(err, IsNil)
 }
 
+type openUnitPort struct {
+	unitName string
+	protocol string
+	number   int
+}
+
+func (oup openUnitPort) step(c *C, ctx *context) {
+	u, err := ctx.st.Unit(oup.unitName)
+	c.Assert(err, IsNil)
+	err = u.OpenPort(oup.protocol, oup.number)
+	c.Assert(err, IsNil)
+}
+
 type ensureDyingUnit struct {
 	unitName string
 }

=== modified file 'instance/instance.go'
--- instance/instance.go	2013-07-17 06:03:50 +0000
+++ instance/instance.go	2013-07-24 03:06:27 +0000
@@ -24,7 +24,7 @@
 }
 
 func (p Port) String() string {
-	return fmt.Sprintf("%s:%d", p.Protocol, p.Number)
+	return fmt.Sprintf("%d/%s", p.Number, p.Protocol)
 }
 
 // Instance represents the the realization of a machine in state.