Suppress "Not forwarding …" messages when forwarding has been disabled

[jandubois]

As suggested in #2596 (comment):

When all port forwarding is disabled, then showing a "Not forwarding …" message for each opened port is pretty useless.

[nirs]

Thanks! I'll try this later.

[nirs]

Are these really info level messages? They are usefull only for debugging your configuration - so debug level.

What if you don't specify rule.Proto? we consider this as ignore both or not ignore any?

[jandubois]

Are these really info level messages? They are usefull only for debugging your configuration - so debug level.

I don't have strong opinions on this, but in some way all the info messages are only useful for debugging your configuration. The only one that will be left is:

INFO[0069] READY. Run `lima` to open the shell.

I would prefer to keep this at the info level, just so that if somebody puts the block to ignore port forwarding in their default.yaml or override.yaml files, and then forgets about it, they will have a reminder in the info log of what they have done.

What if you don't specify rule.Proto? we consider this as ignore both or not ignore any?

lima/pkg/limayaml/defaults.go

Lines 809 to 811 in 52f5ad3

	if rule.Proto == "" {
	rule.Proto = TCP
	}

So you would need to add another rule to ignore with this PR:

portForwards:
- ignore: true
- ignore: true
  proto: udp

We probably should have support for proto: all and default to that.

You will notice that support for UDP and IPv6 is not fully considered in the port forwarding implementation and needs to be improved. Note that support for UDP port forwarding was only added to Lima last week in #2411.

[nirs]

Another issue - this works on single rule level. If you have:

portForwards:
- ignore: true

This is pretty clear, but if you have:

portForwards:
- guestPort: 80
  hostPort: 8080
- ignore: true

This code will treat the second rule as disabling port forwarding, while the purpose of the rule is to disable anything else port 80. In this case the user may want to see the Not forwarding message for port 80.

We can fix this by adding check for len(y.PortForwards) == 1 (hopefully lima append the any port forwading rule later).

This could be much simpler if we have a simple switch to disable this feature. Maybe to keep the simple structrue, add something like:

features:
  portForwarding: false

With this you can keep your complex configuration as is, and enable/disable it with one change in the features map.

Another way is to make portForwards structure more flexible:

portForwarding:
  disabled: true
  rules:
  - guestPort: x
    hostPort: y

It is little bit more complex but much more flexible. And it models exactly nicely the actual thing: port forwarding rules.

[jandubois]

This is pretty clear, but if you have:

portForwards:
- guestPort: 80
  hostPort: 8080
- ignore: true

This code will treat the second rule as disabling port forwarding, while the purpose of the rule is to disable anything else port 80. In this case the user may want to see the Not forwarding message for port 80.

And they will:

for _, rule := range y.PortForwards {
	if rule.Ignore && rule.GuestPortRange[0] == 1 && rule.GuestPortRange[1] == 65535 {
		…
	} else {
		break
	}
}

Note how the else { break } part will exit the loop as soon as it encounters a rule that doesn't ignore all ports. So ignoreTCP and ignoreUDP are only set if there are no other rules in front of them.

[jandubois]

This could be much simpler if we have a simple switch to disable this feature. Maybe to keep the simple structrue, add something like:

features:
  portForwarding: false

I don't see how this is simpler than

portForwards:
- ignore: true

(once we make the default work for UDP as well).

Adding redundant syntactic sugar for functionality that is already available is just adding complexity.

And this additional "feature" is much more limited: what if the users only wants to disable UDP. Or only disable IPv6? You are just adding a special case for the one configuration that you personally are interested in, while adding to the cognitive load of everybody else who wants to wrap their head around how this can be configured.

[nirs]

Are these really info level messages? They are usefull only for debugging your configuration - so debug level.

I don't have strong opinions on this, but in some way all the info messages are only useful for debugging your configuration. The only one that will be left is:

INFO[0069] READY. Run `lima` to open the shell.

I would prefer to keep this at the info level, just so that if somebody puts the block to ignore port forwarding in their default.yaml or override.yaml files, and then forgets about it, they will have a reminder in the info log of what they have done.

This is debugging why it did not work as expected, so debug level.

What can be useful in info level, is message like:

Port forwarding is overridden by overides.yaml

And if we think that using non-default default.yaml is useful, we can log about that also. But I'm not sure about that, it depends if users had problems with such configuration.

If you configured port forwarding, info level message about forwarded port confirms that the system is operating as expected. This is also a change affecting the host, so it is import event - info level.

The messages about "waiting for retirement", "requirement satisfied" are helping the user to get some feedback about the progress. These are also useful when another program is running limactl for reporting progress. Provisioning k8s cluster takes hare about 3 minutes and having no feedback from the tool for 3 minutes is not good user experience.

Maybe the right thing is to log everything to a file, so you always have all the info, and show a nice progress with only high level info for the user.

[jandubois]

everybody else who wants to wrap their head around how this can be configured

Now, if you want to argue that the documentation should be improved beyond having a bunch of samples in default.yaml, then I will wholeheartedly agree.

[jandubois]

Maybe the right thing is to log everything to a file, so you always have all the info, and show a nice progress with only high level info for the user.

We do log everything to files (ha.stderr.log and ha.stdout.log) in the instance directory.

Personally I wouldn't mind hiding most everything until it starts waiting for requirements. Everything before is rarely useful unless you are debugging a failure:

INFO[0000] QEMU binary "/usr/local/bin/qemu-system-x86_64" seems properly signed with the "com.apple.security.hypervisor" entitlement
INFO[0000] Attempting to download the image              arch=x86_64 digest="sha256:0e25ca6ee9f08ec5d4f9910054b66ae7163c6152e81a3e67689d89bd6e4dfa69" location="https://cloud-images.ubuntu.com/releases/24.04/release-20240821/ubuntu-24.04-server-cloudimg-amd64.img"
INFO[0000] Using cache "/Users/jan/Library/Caches/lima/download/by-url-sha256/b2b185213b60ce48564393cf9eeb3c2ce4e92df4d2ca2edad8657c18d0e3052d/data"
INFO[0000] Attempting to download the nerdctl archive    arch=x86_64 digest="sha256:2c841e097fcfb5a1760bd354b3778cb695b44cd01f9f271c17507dc4a0b25606" location="https://github.com/containerd/nerdctl/releases/download/v1.7.6/nerdctl-full-1.7.6-linux-amd64.tar.gz"
INFO[0000] Using cache "/Users/jan/Library/Caches/lima/download/by-url-sha256/b86908f1f5ea2af45aec405f0fd389eba1999b51e3972ca78215ace94d2da2a6/data"
WARN[0001] [hostagent] GRPC port forwarding is experimental
INFO[0001] [hostagent] hostagent socket created at /Users/jan/.lima/default/ha.sock
INFO[0002] [hostagent] Using system firmware ("/usr/local/share/qemu/edk2-x86_64-code.fd")
INFO[0002] [hostagent] Starting QEMU (hint: to watch the boot progress, see "/Users/jan/.lima/default/serial*.log")
INFO[0002] SSH Local Port: 60022

But I think this should be a separate discussion from dropping individual Not forwarding … messages from the hostagent.

[nirs]

@jandubois I tested it:

% cat test.yaml 
vmType: vz
images:
- location: "https://cloud-images.ubuntu.com/releases/24.04/release/ubuntu-24.04-server-cloudimg-arm64.img"
  arch: "aarch64"
networks:
 - socket: "/var/run/socket_vmnet"
containerd:
  system: false
  user: false
portForwards:
- ignore: true
- ignore: true
  proto: udp

% limactl validate test.yaml
FATA[0000] failed to load YAML file "test.yaml": field `portForwards[1].proto` must be "tcp” 

It works with this additional change:

diff --git a/pkg/limayaml/validate.go b/pkg/limayaml/validate.go
index b70fe5a..fe43fe9 100644
--- a/pkg/limayaml/validate.go
+++ b/pkg/limayaml/validate.go
@@ -282,8 +282,8 @@ func Validate(y *LimaYAML, warn bool) error {
                        return fmt.Errorf("field `%s.hostSocket` must be less than UNIX_PATH_MAX=%d characters, but is %d",
                                field, osutil.UnixPathMax, len(rule.HostSocket))
                }
-               if rule.Proto != TCP {
-                       return fmt.Errorf("field `%s.proto` must be %q", field, TCP)
+               if rule.Proto != TCP && rule.Proto != UDP {
+                       return fmt.Errorf("field `%s.proto` must be one of %q, %q", field, TCP, UDP)
                }
                if rule.Reverse && rule.GuestSocket == "" {
                        return fmt.Errorf("field `%s.reverse` must be %t", field, false)

With this got the expected message about disabling tcp and udp port forwarding, and no "not forwarding" logs.

% limactl validate test.yaml                          
INFO[0000] "test.yaml": OK
                              
% limactl start test.yaml --tty=false
INFO[0000] Terminal is not available, proceeding without opening an editor 
INFO[0000] Starting the instance "test" with VM driver "vz" 
INFO[0000] Attempting to download the image              arch=aarch64 digest= location="https://cloud-images.ubuntu.com/releases/24.04/release/ubuntu-24.04-server-cloudimg-arm64.img"
INFO[0000] Using cache "/Users/nsoffer/Library/Caches/lima/download/by-url-sha256/002fbe468673695a2206b26723b1a077a71629001a5b94efd8ea1580e1c3dd06/data" 
INFO[0000] Converting "/Users/nsoffer/.lima/test/basedisk" (qcow2) to a raw disk "/Users/nsoffer/.lima/test/diffdisk" 
3.50 GiB / 3.50 GiB [-------------------------------------] 100.00% 202.21 MiB/s
INFO[0017] Expanding to 100GiB                          
INFO[0018] [hostagent] TCP port forwarding is disabled (except for SSH) 
INFO[0018] [hostagent] UDP port forwarding is disabled  
WARN[0018] [hostagent] GRPC port forwarding is experimental 
INFO[0018] [hostagent] hostagent socket created at /Users/nsoffer/.lima/test/ha.sock 
INFO[0018] [hostagent] Starting VZ (hint: to watch the boot progress, see "/Users/nsoffer/.lima/test/serial*.log") 
INFO[0018] [hostagent] new connection from  to          
INFO[0019] SSH Local Port: 56671                        
INFO[0019] [hostagent] Waiting for the essential requirement 1 of 2: "ssh" 
INFO[0019] [hostagent] [VZ] - vm state change: running  
INFO[0029] [hostagent] Waiting for the essential requirement 1 of 2: "ssh" 
INFO[0029] [hostagent] The essential requirement 1 of 2 is satisfied 
INFO[0029] [hostagent] Waiting for the essential requirement 2 of 2: "user session is ready for ssh" 
INFO[0029] [hostagent] The essential requirement 2 of 2 is satisfied 
INFO[0029] [hostagent] Waiting for the guest agent to be running 
INFO[0029] [hostagent] Guest agent is running           
INFO[0029] [hostagent] Waiting for the final requirement 1 of 1: "boot scripts must have finished" 
INFO[0029] [hostagent] The final requirement 1 of 1 is satisfied 
INFO[0030] READY. Run `limactl shell test` to open the shell. 

There is one new unneeded warning that seems to be added recently:

WARN[0018] [hostagent] GRPC port forwarding is experimental 

It is good to notify about experimental features but I did not enable it. It this requires a warning it should not be enabled by default. I guess this should be removed when 1.0 is released?

[jandubois]

There is one new unneeded warning that seems to be added recently:

WARN[0018] [hostagent] GRPC port forwarding is experimental 

It is good to notify about experimental features but I did not enable it. It this requires a warning it should not be enabled by default. I guess this should be removed when 1.0 is released?

This warning came in with the support for UDP port forwarding last week.

It is my understanding that the GRPC forwarder will remain the default for the 1.0 release unless major issues are discovered with it.

I agree that it should not be considered experimental once it becomes the default in a release.

You can opt-out of the new forwarder by setting LIMA_SSH_PORT_FORWARDER=1 when you start the instance. I would assume that the ssh forwarder will eventually be removed.

[jandubois]

It works with this additional change:

I noticed that the code does not actually implement separate port forwarding rules for UDP. I've implemented it in 392f700 (in a separate PR #2605, built on top of this one). Please test if you have the time!

[jandubois]

I've implemented it in 392f700

I've added an any protocol option to match either tcp or udp, but intentionally did not make it the default because in general there is no connection between the tcp and udp port of the same number. So to disable all port forwarding you would have to specify:

portForwards:
- proto: any
  ignore: true

[nirs]

It works with this additional change:

I noticed that the code does not actually implement separate port forwarding rules for UDP. I've implemented it in e098dd2 (in a separate PR #2605, built on top of this one). Please test if you have the time!

Sure I can test it, but I only test ignoring any in my setup.

[jandubois]

@AkihiroSuda Could you please comment if this is acceptable to you? In #2596 (comment) you wrote:

I'd say "Not forwarding" message is important and should be printed by default.

but this PR only drops those messages if all forwarding has been disabled (and still leaves a single message telling you that it has been disabled).

[AkihiroSuda]

Thanks