Hi, * Dimitris Bliablias <db...@skroutz.gr> [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
signature.asc
Description: Digital signature
