12:57 a.m.
These patches are all closely related, but each fixes a different
problem, so they each should be in a separate patch.
These all came out of Vasiliy's report that type='ethernet tap devices
were all offline and didn't have their IP address or routes added. He
sent a patch (which was an expanded version of 1/1), and the
modification to that patch, along with the other two patches, came out
of my review of his patch.
Laine Stump (2):
qemu: remove unnecessary setting of tap device online state
qemu: set tap device online for type='ethernet'
Vasiliy Tolstov (1):
qemu: fix ethernet network type ip/route assign
src/qemu/qemu_hotplug.c | 15 ---------------
src/qemu/qemu_interface.c | 14 +++++++++-----
2 files changed, 9 insertions(+), 20 deletions(-)
--
2.7.4
12:57 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: fix ethernet network type ip/route assign
From: Vasiliy Tolstov <v.tolstov(a)selfip.ru>
The call to virNetDevIPInfoAddToDev() that sets up tap device IP
addresses and routes was somehow incorrectly placed in
qemuInterfaceStopDevice() instead of qemuInterfaceStartDevice(). This
fixes that error by moving the call to virNetDevIPInfoAddToDev() to
qemuInterfaceStartDevice().
Signed-off-by: Vasiliy Tolstov <v.tolstov(a)selfip.ru>
---
src/qemu/qemu_interface.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index e637d21..e327133 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -108,8 +108,13 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net)
break;
}
- case VIR_DOMAIN_NET_TYPE_USER:
case VIR_DOMAIN_NET_TYPE_ETHERNET:
+ if (virNetDevIPInfoAddToDev(net->ifname, &net->hostIP) < 0)
+ goto cleanup;
+
+ break;
+
+ case VIR_DOMAIN_NET_TYPE_USER:
case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
case VIR_DOMAIN_NET_TYPE_SERVER:
case VIR_DOMAIN_NET_TYPE_CLIENT:
@@ -197,10 +202,6 @@ qemuInterfaceStopDevice(virDomainNetDefPtr net)
}
case VIR_DOMAIN_NET_TYPE_ETHERNET:
- if (virNetDevIPInfoAddToDev(net->ifname, &net->hostIP) < 0)
- goto cleanup;
- break;
-
case VIR_DOMAIN_NET_TYPE_USER:
case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
case VIR_DOMAIN_NET_TYPE_SERVER:
--
2.7.4
12:57 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
The linkstate setting of an <interface> is only meant to change the
online status reported to the guest system by the emulated network
device driver in qemu, but when support for auto-creating tap devices
for <interface type='ethernet'> was added in commit 9717d6, a chunk of
code was also added to qemuDomainChangeNetLinkState() that sets the
online status of the tap device (i.e. the *host* side of the
interface) for type='ethernet'. This was never done for tap devices
used in type='bridge' or type='network' interfaces, nor was it done in
the past for tap devices created by external scripts for
type='ethernet', so we shouldn't be doing it now.
This patch removes the bit of code in qemuDomainChangeNetLinkState()
that modifies online status of the tap device.
---
src/qemu/qemu_hotplug.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 00e4a75..5300bc1 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2324,21 +2324,6 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr driver,
if (ret < 0)
goto cleanup;
- if (virDomainNetGetActualType(dev) == VIR_DOMAIN_NET_TYPE_ETHERNET) {
- switch (linkstate) {
- case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
- case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
- if ((ret = virNetDevSetOnline(dev->ifname, true)) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
- if ((ret = virNetDevSetOnline(dev->ifname, false)) < 0)
- goto cleanup;
- break;
- }
- }
-
/* modify the device configuration */
dev->linkstate = linkstate;
--
2.7.4
3:55 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
Dear Laine,
Laine Stump <laine(a)laine.org> writes:
The linkstate setting of an <interface> is only meant to change
the
online status reported to the guest system by the emulated network
device driver in qemu, but when support for auto-creating tap devices
for <interface type='ethernet'> was added in commit 9717d6, a chunk of
Typo: the commit id is 9c17d6. Including the title of the commit would
have made locating it easier. At first I thought it wasn't even upstream
yet.
(Haven't looked at the actual changes, was mostly interested in what
released versions were affected.)
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
6:01 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
On 08/25/2016 04:55 AM, Sascha Silbe wrote:
Dear Laine,
Laine Stump <laine(a)laine.org> writes:
> The linkstate setting of an <interface> is only meant to change the
> online status reported to the guest system by the emulated network
> device driver in qemu, but when support for auto-creating tap devices
> for <interface type='ethernet'> was added in commit 9717d6, a chunk of
Typo: the commit id is 9c17d6. Including the title of the commit would
have made locating it easier. At first I thought it wasn't even upstream
yet.
Thanks for pointing that out. I fixed it in the commit log before I pushed.
4:41 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
25 Авг 2016 г. 8:58 пользователь "Laine Stump" <laine(a)laine.org> написал:
The linkstate setting of an <interface> is only meant to change the
online status reported to the guest system by the emulated network
device driver in qemu,
I need to set host side status of interface. Without this live migration
with dinamic routing software (ospf with quagga or bird) bring packet
drops. Because on dest interface in up state and kernel try to forward
packets to it, but guest CPU is not running. Also host side status needed
for easy blackhole traffic to guest ip.
but when support for auto-creating tap devices
for <interface type='ethernet'> was added in commit 9717d6, a chunk of
code was also added to qemuDomainChangeNetLinkState() that sets the
online status of the tap device (i.e. the *host* side of the
interface) for type='ethernet'. This was never done for tap devices
used in type='bridge' or type='network' interfaces, nor was it done in
the past for tap devices created by external scripts for
type='ethernet', so we shouldn't be doing it now.
This patch removes the bit of code in qemuDomainChangeNetLinkState()
that modifies online status of the tap device.
---
src/qemu/qemu_hotplug.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 00e4a75..5300bc1 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2324,21 +2324,6 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr
driver,
if (ret < 0)
goto cleanup;
- if (virDomainNetGetActualType(dev) == VIR_DOMAIN_NET_TYPE_ETHERNET) {
- switch (linkstate) {
- case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
- case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
- if ((ret = virNetDevSetOnline(dev->ifname, true)) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
- if ((ret = virNetDevSetOnline(dev->ifname, false)) < 0)
- goto cleanup;
- break;
- }
- }
-
/* modify the device configuration */
dev->linkstate = linkstate;
--
2.7.4
4:42 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov <v.tolstov(a)selfip.ru>
написал:
25 Авг 2016 г. 8:58 пользователь "Laine Stump" <laine(a)laine.org>
написал:
>
> The linkstate setting of an <interface> is only meant to change the
> online status reported to the guest system by the emulated network
> device driver in qemu,
I need to set host side status of interface. Without this live migration
with
dinamic routing software (ospf with quagga or bird) bring packet
drops. Because on dest interface in up state and kernel try to forward
packets to it, but guest CPU is not running. Also host side status needed
for easy blackhole traffic to guest ip.
May be create inside the source link state attribute for host side link
status? So it consistent with ip and route elements?
> but when support for auto-creating tap devices
> for <interface type='ethernet'> was added in commit 9717d6, a chunk
of
> code was also added to qemuDomainChangeNetLinkState() that sets the
> online status of the tap device (i.e. the *host* side of the
> interface) for type='ethernet'. This was never done for tap devices
> used in type='bridge' or type='network' interfaces, nor was it done
in
> the past for tap devices created by external scripts for
> type='ethernet', so we shouldn't be doing it now.
>
> This patch removes the bit of code in qemuDomainChangeNetLinkState()
> that modifies online status of the tap device.
> ---
> src/qemu/qemu_hotplug.c | 15 ---------------
> 1 file changed, 15 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 00e4a75..5300bc1 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2324,21 +2324,6 @@ int
qemuDomainChangeNetLinkState(virQEMUDriverPtr
driver,
> if (ret < 0)
> goto cleanup;
>
> - if (virDomainNetGetActualType(dev) ==
VIR_DOMAIN_NET_TYPE_ETHERNET) {
> - switch (linkstate) {
> - case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
> - case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
> - if ((ret = virNetDevSetOnline(dev->ifname, true)) < 0)
> - goto cleanup;
> - break;
> -
> - case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
> - if ((ret = virNetDevSetOnline(dev->ifname, false)) < 0)
> - goto cleanup;
> - break;
> - }
> - }
> -
> /* modify the device configuration */
> dev->linkstate = linkstate;
>
> --
> 2.7.4
>
11:18 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
On 08/25/2016 05:42 AM, Vasiliy Tolstov wrote:
25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov <v.tolstov(a)selfip.ru
<mailto:v.tolstov@selfip.ru>> написал:
>
> 25 Авг 2016 г. 8:58 пользователь "Laine Stump" <laine(a)laine.org
<mailto:laine@laine.org>> написал:
> >
> > The linkstate setting of an <interface> is only meant to change the
> > online status reported to the guest system by the emulated network
> > device driver in qemu,
>
I need to set host side status of interface. Without this live
migration with dinamic routing software (ospf with quagga or bird)
bring packet drops. Because on dest interface in up state and kernel
try to forward packets to it, but guest CPU is not running.
That shouldn't be a problem, since the IPInfo isn't added to the tap
device until immediately before the guest CPU is started on the
destination (that's the purpose of qemuInterfaceStartDevice()).
Also host side status needed for easy blackhole traffic to guest ip.
Is this something you need to do while the guest is already running? If
not, then I think we don't need anything extra.
May be create inside the source link state attribute for host side
link status? So it consistent with ip and route elements?
If necessary, that might be the right solution, although I still think
it's better to not set the tap device offline, in case it's connected to
a bridge - we wouldn't want to trigger an STP forward delay. Maybe just
delete (and later re-add) the IPInfo would be less disruptive? (Or it
might be *more* disruptive, we'd have to try both).
>
> > but when support for auto-creating tap devices
> > for <interface type='ethernet'> was added in commit 9717d6, a
chunk of
> > code was also added to qemuDomainChangeNetLinkState() that sets the
> > online status of the tap device (i.e. the *host* side of the
> > interface) for type='ethernet'. This was never done for tap devices
> > used in type='bridge' or type='network' interfaces, nor was it
done in
> > the past for tap devices created by external scripts for
> > type='ethernet', so we shouldn't be doing it now.
> >
> > This patch removes the bit of code in qemuDomainChangeNetLinkState()
> > that modifies online status of the tap device.
> > ---
> > src/qemu/qemu_hotplug.c | 15 ---------------
> > 1 file changed, 15 deletions(-)
> >
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index 00e4a75..5300bc1 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -2324,21 +2324,6 @@ int
qemuDomainChangeNetLinkState(virQEMUDriverPtr driver,
> > if (ret < 0)
> > goto cleanup;
> >
> > - if (virDomainNetGetActualType(dev) ==
VIR_DOMAIN_NET_TYPE_ETHERNET) {
> > - switch (linkstate) {
> > - case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
> > - case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
> > - if ((ret = virNetDevSetOnline(dev->ifname, true))
< 0)
> > - goto cleanup;
> > - break;
> > -
> > - case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
> > - if ((ret = virNetDevSetOnline(dev->ifname,
false)) < 0)
> > - goto cleanup;
> > - break;
> > - }
> > - }
> > -
> > /* modify the device configuration */
> > dev->linkstate = linkstate;
> >
> > --
> > 2.7.4
> >
1:19 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
25 Авг 2016 г. 19:18 пользователь "Laine Stump" <laine(a)laine.org>
написал:
Thanks for info.
Why not add ability to specify device state from host side? If attribute is
empty think that device is up. This is reasonable default. I'm use link
status when vm running, for example if we have ddos - I down tap and via
ospf route deleted and traffic blackholed.
>
On 08/25/2016 05:42 AM, Vasiliy Tolstov wrote:
>
> 25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov <v.tolstov(a)selfip.ru>
написал:
> >
> > 25 Авг 2016 г. 8:58 пользователь "Laine Stump" <laine(a)laine.org>
написал:
> > >
> > > The linkstate setting of an <interface> is only meant to change the
> > > online status reported to the guest system by the emulated network
> > > device driver in qemu,
> >
> I need to set host side status of interface. Without this live migration
with
dinamic routing software (ospf with quagga or bird) bring packet
drops. Because on dest interface in up state and kernel try to forward
packets to it, but guest CPU is not running.
That shouldn't be a problem, since the IPInfo isn't added to the tap
device
until immediately before the guest CPU is started on the destination
(that's the purpose of qemuInterfaceStartDevice()).
> Also host side status needed for easy blackhole traffic to guest ip.
Is this something you need to do while the guest is already running? If
not, then I
think we don't need anything extra.
> May be create inside the source link state attribute for host side link
status?
So it consistent with ip and route elements?
If necessary, that might be the right solution, although I still think
it's
better to not set the tap device offline, in case it's connected to a
bridge - we wouldn't want to trigger an STP forward delay. Maybe just
delete (and later re-add) the IPInfo would be less disruptive? (Or it might
be *more* disruptive, we'd have to try both).
> >
> > > but when support for auto-creating tap devices
> > > for <interface type='ethernet'> was added in commit 9717d6, a
chunk
of
> > > code was also added to qemuDomainChangeNetLinkState()
that sets the
> > > online status of the tap device (i.e. the *host* side of the
> > > interface) for type='ethernet'. This was never done for tap
devices
> > > used in type='bridge' or type='network' interfaces, nor was
it done
in
> > > the past for tap devices created by external scripts
for
> > > type='ethernet', so we shouldn't be doing it now.
> > >
> > > This patch removes the bit of code in qemuDomainChangeNetLinkState()
> > > that modifies online status of the tap device.
> > > ---
> > > src/qemu/qemu_hotplug.c | 15 ---------------
> > > 1 file changed, 15 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > > index 00e4a75..5300bc1 100644
> > > --- a/src/qemu/qemu_hotplug.c
> > > +++ b/src/qemu/qemu_hotplug.c
> > > @@ -2324,21 +2324,6 @@ int
qemuDomainChangeNetLinkState(virQEMUDriverPtr driver,
> > > if (ret < 0)
> > > goto cleanup;
> > >
> > > - if (virDomainNetGetActualType(dev) ==
VIR_DOMAIN_NET_TYPE_ETHERNET) {
> > > - switch (linkstate) {
> > > - case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
> > > - case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
> > > - if ((ret = virNetDevSetOnline(dev->ifname, true)) <
0)
> > > - goto cleanup;
> > > - break;
> > > -
> > > - case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
> > > - if ((ret = virNetDevSetOnline(dev->ifname, false))
< 0)
>> > > - goto cleanup;
>> > > - break;
>> > > - }
>> > > - }
>> > > -
>> > > /* modify the device configuration */
>> > > dev->linkstate = linkstate;
>> > >
>> > > --
>> > > 2.7.4
>> >
1:38 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
On 08/25/2016 02:19 PM, Vasiliy Tolstov wrote:
25 Авг 2016 г. 19:18 пользователь "Laine Stump" <laine(a)laine.org
<mailto:laine@laine.org>> написал:
>
> On 08/25/2016 05:42 AM, Vasiliy Tolstov wrote:
>>
>> 25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov
<v.tolstov(a)selfip.ru <mailto:v.tolstov@selfip.ru>> написал:
>> >
>> > 25 Авг 2016 г. 8:58 пользователь "Laine Stump"
<laine(a)laine.org
<mailto:laine@laine.org>> написал:
>> > >
>> > > The linkstate setting of an <interface> is only meant to change
the
>> > > online status reported to the guest system by the emulated network
>> > > device driver in qemu,
>> >
>> I need to set host side status of interface. Without this live
migration with dinamic routing software (ospf with quagga or bird)
bring packet drops. Because on dest interface in up state and kernel
try to forward packets to it, but guest CPU is not running.
>
>
> That shouldn't be a problem, since the IPInfo isn't added to the tap
device until immediately before the guest CPU is started on the
destination (that's the purpose of qemuInterfaceStartDevice()).
>
>
>
>> Also host side status needed for easy blackhole traffic to guest ip.
>
>
> Is this something you need to do while the guest is already running?
If not, then I think we don't need anything extra.
>
>
>
>> May be create inside the source link state attribute for host side
link status? So it consistent with ip and route elements?
>
>
> If necessary, that might be the right solution, although I still
think it's better to not set the tap device offline, in case it's
connected to a bridge - we wouldn't want to trigger an STP forward
delay. Maybe just delete (and later re-add) the IPInfo would be less
disruptive? (Or it might be *more* disruptive, we'd have to try both).
>
>
Thanks for info.
Why not add ability to specify device state from host side? If
attribute is empty think that device is up. This is reasonable
default. I'm use link status when vm running, for example if we have
ddos - I down tap and via ospf route deleted and traffic blackholed.
Yeah, I can see the utility of that. And as long as the default is up,
then nobody is surprised by the results.
So what we're talking about is a new subelement of <source> for any
tap-based interface type (at least):
<interface type='ethernet|bridge|network'>
<source>
<link state='down'/>
</source>
Since it also needs to be supported for qemuDomainChangeNet(), I'm
doubtful this can be done prior to the freeze tonight or early
tomorrow, since DV is in China) though. So will it be okay to have the
patches I've made in this release (which should handle proper operation
for everything except the "need to modify the state while the guest is
running" case)? I don't think any of that will need to be *un*done to
support this new attribute, and it would be nice to have something in
the next release that works at least in the default situation...
2:49 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
25 Авг 2016 г. 21:38 пользователь "Laine Stump" <laine(a)laine.org>
написал:
On 08/25/2016 02:19 PM, Vasiliy Tolstov wrote:
>
> 25 Авг 2016 г. 19:18 пользователь "Laine Stump" <laine(a)laine.org>
написал:
> >
> > On 08/25/2016 05:42 AM, Vasiliy Tolstov wrote:
> >>
> >> 25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov
<v.tolstov(a)selfip.ru>
написал:
> >> >
> >> > 25 Авг 2016 г. 8:58 пользователь "Laine Stump"
<laine(a)laine.org>
написал:
> >> > >
> >> > > The linkstate setting of an <interface> is only meant to
change
the
> >> > > online status reported to the guest system by
the emulated network
> >> > > device driver in qemu,
> >> >
> >> I need to set host side status of interface. Without this live
migration with dinamic routing software (ospf with quagga or bird) bring
packet drops. Because on dest interface in up state and kernel try to
forward packets to it, but guest CPU is not running.
> >
> >
> > That shouldn't be a problem, since the IPInfo isn't added to the tap
device until immediately before the guest CPU is started on the destination
(that's the purpose of qemuInterfaceStartDevice()).
> >
> >
> >
> >> Also host side status needed for easy blackhole traffic to guest ip.
> >
> >
> > Is this something you need to do while the guest is already running?
If
not, then I think we don't need anything extra.
> >
> >
> >
> >> May be create inside the source link state attribute for host side
link status? So it consistent with ip and route elements?
> >
> >
> > If necessary, that might be the right solution, although I still think
it's better to not set the tap device offline, in case it's connected to a
bridge - we wouldn't want to trigger an STP forward delay. Maybe just
delete (and later re-add) the IPInfo would be less disruptive? (Or it might
be *more* disruptive, we'd have to try both).
> >
> >
> Thanks for info.
> Why not add ability to specify device state from host side? If attribute
is
empty think that device is up. This is reasonable default. I'm use link
status when vm running, for example if we have ddos - I down tap and via
ospf route deleted and traffic blackholed.
Yeah, I can see the utility of that. And as long as the default is up,
then nobody
is surprised by the results.
So what we're talking about is a new subelement of <source> for any
tap-based interface type (at least):
<interface type='ethernet|bridge|network'>
<source>
<link state='down'/>
</source>
Since it also needs to be supported for qemuDomainChangeNet(), I'm
doubtful
this can be done prior to the freeze tonight or early tomorrow,
since DV is in China) though. So will it be okay to have the patches I've
made in this release (which should handle proper operation for everything
except the "need to modify the state while the guest is running" case)? I
don't think any of that will need to be *un*done to support this new
attribute, and it would be nice to have something in the next release that
works at least in the default situation...
Thanks, I'm happy with this.
3:01 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
On 08/25/2016 03:49 PM, Vasiliy Tolstov wrote:
25 Авг 2016 г. 21:38 пользователь "Laine Stump" <laine(a)laine.org
<mailto:laine@laine.org>> написал:
>
> On 08/25/2016 02:19 PM, Vasiliy Tolstov wrote:
>>
>> 25 Авг 2016 г. 19:18 пользователь "Laine Stump" <laine(a)laine.org
<mailto:laine@laine.org>> написал:
>> >
>> > On 08/25/2016 05:42 AM, Vasiliy Tolstov wrote:
>> >>
>> >> 25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov
<v.tolstov(a)selfip.ru <mailto:v.tolstov@selfip.ru>> написал:
>> >> >
>> >> > 25 Авг 2016 г. 8:58 пользователь "Laine Stump"
<laine(a)laine.org <mailto:laine@laine.org>> написал:
>> >> > >
>> >> > > The linkstate setting of an <interface> is only meant
to
change the
>> >> > > online status reported to the guest system by the emulated
network
>> >> > > device driver in qemu,
>> >> >
>> >> I need to set host side status of interface. Without this live
migration with dinamic routing software (ospf with quagga or bird)
bring packet drops. Because on dest interface in up state and kernel
try to forward packets to it, but guest CPU is not running.
>> >
>> >
>> > That shouldn't be a problem, since the IPInfo isn't added to the
tap device until immediately before the guest CPU is started on the
destination (that's the purpose of qemuInterfaceStartDevice()).
>> >
>> >
>> >
>> >> Also host side status needed for easy blackhole traffic to guest ip.
>> >
>> >
>> > Is this something you need to do while the guest is already
running? If not, then I think we don't need anything extra.
>> >
>> >
>> >
>> >> May be create inside the source link state attribute for host
side link status? So it consistent with ip and route elements?
>> >
>> >
>> > If necessary, that might be the right solution, although I still
think it's better to not set the tap device offline, in case it's
connected to a bridge - we wouldn't want to trigger an STP forward
delay. Maybe just delete (and later re-add) the IPInfo would be less
disruptive? (Or it might be *more* disruptive, we'd have to try both).
>> >
>> >
>> Thanks for info.
>> Why not add ability to specify device state from host side? If
attribute is empty think that device is up. This is reasonable
default. I'm use link status when vm running, for example if we have
ddos - I down tap and via ospf route deleted and traffic blackholed.
>
>
> Yeah, I can see the utility of that. And as long as the default is
up, then nobody is surprised by the results.
>
> So what we're talking about is a new subelement of <source> for any
tap-based interface type (at least):
>
> <interface type='ethernet|bridge|network'>
> <source>
> <link state='down'/>
> </source>
>
> Since it also needs to be supported for qemuDomainChangeNet(), I'm
doubtful this can be done prior to the freeze tonight or early
tomorrow, since DV is in China) though. So will it be okay to have the
patches I've made in this release (which should handle proper
operation for everything except the "need to modify the state while
the guest is running" case)? I don't think any of that will need to be
*un*done to support this new attribute, and it would be nice to have
something in the next release that works at least in the default
situation...
Thanks, I'm happy with this.
Can you ACK Patches 2/3 and 3/3?
10:36 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
On 08/25/2016 05:41 AM, Vasiliy Tolstov wrote:
25 Авг 2016 г. 8:58 пользователь "Laine Stump" <laine(a)laine.org
<mailto:laine@laine.org>> написал:
>
> The linkstate setting of an <interface> is only meant to change the
> online status reported to the guest system by the emulated network
> device driver in qemu,
I need to set host side status of interface. Without this live
migration with dinamic routing software (ospf with quagga or bird)
bring packet drops. Because on dest interface in up state and kernel
try to forward packets to it, but guest CPU is not running. Also host
side status needed for easy blackhole traffic to guest ip.
(tl;dr - it doesn't seem to me like this should be a problem, since we
don't add the IP addresses or routes to the tap device until just before
starting the guest CPUs)
Hmm, so a L3 analog to the problem that we have at L2 with macvtap
interfaces (the presence of an IFF_UP interface with the same MAC
address as the guest causes traffic for that MAC to be sent to the
destination too soon.
When connecting a tap device to a bridge, it's important for it to be
IFF_UP as soon as possible, because the STP forward delay timer doesn't
start until it's IFF_UP (and since the MAC address of the tap device
itself isn't used for forwarding any traffic, there's no harm to an L2
forwarding tables caused by this).
Of course in the case of taps connected to bridges, we don't have any IP
address set, and also no routes set (although I'm wondering if in the
future we might have routes (but still no IPs)), so we never encounter
the issue with L3 forwarding that we do for type='ethernet'.
Setting the tap device offline does have the effect of eliminating all
IP addresses and routes in a single operation, which works properly for
you. But if that tap happened to be connected to a bridge (outside the
scope of libvirt), waiting to set it IFF_UP could result in a much
longer-than-desired wait before the interface was usable.
But of course if we're not adding the routes and IPs until
qemuInterfaceStartDevice(), the issue wouldn't exist at domain start
time - that function isn't called until right before the CPUs are
started, which is exactly when you want it, so there shouldn't be any
case where either the IP address of the tap device or the routes
associated with it are visible prior to the exact time when you want it
to happen.
There is one issue that may still need to be addressed - there are a few
cases where we stop the guest CPUs temporarily, and then restart them;
qemuInterfaceStopDevice *is* called before the CPUs are stopped, but
because we don't have anything in there to remove the routes or IPs on
the tap device, it would still be seen as a destination for the given
IPs during this time. I'm not sure this is really a problem though,
because we do fully intend to start the same CPU up again and in the
meantime there isn't any other valid destination for the traffic -
removing and re-adding the routes during, e.g a
qemuDomainRevertToSnapshot() would only have the effect of causing a
mini (and single iteration) route flap. So I don't think anything needs
to be done about this either.
Does this all make sense, or am I missing something?
4:42 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state
2016-08-25 8:57 GMT+03:00 Laine Stump <laine(a)laine.org>:
The linkstate setting of an <interface> is only meant to change
the
online status reported to the guest system by the emulated network
device driver in qemu, but when support for auto-creating tap devices
for <interface type='ethernet'> was added in commit 9717d6, a chunk of
code was also added to qemuDomainChangeNetLinkState() that sets the
online status of the tap device (i.e. the *host* side of the
interface) for type='ethernet'. This was never done for tap devices
used in type='bridge' or type='network' interfaces, nor was it done in
the past for tap devices created by external scripts for
type='ethernet', so we shouldn't be doing it now.
This patch removes the bit of code in qemuDomainChangeNetLinkState()
that modifies online status of the tap device.
---
src/qemu/qemu_hotplug.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 00e4a75..5300bc1 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2324,21 +2324,6 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr driver,
if (ret < 0)
goto cleanup;
- if (virDomainNetGetActualType(dev) == VIR_DOMAIN_NET_TYPE_ETHERNET) {
- switch (linkstate) {
- case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
- case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
- if ((ret = virNetDevSetOnline(dev->ifname, true)) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
- if ((ret = virNetDevSetOnline(dev->ifname, false)) < 0)
- goto cleanup;
- break;
- }
- }
-
/* modify the device configuration */
dev->linkstate = linkstate;
--
2.7.4
ACK
--
Vasiliy Tolstov,
e-mail: v.tolstov(a)selfip.ru
12:58 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: set tap device online for type='ethernet'
When support for auto-creating tap devices was added to <interface
type='ethernet'> in commit 9717d6, the code assumed that
virNetDevTapCreate() would honor the *_CREATE_IFUP flag that is
supported by virNetDevTapCreateInBridgePort(). That isn't the case -
the latter function performs several operations, and one of them is
setting the tap device online. But virNetDevTapCreate() *only* creates
the tap device, and relies on the caller to do everything else, so
qemuInterfaceEthernetConnect() needs to call virNetDevSetOnline()
after the device is successfully created.
---
src/qemu/qemu_interface.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index e327133..455c2d0 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -453,6 +453,9 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
if (virNetDevSetMAC(net->ifname, &tapmac) < 0)
goto cleanup;
+ if (virNetDevSetOnline(net->ifname, true) < 0)
+ goto cleanup;
+
if (net->script &&
virNetDevRunEthernetScript(net->ifname, net->script) < 0)
goto cleanup;
--
2.7.4
4:43 p.m.
New subject: [libvirt] [PATCH 3/3] qemu: set tap device online for type='ethernet'
2016-08-25 8:58 GMT+03:00 Laine Stump <laine(a)laine.org>:
When support for auto-creating tap devices was added to
<interface
type='ethernet'> in commit 9717d6, the code assumed that
virNetDevTapCreate() would honor the *_CREATE_IFUP flag that is
supported by virNetDevTapCreateInBridgePort(). That isn't the case -
the latter function performs several operations, and one of them is
setting the tap device online. But virNetDevTapCreate() *only* creates
the tap device, and relies on the caller to do everything else, so
qemuInterfaceEthernetConnect() needs to call virNetDevSetOnline()
after the device is successfully created.
---
src/qemu/qemu_interface.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index e327133..455c2d0 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -453,6 +453,9 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
if (virNetDevSetMAC(net->ifname, &tapmac) < 0)
goto cleanup;
+ if (virNetDevSetOnline(net->ifname, true) < 0)
+ goto cleanup;
+
if (net->script &&
virNetDevRunEthernetScript(net->ifname, net->script) < 0)
goto cleanup;
--
2.7.4
ACK
--
Vasiliy Tolstov,
e-mail: v.tolstov(a)selfip.ru
6:01 p.m.
On 08/25/2016 01:57 AM, Laine Stump wrote:
These patches are all closely related, but each fixes a different
problem, so they each should be in a separate patch.
These all came out of Vasiliy's report that type='ethernet tap devices
were all offline and didn't have their IP address or routes added. He
sent a patch (which was an expanded version of 1/1), and the
modification to that patch, along with the other two patches, came out
of my review of his patch.
Laine Stump (2):
qemu: remove unnecessary setting of tap device online state
qemu: set tap device online for type='ethernet'
Vasiliy Tolstov (1):
qemu: fix ethernet network type ip/route assign
Okay, I've pushed these three patches so there will be working
type='ethernet' interfaces in the next release, and I'm working on
patched that add:
<source>
<link state='down'/>
...
</source>
to allow controlling the link state of the host side of the interface
(the tap device in qemu, or the host side of the veth pair in lxc).
Thanks to Vasiliy for the report, patch, reviews, explanations, and idea
to add host-side link state!
3011
days inactive
3011
days old
16 comments
3 participants
participants (3)
-
Laine Stump -
Sascha Silbe -
Vasiliy Tolstov