hi *, i know this is a old patch but i like to have some progress on it any chance to move it as merge-request to github? maybe we can got it into the master tree?
Thanks, Ansgar 2016-02-15 1:25 GMT+01:00 Dimitris Aragiorgis <[email protected]>: > Hi, > > * Dimitris Bliablias <[email protected]> [2016-02-12 15:10:12 +0200]: > >> Hello, >> >> This patch series adds support for the MacVTap device driver in Ganeti. >> >> As stated in the respective design doc, this implementation targets the >> KVM hypervisor. It also includes some new unit tests to cover the new >> NIC mode introduced and also it documents the new feature in the >> relevant man pages. > > First of all thanks a lot for this work. > > If I understand correctly, this patch set seems to implement a mix of > the functionality of two distinct design docs, design-ifdown.rst and > design-macvtap.rst. > > I was involved heavily in the design-ifdown.rst doc a year ago, so > please allow me to make a few comments. > > The MacVTap changes seem to be OK, I only have doubts about the > special/different name chosen for TAP devices (gnt.macvta.%d); is > it only for _CleanupStaleMacvtapDevs() in ganeti-watcher (patch 25/28)? > However, I think the ifdown part hides some issues. > > In a nutshell there are two major issues with the proposed implementation: > > 1) It finds the TAP name using the NIC's index > 2) It un-configures the TAP based on current instance configuration > stored in config.data > > Let me elaborate a bit more on both issues. Below are two suggestions > that address the above issues. > > > Find the TAP name using the NIC's UUID and not its index > -------------------------------------------------------- > > Assume the following scenario: > > Create an instance with three NICs (id/index): NIC0/0, NIC1/1, NIC2/2. > Then remove the middle one NIC1 without hotplug. > Now the NICs in config.data will be NIC0/0, NIC2/1. > Then remove NIC2/1 with hotplug. > > The HotDelDevice() will include NIC2 object with index 1. Hotplug code > will parse the runtime file find the proper NIC entry based on it's > UUID, remove it correctly using QMP commands *but* then this code in > patch 18/28: > > + tap = utils.ReadFile(self._InstanceNICFile(instance.name, seq)) > + self._UnconfigureNIC(instance, seq, device, tap) > > seq will be 1, and will end up running ifdown on TAP1!!! > > There is another problematic scenario: > > Create an instance with three NICs (id/index): NIC0/0, NIC1/1, NIC2/2. > Then remove the NIC1 without hotplug. > Then append a new one (i.e. --net -1:add) with hotplug. > The NICs in config data will be NIC0/0, NIC2/1, NIC3/2. > The NICs in runtime now will be NIC0/0, NIC1/1, NIC2/2, NIC3/2. > > Here we bump into an existing bug! The created NIC file > /var/run/ganeti/kvm-hypervisor/nic/<instance_name>/2 will overwrite TAP1 > with TAP3 since both have the same index from the config.data perspective > upon TAP creation. > > Note that currently this file is not actually used by Ganeti. But > the code mentioned above will use it! > > The solution for this could be something like this patch: > > https://github.com/dimara/ganeti/commit/5b63e6e8da15a23144dac1c2b6882d705340d7eb > > Note that the equivalent solution for disks is already implemented: > > https://github.com/ganeti/ganeti/commit/3eae7f3401 > > > Use runtime data and not config.data during ifdown > -------------------------------------------------- > > You modified HotModDevice() RPC to pass the old device as well. This > is problematic since there might be a lot of changes (MAC, IP, etc) > in the configuration that are not applied in the up-and-running NIC. > The un-configuration must be done with the info stored in runtime > files. To this end, the ifdown script must not depend on config.data > e.g. instance's tags. So something like this is what I have in mind: > > https://github.com/dimara/ganeti/commit/088284d0aec8392a71c208cfeb161645aab54d6d > > If we would like to do it properly, then a solution could be > to do the same what currently is done in hv_xen.py with _WriteNICInfoFile(). > > https://github.com/ganeti/ganeti/commit/397b784452 > > If so, this should be an hv_base functionality. > > The, the ifdown script could source this file and act properly. > > A more visionary approach that would help a lot on such cases is to > store runtime files in config data as well, something that we discussed > in GanetiCon'15 with Hrvoje [cc'ed] but this requires huge refactoring > effort that probably is not the time to start... > > > Use uniform names for user provided scripts > ------------------------------------------- > > You introduce kvm-ifdown-custom. The equivalent ifup script is currently > kvm-vif-bridge and should be renamed to kvm-ifup-custom keeping backwards > compatibility, perhaps like this: > > https://github.com/dimara/ganeti/commit/e7b7cde0e415039f554b77e3892a35ef6e08cdcb > > > Differentiate ifdown functionality between NIC removal and instance migration > ----------------------------------------------------------------------------- > > The only thing that is still not completely clear is (copying from > doc/design-ifdown.rst): > > 3) What should we cleanup/deconfigure? > > Upon NIC hot-remove we obviously want to wipe everything. But on > instance migration we don't want to reset external configuration like > DNS. So we choose to pass an extra positional argument to the ifdown > script (it already has the TAP name) that will reflect the context it > was invoked with. Please note that de-configuration of external > components is not encouraged and should be done via hooks. Still we > could easily support it via this extra argument. > > This is something that is missing in this patch set. > > > Summary > ------- > > I think we could split this patch set and first implement the > ifdown support with the following changes: > > 1) Create TAP files using the NIC's UUID and not it's index. > 2) Store the environment of the NIC in a separate file that can be > sourced by external scripts. > 3) Add the extra argument that will differentiate ifdown functionality > between NIC removal and instance migration. > > Then the MacVTap implementation can follow up. > > Hope the above makes sense, > dimara > >> >> Thanks, >> Dimitris >> >> >> Dimitris Bliablias (28): >> Update MacVTap ddoc to respect the latest changes >> Introduce the 'macvtap' NIC mode >> Introduce the 'macvtap_mode' nicparam >> Create two new helpers in 'lib/query.py' module >> Include 'macvtap_mode' nicparam to queries' output >> Fix wrong description in getNic{Vlan,Link} methods >> Introduce the 'ComputeMacvtapModeNicParam' method >> Expose macvtap_mode nicparam to LUNetworkConnect lu >> Add MacVTap checks in LUInstance{Create,SetParams} >> Rename 'bridges_exist' RPC call to 'netdevs_exist' >> Validate 'macvtap_mode' nicparam at cluster init >> Include macvtap in NIC configuration scripts >> Introduce an ifdown script for the KVM hypervisor >> Make _{Read,Load}KVMRuntime class methods >> Introduce the '_UnconfigureNIC' helper method >> Introduce the '_HandleInstanceNICs' method >> Prepare KVMHypervisor for the ifdown script >> Apply new NIC configuration methods everywhere >> Create a constant for inst communication tap prefix >> Generate TAP names for the macvtap interfaces >> Introduce the OpenMacVTap method for KVMhypervisor >> Create the '_OpenTapHelper' KVM helper function >> Enable the macvtap NIC functionality >> Introduce the 'ListInstancesMacvtapNICs' method >> watcher: Add a method to cleanup MacVTap devices >> Add an extra arg to 'HotModDevice' function >> Add unit tests for the MacVTap NIC mode >> Document the MacVTap feature in man pages >> >> .gitignore | 1 + >> Makefile.am | 7 ++ >> doc/design-macvtap.rst | 56 +++++----- >> lib/backend.py | 34 +++--- >> lib/bootstrap.py | 10 +- >> lib/client/gnt_instance.py | 3 +- >> lib/client/gnt_network.py | 13 ++- >> lib/cmdlib/instance.py | 6 +- >> lib/cmdlib/instance_create.py | 16 ++- >> lib/cmdlib/instance_migration.py | 6 +- >> lib/cmdlib/instance_operation.py | 10 +- >> lib/cmdlib/instance_set_params.py | 38 ++++--- >> lib/cmdlib/instance_utils.py | 61 ++++++++--- >> lib/cmdlib/network.py | 13 +++ >> lib/config/__init__.py | 2 + >> lib/hypervisor/hv_base.py | 51 ++++++--- >> lib/hypervisor/hv_chroot.py | 4 +- >> lib/hypervisor/hv_kvm/__init__.py | 212 >> +++++++++++++++++++++++++++--------- >> lib/hypervisor/hv_kvm/netdev.py | 99 +++++++++++++++-- >> lib/hypervisor/hv_lxc.py | 16 +-- >> lib/hypervisor/hv_xen.py | 3 +- >> lib/objects.py | 2 +- >> lib/pathutils.py | 1 + >> lib/query.py | 47 ++++++++ >> lib/rapi/client.py | 3 +- >> lib/rpc_defs.py | 10 +- >> lib/server/noded.py | 22 ++-- >> lib/utils/__init__.py | 19 +++- >> lib/watcher/__init__.py | 41 +++++++ >> man/ganeti-watcher.rst | 4 + >> man/gnt-cluster.rst | 11 +- >> man/gnt-instance.rst | 19 ++-- >> src/Ganeti/Constants.hs | 42 ++++++- >> src/Ganeti/HTools/Backend/IAlloc.hs | 1 + >> src/Ganeti/HTools/Nic.hs | 2 +- >> src/Ganeti/Objects/Nic.hs | 1 + >> src/Ganeti/OpCodes.hs | 1 + >> src/Ganeti/OpParams.hs | 7 ++ >> src/Ganeti/Query/Instance.hs | 14 +++ >> src/Ganeti/Query/Network.hs | 20 +++- >> src/Ganeti/Types.hs | 1 + >> test/hs/Test/Ganeti/OpCodes.hs | 5 +- >> test/py/cmdlib/instance_unittest.py | 68 +++++++++++- >> tools/kvm-ifdown.in | 45 ++++++++ >> tools/kvm-ifup.in | 1 + >> tools/net-common.in | 14 +++ >> 46 files changed, 844 insertions(+), 218 deletions(-) >> create mode 100755 tools/kvm-ifdown.in >> >> -- >> 2.1.4
