List all machine IP addrs in YAML and JSON output

[macgreagoir]

When status or show-machine are run with the YAML or JSON format
options, all IP addresses a machine has will be shown.

Fixes lp:1602840

QA steps:

• Add a second IP address to a deployed machne
  • The easiest is probably to sudo ip a a <cidr> dev <dev name> using an existing device
  • The examples below are taken from a machine with two NICs and addresses on two different spaces/subnets
• juju show-machine <machine id> [--format=yaml] should display something similar to this:

$ juju show-machine 0
model: controller
machines:
  "0":
    juju-status:
      current: started
      since: 11 Oct 2016 09:15:07Z
      version: 2.0.0.1
    dns-name: 192.168.124.200
    ip-addresses:
    - 192.168.124.200
    - 192.168.123.40
    instance-id: 4y3haf
    machine-status:
      current: running
      message: Deployed
      since: 11 Oct 2016 07:56:02Z
    series: xenial
    hardware: arch=amd64 cores=2 mem=1024M tags=virtual availability-zone=default
    controller-member-status: has-vote

• juju show-machine <machine id> --format=tabular should not show the list of IP addresses, only the 'DNS' name/address:

$ juju show-machine 0 --format=tabular
Machine  State    DNS              Inst id  Series  AZ
0        started  192.168.124.200  4y3haf   xenial  default

• juju status should show similar output for each deployed machine

[macgreagoir]

AWS example: https://pastebin.canonical.com/167521/

[macgreagoir]

GCE example: https://pastebin.canonical.com/167528/

[dimitern]

I didn't manage to review this in detail, but I have some comments that I think need to be addressed. I'll add comments tomorrow, please don't rush to land this yet.

[frobware]

I didn't see a test case for this.

[macgreagoir]

Done

[frobware]

I'm confused by the names we use. If it is address then why aren't they IP addresses? Similarly, if we don't want addresses then let's use hostnames.

[macgreagoir]

Fair point, they are the same strings used in the rest of this test suite in place of address strings, but they can be changed.

[dimitern]

+1; I'd also suggest adding tests with full IPv6 addresses in the range reserved for documentation purposes (2001:db8::/32).

[macgreagoir]

Done, migrated tests to IP addresses and added an IPv6 test.

[frobware]

I was looking at the output format: shouldn't we indent the list of ip-addresses:

ip-addresses:
  - 10.77.58.116

[macgreagoir]

Just noting (after chat with babbageclunk) the indentation as standard in go-yaml:
https://github.com/go-yaml/yaml/blame/v2/README.md#L127

[dimitern]

LGTM with a couple of suggestions.

[dimitern]

This loop is in quite similar to what network.FilterUnusableHostPorts() does, unfortunately not for []network.Address.
Nevertheless, please use the same approach here - switch on mAddr.Scope and filter ScopeMachineLocal and ScopeLinkLocal. This will cover the full loopback ranges 127/8 and ::1/128 on both IPv4 and IPv6 addresses, and will also not show the linkewise-unusable link local addresses.

[macgreagoir]

Done

[frobware]

I'm confused by the names we use. If it is address then why aren't they IP addresses? Similarly, if we don't want addresses then let's use hostnames.

[macgreagoir]

Fair point, they are the same strings used in the rest of this test suite in place of address strings, but they can be changed.

[dimitern]

+1; I'd also suggest adding tests with full IPv6 addresses in the range reserved for documentation purposes (2001:db8::/32).

[macgreagoir]

Done, migrated tests to IP addresses and added an IPv6 test.

[dimitern]

You could drop the default and do the append after the switch:

for _, mAddr := range mAddrs {
  switch mAddr.Scope {
    case network.ScopeMachineLocal, network.ScopeLinkLocal:
      continue
  }
  status.IPAddresses = append(status.IPAddresses, mAddr.Value)
}

[macgreagoir]

Done

[kat-co]

Please add comments to these (and existing) fields.

[macgreagoir]

OK, some added. Hopefully some are even accurate :-)

[macgreagoir]

As discussed in IRC, we are adding another field to the struct, but not changing any existing.

[macgreagoir]

$$merge$$

[jujubot]

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju