[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[Maxime Devos <maximedevos@telenet.be>]

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

On Sun, 2021-04-11 at 20:07 +0200, david larsson wrote:
> Hi Maxime!
> 
> On 2021-04-11 17:33, Maxime Devos wrote:
> > Please corect the galera package to refer to the coreutils (ls, stat, 
> > ...)
> > by absolute file name instead, using something like
> > 
> > (add-after 'install
> >   (substitute* "INSTALL-LOCATION/wsrep_sst_rsync"
> >     (("\\bls\\b") (string-append (assoc-ref inputs "coreutils") 
> > "/bin/ls"))
> >     ...))
> > 
> > (Likewise for rsync, gawk, iproute ...)
> > 
> > Don't use (which "ls") instead of string-append + assoc-ref! (which 
> > "ls") is
> > incorrect when cross-compiling;
> > 
> > That way, people don't have to fiddle with PATH in their configuration 
> > file.
> 
> I think you misundestood here - these rsync, gawk, iproute etc are 
> executed as part of scripts in the mysqld/bin package output folder

Ok, I should have asked to modify the mariadb package instead.

>  (see 
> `ls -la $(dirname $(readlink -f $(which mysqld)))/` ) because the Galera 
> add-on was (at least partially) merged into the regular mariadb sources, 
> so the scripts where the binaries are needed are not from the galera 
> package outputs but the mysql one. The galera package in guix master 
> today only provides the #$galera "/lib/libgalera_smm.so file.

So the mariadb package has the scripts, and the galera package has a library.

>  I agree it 
> would be nice to patch all the shell scripts in the $#mysql/bin folder 
> but this would 1. require maintaining all the additional patching of 
> those scripts as part of the mysql package

This shouldn't be complicated, though possibly somewhat tedious.
I took a look at /gnu/store/bjgz8jnfsbb4qvaa9csfy8i3x1i3ivp7-mariadb-10.5.8/bin/wsrep_*
(your hash may vary).  The following should be ‘absolutised’:

* In wsrep_sst_mariabackup:

  OS=$(uname)
  sfmt="tar"
  if pv --help 2>/dev/null | grep -q FORMAT;then
  mariabackup
  wsrep_log_error
  mbstream
  xbcrypt (*)
  [more things]
* In wsrep_sst_*: other things (eg. rm)

The following shouldn't be required, and could be commented out:
# Setting the path for lsof on CentOS
export PATH="/usr/sbin:/sbin:$PATH"

It's a little tedious, but it should be worth it.  This could be done
in the post-install phase of mariadb.  For an (almost) good example on
how to ‘absolutise’, see 'xvfb-run'.  Actually, it uses "which" which
is incorrect when cross-compiling, but that can be worked-around by
adding (setenv "PATH" (string-append BINDIR-OF-INPUTS-COREUTILS ":"
                                     BINDIR-OF-INPUTS-AWK ...))

About xbcrypt (*): I have no idea from which package this comes.
Is it an optional dependency?  If it is optional, _not_ patching it
could make sense, as to avoid increasing the closure.  This would
extra-environment.
>  and 2. since custom 
> wsrep_sst_<X> shell scripts are possible and are usually added to 
> /usr/bin/wsrep_sst_<X> those binaries will also require setting 
> additional environment variables or the same type of patching
> (which would therefore "require" this patch to make it easier).

I'd recommend that these custom shell scripts are patched as well,
but idk how they would be used with mariadb, perhaps there are
some complications here.

That said, if that's too much work or too error-prone, I've an alternative
proposal below, which is a little more ‘high level’ than asking the user to
spell out the PATH:

Take a look at 'nscd-shepherd-service' in gnu/serices/base.scm:

  [snip]
  #:environment-variables
  (list (string-append "LD_LIBRARY_PATH="
          (string-join
            (map (lambda (dir)
              (string-append dir "/lib"))
            (list #$@name-services))
            ":")))))

Replace LD_LIBRARY_PATH with PATH.  Add a "extensions" field to
<mysql-configuration>.  Replace 'name-services' with 'extensions'.

Procedures you need to modify:
  * mysql-cofiguration-file: add the field to $ <mysql-configuration>
  * mysql-shepherd-service: add #:environment-variables as appropriate
  * mysql-upgrade-shepherd-service: also, maybe, I don't know?

Possible problems: maybe some scripts need some additional environment
variables.  Mabe provide both an "extensions" field, and an
"extra-environment" field, and combine the results?

> > If possible, consider writing a "system test" automatically testing
> > some very basic functionality of mariadb + galera 
> > (gnu/tests/databases.scm).
> 
> I would have liked to, but Im sorry - I do not have the time/skill for 
> this for the time being. I hope the current patch is simple enough that 
> an additional test is not currently needed.

For the patch as you've submitted it, the lack of a system test shouldn't
be a problem, as Guix System doesn't support mariadb + galera.  The system
test is more for if ‘we’ add a 'galera' field to 'mysql-configuration' as
I suggested.

> > It seems extra-environment is still useful.  "USER=mysql" should 
> > probably be
> > added automatically, though (see my proposal below).
> 
> Yes, in particular for custom wsrep scripts.
>  The "USER=mysql" may not be 
> needed per se for the example snippet config I sent earlier as I did not 
> make sure that the list of environment variables there were the minimal 
> amount needed; I mainly hoped to provide a repeatable example of 
> something to test with.

Warning: I didn't actually test your patch.  I don't have a mysql service.

> > Perhaps you could extend "mysql-configuration" with a "galera" field
> > (with #f as default)?  Theoretical example:
> > 
> > (mysql-configuration [...] (galera [...]))
>
> > .. and modify mysql-service-type to insert appropriate configuration 
> > entries
> > and perhaps add things to the PATH of the shepherd service as 
> > appropriate.
> > 
> > Escape hatches like "extra-content" are useful, but this seems a bit
> > neater.
> 
> This looks nice! I agree that something like what you show here is the 
> better option, at the same time I don't have the time/skill to provide a 
> patch with a well-featured interface to the galera options for the time 
> being. The "escape hatch" would therefore be very useful for now, and 
> less hassle to maintain - compare with for example the vpn service and 
> the amount of emails in the lists regarding lack of options that could 
> have easily been added via some g-expression strings.

Somewhat off-topic, which e-mails would these be?

> In general I don't 
> see the harm in providing both "the escape hatch" way to add options to 
> a configuration file and the guile interface which is otherwise nicer 
> (IMO).

Agreed.  I believe the current plan is to:

* add the 'extra-environment' escape hatch.
* (possibly [dubious-discuss]) Add the extensions field (a list of
  package objects).  This adds the listed packages to PATH.
  Slightly neater than 'extra-environment', but is not as general
  as 'extra-environment'.
* The contents of 'extra-environment' and and 'extensions' are merged.

> Best regards,
> David
^W^W^W^W^W Maxime

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]