[bug#29466] [PATCH] services: web: Add support for configuring the nginx server names hash.

[Christopher Baines <mail@cbaines.net>]

[-- Attachment #1: Type: text/plain, Size: 3850 bytes --]


Ludovic Courtès writes:

>> The nginx service can fail to start if the server names hash bucket size is
>> too small, which can happen on some systems, and when using QEMU, depending on
>> the CPU.
>>
>> * gnu/services/web.scm (<nginx-configuration>): Add
>>   server-names-hash-bucket-size and server-names-hash-bucket-max-size.
>>   (default-nginx-config): Add support for the new hash bucket size parameters.
>>   (nginx-service, nginx-activation): Pass the new hash bucket size parameters
>>   through to the default-nginx-config procedure.
>> * doc/guix.texi (Web Services): Document the new hash bucket size parameters.
>
> LGTM!
>
> However…
>
>> -(define (default-nginx-config nginx log-directory run-directory server-list upstream-list)
>> +(define (default-nginx-config nginx log-directory run-directory server-list
>> +                              upstream-list server-names-hash-bucket-size
>> +                              server-names-hash-bucket-max-size)
>
> That’s too many positional parameters.  And should we use a gexp
> compiler for <nginx-configuration> anyway?
>
>>  (define nginx-shepherd-service
>>    (match-lambda
>>      (($ <nginx-configuration> nginx log-directory run-directory server-blocks
>> -                              upstream-blocks file)
>> +                              upstream-blocks server-names-hash-bucket-size
>> +                              server-names-hash-bucket-max-size file)
>
> Likewise, at this stage, we should probably use ‘match-record’ to avoid
> mistakes.

I've sent a couple of additional patches now, the first makes the change
to using match-record, and the second splits out the configuration file
part of the <nginx-configuration> record, and adds a gexp compiler for
it.

Let me know what you think about the 2nd patch? I don't particularly
like the duplication between the two records, as both the
<nginx-configuration> and <nginx-config-file> records need the package,
log directory and run directory for different reasons.

In summary:

 - <nginx-configuration>
     nginx: used for the nginx binary in the shepherd service
     log-directory: created in the activation service
     run-directory: created in the activation service

 - <nginx-config-file>
     nginx: used for the mime.types file
     log-directory: written in to the config file
     run-directory: written in to the config file

It's important that it's possible to specify the log directory and run
directory if you don't use the <nginx-config-file>, and instead use
something opaque (like a <local-file>). But I dislike the duplication
that this is adding.

I wonder if there is a better way of supporting the use of a raw
configuration file, rather than the record. Possibly by providing an
easy way to create a custom nginx-service-type, with a different
activation phase? I think that would allow for making the original
<nginx-configuration> compile to the configuration file, but then have
something like this for a custom config file.

  (service (nginx-service-type-with-custom-activation
             nginx-service-type
             #:run-directory "/var/run/nginx-custom"
             #:log-directory "/var/run/nginx-custom")
           (local-file "nginx-custom.conf"))

One downside with this approach is that service extensions use the
service-type, so modifying it would mean that services that extend nginx
wouldn't work with this service type (you'd have to have the original as
well, or modify the service with the extension).

In the 2nd patch, I put in some (bad) handling of the extension case, as
with an opaque config file, you can't do anything. Previously, the
configuration was changed, but then this is later ignored, as the file
takes precedence.

Thanks for reviewing,

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]