bug#42252: Not possible to reliably port forward with "guix system vm" anymore

bug#42252: Not possible to reliably port forward with "guix system vm" anymore

[Christopher Lemmer Webber]

In commit 5379392731b52eef22b4936637eb592b93e04318, the following change
was introduced:

  modified   gnu/system/vm.scm
  @@ -941,6 +941,7 @@ with '-virtfs' options for the host file systems listed in SHARED-FS."
               '())
   
        "-no-reboot"
  +     "-nic" "user,model=virtio-net-pci"
        "-object" "rng-random,filename=/dev/urandom,id=guixsd-vm-rng"
        "-device" "virtio-rng-pci,rng=guixsd-vm-rng"

Unfortunately, this means that in our docs where we suggest doing the
following:

  `guix system vm config.scm` -nic user,model=virtio-net-pci,hostfwd=tcp::10022-:22

Since we now provide our own similar "-nic" field this creates a
*second* network interface at the same address and there is a race as in
terms of which handles connections.  Depending on the race result,
connections to the forwarded port may hang indefinitely.

Ironically, this regression was introduced to solve another regression!
From the commit message:

  This fixes a regression introduced in 8e53fe2b91d2776bc1529e7b34967c8f1d9edc32
  where 'guix system vm' would no longer be using virtio.

What's the right solution?  One could be that "guix system vm" itself
could take an argument that sets up port forwarding in the generated
shell script.  Eg:

  guix system vm config.scm --hostfwd=tcp::10022-:22 --hostfwd=tcp::8888-:80

kind of ugly, but it could work.  WDYT?

 - Chris

bug#42252: Not possible to reliably port forward with "guix system vm" anymore

[Christopher Lemmer Webber]

Christopher Lemmer Webber writes:

>   guix system vm config.scm --hostfwd=tcp::10022-:22 --hostfwd=tcp::8888-:80
>
> kind of ugly, but it could work.  WDYT?

Kind of uglier, but more versatile:

  guix system vm config.scm --nic=user,model=virtio-net-pci,hostfwd=tcp::10022-:22,hostfwd=tcp::8888-:80

bug#42252: Not possible to reliably port forward with "guix system vm" anymore

[Christopher Lemmer Webber]

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

Christopher Lemmer Webber writes:

> Christopher Lemmer Webber writes:
>
>>   guix system vm config.scm --hostfwd=tcp::10022-:22 --hostfwd=tcp::8888-:80
>>
>> kind of ugly, but it could work.  WDYT?
>
> Kind of uglier, but more versatile:
>
>   guix system vm config.scm --nic=user,model=virtio-net-pci,hostfwd=tcp::10022-:22,hostfwd=tcp::8888-:80

Here's a patch that implements just that.  Seems to work fine here!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-guix-system-Add-nic-option-for-vm-subcommand.patch --]
[-- Type: text/x-patch, Size: 7676 bytes --]

From b008aad5d00fb6a87eb5a03d1849d6f52fb2307d Mon Sep 17 00:00:00 2001
From: Christopher Lemmer Webber <cwebber@dustycloud.org>
Date: Tue, 7 Jul 2020 18:13:29 -0400
Subject: [PATCH] guix system: Add --nic option for 'vm' subcommand.

This allows passing in explicit options for the qemu -nic keyword argument
for use in port forwarding, etc.

* (show-help, %options, process-action): Add and use command line
  argument of --nic for `guix system vm' and pass along appropriately.
  guix/scripts/system.scm (system-derivation-for-action):
  (perform-action): Define and use #:nic-options keyword.
* gnu/system/vm.scm (common-qemu-options)
  (system-qemu-image/shared-store-script): Likewise.
  (default-nic-options): New variable.
* doc/guix.texi (Running Guix in a VM): Use --nic argument in system vm port
  forwarding example.
---
 doc/guix.texi           |  2 +-
 gnu/system/vm.scm       | 16 ++++++++++++----
 guix/scripts/system.scm | 18 ++++++++++++++----
 3 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 992bc303bb..4e76eb832a 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -28315,7 +28315,7 @@ To enable SSH inside a VM you need to add an SSH server like
 22 by default, to the host.  You can do this with
 
 @example
-`guix system vm config.scm` -nic user,model=virtio-net-pci,hostfwd=tcp::10022-:22
+`guix system vm config.scm --nic=user,model=virtio-net-pci,hostfwd=tcp::10022-:22`
 @end example
 
 To connect to the VM you can run
diff --git a/gnu/system/vm.scm b/gnu/system/vm.scm
index f2b6b71b4d..78f89f6568 100644
--- a/gnu/system/vm.scm
+++ b/gnu/system/vm.scm
@@ -803,7 +803,11 @@ bootloader refers to: OS kernel, initrd, bootloader data, etc."
               #:register-closures? #f
               #:copy-inputs? full-boot?))
 
-(define* (common-qemu-options image shared-fs)
+(define default-nic-options
+  "user,model=virtio-net-pci")
+
+(define* (common-qemu-options image shared-fs
+                              #:key nic-options)
   "Return the a string-value gexp with the common QEMU options to boot IMAGE,
 with '-virtfs' options for the host file systems listed in SHARED-FS."
 
@@ -819,7 +823,9 @@ with '-virtfs' options for the host file systems listed in SHARED-FS."
             '())
 
      "-no-reboot"
-     "-nic" "user,model=virtio-net-pci"
+
+     ;; Construct -nic while forwarding all --hostfwd=... options
+     "-nic" #$(or nic-options default-nic-options)
      "-object" "rng-random,filename=/dev/urandom,id=guixsd-vm-rng"
      "-device" "virtio-rng-pci,rng=guixsd-vm-rng"
 
@@ -840,7 +846,8 @@ with '-virtfs' options for the host file systems listed in SHARED-FS."
                                                 (disk-image-size
                                                  (* (if full-boot? 500 70)
                                                     (expt 2 20)))
-                                                (options '()))
+                                                (options '())
+                                                (nic-options #f))
   "Return a derivation that builds a script to run a virtual machine image of
 OS that shares its store with the host.  The virtual machine runs with
 MEMORY-SIZE MiB of memory.
@@ -874,7 +881,8 @@ it is mostly useful when FULL-BOOT?  is true."
                                 (string-join #$kernel-arguments " "))))
               #$@(common-qemu-options image
                                       (map file-system-mapping-source
-                                           (cons %store-mapping mappings)))
+                                           (cons %store-mapping mappings))
+                                      #:nic-options nic-options)
               "-m " (number->string #$memory-size)
               #$@options))
 
diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm
index 61eeec622b..32cea9b184 100644
--- a/guix/scripts/system.scm
+++ b/guix/scripts/system.scm
@@ -684,7 +684,7 @@ checking this by themselves in their 'check' procedure."
 (define* (system-derivation-for-action os base-image action
                                        #:key image-size file-system-type
                                        full-boot? container-shared-network?
-                                       mappings)
+                                       mappings nic-options)
   "Return as a monadic value the derivation for OS according to ACTION."
   (case action
     ((build init reconfigure)
@@ -703,7 +703,8 @@ checking this by themselves in their 'check' procedure."
                                             (if full-boot?
                                                 image-size
                                                 (* 70 (expt 2 20)))
-                                            #:mappings mappings))
+                                            #:mappings mappings
+                                            #:nic-options nic-options))
     ((disk-image)
      (lower-object
       (system-image
@@ -764,6 +765,7 @@ and TARGET arguments."
                          use-substitutes? bootloader-target target
                          image-size file-system-type full-boot?
                          container-shared-network?
+                         nic-options
                          (mappings '())
                          (gc-root #f))
   "Perform ACTION for OS.  INSTALL-BOOTLOADER? specifies whether to install
@@ -819,7 +821,8 @@ static checks."
                                                 #:image-size image-size
                                                 #:full-boot? full-boot?
                                                 #:container-shared-network? container-shared-network?
-                                                #:mappings mappings))
+                                                #:mappings mappings
+                                                #:nic-options nic-options))
 
        ;; For 'init' and 'reconfigure', always build BOOTCFG, even if
        ;; --no-bootloader is passed, because we then use it as a GC root.
@@ -965,6 +968,8 @@ Some ACTIONS support additional ARGS.\n"))
       --share=SPEC       for 'vm', share host file system according to SPEC"))
   (display (G_ "
       --expose=SPEC      for 'vm', expose host file system according to SPEC"))
+  (display (G_ "
+      --nic=NIC-OPTIONS  for 'fm', set the value passed to qemu's -nic option"))
   (display (G_ "
   -N, --network          for 'container', allow containers to access the network"))
   (display (G_ "
@@ -1014,6 +1019,10 @@ Some ACTIONS support additional ARGS.\n"))
                  (lambda (opt name arg result)
                    (alist-cons 'image-size (size->number arg)
                                result)))
+         (option '("nic") #t #f
+                 (lambda (opt name arg result)
+                   (alist-cons 'nic-options arg
+                               result)))
          (option '(#\N "network") #f #f
                  (lambda (opt name arg result)
                    (alist-cons 'container-shared-network? #t result)))
@@ -1172,7 +1181,8 @@ resulting from command-line parsing."
                                #:install-bootloader? bootloader?
                                #:target target-file
                                #:bootloader-target bootloader-target
-                               #:gc-root (assoc-ref opts 'gc-root)))))
+                               #:gc-root (assoc-ref opts 'gc-root)
+                               #:nic-options (assoc-ref opts 'nic-options)))))
           #:target target
           #:system system)))
     (warn-about-disk-space)))
-- 
2.26.2

bug#42252: Not possible to reliably port forward with "guix system vm" anymore

[Bengt Richter]

Hi

On +2020-07-07 16:40:21 -0400, Christopher Lemmer Webber wrote:
> In commit 5379392731b52eef22b4936637eb592b93e04318, the following change
> was introduced:
> 
>   modified   gnu/system/vm.scm
>   @@ -941,6 +941,7 @@ with '-virtfs' options for the host file systems listed in SHARED-FS."
>                '())
>    
>         "-no-reboot"
>   +     "-nic" "user,model=virtio-net-pci"
>         "-object" "rng-random,filename=/dev/urandom,id=guixsd-vm-rng"
>         "-device" "virtio-rng-pci,rng=guixsd-vm-rng"
> 
> Unfortunately, this means that in our docs where we suggest doing the
> following:
> 
>   `guix system vm config.scm` -nic user,model=virtio-net-pci,hostfwd=tcp::10022-:22
> 
> Since we now provide our own similar "-nic" field this creates a
> *second* network interface at the same address and there is a race as in
> terms of which handles connections.  Depending on the race result,
> connections to the forwarded port may hang indefinitely.
> 
> Ironically, this regression was introduced to solve another regression!
> From the commit message:
> 
>   This fixes a regression introduced in 8e53fe2b91d2776bc1529e7b34967c8f1d9edc32
>   where 'guix system vm' would no longer be using virtio.
>

This reminds a bit of doctors prescribing powerful medicine with side-effect so bad
that they have to prescribe a medicine for that, which in turn has side-effects,
in what I think is called prescription cascading, and people wind up on 25 pills a day.

"First, do no harm." :)

I wouldn't say anything, except ISTM your fix on top of a fix
is not the first to remind me of cascading :)

> What's the right solution?  One could be that "guix system vm" itself
> could take an argument that sets up port forwarding in the generated
> shell script.  Eg:
> 
>   guix system vm config.scm --hostfwd=tcp::10022-:22 --hostfwd=tcp::8888-:80
> 
> kind of ugly, but it could work.  WDYT?
> 
>  - Chris
> 
> 
> 

I'm not saying your solution is bad, I'm just saying cascading fixes may be a symptom
to diagnose, in case it indicates something like bad mutations involving bad genes
that will compromise the health of the guix ecology.

How is a "fix" judged with respect to the big picture?

Is there a higher level layered[1] design for the use of guix, like e.g. [2] which a proposed fix
might violate and therefore should be rejected, even though it makes something "work"?

Well, it's probably in an old paper by Ludo in some form, but I wonder
what concepts of layering guix developers are consciously using
when putting stuff between the declarations at the top and
the images at the bottom.

[1] https://en.wikipedia.org/wiki/Abstraction_layer
[2] https://en.wikipedia.org/wiki/OSI_model

-- 
Regards,
Bengt Richter
 

bug#42252: Not possible to reliably port forward with "guix system vm" anymore

[Christopher Lemmer Webber]

[+ Cc: Marius Bakke]
because I don't have enough info to respond fully myself.

Bengt Richter writes:

> Hi
>
> On +2020-07-07 16:40:21 -0400, Christopher Lemmer Webber wrote:
>> In commit 5379392731b52eef22b4936637eb592b93e04318, the following change
>> was introduced:
>> 
>>   modified   gnu/system/vm.scm
>>   @@ -941,6 +941,7 @@ with '-virtfs' options for the host file systems listed in SHARED-FS."
>>                '())
>>    
>>         "-no-reboot"
>>   +     "-nic" "user,model=virtio-net-pci"
>>         "-object" "rng-random,filename=/dev/urandom,id=guixsd-vm-rng"
>>         "-device" "virtio-rng-pci,rng=guixsd-vm-rng"
>> 
>> Unfortunately, this means that in our docs where we suggest doing the
>> following:
>> 
>>   `guix system vm config.scm` -nic user,model=virtio-net-pci,hostfwd=tcp::10022-:22
>> 
>> Since we now provide our own similar "-nic" field this creates a
>> *second* network interface at the same address and there is a race as in
>> terms of which handles connections.  Depending on the race result,
>> connections to the forwarded port may hang indefinitely.
>> 
>> Ironically, this regression was introduced to solve another regression!
>> From the commit message:
>> 
>>   This fixes a regression introduced in 8e53fe2b91d2776bc1529e7b34967c8f1d9edc32
>>   where 'guix system vm' would no longer be using virtio.
>>
>
> This reminds a bit of doctors prescribing powerful medicine with side-effect so bad
> that they have to prescribe a medicine for that, which in turn has side-effects,
> in what I think is called prescription cascading, and people wind up on 25 pills a day.
>
> "First, do no harm." :)

Well, I'm definitely not actively trying to harm ;)

> I wouldn't say anything, except ISTM your fix on top of a fix
> is not the first to remind me of cascading :)
>
>> What's the right solution?  One could be that "guix system vm" itself
>> could take an argument that sets up port forwarding in the generated
>> shell script.  Eg:
>> 
>>   guix system vm config.scm --hostfwd=tcp::10022-:22 --hostfwd=tcp::8888-:80
>> 
>> kind of ugly, but it could work.  WDYT?
>> 
>>  - Chris
>
> I'm not saying your solution is bad, I'm just saying cascading fixes may be a symptom
> to diagnose, in case it indicates something like bad mutations involving bad genes
> that will compromise the health of the guix ecology.
>
> How is a "fix" judged with respect to the big picture?

You raise a point in that my "fix to a fix" was a solution when I don't
fully understand the problem that was being fixed.  Of course, this
isn't uncommon in software development, but that doesn't make it great.
I only understood as much context as I could to make my workaround to
the problem.  Is it the right long-term solution?  I'm not sure, but
I think Marius Bakke has more context to be able to reply than I do.
What I do know is that the present instructions we have for port
forwarding are now effectively broken, and this at least provides a way
to get back there.  It might not be the right one.

For the rest of your email, I do think Guix is well layered... but even
well layered systems sometimes have intermediate proposed solutions to
bugs.  Sometimes the right design is found along the way.

But part of the goal of submitting such a patch is to get code review
and provoke discussion of what the right thing is.  I *did* make it
clear that I thought it was ugly, but workable.  :)

It might not be the totally wrong thing either though... if there are
enough modifications that users might make to the -nic flag, but we
don't know how to nicely abstract over all of them yet, but for now we
do need to supply a default, this can be an escape hatch at least for
now.  This wouldn't be uncommon; that's very similar to how Guix system
configuration tends to go (we supply configuration builders for the most
common options but sometimes provide a way to just slot in a manual
config file when need be).

 - Chris

bug#42252: Not possible to reliably port forward with "guix system vm" anymore

[Marius Bakke]

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

Hello!

Sorry for this breakage, and thanks for the analysis!

Christopher Lemmer Webber <cwebber@dustycloud.org> writes:

> In commit 5379392731b52eef22b4936637eb592b93e04318, the following change
> was introduced:
>
>   modified   gnu/system/vm.scm
>   @@ -941,6 +941,7 @@ with '-virtfs' options for the host file systems listed in SHARED-FS."
>                '())
>    
>         "-no-reboot"
>   +     "-nic" "user,model=virtio-net-pci"
>         "-object" "rng-random,filename=/dev/urandom,id=guixsd-vm-rng"
>         "-device" "virtio-rng-pci,rng=guixsd-vm-rng"
>
> Unfortunately, this means that in our docs where we suggest doing the
> following:
>
>   `guix system vm config.scm` -nic user,model=virtio-net-pci,hostfwd=tcp::10022-:22
>
> Since we now provide our own similar "-nic" field this creates a
> *second* network interface at the same address and there is a race as in
> terms of which handles connections.  Depending on the race result,
> connections to the forwarded port may hang indefinitely.
>
> Ironically, this regression was introduced to solve another regression!
>>From the commit message:
>
>   This fixes a regression introduced in 8e53fe2b91d2776bc1529e7b34967c8f1d9edc32
>   where 'guix system vm' would no longer be using virtio.
>
> What's the right solution?  One could be that "guix system vm" itself
> could take an argument that sets up port forwarding in the generated
> shell script.  Eg:
>
>   guix system vm config.scm --hostfwd=tcp::10022-:22 --hostfwd=tcp::8888-:80
>
> kind of ugly, but it could work.  WDYT?

My motivation for the breaking commit was just that 'guix system vm' and
system tests would use virtio by default.  Without it, system tests with
forwarded ports used a different driver than those without forwardings.
It's a very minor issue and can be solved in other ways.  :-)

If no -nic parameter is specified on the QEMU command line, QEMU will
create one, emulating an Intel NIC.  I did not consider the discrepancy
this caused with the documentation when we unconditionally pass a -nic
parameter!

I think we should revert the commit, so that '`guix system vm` -nic foo'
works as expected for end users.  In fact I just did so.  :-)

Fixed in 1abf205d11c8b941d7d89855cb55a9cfde078838, thanks!

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