feat: resolve special IP host-gateway #453

TrungBui59 · 2023-06-14T21:21:21Z

Issue #209

Description of changes:

I add the host-gateway-ip to the finch config file and use that to make the host-gateway-ip configurable using nerdctl

Testing done:
I have done some unit test, it passed most of the e2e tests, but now it show some weird error that I don't think is a problem because of my code change

I've reviewed the guidance in CONTRIBUTING.md

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ningziwen · 2023-06-14T21:27:06Z

@TrungBui59 Thanks. Looks like your screenshot only covers the final summary of the failing e2e tests. Could you scroll up, find the verbose logs for each failing scenario, and paste here (Please paste instead of screenshotting for convenient search)?

cmd/finch/nerdctl.go

TrungBui59 · 2023-06-14T21:38:31Z

@TrungBui59 Thanks. Looks like your screenshot only covers the final summary of the failing e2e tests. Could you scroll up, find the verbose logs for each failing scenario, and paste here (Please paste instead of screenshotting for convenient search)?

Here are the log, for the errors that is causing by my changes, it is mostly timeout issue

ningziwen · 2023-06-15T06:58:49Z

There are 3 ways of filling host gateway IP in nerdctl: config, command arg and env var.

Finch should also support all of them for feature parity.

By default users can already use command arg to fill IP in Finch, as Finch inherits commands from Nerdctl. Env var should also work as Finch passes the envs to nerdctl commands.

Only Finch config needs some additional work. If we fill the value from Finch config to nerdctl command arg (as the current revision of the PR is doing) or env var, they may have conflict with the user-input command arg and env var. Finch can set some rules to let one override another, but that would cause unnecessary business logic in Finch. We could have directly passed the value from Finch config to nerdctl config, so nerdctl will decide whatever rules to let one override another.

TrungBui59 · 2023-06-15T07:26:23Z

There are 3 ways of filling host gateway IP in nerdctl: config, command arg and env var.

Finch should also support all of them for feature parity.

By default users can already use command arg to fill IP in Finch, as Finch inherits commands from Nerdctl. Env var should also work as Finch passes the envs to nerdctl commands.

Only Finch config needs some additional work. If we fill the value from Finch config to nerdctl command arg (as the current revision of the PR is doing) or env var, they may have conflict with the user-input command arg and env var. Finch can set some rules to let one override another, but that would cause unnecessary business logic in Finch. We could have directly passed the value from Finch config to nerdctl config, so nerdctl will decide whatever rules to let one override another.

Oh I see, I will working on that approach right away

pkg/config/defaults.go

cmd/finch/nerdctl_test.go

pkg/config/nerdctl_config_applier_test.go

ningziwen · 2023-06-19T04:52:45Z

Could you add e2e tests?
The host-gateway-ip in finch config is under e2e/vm? Similar to https://github.com/runfinch/finch/blob/main/e2e/vm/additional_disk_test.go

The host-gateway-ip arg and env var can be added in common-tests repo around here. (can be a separated PR as it is separated repo) https://github.com/runfinch/common-tests/blob/6c7275007bf34fb6ddecc4013c16f1d79ff6d1d0/tests/run.go#L335

ningziwen · 2023-06-20T06:56:01Z

Please rename the PR to conventional commit message style to pass the PR title check. https://www.conventionalcommits.org/en/v1.0.0/

TrungBui59 · 2023-06-20T06:58:27Z

@ningziwen Sorry for not being responsive, I was busy with my intern work. I still dont know how to make it change the host-gateway-ip when the VM starts/restarts. I can make it change when we init the VM. Any suggestion on how to make it work with start and restart? so far, I have tried reading the finch config during the postVMStartAction, but it still does not work as I planned

TrungBui59 · 2023-06-20T06:58:36Z

@ningziwen Sorry for not being really responsive, I was busy with my intern work. I still dont know how to make it change the host-gateway-ip when the VM starts/restarts. I can make it change when we init the VM. Any suggestion on how to make it work with start and restart? so far, I have tried reading the finch config during the postVMStartAction, but it still does not work as I planned

ningziwen · 2023-06-20T07:07:28Z

@TrungBui59 Reading the code, it seems passing it here should work. Have you tried it? What is the error?

TrungBui59 · 2023-06-20T07:11:33Z

@TrungBui59 Reading the code, it seems passing it here should work. Have you tried it? What is the error?

@ningziwen The current approach is that I will pass the already existing finch config to the nerdctl, but when we restart it, the config change. And I still haven't found a way to pass the new value to it. I have tried to read the new config and create a new NerdctlConfigApplier with a new config, but it doesn't work

pendo324

concept / implementation LGTM, just a few more minor things in addition to what @ningziwen already commented on

pendo324 · 2023-06-21T16:04:28Z

pkg/config/config.go

@@ -67,6 +68,7 @@ type Nerdctl struct {
 	CgroupManager    string   `toml:"cgroup_manager,omitempty"`
 	InsecureRegistry bool     `toml:"insecure_registry,omitempty"`
 	HostsDir         []string `toml:"hosts_dir,omitempty"`
+	HostGatewayIP *string `toml:"host_gateway_ip,omitempty"`


nit: formatting

pkg/config/config.go

pendo324 · 2023-06-21T16:04:48Z

pkg/config/nerdctl_config_applier.go

@@ -9,10 +9,12 @@ import (
 	"path"
 	"strings"

+	"github.com/lima-vm/lima/pkg/networks"


nit: import order

How can I know prevent these nit issue? I ran golang-lint and it didnt give me anything

Hi @TrungBui59, golang-lint will lint code, but not format it. You can use gofmt -w . from the top level directory of a Go project to format all Go files, writing the correctly formatted files. Go plugins for modern IDEs will typically do this every time a Go file is saved.

See https://go.dev/blog/gofmt

pendo324 · 2023-06-21T16:05:36Z

pkg/config/nerdctl_config_applier.go

@@ -23,6 +25,7 @@ const (
 )

 type nerdctlConfigApplier struct {
+	fc			   *Finch


nit: formatting

pendo324 · 2023-06-21T16:05:40Z

pkg/config/nerdctl_config_applier.go

 	return &nerdctlConfigApplier{
+		fc: 			fc,


nit: formatting

TrungBui59 · 2023-06-21T16:47:30Z

@TrungBui59 Reading the code, it seems passing it here should work. Have you tried it? What is the error?

@ningziwen The current approach is that I will pass the already existing finch config to the nerdctl, but when we restart it, the config change. And I still haven't found a way to pass the new value to it. I have tried to read the new config and create a new NerdctlConfigApplier with a new config, but it doesn't work

Hi @pendo324, I can only add the host-gateway-ip when we init the VM. However, @ningziwen wants me to make the host-gateway-ip to be configurable when the user starts/restarts the VM. I have tried to update the finch config at the postVMStartAction state, but it is not working. Can you give some insight on how to make it work?

pkg/config/nerdctl_config_applier.go

TrungBui59 · 2023-06-26T22:57:04Z

Could you add e2e tests? The host-gateway-ip in finch config is under e2e/vm? Similar to https://github.com/runfinch/finch/blob/main/e2e/vm/additional_disk_test.go

The host-gateway-ip arg and env var can be added in common-tests repo around here. (can be a separated PR as it is separated repo) https://github.com/runfinch/common-tests/blob/6c7275007bf34fb6ddecc4013c16f1d79ff6d1d0/tests/run.go#L335

Sure, I will also need to change some unit test

cmd/finch/nerdctl_test.go

cmd/finch/support_bundle.go

pkg/config/nerdctl_config_applier_test.go

cmd/finch/nerdctl_test.go

TrungBui59 · 2023-09-01T23:30:21Z

@ningziwen sorry for the late response, I have finalized by adding unit tests and e2e tests

cmd/finch/nerdctl.go

pkg/config/config.go

e2e/vm/host_gateway_ip_test.go

Signed-off-by: TrungBui59 <[email protected]>

ningziwen · 2023-09-06T00:09:16Z

pkg/config/config.go

@@ -54,7 +54,8 @@ type Finch struct {
 	// Requires macOS 13.0 or later and an Apple Silicon (ARM64) mac.
 	// Has no effect on systems where Rosetta 2 is not available (Intel/AMD64 macs, or macOS < 13.0).
 	// This setting will only be applied on vm init.
-	Rosetta *bool `yaml:"rosetta,omitempty"`
+	Rosetta     *bool   `yaml:"rosetta,omitempty"`
+	HostGatewayIp *string `yaml:"host_gateway_ip,omitempty"`


Please make the naming consistent with line 76. Probably follow the pattern in nerdctl. https://github.com/containerd/nerdctl/blob/1f8f8af4d19d651a6300d8d48f3f21c91048a716/pkg/config/config.go#L41

HostGatewayIP

Just fix it

ningziwen

One nit pick. Also need to fix the errors in CI.

Others LGTM.

Need a follow-up PR in https://github.com/runfinch/common-tests to test CLI flag and env var.

Signed-off-by: TrungBui59 <[email protected]>

TrungBui59 · 2023-09-06T00:21:39Z

One nit pick. Also need to fix the errors in CI.

Others LGTM.

Need a follow-up PR in https://github.com/runfinch/common-tests to test CLI flag and env var.

@ningziwen I just finish with it, I will do the follow up work on the common tests file. Can you assign me another issue to do?

ningziwen · 2023-09-06T00:24:02Z

One nit pick. Also need to fix the errors in CI.
Others LGTM.
Need a follow-up PR in https://github.com/runfinch/common-tests to test CLI flag and env var.

@ningziwen I just finish with it, I will do the follow up work on the common tests file. Can you assign me another issue to do?

Same issue is fine. One issue can map to multiple PRs.

ningziwen · 2023-09-06T00:27:06Z

Could you make the PR title more accurate? Such as "feat: make host gateway ip configurable". The PR title will be the final commit message.

ningziwen · 2023-09-06T00:27:44Z

Please follow the guideline to fix DCO. https://github.com/runfinch/finch/pull/453/checks?check_run_id=16527210509

go linter and go-mod-tidy-check are still failing.

TrungBui59 · 2023-09-06T00:27:59Z

One nit pick. Also need to fix the errors in CI.

Others LGTM.

Need a follow-up PR in https://github.com/runfinch/common-tests to test CLI flag and env var.

@ningziwen I just finish with it, I will do the follow up work on the common tests file. Can you assign me another issue to do?

Same issue is fine. One issue can map to multiple PRs.

Sure, I will do it. I mean like after the follow up work, can you assign me another one? I am still a newbie with Finch so I will need sometime to understand and find a way to address it

ginglis13 · 2023-09-06T00:20:45Z

pkg/config/config.go

@@ -54,7 +54,8 @@ type Finch struct {
 	// Requires macOS 13.0 or later and an Apple Silicon (ARM64) mac.
 	// Has no effect on systems where Rosetta 2 is not available (Intel/AMD64 macs, or macOS < 13.0).
 	// This setting will only be applied on vm init.
-	Rosetta *bool `yaml:"rosetta,omitempty"`
+	Rosetta     *bool   `yaml:"rosetta,omitempty"`
+	HostGatewayIp *string `yaml:"host_gateway_ip,omitempty"`


Please add godoc comment i.e.

Suggested change

HostGatewayIp *string `yaml:"host_gateway_ip,omitempty"`

// HostGatewayIp sets the default host gateway ip....

HostGatewayIp *string `yaml:"host_gateway_ip,omitempty"`

ginglis13 · 2023-09-06T00:22:06Z

pkg/config/nerdctl_config_applier.go

@@ -124,6 +126,10 @@ func updateNerdctlConfig(fs afero.Fs, user string, rootless bool) error {
 	}

 	cfg.Namespace = nerdctlNamespace
+	cfg.HostGatewayIP = "192.168.5.2"


instead of having this magic string down here, could we define as a constant defaultNerdctlHostGatewayIP here https://github.com/runfinch/finch/pull/453/files#diff-531afb9f97a7384e0de895046f9e14befbebc49c896b080fce4c41c75f14f1b4L20-L21

then you can also reference it in unit tests where you expect the HostGatewayIP to be the default value.

cmd/finch/nerdctl_test.go

ningziwen · 2023-09-06T21:03:30Z

One nit pick. Also need to fix the errors in CI.

Others LGTM.

Need a follow-up PR in https://github.com/runfinch/common-tests to test CLI flag and env var.

@ningziwen I just finish with it, I will do the follow up work on the common tests file. Can you assign me another issue to do?

Same issue is fine. One issue can map to multiple PRs.

Sure, I will do it. I mean like after the follow up work, can you assign me another one? I am still a newbie with Finch so I will need sometime to understand and find a way to address it

@TrungBui59 We don't do assignments proactively normally. Feel free to try out any issues that you are interested. If you are concerned about the difficulty, you can filter "good first issue". If you are not clear about the requirements of specific issue, you can also reply to clarify. There are also other simple but interesting issues under other subprojects of runfinch, such as runfinch/common-tests#72. Once you find the issue you want to take, you can reply it and we will assign to you.

ningziwen · 2023-09-07T19:28:54Z

@TrungBui59 e2e tests are failing in CI. Could you fix it?

ningziwen reviewed Jun 14, 2023

View reviewed changes

cmd/finch/nerdctl.go Outdated Show resolved Hide resolved

TrungBui59 force-pushed the main branch from 9e86d79 to d8995bc Compare June 15, 2023 18:54

ningziwen reviewed Jun 15, 2023

View reviewed changes

pkg/config/defaults.go Outdated Show resolved Hide resolved

TrungBui59 marked this pull request as draft June 15, 2023 21:45

TrungBui59 force-pushed the main branch from d8995bc to 6786d0e Compare June 16, 2023 21:29

TrungBui59 closed this Jun 17, 2023

TrungBui59 force-pushed the main branch 2 times, most recently from 2f506f9 to fef6e77 Compare June 17, 2023 07:56

ningziwen reopened this Jun 19, 2023

ningziwen reviewed Jun 19, 2023

View reviewed changes

cmd/finch/nerdctl_test.go Outdated Show resolved Hide resolved

ningziwen reviewed Jun 19, 2023

View reviewed changes

pkg/config/nerdctl_config_applier_test.go Show resolved Hide resolved

ningziwen requested review from pendo324 and removed request for pendo324 June 19, 2023 04:53

ningziwen marked this pull request as ready for review June 19, 2023 04:53

TrungBui59 changed the title ~~Rough Solution for #209~~ feat: resolve special IP host-gateway Jun 20, 2023

pendo324 reviewed Jun 21, 2023

View reviewed changes

ningziwen reviewed Jun 26, 2023

View reviewed changes

pkg/config/nerdctl_config_applier.go Outdated Show resolved Hide resolved

ningziwen reviewed Jun 29, 2023

View reviewed changes

cmd/finch/nerdctl_test.go Outdated Show resolved Hide resolved

ningziwen reviewed Jun 29, 2023

View reviewed changes

cmd/finch/support_bundle.go Outdated Show resolved Hide resolved

ningziwen reviewed Jun 29, 2023

View reviewed changes

pkg/config/nerdctl_config_applier_test.go Outdated Show resolved Hide resolved

ningziwen reviewed Jun 30, 2023

View reviewed changes

cmd/finch/nerdctl_test.go Outdated Show resolved Hide resolved

TrungBui59 closed this Aug 22, 2023

TrungBui59 force-pushed the main branch from 49728d9 to bbdd782 Compare August 22, 2023 17:07

Merge branch 'upstream/main' and adding test

faa3b20

TrungBui59 reopened this Sep 1, 2023

ningziwen reviewed Sep 2, 2023

View reviewed changes

cmd/finch/nerdctl.go Outdated Show resolved Hide resolved

ningziwen reviewed Sep 2, 2023

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

ningziwen reviewed Sep 2, 2023

View reviewed changes

e2e/vm/host_gateway_ip_test.go Show resolved Hide resolved

Fixing nit issue

410c2b7

ningziwen reviewed Sep 3, 2023

View reviewed changes

e2e/vm/host_gateway_ip_test.go Outdated Show resolved Hide resolved

Adding test for finch config file for test case

65921b9

Signed-off-by: TrungBui59 <[email protected]>

ningziwen reviewed Sep 6, 2023

View reviewed changes

ningziwen previously approved these changes Sep 6, 2023

View reviewed changes

ningziwen requested review from pendo324 and ginglis13 September 6, 2023 00:11

Fixing host_gateway_ip to follow nerdctl standard

c540203

Signed-off-by: TrungBui59 <[email protected]>

TrungBui59 dismissed ningziwen’s stale review via c540203 September 6, 2023 00:19

ginglis13 reviewed Sep 6, 2023

View reviewed changes

Merge branch 'runfinch:main' into main

8f618b9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: resolve special IP host-gateway #453

feat: resolve special IP host-gateway #453

TrungBui59 commented Jun 14, 2023

ningziwen commented Jun 14, 2023

TrungBui59 commented Jun 14, 2023

ningziwen commented Jun 15, 2023

TrungBui59 commented Jun 15, 2023

ningziwen commented Jun 19, 2023

ningziwen commented Jun 20, 2023 •

edited

Loading

TrungBui59 commented Jun 20, 2023

TrungBui59 commented Jun 20, 2023

ningziwen commented Jun 20, 2023

TrungBui59 commented Jun 20, 2023

pendo324 left a comment •

edited

Loading

pendo324 Jun 21, 2023

pendo324 Jun 21, 2023

TrungBui59 Jun 26, 2023

ginglis13 Jun 28, 2023

pendo324 Jun 21, 2023

pendo324 Jun 21, 2023

TrungBui59 commented Jun 21, 2023

TrungBui59 commented Jun 26, 2023

TrungBui59 commented Sep 1, 2023

ningziwen Sep 6, 2023

TrungBui59 Sep 6, 2023

ningziwen left a comment •

edited

Loading

TrungBui59 commented Sep 6, 2023 •

edited

Loading

ningziwen commented Sep 6, 2023

ningziwen commented Sep 6, 2023

ningziwen commented Sep 6, 2023

TrungBui59 commented Sep 6, 2023

ginglis13 Sep 6, 2023

ginglis13 Sep 6, 2023

ningziwen commented Sep 6, 2023

ningziwen commented Sep 7, 2023

	HostGatewayIp *string `yaml:"host_gateway_ip,omitempty"`
	// HostGatewayIp sets the default host gateway ip....
	HostGatewayIp *string `yaml:"host_gateway_ip,omitempty"`

feat: resolve special IP host-gateway #453

Are you sure you want to change the base?

feat: resolve special IP host-gateway #453

Conversation

TrungBui59 commented Jun 14, 2023

License Acceptance

ningziwen commented Jun 14, 2023

TrungBui59 commented Jun 14, 2023

ningziwen commented Jun 15, 2023

TrungBui59 commented Jun 15, 2023

ningziwen commented Jun 19, 2023

ningziwen commented Jun 20, 2023 • edited Loading

TrungBui59 commented Jun 20, 2023

TrungBui59 commented Jun 20, 2023

ningziwen commented Jun 20, 2023

TrungBui59 commented Jun 20, 2023

pendo324 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TrungBui59 commented Jun 21, 2023

TrungBui59 commented Jun 26, 2023

TrungBui59 commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningziwen left a comment • edited Loading

Choose a reason for hiding this comment

TrungBui59 commented Sep 6, 2023 • edited Loading

ningziwen commented Sep 6, 2023

ningziwen commented Sep 6, 2023

ningziwen commented Sep 6, 2023

TrungBui59 commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningziwen commented Sep 6, 2023

ningziwen commented Sep 7, 2023

ningziwen commented Jun 20, 2023 •

edited

Loading

pendo324 left a comment •

edited

Loading

ningziwen left a comment •

edited

Loading

TrungBui59 commented Sep 6, 2023 •

edited

Loading