Merge lp:~wallyworld/juju-core/watcher-address-issues into lp:juju-core/1.18

[Ian Booth]

Ensure unit addresses populated

Change the internals of the megawatcher to ensure
that machine addresses are correct and that the
public and private addresses for untis come from
the assigned machine.

Ensure unit addresses populated

Change the internals of the megawatcher to ensure
that machine addresses are correct and that the
public and private addresses for untis come from
the assigned machine.

[William Reade (fwereade)]

LGTM if frankban's happy as well.

[William Reade (fwereade)]

frankban makes a good point, that the comment in :8-9 is not correct -- we don't get them for the local provider, but we should do everywhere else, via instancepoller; and I think we probably *should* fix local.Instance to use fancy lxc-ls such that we *do* get provider addresses as well.

[Francesco Banconi (frankban)]

Hi Ian, this branch looks good with some comments below.

As I mentioned in the bug, not sure why LXC addresses have empty scopes.
I'll try to describe my concerns: API clients are often interested in the public address of a unit/machine.
When using cloud providers, fetching them is trivial:
  - just pick the first "ipv4" address with "public" NetworkScope.
When using local envs, currently is more tricky, and I am not sure about the logic: the scope is empty so,
the logic above might become:
  - try to find an "ipv4" address with "public" NetworkScope
  - if none is found, pick the first "ipv4" address with an empty scope, it must be the public one.
Is this correct? If so, we can surely make quickstart/GUI work like that. Anyway, it does not seem the best client story we can have.

8 + // As of 1.18, provider addresses are not gathered anymore so this field
9 + // will not be populated. It is retained for backwards compatibility.

This does not seem to reflect reality. AFAICT provider addresses are still used and populated for top level machines.

25 + addresses := make([]instance.Address, len(merged))
26 + for _, address := range merged {
27 + addresses = append(addresses, address)
28 + }

It seems we are creating the addresses with a specific length but then we append to it. This way we have len(merged) empty addresses at the beginning of the slice. I guess we should either assign by index or start with a 0 length slice.

37 +// Note: As of 1.18, provider addresses are no longer populated.

Same as above.

[John A Meinel (jameinel)]

(01:54:07 PM) jam: fwereade: I'm thinking we need james' patch to land on the 1.18 branch as well, right?
(01:54:54 PM) jam: frankban: I believe LXC has an empty scope because we can't tell whether it is public or private.
(01:54:58 PM) jam: It is just the address we got from eth0
(01:55:03 PM) jam: on Clouds that is actually a private address
(01:55:13 PM) jam: for LXC it is probably the "most public address we can get"
(01:55:24 PM) jam: though still since it is on LXCBR0 it is probably a private address.

[Ian Booth (wallyworld)]

> Hi Ian, this branch looks good with some comments below.
>
> As I mentioned in the bug, not sure why LXC addresses have empty scopes.
> I'll try to describe my concerns: API clients are often interested in the
> public address of a unit/machine.
> When using cloud providers, fetching them is trivial:
> - just pick the first "ipv4" address with "public" NetworkScope.
> When using local envs, currently is more tricky, and I am not sure about the
> logic: the scope is empty so,
> the logic above might become:
> - try to find an "ipv4" address with "public" NetworkScope
> - if none is found, pick the first "ipv4" address with an empty scope, it
> must be the public one.
> Is this correct? If so, we can surely make quickstart/GUI work like that.
> Anyway, it does not seem the best client story we can have.
>

The current logic to put public/private address on the unit item in the mega watcher calls Pubic/PrivateAddress() on state.Unit. These methods look at the addresses gathered by the instance poller and stored on the machhne and return the ones considered to be most public/private respectively. For the local provider, I'm not sure what the instance poller does, I'd have to check.

> 8 + // As of 1.18, provider addresses are not gathered anymore so
> this field
> 9 + // will not be populated. It is retained for backwards
> compatibility.
>
> This does not seem to reflect reality. AFAICT provider addresses are still
> used and populated for top level machines.
>

I think there's crossed wires here. My interpretation of "provider addresses" is the address getters on the provider itself and these have been removed. Maybe that's not what you mean. I'll remove the comment.

> 25 + addresses := make([]instance.Address, len(merged))
> 26 + for _, address := range merged {
> 27 + addresses = append(addresses, address)
> 28 + }
>
> It seems we are creating the addresses with a specific length but then we
> append to it. This way we have len(merged) empty addresses at the beginning of
> the slice. I guess we should either assign by index or start with a 0 length
> slice.
>

Fixed.

> 37 +// Note: As of 1.18, provider addresses are no longer populated.
>
> Same as above.

Removed comment.

[Francesco Banconi (frankban)]

LGTM, thank you!

review: Approve

[Ian Booth (wallyworld)]

FWIW, I deployed wordpress and juju-gui using local provider and the gui correctly showed the public addresses of both units (wordpress and juju-gui)

=== modified file 'state/machine.go'
--- state/machine.go	2014-03-26 11:34:48 +0000
+++ state/machine.go	2014-04-03 10:11:00 +0000
@@ -813,23 +813,28 @@
 	return ok
 }
 
+func mergedAddresses(machineAddresses, providerAddresses []address) []instance.Address {
+	merged := make(map[string]instance.Address)
+	for _, address := range machineAddresses {
+		merged[address.Value] = address.InstanceAddress()
+	}
+	for _, address := range providerAddresses {
+		merged[address.Value] = address.InstanceAddress()
+	}
+	addresses := make([]instance.Address, 0, len(merged))
+	for _, address := range merged {
+		addresses = append(addresses, address)
+	}
+	return addresses
+}
+
 // Addresses returns any hostnames and ips associated with a machine,
 // determined both by the machine itself, and by asking the provider.
 //
 // The addresses returned by the provider shadow any of the addresses
 // that the machine reported with the same address value.
 func (m *Machine) Addresses() (addresses []instance.Address) {
-	merged := make(map[string]instance.Address)
-	for _, address := range m.doc.MachineAddresses {
-		merged[address.Value] = address.InstanceAddress()
-	}
-	for _, address := range m.doc.Addresses {
-		merged[address.Value] = address.InstanceAddress()
-	}
-	for _, address := range merged {
-		addresses = append(addresses, address)
-	}
-	return
+	return mergedAddresses(m.doc.MachineAddresses, m.doc.Addresses)
 }
 
 // SetAddresses records any addresses related to the machine, sourced

=== modified file 'state/megawatcher.go'
--- state/megawatcher.go	2014-03-21 09:56:17 +0000
+++ state/megawatcher.go	2014-04-03 10:11:00 +0000
@@ -32,7 +32,7 @@
 		Life:                     params.Life(m.Life.String()),
 		Series:                   m.Series,
 		Jobs:                     paramsJobsFromJobs(m.Jobs),
-		Addresses:                addressesToInstanceAddresses(m.Addresses),
+		Addresses:                mergedAddresses(m.MachineAddresses, m.Addresses),
 		SupportedContainers:      m.SupportedContainers,
 		SupportedContainersKnown: m.SupportedContainersKnown,
 	}
@@ -87,13 +87,11 @@
 
 func (u *backingUnit) updated(st *State, store *multiwatcher.Store, id interface{}) error {
 	info := &params.UnitInfo{
-		Name:           u.Name,
-		Service:        u.Service,
-		Series:         u.Series,
-		PublicAddress:  u.PublicAddress,
-		PrivateAddress: u.PrivateAddress,
-		MachineId:      u.MachineId,
-		Ports:          u.Ports,
+		Name:      u.Name,
+		Service:   u.Service,
+		Series:    u.Series,
+		MachineId: u.MachineId,
+		Ports:     u.Ports,
 	}
 	if u.CharmURL != nil {
 		info.CharmURL = u.CharmURL.String()
@@ -114,10 +112,29 @@
 		info.Status = oldInfo.Status
 		info.StatusInfo = oldInfo.StatusInfo
 	}
+	publicAddress, privateAddress, err := getUnitAddresses(st, u.Name)
+	if err != nil {
+		return err
+	}
+	info.PublicAddress = publicAddress
+	info.PrivateAddress = privateAddress
 	store.Update(info)
 	return nil
 }
 
+// getUnitAddresses returns the public and private addresses on a given unit.
+// As of 1.18, the addresses are stored on the assigned machine but we retain
+// this approach for backwards compatibility.
+func getUnitAddresses(st *State, unitName string) (publicAddress, privateAddress string, err error) {
+	u, err := st.Unit(unitName)
+	if err != nil {
+		return "", "", err
+	}
+	publicAddress, _ = u.PublicAddress()
+	privateAddress, _ = u.PrivateAddress()
+	return publicAddress, privateAddress, nil
+}
+
 func (svc *backingUnit) removed(st *State, store *multiwatcher.Store, id interface{}) error {
 	store.Remove(params.EntityId{
 		Kind: "unit",

=== modified file 'state/megawatcher_internal_test.go'
--- state/megawatcher_internal_test.go	2014-03-21 11:00:41 +0000
+++ state/megawatcher_internal_test.go	2014-04-03 10:11:00 +0000
@@ -420,6 +420,44 @@
 				StatusInfo:    "another failure",
 			},
 		},
+	}, {
+		about: "unit addresses are read from the assigned machine for recent Juju releases",
+		setUp: func(c *gc.C, st *State) {
+			wordpress := AddTestingService(c, st, "wordpress", AddTestingCharm(c, st, "wordpress"))
+			u, err := wordpress.AddUnit()
+			c.Assert(err, gc.IsNil)
+			err = u.OpenPort("tcp", 12345)
+			c.Assert(err, gc.IsNil)
+			m, err := st.AddMachine("quantal", JobHostUnits)
+			c.Assert(err, gc.IsNil)
+			err = u.AssignToMachine(m)
+			c.Assert(err, gc.IsNil)
+			publicAddress := instance.NewAddress("public")
+			publicAddress.NetworkScope = instance.NetworkPublic
+			privateAddress := instance.NewAddress("private")
+			privateAddress.NetworkScope = instance.NetworkCloudLocal
+			err = m.SetAddresses([]instance.Address{publicAddress, privateAddress})
+			c.Assert(err, gc.IsNil)
+			err = u.SetStatus(params.StatusError, "failure", nil)
+			c.Assert(err, gc.IsNil)
+		},
+		change: watcher.Change{
+			C:  "units",
+			Id: "wordpress/0",
+		},
+		expectContents: []params.EntityInfo{
+			&params.UnitInfo{
+				Name:           "wordpress/0",
+				Service:        "wordpress",
+				Series:         "quantal",
+				PublicAddress:  "public",
+				PrivateAddress: "private",
+				MachineId:      "0",
+				Ports:          []instance.Port{{"tcp", 12345}},
+				Status:         params.StatusError,
+				StatusInfo:     "failure",
+			},
+		},
 	},
 	// Service changes
 	{