[Ovirt-devel] [PATCH ovirt-node] Allow the appliance to manage the hardware it is running on - part 2

[Jim Meyering]

> From: Chris Lalancette <clalance at redhat.com>
>
>     A rather large re-write of the ovirt-node.spec file.  The reason
>     for this is so that we can introduce the concept of the "stateful" oVirt
>     node; that is, one which is already installed and running with an OS.
>     To accomplish this, we add two subpackages: ovirt-node-stateless,
>     and ovirt-node-stateful.  They both depend on ovirt-node,
>     which holds most of the necessary binaries.  The two subpackages have
>     subtly different %post scripts, since you don't necessarily want to do all
>     of the stuff from stateless on a stateful box.  There is also a minor change
>     to common-pkgs to require ovirt-node-stateless, the addition of
>     an init script for ovirt-node-stateful, and the addition of two
>     helper scripts for the stateful case to let you turn your machine into
>     an oVirt node.
...
Substance looks fine, so...

> diff --git a/ovirt-listen-awake/ovirt-convert-host b/ovirt-listen-awake/ovirt-convert-host
> new file mode 100755
> index 0000000..0ef374d
> --- /dev/null
> +++ b/ovirt-listen-awake/ovirt-convert-host
> @@ -0,0 +1,51 @@
> +#!/bin/bash
> +

What do you think about sourcing /etc/init.d/functions here?
Then you could remove all the /sbin/ and /usr/sbin/ prefixes below
and invoke e.g., "chkconfig" rather than "/sbin/chkconfig".

> +. /etc/init.d/ovirt-functions
> +
> +if [ $# -ne 0 ]; then
> +    echo "Usage: ovirt-convert-host"
> +    exit 1
> +fi
> +
> +backup_file() {
> +    dir=$(dirname $1)
> +    mkdir -p $OVIRT_BACKUP_DIR/${dir:1}
> +    cp -pf $1 $OVIRT_BACKUP_DIR/${dir:1}

How about this? (removing the :1, which I had to look up,
and adding double quotes, just in case.  No use of backup_file
requires double quotes around $1, but better to add them now,
in case the function is used differently later.

    dir=$(dirname "$1")
    mkdir -p "$OVIRT_BACKUP_DIR$dir"
    cp -pf "$1" "$OVIRT_BACKUP_DIR$dir"

$dir always starts with "/", but it would be prudent to insert the
equivalent of assert(dir[0] == '/'); before the mkdir, i.e.,

   case $dir in /*);; *) die "unexpected non-absolute dir: $dir";; esac

...
> +backup_file /etc/hosts
> +if [ $( grep -c "physical.priv.ovirt.org" /etc/hosts ) -eq 0 ]; then
> +    echo "192.168.50.1 physical.priv.ovirt.org" >> /etc/hosts
> +fi

If you tighten up this regexp, it won't match commented-out or slightly
different lines.  You can also remove the use of "test" (aka '['):

  grep -qE '^[0-9.]+[[:space:]]+physical\.priv\.ovirt\.org' /etc/hosts \
          || echo "192.168.50.1 physical.priv.ovirt.org" >> /etc/hosts

Hmm... I guess officially the regexp should start with ^[[:space:]]*.
And I prefer to use single quotes so I don't have to worry about
embedded backslashes or '$' signs.  Even in the following 'echo',
using single quotes is nicer, since then you know right away that
the string won't be shell-interpolated.

grep -qE \
  '^[[:space:]]*[0-9.]+[[:space:]]+physical\.priv\.ovirt\.org' /etc/hosts \
             || echo '192.168.50.1 physical.priv.ovirt.org' >> /etc/hosts

Rats, it's going to wrap.  to avoid matching the bogus

  192.168.50.1 physical.priv.ovirt.organophosphate.com

you'd add a little more.  And with all this added obfuscation (this is
where I miss the likes of \d \S and \s), it'd be good to add a comment:

# Add an IP->hostname entry for "physical" if it's not already there.
grep -qE \
  '^[[:space:]]*[0-9.]+[[:space:]]+physical\.priv\.ovirt\.org($|#|[[:space:]])'\
      /etc/hosts \
             || echo '192.168.50.1 physical.priv.ovirt.org' >> /etc/hosts

Man, that horse is dead.  I hope.

> +if [ $( grep -c "management.priv.ovirt.org" /etc/hosts ) -eq 0 ]; then
> +    echo "192.168.50.2 management.priv.ovirt.org" >> /etc/hosts
> +fi

Same here.

> +/sbin/chkconfig ovirt-listen-awake on
> +/sbin/chkconfig collectd on
> +
> +backup_file /etc/sysconfig/libvirtd
> +backup_file /etc/libvirt/qemu.conf
> +backup_file /etc/libvirt/libvirtd.conf
> +backup_file /etc/sasl2/libvirt.conf
> +ovirt_setup_libvirtd
> +
> +backup_file /etc/sysconfig/iptables
> +/usr/sbin/lokkit -n -t ovirtbr
> +
> +echo "Setup complete.  To make the changes take effect, shut down any running
> +guests and reboot the host"
> diff --git a/ovirt-listen-awake/ovirt-unconvert-host b/ovirt-listen-awake/ovirt-unconvert-host
> new file mode 100755
> index 0000000..cfaa51d
> --- /dev/null
> +++ b/ovirt-listen-awake/ovirt-unconvert-host
> @@ -0,0 +1,30 @@
> +#!/bin/bash
> +
> +. /etc/init.d/ovirt-functions
> +
> +if [ $# -ne 0 ]; then
> +    echo "Usage: ovirt-unconvert-host"
> +    exit 1
> +fi
> +
> +if [ ! -d $OVIRT_BACKUP_DIR ]; then
> +    echo "The oVirt backup directory $OVIRT_BACKUP_DIR doesn't exist; can't"
> +    echo "unconvert the host"
> +    exit 2
> +fi
> +
> +unbackup_file() {
> +    # note that $1 will have a / on the front, so we don't need to add our own
> +    cp -pf $OVIRT_BACKUP_DIR$1 $1

Who knows what $OVIRT_BACKUP_DIR might contain ;-)

    cp -pf "$OVIRT_BACKUP_DIR$1" "$1"
...

> diff --git a/ovirt-node.spec b/ovirt-node.spec
...
> +%post stateless
>  /sbin/chkconfig --add ovirt-early
>  /sbin/chkconfig ovirt-early on
>  /sbin/chkconfig --add ovirt
>  /sbin/chkconfig ovirt on
>  /sbin/chkconfig --add ovirt-post
>  /sbin/chkconfig ovirt-post on
...
> +. /etc/init.d/ovirt-functions ; ovirt_setup_libvirtd

Using two separate lines would be more readable.
Otherwise, it's easy to miss the latter stmt.

...
> diff --git a/scripts/ovirt-functions b/scripts/ovirt-functions
> index 083e13d..24a28df 100644
> --- a/scripts/ovirt-functions
> +++ b/scripts/ovirt-functions
> @@ -5,6 +5,8 @@ OVIRT_LOGFILE=/var/log/ovirt.log
>  # label of the oVirt partition
>  OVIRT_LABEL=OVIRT
>
> +OVIRT_BACKUP_DIR=/var/lib/ovirt-backup
> +
>  find_srv()
>  {
>      local dnsreply
> @@ -21,3 +23,26 @@ die()
>  {
>      echo "$@" 1>&2; failure; echo 1>&2; exit 1
>  }
> +
> +ovirt_setup_libvirtd() {
> +    # just to get a boot warning to shut up
> +    touch /etc/resolv.conf
> +
> +    # make libvirtd listen on the external interfaces
> +    sed -i -e 's/^#\(LIBVIRTD_ARGS="--listen"\).*/\1/' \
> +	/etc/sysconfig/libvirtd
> +
> +    # set up qemu daemon to allow outside VNC connections
> +    sed -i -e 's/^[[:space:]]*#[[:space:]]*\(vnc_listen = "0.0.0.0"\).*/\1/' \
> +	/etc/libvirt/qemu.conf
> +    # set up libvirtd to listen on TCP (for kerberos)
> +    sed -i -e "s/^[[:space:]]*#[[:space:]]*\(listen_tcp\)\>.*/\1 = 1/" \
> +	-e "s/^[[:space:]]*#[[:space:]]*\(listen_tls\)\>.*/\1 = 0/" \
> +	/etc/libvirt/libvirtd.conf
> +
> +    # with libvirt (0.4.0), make sure we we setup gssapi in the mech_list
> +    if [ `egrep -c "^mech_list: gssapi" /etc/sasl2/libvirt.conf` -eq 0 ]; then
> +	sed -i -e "s/^\([[:space:]]*mech_list.*\)/#\1/" /etc/sasl2/libvirt.conf
> +	echo "mech_list: gssapi" >> /etc/sasl2/libvirt.conf
> +    fi
> +}

This is probably just moved code, but you can avoid a "test" and a ``
subshell.  Plus, it's better not to use "egrep".  grep -E is more
standard and portable.  If you don't like the -q option (some non-POSIX
systems lack it), and the output would be a problem, just redirect stdout
to /dev/null.  Finally, with three uses of "/etc/sasl2/libvirt.conf",
it's worthwhile to factor it out:

    sasl_libvirt_conf=/etc/sasl2/libvirt.conf
    if ! grep -qE '^mech_list: gssapi' $sasl_libvirt_conf; then
        sed -i 's/^\([[:space:]]*mech_list.*\)/#\1/' $sasl_libvirt_conf
        echo 'mech_list: gssapi' >> $sasl_libvirt_conf
    fi