Package: guix-patches;
Reported by: Roman Scherer <roman <at> burningswell.com>
Date: Fri, 27 Dec 2024 16:48:02 UTC
Severity: normal
Tags: patch
Done: Ludovic Courtès <ludo <at> gnu.org>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Roman Scherer <roman <at> burningswell.com> To: Ludovic Courtès <ludo <at> gnu.org> Cc: Roman Scherer <roman <at> burningswell.com>, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Josselin Poiret <dev <at> jpoiret.xyz>, Christopher Baines <guix <at> cbaines.net>, 75144 <at> debbugs.gnu.org Subject: [bug#75144] [PATCH] machine: Implement 'hetzner-environment-type'. Date: Sun, 19 Jan 2025 17:59:06 +0100
[Message part 1 (text/plain, inline)]
Hi Ludo, thanks for your review. Here is a v2, I hope I addressed your previous comments with it, but I need some help. As you suggested I also added some tests. Some use mocking, and some run against the Hetzner API, if the GUIX_HETZNER_API_TOKEN env var is set. ./pre-inst-env make check TESTS="tests/machine/hetzner/http.scm" ./pre-inst-env make check TESTS="tests/machine/hetzner.scm" All tests pass when I run them in the Geiser REPL, where I developed them. But I have some trouble with one test that uses mocking. The "deploy-machine-mock-with-unprovisioned-server" test in tests/machine/hetzner.scm only fails when run in the terminal. :? I'm using the "mock" function from (guix tests) to mock some HTTP and SSH calls. The issue is that I see different behaviour whether I run the tests in Geiser vs in the Terminal. In Geiser I see the following output for this test, in it passes: ------------------------------------------------------------------------------- creating 'cx42' server for 'guix-x86'... successfully created 'cx42' x86 server for 'guix-x86' enabling rescue system on 'guix-x86'... MOCK ENABLE RESUCE successfully enabled rescue system on 'guix-x86' powering on server for 'guix-x86'... MOCK POWER ON successfully powered on server for 'guix-x86' connecting via SSH to '1.2.3.4' using '/tmp/guix-hetzner-machine-test-key'... MOCK OPEN SSH SESSION installing rescue system packages on 'guix-x86'... MOCK RUNNING SCRIPT: /tmp/guix/deploy/hetzner-machine-rescue-install-packages successfully installed rescue system packages on 'guix-x86' setting up partitions on 'guix-x86'... MOCK RUNNING SCRIPT: /tmp/guix/deploy/hetzner-machine-rescue-partition successfully setup partitions on 'guix-x86' installing guix operating system on 'guix-x86'... MOCK RUNNING SCRIPT: /tmp/guix/deploy/hetzner-machine-rescue-install-os successfully installed guix operating system on 'guix-x86' rebooting server for 'guix-x86'... successfully rebooted server for 'guix-x86' connecting via SSH to '1.2.3.4' using '/tmp/guix-hetzner-machine-test-key'... MOCK OPEN SSH SESSION ------------------------------------------------------------------------------- You can see that calls to "hetzner-machine-ssh-run-script" are mocked, because "MOCK RUNNING SCRIPT" is printed multiple times. But in a "guix shell -D" terminal I see the following output for the test, and it is failing: ------------------------------------------------------------------------------- creating 'cx42' server for 'guix-x86'... successfully created 'cx42' x86 server for 'guix-x86' enabling rescue system on 'guix-x86'... MOCK ENABLE RESUCE successfully enabled rescue system on 'guix-x86' powering on server for 'guix-x86'... MOCK POWER ON successfully powered on server for 'guix-x86' connecting via SSH to '1.2.3.4' using '/tmp/guix-hetzner-machine-test-key'... MOCK OPEN SSH SESSION installing rescue system packages on 'guix-x86'... test-name: deploy-machine-mock-with-unprovisioned-server location: /home/roman/workspace/guix/tests/machine/hetzner.scm:189 actual-value: #f actual-error: + (guile-ssh-error + "%gssh-make-sftp-session" + "Could not create a SFTP session" + #<session #<undefined>@1.2.3.4:22 (disconnected) ffff85596de0> + #f) result: FAIL ;;; [2025/01/19 17:39:16.791023, 0] [GSSH ERROR] Could not create a SFTP session: #<session #<undefined>@1.2.3.4:22 (disconnected) ffff85596de0> ------------------------------------------------------------------------------- The tests fails here trying to use a disconnected SSH session object, that I returned in a mocked call. This code should actually never be reached, because I mock the "hetzner-machine-ssh-run-script" call. But for some reason the mock is not working here. The "MOCK RUNNING SCRIPT" output is missing. Do you have any ideas what could be going on here? I suspect this might be due to some optimization or env issue, but I'm pretty lost. I attached a WIP v2 for now. Will send a v3 and a separate patch for the ssh modification once I fixed this mock test. Thanks, Roman.
[v2-0001-machine-Implement-hetzner-environment-type.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
Ludovic Courtès <ludo <at> gnu.org> writes:
> Hello Roman,
>
> Roman Scherer <roman <at> burningswell.com> skribis:
>
>> * gnu/machine/hetzner.scm: New file.
>> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
>> * guix/ssh.scm (open-ssh-session): Add stricthostkeycheck option.
>> * doc/guix.texi (Invoking guix deploy): Add documentation for
>> 'hetzner-configuration'.
>>
>> Change-Id: Idc17dbc33279ecbf3cbfe2c53d7699140f8b9f41
>
> Thumbs up for this big piece of work, one that I think is important for
> the project! ‘guix deploy’ is a great idea but it desperately needs
> more backends like this one.
>
> I’m not familiar with Hetzner so I’ll comment on more general aspects.
> Chris, perhaps you can provide feedback on Hetzner-specific issues? I
> think we could put this backend to good use for Guix infra since a few
> services are running at Hetzner.
>
>> +@deftp {Data Type} hetzner-configuration
>> +This is the data type describing the server that should be created for a
>> +machine with an @code{environment} of @code{hetzner-environment-type}.
>
> Could you add a sentence providing more context like:
>
> It allows you to configure deployment to a @acronym{VPS, virtual
> private server} hosted by @uref{https://www.hetzner.com, Hetzner}.
>
>> +@item @code{authorize?} (default: @code{#t})
>> +If true, the coordinator's public signing key
>
> “coordinator” has nothing to do here I guess.
>
>> +@item @code{labels} (default: @code{'()})
>> +A user defined alist of key/value pairs attached to the server. Keys and
>> +values must be strings. For more information, see
>> +@uref{https://docs.hetzner.cloud/#labels, Labels}.
>
> Maybe add a short example?
>
>> +@item @code{location} (default: @code{"fsn1"})
>> +The name of a @uref{https://docs.hetzner.com/cloud/general/locations,
>> +location} to create the server in.
>
> Maybe add: “For example, @code{"fsn1"} corresponds to the Hetzner site
> in Falkenstein, Germany, while @code{"sin"} corresponds to its site in
> Singapore.”
>
>> +@item @code{server-type} (default: @code{"cx42"})
>> +The name of the
>> +@uref{https://docs.hetzner.com/cloud/servers/overview#server-types,
>> +server type} this server should be created with.
>
> Likewise, an example would be elcome.
>
>> +@item @code{ssh-key}
>> +The path to the SSH private key to use to authenticate with the remote
>> +host.
>
> s/path to/file name of/
>
>> +The following example shows the definition of 2 machines that are
>
> s/2/two/
>
>> +vCPUs and 32 GB of RAM on the @code{aarch64} architecture, the second
>
> s/@code{aarch64}/AArch64/
>
>> +shared vCPUs and 32 GB of RAM on the @code{x86_64} architecture.
>
> Drop @code.
>
>> +@lisp
>> +(use-modules (gnu machine)
>> + (gnu machine hetzner))
>> +
>> +(list (machine
>> + (operating-system %hetzner-os-arm)
>> + (environment hetzner-environment-type)
>> + (configuration (hetzner-configuration
>> + (server-type "cax41")
>> + (ssh-key "/home/charlie/.ssh/id_rsa"))))
>> + (machine
>> + (operating-system %hetzner-os-x86)
>> + (environment hetzner-environment-type)
>> + (configuration (hetzner-configuration
>> + (server-type "cpx51")
>> + (ssh-key "/home/charlie/.ssh/id_rsa")))))
>
> Nice!
>
>> +API key} should provision 2 machines for you.
>
> s/2/two/
>
>> + #:use-module (ice-9 receive)
>
> The code base preferable uses SRFI-71 for multiple-value returns.
>
>> + (raise (formatted-message
>> + (G_ "Expected a list of Hetzner API responses")))))
>
> Messages should start with a lower-case letter (for all the messages in
> this file).
>
> Please add the file to ‘po/guix/POTFILES.in’ so that it’s actually
> subject to translation.
>
>> +(define (hetzner-api-response-read port)
>> + "Read the Hetzner API response from PORT."
>> + (let* ((response (read-response port))
>> + (body (read-response-body response)))
>> + (hetzner-api-response
>> + (body (json-string->scm (bytevector->string body "UTF-8")))
>
> Just ‘string->utf8’ (shorter).
>
> More importantly: instead of ‘json-string->scm’ (which gives an alist,
> leading to ‘assoc-ref’ calls all over the code base along with free-form
> alists, which is very error-prone), could you use ‘define-json-mapping’?
>
> In essence it’s like ‘define-record-type’ but it additionally define how
> to map a JSON dictionary to a Scheme record. There are several examples
> in Guix, such as (guix swh).
>
> For clarity, it might be useful to move all the hetzner-api-* bits to a
> separate module, for example (gnu machine hetzner http). WDYT?
>
>
> The rest of the code looks nice to me (modulo alists :-)) but that’s
> about all I can say. It’s quite a significant body of code. What would
> you suggest to prevent bitrot and support maintenance? Are there parts
> of it that could be usefully tested automatically, possibly by mocking
> part of the Hetzner API? Or are there tips on how you tested it that
> could be written down in the file itself?
>
>
> Could you move the (guix ssh) bits to a separate patch?
>
>> +++ b/guix/ssh.scm
>> @@ -103,7 +103,8 @@ (define* (open-ssh-session host #:key user port identity
>> host-key
>> (compression %compression)
>> (timeout 3600)
>> - (connection-timeout 10))
>> + (connection-timeout 10)
>> + (stricthostkeycheck #t))
>> "Open an SSH session for HOST and return it. IDENTITY specifies the file
>> name of a private key to use for authenticating with the host. When USER,
>> PORT, or IDENTITY are #f, use default values or whatever '~/.ssh/config'
>
> Please update the docstring.
>
> Rather ‘strict-host-key-check?’ to match naming conventions, even if
> Guile-SSH calls it that way.
>
>> @@ -137,7 +138,8 @@ (define* (open-ssh-session host #:key user port identity
>>
>> ;; Speed up RPCs by creating sockets with
>> ;; TCP_NODELAY.
>> - #:nodelay #t)))
>> + #:nodelay #t
>> + #:stricthostkeycheck stricthostkeycheck)))
>
> Not sure what this does actually. Looks like the main part is the
> “when stricthostkeycheck” condition that comes below, no?
>
> Could you send a second version?
>
> Thank you!
>
> Ludo’.
[signature.asc (application/pgp-signature, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.