Fixes lp:1575676: Detect LXD default profile's bridge name and use it

[dimitern]

Changed tools/lxdclient code not to assume the default LXD bridge name
is always "lxdbr0". Instead, the "default" profile's bridge device name
is determined and used in Client.

See http://pad.lv/1575676 for details.

QA steps to verify the fix:

1. LXD provider (where it appears):

   $ sudo dpkg-reconfigure -p medium lxd
   # rename the lxdbr0 to something else, keep the rest the same.
   $ juju bootstrap --upload-tools lxd localhost
   # expect success, before this fix it failed to complete due to lxdbr0
   # not existing. 
   $ juju deploy ubuntu # also OK

1. Manual provider (for completeness):

   $ lxc launch ubuntu:xenial man-x1
   $ lxc info man-x1 | grep eth0    # find eth0's address, e.g. 10.0.1.2
   $ lxc file push ~/.ssh/<ssh-key.pub> man-x1/home/ubuntu/.ssh/authorized_keys
   # to allow the below to work
   $ ssh ubuntu@10.0.1.2            # should work now
   $ juju bootstrap --upload-tools man manual/10.0.1.2

[dimitern]

@wallyworld As OCR, can you have a look please?

[fwereade]

d?

[dimitern]

Good point - removing.

[fwereade]

every such short-circuit makes me have to stop and double-check that there's no opportunity for hidden-nils to creep in; even though this one is safe, it's annoying to have to stop and check what really happens when you use that construct. revert?

[dimitern]

Reverting.

[fwereade]

LGTM

[babbageclunk]

Was this just reformatting the comment? It's a bit confusing - probably better to leave it in a non-standard format than muddy the water with a formatting change in the middle of changing behaviour.

[dimitern]

Yeah, simple reformatting. Will revert as suggested.

[babbageclunk]

I don't think this matches the comment above - if eth0 isn't in config.Devices it will return an error. (Unless maybe the code further up does the "use whatever they set up" bit?) Should it be

if ok && (eth0[configTypeKey] != configTypeNic || eth0[configNicTypeKey] != configBridged) {

instead?

[dimitern]

Good catch! Will update the comment and change the conditional as suggested.

[dimitern]

I changed the logic to first check for !ok and return an error when eth0 is missing, and then check its attributes. Seems to read better now.

[babbageclunk]

Flip the sense of this and switch the legs of the if around - double negatives are confusing!

Actually, os.IsNotExist can take nil, so it might be clearer to get rid of the nesting:

if os.IsNotExist(err) {
    return "", bridgeConfigError("lxdbr0 configured but no config file found at " + LXDBridgeFile)
else if err != nil {
    return "", errors.Trace(err)
}

[dimitern]

Nice! This is mostly pre-existing code factored out into a function, but will change as suggested.

[babbageclunk]

This comment needs updating - it still talks about lxdbr0. (Which would retroactively justify the reformatting! ;)

[dimitern]

Fair point - will update the comment and the error below as needed.

[dimitern]

Actually, no - this specific helper func is only called to verify the default lxdbr0 config is sane. If the user changed the default profile's bridge name, it won't be ever called, so the comment still applies (will revert the formatting change though).

[babbageclunk]

It might be worth keeping the note that this will start the bridge if it's never been activated?

[dimitern]

Updated.

[babbageclunk]

LGTM other than the question about the if !ok || ... logic in verifyDefaultProfileBridgeConfig.

[dimitern]

$$fixes-1575676$$
$$JFDI$$

[jujubot]

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