On 06/20/2016 09:47 PM, Simon Walter wrote:
> Hi Greg,
>
> I've added a branch called add-netconf, which you have probably seen.
>
> I thought to just add one feature per branch so that review is easier.
> Let me know how you prefer to collaborate.

That's perfect. Branch as often as you see fit, that's what they're for.


>
> I was not sure about what style/standards we are using. However, the
> hashbang tells me bash. I looked at the other templates, and most use
> bash.

Yes it's Bash, so I use Bash'isms including the ugly [[ expr ]]
syntax. It's more efficient to use builtins over external utilities,
including [ (test) which is also an external program.

Regarding style, I'm comfortable reading shell programs regardless
of individual coding preferences. It's usually best to match the
existing style, however I didn't entirely do this with lxc-devuan
because it's a new shell program, and I prefer builtins for
efficiency. Therefore the new code is clearly Bash.

I don't have the time now for a coding style discussion, but here's
just a couple things:

Variables:
Upper-case names which (can) derive value from an environment var.
Declare function variables local.
Initialize variables.

Tabs:  there are some strong opinions on this, but here goes:
I don't use tabs unless absolutely necessary, and in shell scripts
it's not necessary.
    Caveat: here docs for config files with syntax that requires tabs.

Tabs display differently in different environments.
If tabs are used, placing *only* at beginning of a line will at least
keep indentation consistent.
But as I said, I don't use tabs in shell scripts.

Indentation:
Yes, please do it. Whitespace is good.

Obviously I'm not a stickler with Bash scripts.
Just keep code as simple and readable as possible.
And if something is not at all obvious, then throw in a comment.


>
> I wasn't sure if having all the settings in one argument or separate
> was better. The logic behind having them all in one argument is that
> all those are needed and cannot be derived from one another. So IP,
> netmask, and gateway are all in one comma separated argument. Maybe
> the parameter name should be "interface" or "ifconfig".

Sorry about the length here but I don't see how to further simplify.

To try and put things delicately, I think we need to consider an
approach that accomplishes the same thing (external to container) and
covers more use cases.

For instance, cloning customized containers is a very common
practice. However an internally configured IP will *prevent* cloning
the container to use as a drop in for another. At least *not* if the
intent is to use them at the same time.

I've also setup Vagrant boxes, and trust me, an internally configured
IP will severely limit its potential uses as a Vagrant box. I.e. the
box added via: `vagrant box add user/box` and specified in Vagrantfile
as `config.vm.box = user/box`). You could have a whole bunch of
Vagrantfile's all referring to the same user/box and a hard-coded IP
will prevent using more than one at a time.

IMO, configuring the network interface *internally* within the
container will definitely limit the usefulness of the container.

I'm not trying to shoot down the idea here. I just want to point the
way towards, IMHO, a more effective way to implement a version
of the idea.

We really need to target the LXC config, and not the internal
interfaces file.

Using the lxc.network.* options we can accomplish the same thing.
It's just harder to get right because of the `/etc/lxc/default.conf`
network options that may, or may not be present.

You can still have a "combined arg" if you want, but I think it's
important to first get the individual options working correctly.

I suggest using the same names, or similar to, lxc.network
options. That will help prevent users from getting confused
about what's being affected.

Ex.
--net-type [veth]
--net-flags [up|down]
--net-link [linkname]
--net-hwaddr [MAC]
--net-ipv4 [x.y.z.t/cidr]
--net-ipv4-gw [x.y.z.t | auto]
--net-mtu [maxmtu]
--net-ipv6 [x::y]
--net-ipv6-gw [x::y | auto]

Of course, I'm *not* suggesting that if one option is coded then *all*
must be coded.

For reference here's a container.conf link:
http://man7.org/linux/man-pages/man5/lxc.container.conf.5.html

Things start to get complex when considering the possible combinations
of what may or may not already be specified in `/etc/lxc/default.conf`.

Use divide and conquer to keep things as simple as possible:
1. Restrict use to when only 0 or 1 layer-2 interface is present in
     `/etc/lxc/default.conf`
2. I've already isolated all network options from the rest of the
     config: Use Bash associative array to enumerate options from
     `${path}/config-network`. The keys can be the option names.
3. CLI --net-* options (stored separately) then override, or add
     to options stored in the enumerated default options array.
4. Perform edits to ensure keys like --net-type and --net-flags exist,
     and add them as needed.
5. Generate the lxc.network options from the resulting array entries
     and append to `$path/config` after comment "# Add net config".

By timing correctly and inserting lxc.network entries after the
comment "# Add net config", you won't have to worry about my earlier
code that assigns hwaddr from `/etc/ethers.used`. It will even retain
the comment as part of the entry value. And the hwaddr can also be
overridden by --net-hwaddr, giving *all* CLI options the last word.

We should consider some different default.conf use cases, and think
through how it should behave. That's a straight forward way to derive
the edit requirements needed for step 4 above.

I realize this might be a lot to take in. But IMHO, it's the correct
way to achieve a *flexible* and effective solution using CLI options.

What do you think. Is this something you would like to do?


BTW, I'm adding an issue for this enhancement.


>
> For my next edit, I would like to add the password parameter and if
> not specified would set a random one.
>
> Then I was thinking of adding a parameter for MAC address, in case it
> should be explicitly set or not set or set to a random address.

Go ahead. However regarding the MAC, implementing my previous
recommendation will handle it nicely.


>
> I would appreciate your comments on that and of course on the netconf
> parameter and it's functionality.

If you want to keep netconf option as a way to internally configure
the IP/Mask/GW in `interfaces` file, then somehow the Usage doc
must make it *clear* to the user, about it being internal to the
container.

A warning about a possible conflict with /etc/lxc/default.conf also
seems appropriate.

I also think more testing is in order to determine how it might
(mis)behave if there *is* a conflict between the default.conf
and the internal config. Because that's sure to happen.


>
> One question, is --main-only the default? What is the default for the
> Devuan install? I am not sure, but I think the default is only main.
> If so, maybe we should reverse that argument to be something like
> --extra-repos. Or even --add-repos=main,non-free. That way the use can
> specify exactly the additional repositories on the command line.

--main-only isn't the default, however it should be properly
initialized (mainonly=0) before template options are parsed.

if [ "$mainonly" = 1 ]; then <=== Can't be 1 without --main-only
non_main=''
else
non_main=' contrib non-free'
fi

When asking, what are the people coming from Debian going to
expect? The answer I come up with is: There's a good chance they'd
expect --main-only to still be an option in Devuan.

I wouldn't want to prevent base use cases from Debian to stop working
with lxc-devuan.

BTW, "main", contrib & non-free refer to the *component* of a repository,
which are subdirectories under ./distribution/pool/.

Being each distribution has its own repository, technically the
repo is everything within ./distribution

At least that's how I'm interpreting how Debian defines the terms.

See:
https://wiki.debian.org/SourcesList
https://wiki.debian.org/DebianRepository


>
> I think that would be very useful for orchestration tools such as
> vagrant or cdist for example.

Perhaps for cdist, although I've never used it.

Not so much for Vagrant:

For the following /etc/lxc/default.conf (network config options):

lxc.network.type = veth
lxc.network.flags = up
lxc.network.link = lxcbr0
lxc.network.ipv4 = 0.0.0.0

these Vagrantfile net customizations will work,
with *no* "hard-wiring" required in the box:

# Set VM node/host name & MAC address (tested)
config.vm.define "dev1a" do |node|
   node.vm.hostname = "dev1a.mydomain.com"
   node.vm.provider :lxc do |lxc|
     lxc.container_name = :machine
     lxc.customize 'network.hwaddr', '00:16:3e:00:9f:b1'  # MAC
   end
end

# Set VM node/host name & IP/Gateway address (untested but should work)
config.vm.define "dev1a" do |node|
   node.vm.hostname = "dev1a.mydomain.com"
   node.vm.provider :lxc do |lxc|
     lxc.container_name = :machine
     lxc.customize 'network.ipv4', '10.0.159.77/24'       # IP/CIDR
     lxc.customize 'network.ipv4.gateway', 'auto'         # Gateway IP
   end
end

Therefore for Vagrant, the appropriate place for orchestration/config
to target is the Vagrantfile. The boxed container should *not* have a
pre-assigned IP address.


Best regards,

Greg