Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Improvements to ingress address watcher #7184
Conversation
| + return result | ||
| +} | ||
| + | ||
| +func (w *IngressAddressWatcher) Changes() <-chan []string { |
axw
Mar 31, 2017
Member
I think it would be helpful to group the interface methods together, either above or below the guts.
| + | ||
| + u, err := w.backend.Unit(name) | ||
| + if errors.IsNotFound(err) { | ||
| + logger.Warningf("unit %s is not found, can't get address", name) |
axw
Mar 31, 2017
Member
does this really need a warning? isn't it normal for to get
- RUW.Changed
- (unit departed, then removed)
- RUW.Departed
?
so just ignore the NotFound, and we'll report the address removal when the Departed event comes along
| + | ||
| + // TODO - start watcher to pick up machine address changes | ||
| + // We need to know whether to look at the public or cloud local address. | ||
| + // For now, we'll use the public address and later if needed use a watcher |
axw
Mar 31, 2017
Member
that's probably fine in general, but we still need to watch addresses in case (a) the machine doesn't have an address yet, or (b) the public address changes
| + // For now, we'll use the public address and later if needed use a watcher | ||
| + // parameter to look at the cloud local address. | ||
| + addr, err := u.PublicAddress() | ||
| + if errors.IsNotAssigned(err) { |
axw
Mar 31, 2017
Member
both of these warnings should be removed (or changed to debug) when we start watching addresses
| + w.known[name] = addr.Value | ||
| + } | ||
| + for _, name := range c.Departed { | ||
| + // If the unit is departing and we have seen it's address, |
| + address, ok := w.known[name] | ||
| + if ok { | ||
| + delete(w.known, name) | ||
| + result.Remove(address) |
axw
Mar 31, 2017
Member
this assumes that there's 1:1 between RUW and address, which is not necessarily true. you could have two units on the same machine. so I think you need a counter as well.
| + return result, nil | ||
| +} | ||
| + | ||
| +func (w *IngressAddressWatcher) Kill() { |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wallyworld commentedMar 31, 2017
Description of change
The way the firewaller watches for ingress address changes is altered to use a stringswatcher instead of a notifywatcher. The watcher event now contains the working set of ingress addresses; there is no need for a separate api call to get the addresses, so this facade method is removed.
A new IngressAddressWatcher is added which watches for units on a given relation to enter'leave scope, and returns the set of public addresses for all units in scope. This is not used yet but will be in a subsequent PR.
QA steps
Run up a CMR scenario on AWS.
Ensure the units can talk to each other.
Check the AWS sec group rules to ensure the expected subnets are used.