monitor: remove QAPI_EVENT_VSERPORT_CHANGE throttle

From: Marc-André Lureau <marcandre.lureau@redhat.com>

QAPI_EVENT_VSERPORT_CHANGE reports changes of a virtio serial port
state. However, the events may be for different ports, but the throttle
mechanism may replace the event for a different port, since it only
checks the event type.

libvirt relies on a correct state to be reported for all channels: the
qemu-ga commands may no longer work if the state is reported
disconnected. This can be triggered easily by having more than 1
virtio-serial (qemu-ga + spice agent for example), and restarting
quickly daemons or more realistically going quickly in and out of
suspend.

In a future patch, we may want to throttle events based on their
arguments, but this will likely require dynamic allocations and more
complicated code to insert/lookup pending events based on various
arguments ("id" in QAPI_EVENT_VSERPORT_CHANGE case).

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1244064

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 1 -
 1 file changed, 1 deletion(-)

On Tue, Aug 11, 2015 at 07:04:07PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> QAPI_EVENT_VSERPORT_CHANGE reports changes of a virtio serial port
> state. However, the events may be for different ports, but the throttle
> mechanism may replace the event for a different port, since it only
> checks the event type.
> 
> libvirt relies on a correct state to be reported for all channels: the
> qemu-ga commands may no longer work if the state is reported
> disconnected. This can be triggered easily by having more than 1
> virtio-serial (qemu-ga + spice agent for example), and restarting
> quickly daemons or more realistically going quickly in and out of
> suspend.
> 
> In a future patch, we may want to throttle events based on their
> arguments, but this will likely require dynamic allocations and more
> complicated code to insert/lookup pending events based on various
> arguments ("id" in QAPI_EVENT_VSERPORT_CHANGE case).
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1244064
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

> diff --git a/monitor.c b/monitor.c
> index aeea2b5..e4d56f7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -558,7 +558,6 @@ static void monitor_qapi_event_init(void)
>      monitor_qapi_event_throttle(QAPI_EVENT_BALLOON_CHANGE, 1000);
>      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);
>      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000);
> -    monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
>  
>      qmp_event_set_func_emit(monitor_qapi_event_queue);
>  }

I wonder if we should add a big warnng comment here saying only
to add events if they don't have context sensitive args.


Regards,
Daniel

On 08/11/15 19:04, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> QAPI_EVENT_VSERPORT_CHANGE reports changes of a virtio serial port
> state. However, the events may be for different ports, but the throttle
> mechanism may replace the event for a different port, since it only
> checks the event type.
> 
> libvirt relies on a correct state to be reported for all channels: the
> qemu-ga commands may no longer work if the state is reported
> disconnected. This can be triggered easily by having more than 1
> virtio-serial (qemu-ga + spice agent for example), and restarting
> quickly daemons or more realistically going quickly in and out of
> suspend.
> 
> In a future patch, we may want to throttle events based on their
> arguments, but this will likely require dynamic allocations and more
> complicated code to insert/lookup pending events based on various
> arguments ("id" in QAPI_EVENT_VSERPORT_CHANGE case).
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1244064
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/monitor.c b/monitor.c
> index aeea2b5..e4d56f7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -558,7 +558,6 @@ static void monitor_qapi_event_init(void)
>      monitor_qapi_event_throttle(QAPI_EVENT_BALLOON_CHANGE, 1000);
>      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);
>      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000);
> -    monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
>  
>      qmp_event_set_func_emit(monitor_qapi_event_queue);
>  }
> 

I don't mind the change (and the point of argument sensitivity is not
lost on me), but note that this undoes the protection that is spelled
out in the leading comment of the function, not visible in the context here:

/* Limit guest-triggerable events to 1 per second */

That was probably put in place in order to prevent a "malicious" guest
from spamming the log files (and the CPU usage) of the management apps.

One solution to that would be arg sensitivity. Another would be a
burst-capable (ie. a slowly re-filling, limited size token bucket)
throttle, maintained per event type.

Until one of those gets written, I guess this patch is acceptable -- as
long as mgmt people are okay with it. Daniel seems to be, so I don't mind.

Thanks
Laszlo

On Tue, Aug 11, 2015 at 08:21:18PM +0200, Laszlo Ersek wrote:
> On 08/11/15 19:04, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > QAPI_EVENT_VSERPORT_CHANGE reports changes of a virtio serial port
> > state. However, the events may be for different ports, but the throttle
> > mechanism may replace the event for a different port, since it only
> > checks the event type.
> > 
> > libvirt relies on a correct state to be reported for all channels: the
> > qemu-ga commands may no longer work if the state is reported
> > disconnected. This can be triggered easily by having more than 1
> > virtio-serial (qemu-ga + spice agent for example), and restarting
> > quickly daemons or more realistically going quickly in and out of
> > suspend.
> > 
> > In a future patch, we may want to throttle events based on their
> > arguments, but this will likely require dynamic allocations and more
> > complicated code to insert/lookup pending events based on various
> > arguments ("id" in QAPI_EVENT_VSERPORT_CHANGE case).
> > 
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1244064
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  monitor.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index aeea2b5..e4d56f7 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -558,7 +558,6 @@ static void monitor_qapi_event_init(void)
> >      monitor_qapi_event_throttle(QAPI_EVENT_BALLOON_CHANGE, 1000);
> >      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);
> >      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000);
> > -    monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
> >  
> >      qmp_event_set_func_emit(monitor_qapi_event_queue);
> >  }
> > 
> 
> I don't mind the change (and the point of argument sensitivity is not
> lost on me), but note that this undoes the protection that is spelled
> out in the leading comment of the function, not visible in the context here:
> 
> /* Limit guest-triggerable events to 1 per second */
> 
> That was probably put in place in order to prevent a "malicious" guest
> from spamming the log files (and the CPU usage) of the management apps.
> 
> One solution to that would be arg sensitivity. Another would be a
> burst-capable (ie. a slowly re-filling, limited size token bucket)
> throttle, maintained per event type.
> 
> Until one of those gets written, I guess this patch is acceptable -- as
> long as mgmt people are okay with it. Daniel seems to be, so I don't mind.

Not having rate limiting is certainly an undesirable situation, but
rate limiting which discards unrecoverable data is even worse as it
makes the event useless for the app. We've got a number of events which
are not rate limited for this reason. Ultimately we do need to figure
out a better way to rate limit such events, but until then we have no
option but to skip rate limiting for events which have this issue.

NB, some events which have args are acceptable to rate limit. eg the
balloon change event is fine, because apps generally only care about
the /current/ balloon level, so if 3 balloon events are emitted and
the first 2 are discarded that is fine, because the most recent balloon
level is the only one that matters.

Regards,
Daniel

On (Tue) 11 Aug 2015 [20:21:18], Laszlo Ersek wrote:
> On 08/11/15 19:04, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > QAPI_EVENT_VSERPORT_CHANGE reports changes of a virtio serial port
> > state. However, the events may be for different ports, but the throttle
> > mechanism may replace the event for a different port, since it only
> > checks the event type.
> > 
> > libvirt relies on a correct state to be reported for all channels: the
> > qemu-ga commands may no longer work if the state is reported
> > disconnected. This can be triggered easily by having more than 1
> > virtio-serial (qemu-ga + spice agent for example), and restarting
> > quickly daemons or more realistically going quickly in and out of
> > suspend.
> > 
> > In a future patch, we may want to throttle events based on their
> > arguments, but this will likely require dynamic allocations and more
> > complicated code to insert/lookup pending events based on various
> > arguments ("id" in QAPI_EVENT_VSERPORT_CHANGE case).
> > 
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1244064
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  monitor.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index aeea2b5..e4d56f7 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -558,7 +558,6 @@ static void monitor_qapi_event_init(void)
> >      monitor_qapi_event_throttle(QAPI_EVENT_BALLOON_CHANGE, 1000);
> >      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);
> >      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000);
> > -    monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
> >  
> >      qmp_event_set_func_emit(monitor_qapi_event_queue);
> >  }
> > 
> 
> I don't mind the change (and the point of argument sensitivity is not
> lost on me), but note that this undoes the protection that is spelled
> out in the leading comment of the function, not visible in the context here:
> 
> /* Limit guest-triggerable events to 1 per second */
> 
> That was probably put in place in order to prevent a "malicious" guest
> from spamming the log files (and the CPU usage) of the management apps.
> 
> One solution to that would be arg sensitivity. Another would be a
> burst-capable (ie. a slowly re-filling, limited size token bucket)
> throttle, maintained per event type.
> 
> Until one of those gets written, I guess this patch is acceptable -- as
> long as mgmt people are okay with it. Daniel seems to be, so I don't mind.

OK - so I'll queue it.

Thanks,

		Amit