finch
finch copied to clipboard
runfinch
feat: resolve special IP host-gateway
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
- [x] 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.
@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)?
@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
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.
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
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
Please rename the PR to conventional commit message style to pass the PR title check. https://www.conventionalcommits.org/en/v1.0.0/
@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
@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
@TrungBui59 Reading the code, it seems passing it here should work. Have you tried it? What is the error?
@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
@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?
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
@ningziwen sorry for the late response, I have finalized by adding unit tests and e2e tests
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?
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.
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.
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.
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
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 https://github.com/runfinch/common-tests/issues/72. Once you find the issue you want to take, you can reply it and we will assign to you.