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
_______________________________________________
Dng mailing list
Dng@lists.dyne.org
https://mailinglists.dyne.org/cgi-bin/mailman/listinfo/dng
