On Mon, Jan 17, 2011 at 10:47 AM, Apollon Oikonomopoulos
<[email protected]> wrote:
> This patch introduces network configuration for KVM in Ganeti.
>
> There are three problems with having KVM perform network configuration via
> ifup
> scripts:
> a) Ganeti never gets to know the tap interface that is associated with an
> instance's NIC
> b) Migration of routed instances will cause network problems because the
> incoming KVM side configures the network as soon as it is spawned and not
> as soon as the migration finishes. This means that all routing
> configuration will be present in both, primary and secondary, nodes at the
> same time, possibly causing network disruption during the migration.
> c) We never get to know if the network configuration succeeded or not.
>
> This patch moves network configuration from KVM to Ganeti, using KVM's ability
> to receive already open tap devices as file descriptors.
>
> _WriteNetScript is removed from hv_kvm.py, together with its unit tests.
>
> Minor modifications are made to _ExecKVMRuntime to handle tap device
> initialization. NIC <-> tap associations are stored under a new directory,
> _ROOT_DIR/nic in a file-per-nic fashion.
>
> The end-user semantics remain the same: The user can override the network
> configuration by providing _KVM_NET_SCRIPT. If this is not present or
> executable, the default constants.KVM_IFUP script is run.
>
> Signed-off-by: Apollon Oikonomopoulos <[email protected]>
> ---
> lib/hypervisor/hv_kvm.py | 234
> +++++++++++++----------------
> test/ganeti.hypervisor.hv_kvm_unittest.py | 47 ------
> 2 files changed, 107 insertions(+), 174 deletions(-)
>
> diff --git a/lib/hypervisor/hv_kvm.py b/lib/hypervisor/hv_kvm.py
> index ee895f3..b2fafd2 100644
> --- a/lib/hypervisor/hv_kvm.py
> +++ b/lib/hypervisor/hv_kvm.py
> @@ -33,6 +33,7 @@ import logging
> import pwd
> import struct
> import fcntl
> +import glob
Not needed anymore.
> from cStringIO import StringIO
>
> from ganeti import utils
> + @classmethod
> + def _InstanceAllNICFiles(cls, instance_name):
> + """Returns all existing NIC filenames
> +
> + """
> + instance_nic_dir = cls._InstanceNICDir(instance_name)
> + files = [ utils.PathJoin(instance_nic_dir, name) for name in
> + utils.ListVisibleFiles(instance_nic_dir) ]
> + return sorted(files)
> +
With the change below we could drop this function.
> + @classmethod
> def _TryReadUidFile(cls, uid_file):
> """Try to read a uid file
>
> @@ -464,6 +392,9 @@ class KVMHypervisor(hv_base.BaseHypervisor):
> utils.RemoveFile(uid_file)
> if uid is not None:
> uidpool.ReleaseUid(uid)
> + for nicfile in cls._InstanceAllNICFiles(instance_name):
> + utils.RemoveFile(nicfile)
> + utils.RemoveDir(cls._InstanceNICDir(instance_name))
How about using shutil.rmtree ?
> + def _ConfigureNIC(instance, seq, nic, tap):
> + """Run the network configuration script for a specified NIC
>
> @param instance: instance we're acting on
> @type instance: instance object
> @@ -495,22 +424,39 @@ class KVMHypervisor(hv_base.BaseHypervisor):
> @type seq: int
> @param nic: nic we're acting on
> @type nic: nic object
> - @return: netscript file name
> - @rtype: string
> + @param tap: the host's tap interface this NIC corresponds to
> + @type tap: str
>
> """
> - script = _WriteNetScript(instance, nic, seq)
>
> - # As much as we'd like to put this in our _ROOT_DIR, that will happen to
> be
> - # mounted noexec sometimes, so we'll have to find another place.
> - (tmpfd, tmpfile_name) = tempfile.mkstemp()
> - tmpfile = os.fdopen(tmpfd, 'w')
> - try:
> - tmpfile.write(script)
> - finally:
> - tmpfile.close()
> - os.chmod(tmpfile_name, 0755)
> - return tmpfile_name
> + if instance.tags:
> + tags = " ".join(instance.tags)
> + else:
> + tags = ""
> +
> + env = {
> + "PATH": "%s:/sbin:/usr/sbin" % os.environ["PATH"],
> + "INSTANCE": instance.name,
> + "MAC": nic.mac,
> + "MODE": nic.nicparams[constants.NIC_MODE],
> + "INTERFACE": tap,
> + "INTERFACE_INDEX": str(seq),
> + "TAGS": tags,
> + }
> +
> + if nic.ip:
> + env["IP"] = nic.ip
> +
> + if nic.nicparams[constants.NIC_LINK]:
> + env["LINK"] = nic.nicparams[constants.NIC_LINK]
> +
> + if nic.nicparams[constants.NIC_MODE] == constants.NIC_MODE_BRIDGED:
> + env["BRIDGE"] = nic.nicparams[constants.NIC_LINK]
> +
> + result = utils.RunCmd([constants.KVM_IFUP, tap], env=env)
> + if result.failed:
> + logging.warning("Failed to configure interface %s: %s (%s)" %
> + (tap, result.fail_reason, result.output))
>
Should it be a HypervisorError here? Does it make sense to go on if
configuring the network fails, and just logging a warning?
LGTM for the rest
Thanks,
Guido
