On 11:55 Mon 17 Jan , Guido Trotter wrote:
> 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.
Ooops, missed that...
>
> > 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 ?
Hmmm, not a bad idea. I'll change this and remove _InstanceAllNICFiles.
>
> > + 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?
I was thinking of that as well. Thing is, at the point this happens, the
KVM instance has already started and should be cleaned up. What do you
think?
Thanks,
Apollon
