[libvirt] [PATCH] network: Add support for configuring dhcp lease time

[Nehal J Wani]

On Fri, Sep 23, 2016 at 4:51 PM, Peter Krempa <pkrempa at redhat.com> wrote:
> On Fri, Sep 23, 2016 at 15:33:46 +0530, Nehal J Wani wrote:
>> On Fri, Sep 23, 2016 at 1:20 PM, Peter Krempa <pkrempa at redhat.com> wrote:
>> > On Fri, Sep 23, 2016 at 12:19:49 +0530, Nehal J Wani wrote:
>> >> The default dhcp lease time set by dnsmasq is only one hour, which can be
>> >> pretty small for developers relying on ip address(es) to be consistent
>> >> across reboots.
>> >
>> > This should be handled through the leases file which is supposed to
>> > remember the IP addresses given to a host despite a short lease time. Is
>> > that broken for you?
>>
>> We can't ask developers to parse the .status file and the DHCP leases
>> API would never show expired leases. Or am I misinterpreting what you
>
> If a lease is expired then the VM is necessarily not using it for a
> while.
>
>> mean by 'remember'? Even dnsmasq depends on the output for the init
>
> I meant 'remember' as dnsmasq should assign the same address even if the
> lease was expired for a while.

Yes, but then you are assuming that dnsmasq hasn't been restarted
after the lease has expired. User may switch distros in between or
shutdown is computer, etc.
>
>> event sent to the leaseshelper program, which will not output lease
>> which have expired.
>
> So then maybe this is the actual bug. The expired leases still can be
> renewed and AFAIK dnsmasq reloads them even for the expired entries so
> we probably should remove that part from the leaseshelper.

After addition of leaseshelper, because of the --leasefile-ro option
that we specify to dnsmasq, the leases database is entirely controlled
by it. If the developer shuts down his VM, the lease expires and then
he restarts the computer (which will involve restarting the dnsmasq
service), dnsmasq will rely on the leaseshelper program for the
initial input (aka init event). The first parameter from the 'init'
output is the expiry time of the lease. If the lease has already
expired, then we can't print it in virLeasePrintLeases(). Hence I fail
to understand why you would think that this may be a bug.

Why would libvirt/leaseshelper renew a certain lease? Isn't that the
job of the client?
>
>>
>> >
>> >>
>> >> This patch adds support for setting a lease time in the network definition.
>> >> For now, all IP ranges under one <dhcp> will share a common leasetime.
>> >>
>> >> An example:
>> >> <dhcp>
>> >>   <leasetime units='hours'>12</leasetime>
>> >> </dhcp>
>> >
>> > This sounds like a reasonable feature, but I don't really want to sell
>> > it as a workaround for the problem you've described above. Setting the
>> > lease time is useful, but guests should get the same IP addresses
>> > thanks to the lease file.
>>
>> Actually, the major reason behind this feature was mentioned at
>> https://www.redhat.com/archives/libvir-list/2016-April/msg01071.html
>
> As I've said, adding ability to specify longer lease time may be
> desired, but the fact that you don't get the address most probably is
> not related to this and should be addressed separately.
>
>> Let's say that the lease time is 2 hours and I shutdown my VM. Now I
>> reboot it after 2hours 5mins, the lease will be gone, since it will be
>
> The lease will expire, but dnsmasq still can reuse that address for the
> particular mac address until the pool is depleted.
>
>> expired. The .status file is read and the leases which have expired
>> are removed. So, there is no guarantee that my VM will get the same IP
>
> Again, this looks like the actual bug.
>
>> address again. Hence the developer would want to increase the lease
>
> No it's not guaranteed, but very likely.
>
>> time to something big, like 1 week, and go to sleep happily when the
>> VM is turned off at night :)
>
> That's a rather weak point. Setting too long lease time may on the other
> hand exhaust the pool. Usually the configured DHCP lease times are
> rather short.
>
>> >> The value for attribute 'units' can be seconds/hours/days/weeks/s/h/d/w.
>> >> If the leasetime specified is -1, it is considered to be infinite.
>
> [...]
>
>> >> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
>> >> index 1b4f980..023edfb 100644
>> >> --- a/docs/schemas/basictypes.rng
>> >> +++ b/docs/schemas/basictypes.rng
>> >> @@ -13,6 +13,12 @@
>> >>        <param name='pattern'>[0-9]+</param>
>> >>      </data>
>> >>    </define>
>> >> +  <!-- Our long doesn"t allow a leading "+" in its lexical form -->
>> >> +  <define name='long'>
>> >> +    <data type='long'>
>> >
>> > This looks weird. Also why would you need a separate type for this?
>> >
>>
>> I didn't find any available data type for signed integers. Which one
>> would you rather use? (Signed, because we want to support -1 too)
>
> A quick grep revealled that there's a builtin "long" type used in at
> least one case.
>
>>
>> >> +      <param name='pattern'>-?[0-9]+</param>
>> >> +    </data>
>
> [...]
>
>> >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>> >> index aa39776..d2372f2 100644
>> >> --- a/src/conf/network_conf.c
>> >> +++ b/src/conf/network_conf.c
>> >> @@ -1032,21 +1032,76 @@ virNetworkDHCPHostDefParseXML(const char *networkName,
>
> [...]
>
>> >> +
>> >> +    save = ctxt->node;
>> >> +    ctxt->node = node;
>> >> +
>> >> +    leasetimeRV = virXPathLongLong("string(./text())", ctxt, &leasetime);
>> >> +    if (leasetimeRV == -2) {
>> >> +        virReportError(VIR_ERR_XML_ERROR,
>> >> +                       _("Invalid long long value specified for leasetime "
>> >> +                         "in definition of network '%s'"),
>> >> +                       networkName);
>> >> +        goto cleanup;
>> >> +    } else {
>> >> +        if (leasetime < 0) {
>> >> +            leasetime = -1;       /* infinite */
>> >> +        } else {
>> >> +            unit = virXPathString("string(./@unit)", ctxt);
>> >> +            if (virScaleTime(&leasetime, unit, scale,
>> >> +                             VIR_NETWORK_DHCP_LEASE_TIME_MAX) < 0) {
>> >> +                // let virScaleTime() report the appropriate error
>> >
>> > '//' comments should not be used in libvirt. Also the comment does not
>> > make sense since it's obvious what is happening.
>>
>> I have a query regarding this. If we rely on virScaleTime to report
>> the appropriate error, it wouldn't be including the network name. Is
>
> That happens in quite a few cases.
>
>> that bad? Should I be modifying virScaleTime to return different
>> return values for different errors and shape the error message
>> appropriately here? (That would sort be inconsistent with
>
> No, not really.
>
>> virScaleInteger which doesn't have different error codes for different
>> scenarios)
>>
>> Whoops on the comment. Should have read hacking.html more carefully!
>
> [...]
>
>> >> @@ -2675,6 +2731,13 @@ virNetworkIPDefFormat(virBufferPtr buf,
>> >>          virBufferAddLit(buf, "<dhcp>\n");
>> >>          virBufferAdjustIndent(buf, 2);
>> >>
>> >> +        if (def->leasetime) {
>> >> +            virBufferAddLit(buf, "<leasetime");
>> >> +            virBufferAsprintf(buf, " unit='seconds'>%lld",
>> >
>> > Why aren't we remembering the unit the user used originally? Other
>> > places can't do that since the unit was added later, but with this newly
>> > added code you can remember and format back the unit just fine.
>> >
>>
>> Actually, I followed what was originally done for memory units. Even
>> if user specifies MiB, everything is stored in KiB and on dumpxml, the
>
> This was because libvirt added support for scaled integers later than
> the original code. Thus we needed to keep compatibility. This is not
> required for new elements.
>
>> user is shown the unit in KiB. In case we want to remember the unit
>> specified by the user, we'll have to add another member in the
>> _virNetworkIPDef struct. Since we haven't done this anywhere, I was
>> apprehensive about having inconsistent behaviour for 'unit'.
>>
>> >> +                              def->leasetime);
>> >> +            virBufferAddLit(buf, "</leasetime>\n");
>> >> +        }
>
> [...]
>
>> >> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> >> index b57a195..81a60e6 100644
>> >> --- a/src/util/virutil.c
>> >> +++ b/src/util/virutil.c
>> >> @@ -275,6 +275,76 @@ virHexToBin(unsigned char c)
>> >>      }
>> >>  }
>> >>
>> >> +/**
>> >> + * virScaleTime:
>> >
>> > This change should be done as a separate patch.
>> >
>> >> + * @value: pointer to the integer which is supposed to hold value
>> >> + * @unit: pointer to the string holding the unit
>> >> + * @scale: integer holding the value of scale
>> >
>> > ^^ at least this value is not documented enough.
>> >
>> >> + * @limit: upper limit on scaled value
>> >> + *
>> >> + * Scale an integer @value in-place by an optional case-insensitive @unit,
>> >
>> > The statement about case insensitivity doesn't look to be true according
>> > to the code blow.
>>
>> With case insensitivity, 'Seconds' and 'SEconDs' would also be valid
>> inputs, and it would be difficult to accommodate it in the .rng file,
>> plus I was told that if attributes are words, then we shouldn't be
>> going for case-insensitivity anyway. I'll fix this in the next patch
>> set.
>
> Either case, the comment and code should not behave differently. The
> comment looks copied from the memory number scaler, but the code
> certainly doesn't behave that way.
>
>> >
>> >> + * defaulting to @scale if @unit is NULL or empty. Recognized units include
>> >> + * (w)eeks, (d)ays, (h)ours, (m)inutes and (s)econds. Ensure that the result
>> >> + * does not exceed @limit.  Return 0 on success, -1 with error message raised
>> >> + * on failure.
>> >
>> > This does not document the scale of the returned value at all.
>> >
>> >> + */
>> >> +int
>> >> +virScaleTime(long long *value, const char *unit,
>> >> +             long long scale, long long limit)
>> >
>> > Scale can't really be negative. Also the result being negative does not
>> > make much sense to me.
>>
>> Should this be changed to unsigned long long or unsigned long? Since
>> leasetime has to be a signed integer, what would be the ideal way to
>> pass it to the modified function? Should I typecast it?
>
> Negative scale doesn't make sense. For time values, negative time value
> might make sense for relative time values. You need to check then that
> the overflow check works for negative numbers as well, which it doesn't
> since it was just copied from the memory integer scaler.
>
>



-- 
Nehal J Wani