On Mon, Jan 17, 2011 at 12:00 PM, Apollon Oikonomopoulos <[email protected]> wrote: > 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? >
Has it? Wouldn't it be better to first set up the network and then start the instance? Since we open the tap before... Thanks, Guido
