9:56 a.m.
On 04.03.2014 16:46, Michal Privoznik wrote:
On 04.03.2014 12:56, Daniel P. Berrange wrote:
> On Mon, Mar 03, 2014 at 06:22:44PM +0100, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=981729
>>
>> This config tunable allows users to determine the maximum number of
>> accepted but yet not authenticated users.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> daemon/libvirtd-config.c | 1 +
>> daemon/libvirtd-config.h | 1 +
>> daemon/libvirtd.aug | 1 +
>> daemon/libvirtd.c | 1 +
>> daemon/libvirtd.conf | 4 ++++
>> daemon/test_libvirtd.aug.in | 1 +
>> src/locking/lock_daemon.c | 3 +--
>> src/lxc/lxc_controller.c | 2 +-
>> src/rpc/virnetserver.c | 52
>> +++++++++++++++++++++++++++++++++++++++++++--
>> src/rpc/virnetserver.h | 1 +
>> 10 files changed, 62 insertions(+), 5 deletions(-)
>>
>> diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
>> index c816fda..04482c5 100644
>> --- a/daemon/libvirtd-config.c
>> +++ b/daemon/libvirtd-config.c
>> @@ -415,6 +415,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
>> GET_CONF_INT(conf, filename, max_workers);
>> GET_CONF_INT(conf, filename, max_clients);
>> GET_CONF_INT(conf, filename, max_queued_clients);
>> + GET_CONF_INT(conf, filename, max_anonymous_clients);
>>
>> GET_CONF_INT(conf, filename, prio_workers);
>>
>
> You need a 'data->max_anonymous_clients = 20' initialization somewher
> in this file, otherwise it'll default to 0.
And I think we want to leave it that way. The value of zero means the
feature is disabled. That is, libvirt won't take any actions if count of
anonymous clients exceed certain value. It will still take an action
though if the number of total clients (both auth and unauth) exceeds
max_clients. The action is stop accept()-ing new clients. Trying to
invent a bright formula to compute the correct value may lead to us
adapting to a certain use case while forgetting about other. And we've
been there before (remember 'Set reasonable RSS limit on domain
startup'?). If a specific management application using libvirt requires
these values it should set it explicitly in the config file rather than
relying on libvirt defaults (which would change as libvirt adapts to
different management application).
What we can do is change the commented default value in config file. Is
that what you had in mind in the first place?
>
> Also, don't we want to increase 'max_clients' to something much
> larger like 5000 now that we rely on max_anonymous_clients to
> protect against DOS attack.
>
>> diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
>> index 073c178..880f46a 100644
>> --- a/daemon/libvirtd.conf
>> +++ b/daemon/libvirtd.conf
>> @@ -263,6 +263,10 @@
>> # connection succeeds.
>> #max_queued_clients = 1000
>>
>> +# The maximum length of queue of accepted but not yet not
>> +# authenticated clients. The default value is zero, meaning
>> +# the feature is disabled.
>> +#max_anonymous_clients = 20
>
> same not about increasing max_clients value here.
>
>> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
>> index e047751..054ece2 100644
>> --- a/src/locking/lock_daemon.c
>> +++ b/src/locking/lock_daemon.c
>> @@ -145,8 +145,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config,
>> bool privileged)
>> }
>>
>> if (!(lockd->srv = virNetServerNew(1, 1, 0, config->max_clients,
>> - -1, 0,
>> - false, NULL,
>> + 0, -1, 0, false, NULL,
>> virLockDaemonClientNew,
>>
>> virLockDaemonClientPreExecRestart,
>> virLockDaemonClientFree,
>
> We need to support max_anonymous_clients in the lock daemon config file
> too and increase its max clients to something huge too. Each VM started
> corresponds to an open client with the lock daemon, so we need that
> value to be quite high.
I forgot to reply to this block. But my argumentation stays the same
here. Moreover, there's no auth or unauth users in virlockd. I mean, all
users are unauth by default and virlockd has no authentication
mechanisms to make them authenticate. So either we will pass 0 here and
let the rest of code deal with it as if the feature is disabled or pass
config->max_clients which results in the same behavior in the end.
>
>> @@ -457,6 +468,7 @@ virNetServerPtr
>> virNetServerNewPostExecRestart(virJSONValuePtr object,
>> unsigned int max_workers;
>> unsigned int priority_workers;
>> unsigned int max_clients;
>> + unsigned int nclients_unauth_max;
>> unsigned int keepaliveInterval;
>> unsigned int keepaliveCount;
>> bool keepaliveRequired;
>> @@ -482,6 +494,11 @@ virNetServerPtr
>> virNetServerNewPostExecRestart(virJSONValuePtr object,
>> _("Missing max_clients data in JSON
>> document"));
>> goto error;
>> }
>> + if (virJSONValueObjectGetNumberUint(object,
>> "nclients_unauth_max", &nclients_unauth_max) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Missing nclients_unauth_max data in JSON
>> document"));
>
> We can't raise an error here - that'll cause failure upon RPM upgrade
> from version which didn't have this setting. Need to set some kind of
> sensible default value upon upgrade.
The sensible default is zero in my opinion. If management application
wants to override this, they still can set the value in the config and
then softly reset libvirtd (by softly I mean sending it signal to reload
the config).
Michal
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list