Bug 1378129

I was asked to clone this for etcd3-docker as well.

+++ This bug was initially created as a clone of Bug #1376565 +++

Description of problem:
Unlike etcd rpm that it will read configuration from /etc/etcd/etcd.conf, etcd-docker hard-coded the environment variables to run the etcd binary.

# cat /var/lib/containers/atomic/etcd/config.json
...
                "args": [
                        "/usr/bin/etcd-env.sh",
                        "/usr/bin/etcd"
...
# cat /var/lib/containers/atomic/etcd/rootfs/usr/bin/etcd-env.sh 
#!/bin/sh

ipaddress=$(hostname -I | cut -f1 -d' ')
if test x$NAME == x; then
    NAME=$HOSTNAME
fi
export ETCD_NAME=$HOSTNAME
export ETCD_DATA_DIR=/var/lib/etcd/${NAME}.etcd
export ETCD_ADVERTISE_CLIENT_URLS=http://${ipaddress}:2379,http://${ipaddress}:4001
export ETCD_LISTEN_CLIENT_URLS=http://0.0.0.0:2379,http://0.0.0.0:4001
export ETCD_INITIAL_ADVERTISE_PEER_URLS=http://${ipaddress}:2380,http://${ipaddress}:7001
export ETCD_LISTEN_PEER_URLS=http://0.0.0.0:2380,http://0.0.0.0:7001
export ETCD_INITIAL_CLUSTER=$HOSTNAME=http://${ipaddress}:2380,$HOSTNAME=http://${ipaddress}:7001

# Execute the commands passed to this script
exec "$@

Looks at the etcd rpm's unit file,
# cat /usr/lib/systemd/system/etcd.service
...
EnvironmentFile=-/etc/etcd/etcd.conf
...
# set GOMAXPROCS to number of processors
ExecStart=/bin/bash -c "GOMAXPROCS=$(nproc) /usr/bin/etcd --name=\"${ETCD_NAME}\" --data-dir=\"${ETCD_DATA_DIR}\" --listen-client-urls=\"${ETCD_LISTEN_CLIENT_URLS}\""
...

Personally, I think there are two fundamental issues. One is that a configuration file needs to be made available for users to edit like etcd rpm. The other is that etcd-docker needs to use the environment variables to start the etcd binary.

Version-Release number of selected component (if applicable):
# etcd --version
etcd Version: 2.3.7
Git SHA: fd17c91
Go Version: go1.6.2
Go OS/Arch: linux/amd64

How reproducible:
always

--- Additional comment from Avesh Agarwal on 2016-09-15 14:44:54 EDT ---

CAI,

1. Not sure why would you think this is a regression? You can not compare containers with rpms?

2. Why would you allow a container read files on the host file system?

3. Its not hardcoded, those are just default values. You can still override default values.

Giuseppe can provide more info to you here.

--- Additional comment from CAI Qian on 2016-09-15 15:08:39 EDT ---

(In reply to Avesh Agarwal from comment #1)
> CAI,
> 
> 1. Not sure why would you think this is a regression? You can not compare
> containers with rpms?
It was considered an user-experience regression with regards users to make persist configuration changes. It would be fine if we fix this in documentation by providing an accepted new method.
> 2. Why would you allow a container read files on the host file system?
Ideally, it would be the same way of people using configuration file as in the etcd rpm, so people don't need to change their scripts, documentation and minimize the surprise.

--- Additional comment from Avesh Agarwal on 2016-09-15 16:15:35 EDT ---

I have been discussing about it with Giuseepe and he is suggesting to implement --env flag with atomic install to pass any env vars to a container for configuration. He is also going to discuss it with Dan Walsh.

I also discussed with him the possibility of mounting a conf file inside /etc between host and containers to have similar experience like rpms, but his concern is that a) running multiple instances of the same container on the host (something I think might not be needed most of the time, but its ok)? b) we also break the possibility to run it as an user container with bubblewrap, and it is something we want to do later.

--- Additional comment from Avesh Agarwal on 2016-09-15 16:23:30 EDT ---

I have cc'ed to Dan Walsh too to get his views.

The issue here is it that with system containers, we provide a file manifest.json that has default env vars and their default values and only these env vars' values can be can be overridden by --set flag of atomic install.

The issue here is that how to set/provide those env vars (required to configure an application) not part of manifest.json?

--- Additional comment from Daniel Walsh on 2016-09-16 07:24:55 EDT ---

Can we just leak the environment variables into the container?

--- Additional comment from Avesh Agarwal on 2016-09-20 14:35:36 EDT ---

Similar flannel-docker (https://bugzilla.redhat.com/show_bug.cgi?id=1376572) bug has been closed as a not a bug, as per the comments in that bug, similar user experience is not expected. So removing regression and blocker bug flags.

--- Additional comment from CAI Qian on 2016-09-20 14:50:53 EDT ---

I am not going to argue for flannel-docker, since there seems only 2 config settings to change. However, etcd-docker is worse with lots of settings that does not provide a proper way to configuration them right now.

--- Additional comment from Avesh Agarwal on 2016-09-20 14:54:58 EDT ---

Thats why I have not closed the bug. The best I can do is based on what is supported by atomic that means to have etcd config variables set at install time via --set flag to atomic.

--- Additional comment from Avesh Agarwal on 2016-09-20 14:59:05 EDT ---

Also as I said in https://bugzilla.redhat.com/show_bug.cgi?id=1376572 that I am fine with bind mounting /etc/etcd/etcd.conf in container on host so that it can be configure like in rpm but Giuseppe does not like that.

--- Additional comment from Giuseppe Scrivano on 2016-09-21 10:08:20 EDT ---

there is a patch here so that it is possible to configure more variables in the etcd container:

https://github.com/aveshagarwal/etcd-container/pull/6

--set can be used to pass environment variables that are accepted by the image, that means having something like: "VARIABLE=$VARIABLE" in the env block of the config.json.template file, which is feed directly to runC.

The manifest.json is used to provide a default value, when the user doesn't specify any, so that the image can have a default configuration and not expect --set for each accepted variable.

I don't see why this is less flexible, both the user and the developer of the image should know what values are accepted.

If we bind mount the file, we won't be able to have multiple instances of etcd running (unless we are going to provide a different conf file for each of them):

e.g. if we don't bind the configuration file, we can use something like this to spin 3 etcd instances:

for i in 0 1 2; do
    ETCD_NAME=$HOSTNAME
    ./atomic install --system \
             --set ETCD_NAME=$ETCD_NAME --set ETCD_ADVERTISE_CLIENT_URLS=http://${ipaddress}:$((2379 + $i + 2)),http://${ipaddress}:$((4001 + $i)) \
             --set ETCD_LISTEN_CLIENT_URLS=http://0.0.0.0:$((2379 + $i * 2)),http://0.0.0.0:$((4001 + $i)) \
             --set ETCD_INITIAL_ADVERTISE_PEER_URLS=http://${ipaddress}:$((2380 + $i * 2)),http://${ipaddress}:$((7001 + $i)) \
             --set ETCD_LISTEN_PEER_URLS=http://0.0.0.0:$((2380 + $i * 2)),http://0.0.0.0:$((7001 + $i)) \
             --set ETCD_INITIAL_CLUSTER=$ETCD_NAME=http://${ipaddress}:$((2380 + $i * 2)),$ETCD_NAME=http://${ipaddress}:$((7001 + $i)) \
             --name etcd$i gscrivano/etcd

    systemctl start etcd$i
done