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.
Message #17 received at 75144 <at> debbugs.gnu.org (full text, mbox):
From: Roman Scherer <roman <at> burningswell.com> To: Roman Scherer <roman <at> burningswell.com> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Ludovic Courtès <ludo <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Christopher Baines <guix <at> cbaines.net>, 75144 <at> debbugs.gnu.org Subject: Re: [bug#75144] [PATCH] machine: Implement 'hetzner-environment-type'. Date: Sat, 25 Jan 2025 14:37:16 +0100
[Message part 1 (text/plain, inline)]
I made a `mock*` macro to get around this ugly nesting in the meantime.
https://github.com/r0man/guix/blob/hetzner-machine-v2-mock-star/tests/machine/hetzner.scm#L165-L248
But I'm still wondering why the `mock` in `deploy-machine-mock-with-unprovisioned-server` is working in the REPL,
but failing when I run the test with make ...
Roman Scherer <roman <at> burningswell.com> writes:
> 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.
>
> [2. text/x-patch; v2-0001-machine-Implement-hetzner-environment-type.patch]...
>
>
> 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.