Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add finch vm settings subcommand #887

Merged
merged 9 commits into from
Apr 19, 2024

Conversation

haytok
Copy link
Member

@haytok haytok commented Apr 4, 2024

The current implementation requires updating ~/.finch/finch.yaml to change the CPU and Memory settings in the VM.

However, the following issue provides a feature request to set the CPU and memory resources to be allocated to the VM from CLI options.

Therefore, this fix adds the finch vm settings command to add the ability to set the CPU and memory resources allocated to VMs from the CLI.

Issue #, if available: #683

Description of changes: Details are described in this commit message.

Testing done: Yes

  • 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.

cmd/finch/virtual_machine_settings.go Outdated Show resolved Hide resolved
cmd/finch/virtual_machine_settings.go Outdated Show resolved Hide resolved
cmd/finch/virtual_machine_settings.go Outdated Show resolved Hide resolved
pkg/config/lima_config_applier.go Outdated Show resolved Hide resolved
@Shubhranshu153
Copy link
Contributor

@haytok
some errors in the unit test and you might need to sign the commit.
Thanks

@haytok
Copy link
Member Author

haytok commented Apr 12, 2024

We are looking into resolving the following error.

import cycle not allowed in test
Errors

haytok ~/workspace/haytok/finch [add-settings-subcommand]
> make test-unit
go test github.com/runfinch/finch/benchmark github.com/runfinch/finch/benchmark/all github.com/runfinch/finch/benchmark/container github.com/runfinch/finch/benchmark/vm github.com/runfinch/finch/cmd/finch github.com/runfinch/finch/pkg/command github.com/runfinch/finch/pkg/config github.com/runfinch/finch/pkg/dependency github.com/runfinch/finch/pkg/dependency/credhelper github.com/runfinch/finch/pkg/dependency/vmnet github.com/runfinch/finch/pkg/disk github.com/runfinch/finch/pkg/flog github.com/runfinch/finch/pkg/fmemory github.com/runfinch/finch/pkg/fssh github.com/runfinch/finch/pkg/lima github.com/runfinch/finch/pkg/lima/wrapper github.com/runfinch/finch/pkg/mocks github.com/runfinch/finch/pkg/path github.com/runfinch/finch/pkg/support github.com/runfinch/finch/pkg/system github.com/runfinch/finch/pkg/version github.com/runfinch/finch/pkg/winutil -shuffle on
# github.com/runfinch/finch/pkg/config
package github.com/runfinch/finch/pkg/config
	imports github.com/runfinch/finch/pkg/mocks
	imports github.com/runfinch/finch/pkg/config: import cycle not allowed in test
FAIL	github.com/runfinch/finch/pkg/config [setup failed]
?   	github.com/runfinch/finch/benchmark	[no test files]
ok  	github.com/runfinch/finch/benchmark/all	0.449s [no tests to run]
ok  	github.com/runfinch/finch/benchmark/container	0.635s [no tests to run]
ok  	github.com/runfinch/finch/benchmark/vm	0.274s [no tests to run]
ok  	github.com/runfinch/finch/cmd/finch	0.740s
ok  	github.com/runfinch/finch/pkg/command	0.572s
?   	github.com/runfinch/finch/pkg/flog	[no test files]
?   	github.com/runfinch/finch/pkg/fmemory	[no test files]
ok  	github.com/runfinch/finch/pkg/dependency	0.616s
?   	github.com/runfinch/finch/pkg/lima/wrapper	[no test files]
?   	github.com/runfinch/finch/pkg/mocks	[no test files]
ok  	github.com/runfinch/finch/pkg/dependency/credhelper	0.595s
?   	github.com/runfinch/finch/pkg/system	[no test files]
?   	github.com/runfinch/finch/pkg/version	[no test files]
ok  	github.com/runfinch/finch/pkg/dependency/vmnet	0.573s
ok  	github.com/runfinch/finch/pkg/disk	0.566s
ok  	github.com/runfinch/finch/pkg/fssh	0.656s
ok  	github.com/runfinch/finch/pkg/lima	0.817s
ok  	github.com/runfinch/finch/pkg/path	0.882s
ok  	github.com/runfinch/finch/pkg/support	1.982s
ok  	github.com/runfinch/finch/pkg/winutil	1.068s
FAIL
make: *** [test-unit] Error 1
haytok ~/workspace/haytok/finch [add-settings-subcommand]
> make check-licenses
go mod download
GOBIN=/Users/haytok/workspace/haytok/finch/tools_bin go install github.com/google/go-licenses
/Users/haytok/workspace/haytok/finch/tools_bin/go-licenses check --ignore golang.org/x,github.com/runfinch/finch --allowed_licenses Apache-2.0,BSD-2-Clause,BSD-3-Clause,ISC,MIT --include_tests ./...
W0412 23:48:14.745128   34321 library.go:141] "github.com/shirou/gopsutil/v3/disk" contains non-Go code that can't be inspected for further dependencies:
/Users/haytok/go/pkg/mod/github.com/shirou/gopsutil/v3@v3.24.3/disk/iostat_darwin.c
/Users/haytok/go/pkg/mod/github.com/shirou/gopsutil/v3@v3.24.3/disk/iostat_darwin.h
F0412 23:48:14.745449   34321 main.go:75] errors for ["github.com/runfinch/finch/benchmark" "github.com/runfinch/finch/benchmark/all" "github.com/runfinch/finch/benchmark/container" "github.com/runfinch/finch/benchmark/vm" "github.com/runfinch/finch/pkg/flog" "github.com/runfinch/finch/pkg/system" "github.com/runfinch/finch/pkg/command" "github.com/runfinch/finch/pkg/fmemory" "github.com/runfinch/finch/pkg/fssh" "github.com/runfinch/finch/pkg/config" "github.com/runfinch/finch/pkg/dependency" "github.com/runfinch/finch/pkg/path" "github.com/runfinch/finch/pkg/dependency/credhelper" "github.com/runfinch/finch/pkg/dependency/vmnet" "github.com/runfinch/finch/pkg/disk" "github.com/runfinch/finch/pkg/lima" "github.com/runfinch/finch/pkg/lima/wrapper" "github.com/runfinch/finch/pkg/version" "github.com/runfinch/finch/pkg/support" "github.com/runfinch/finch/cmd/finch" "github.com/runfinch/finch/e2e" "github.com/runfinch/finch/e2e/container" "github.com/runfinch/finch/e2e/vm" "github.com/runfinch/finch/pkg/mocks" "github.com/runfinch/finch/pkg/winutil" "github.com/runfinch/finch/benchmark/all [github.com/runfinch/finch/benchmark/all.test]" "github.com/runfinch/finch/benchmark/all.test" "github.com/runfinch/finch/benchmark/container [github.com/runfinch/finch/benchmark/container.test]" "github.com/runfinch/finch/benchmark/container.test" "github.com/runfinch/finch/benchmark/vm [github.com/runfinch/finch/benchmark/vm.test]" "github.com/runfinch/finch/benchmark/vm.test" "github.com/runfinch/finch/cmd/finch [github.com/runfinch/finch/cmd/finch.test]" "github.com/runfinch/finch/cmd/finch.test" "github.com/runfinch/finch/e2e/container [github.com/runfinch/finch/e2e/container.test]" "github.com/runfinch/finch/e2e/container.test" "github.com/runfinch/finch/e2e/vm [github.com/runfinch/finch/e2e/vm.test]" "github.com/runfinch/finch/e2e/vm.test" "github.com/runfinch/finch/pkg/command [github.com/runfinch/finch/pkg/command.test]" "github.com/runfinch/finch/pkg/command_test [github.com/runfinch/finch/pkg/command.test]" "github.com/runfinch/finch/pkg/command.test" "github.com/runfinch/finch/pkg/config [github.com/runfinch/finch/pkg/config.test]" "github.com/runfinch/finch/pkg/config.test" "github.com/runfinch/finch/pkg/dependency [github.com/runfinch/finch/pkg/dependency.test]" "github.com/runfinch/finch/pkg/dependency.test" "github.com/runfinch/finch/pkg/dependency/credhelper [github.com/runfinch/finch/pkg/dependency/credhelper.test]" "github.com/runfinch/finch/pkg/dependency/credhelper.test" "github.com/runfinch/finch/pkg/dependency/vmnet [github.com/runfinch/finch/pkg/dependency/vmnet.test]" "github.com/runfinch/finch/pkg/dependency/vmnet.test" "github.com/runfinch/finch/pkg/disk [github.com/runfinch/finch/pkg/disk.test]" "github.com/runfinch/finch/pkg/disk.test" "github.com/runfinch/finch/pkg/fssh [github.com/runfinch/finch/pkg/fssh.test]" "github.com/runfinch/finch/pkg/fssh.test" "github.com/runfinch/finch/pkg/lima_test [github.com/runfinch/finch/pkg/lima.test]" "github.com/runfinch/finch/pkg/lima.test" "github.com/runfinch/finch/pkg/path [github.com/runfinch/finch/pkg/path.test]" "github.com/runfinch/finch/pkg/path.test" "github.com/runfinch/finch/pkg/support [github.com/runfinch/finch/pkg/support.test]" "github.com/runfinch/finch/pkg/support.test" "github.com/runfinch/finch/pkg/winutil [github.com/runfinch/finch/pkg/winutil.test]" "github.com/runfinch/finch/pkg/winutil.test"]:
github.com/runfinch/finch/pkg/config: -: import cycle not allowed in test
make: *** [check-licenses] Error 1

Other errors have been fixed.

@haytok
Copy link
Member Author

haytok commented Apr 15, 2024

@Shubhranshu153
Refactored and fixed so that the test passes. Could you review it again?

The current implementation requires updating ~/.finch/finch.yaml to change
the CPU and Memory settings in the VM.

However, the following issue provides a feature request to set the CPU and
memory resources to be allocated to the VM from CLI options.

- runfinch#683

Therefore, this fix adds the finch vm settings command to add the ability
to set the CPU and memory resources allocated to VMs from the CLI.

Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
Based on the review, the code has been modified.

Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
fix to pass CI

Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
fix to pass CI

Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
Refactoring so that make test-unit passes.
Also, modified and added tests.

Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
@haytok
Copy link
Member Author

haytok commented Apr 15, 2024

The details of the macos-latest CI failure are as follows

Use "finch [command] --help" for more information about a command.
[71---](https://github.com/runfinch/finch/actions/runs/8689692438/job/23829416605?pr=887#step:5:72) FAIL: TestSettingsVMAction_run (0.00s)
[72](https://github.com/runfinch/finch/actions/runs/8689692438/job/23829416605?pr=887#step:5:73) --- FAIL: TestSettingsVMAction_run/should_update_vm_settings (0.00s)
[73](https://github.com/runfinch/finch/actions/runs/8689692438/job/23829416605?pr=887#step:5:74) validate_darwin.go:39: Unexpected call to *mocks.Logger.Infof([The specified number of CPUs (%d) is greater than CPUs available on this system (%d ), 74 which may lead to severe performance degradation.
[74](https://github.com/runfinch/finch/actions/runs/8689692438/job/23829416605?pr=887#step:5:75) which may lead to severe performance degradation 6 4]) at /Users/runner/work/finch/finch/pkg/config/validate_darwin.go:39 because: there are no expected calls of the method "Infof" for that receiver

Logger.Infof() was not called in the local environment, but this function is called due to the small number of CPUs allocated to the GitHub Actions job.

Fix to use fewer CPUs in test cases.

Changed the CPUs value of the test to a value smaller than the CPUs
allocated to GitHub Actions.

Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
We implemented the tests by separating the test cases for macOS and
Windows.

Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
@haytok
Copy link
Member Author

haytok commented Apr 15, 2024

The unit-tests (windows-latest) is different on Windows than on macOS environments, where validation() is not implemented.

This was causing errors in CI.
I implemented the tests by separating the test cases for macOS and Windows to pass CI.

@haytok
Copy link
Member Author

haytok commented Apr 17, 2024

Fix tests so that CI passes. Could you please run CI again and review ?

@haytok
Copy link
Member Author

haytok commented Apr 18, 2024

the same error as below has occured.

Therefore, correct the test cpus.

fix to pass CI

Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
@haytok
Copy link
Member Author

haytok commented Apr 18, 2024

The fix was completed so that the test passes.

I ran the tests again with the cpus set to the cpus (3) used in the GitHub Actions job at the following location in the local environment.

haytok ~/workspace/haytok/finch [add-settings-subcommand]
> git diff pkg/config/validate_darwin.go
diff --git a/pkg/config/validate_darwin.go b/pkg/config/validate_darwin.go
index 141fedf..a7a54bf 100644
--- a/pkg/config/validate_darwin.go
+++ b/pkg/config/validate_darwin.go
@@ -35,6 +35,7 @@ func validate(cfg *Finch, log flog.Logger, systemDeps LoadSystemDeps, mem fmemor
        }

        totalCPUs := systemDeps.NumCPU()
+       totalCPUs = 3
        if *cfg.CPUs > totalCPUs {
                log.Infof(
                        "The specified number of CPUs (%d) is greater than CPUs available on this system (%d),\n"+
Check test and lint

haytok ~/workspace/haytok/finch/cmd/finch [add-settings-subcommand]
> go test -run TestNewSettingsMCommand
PASS
ok  	github.com/runfinch/finch/cmd/finch	0.150s
haytok ~/workspace/haytok/finch/cmd/finch [add-settings-subcommand]
> go test -run TestSettingsVMAction_runAdapter
PASS
ok  	github.com/runfinch/finch/cmd/finch	0.151s
haytok ~/workspace/haytok/finch/cmd/finch [add-settings-subcommand]
> go test -run TestSettingsVMAction_run
PASS
ok  	github.com/runfinch/finch/cmd/finch	0.154s
haytok ~/workspace/haytok/finch/cmd/finch [add-settings-subcommand]
> cd ../../pkg/config/
haytok ~/workspace/haytok/finch/pkg/config [add-settings-subcommand]
> go test -run Test_ModifyFinchConfig
PASS
ok  	github.com/runfinch/finch/pkg/config	0.120s
haytok ~/workspace/haytok/finch/pkg/config [add-settings-subcommand]
> go test -run Test_loadFinchConfig
PASS
ok  	github.com/runfinch/finch/pkg/config	0.119s
haytok ~/workspace/haytok/finch [add-settings-subcommand]
> make lint
env GOOS=windows /Users/haytok/go/bin/golangci-lint run
env GOOS=darwin /Users/haytok/go/bin/golangci-lint run

@haytok
Copy link
Member Author

haytok commented Apr 18, 2024

The following error occurred in windows-e2e-tests (self-hosted, windows, amd64, test, test-e2e-container).

Erros in the workflow

------------------------------
Finch Container Development E2E Tests inspect a container should show the information of a container with --type=container flag
C:/Users/Administrator/go/pkg/mod/github.com/runfinch/common-tests@v0.7.21/tests/inspect.go:46
  3efcb40acce727ddba120f23d0f642421f743bca35f39e6625e436deae108c13
  No containers to be removed
  fb9c9aef62af
  No images to be removed
  f4d57724a12f864ab57bcd06ecb5c5a4bc6544704b5c2285893bfe8e7605616a
  bridge
  host
  none
  No networks to be removed
  2024-04-18 02:42:16.483567601 +0000 UTC finch /images/create {"name":"localhost:49273/docker/library/alpine:latest"}
  localhost:49273/docker/library/alpine:latest:                                     resolved       |++++++++++++++++++++++++++++++++++++++| 
  index-sha256:dc9e1ce1980c9c1208022dc39d40e3724a5dff1a4387534ddef3e5ec8a7fe42e:    done           |++++++++++++++++++++++++++++++++++++++| 
  manifest-sha256:6457d53fb065d6f250e1504b9bc42d5b6c65941d57532c072d929dd0628977d0: done           |++++++++++++++++++++++++++++++++++++++| 
  config-sha256:05455a08881ea9cf0e752bc48e61bbd71a34c029bb13df01e40e3e70e0d007bd:   done           |++++++++++++++++++++++++++++++++++++++| 
  elapsed: 0.1 s                                                                    total:   0.0 B (0.0 B/s)                                         
  2024-04-18 02:42:16.599484662 +0000 UTC finch /snapshot/prepare {"key":"8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1","parent":"sha256:d4fc045c9e3a848011de66f34b81f052d4f2c15a17bb196d637e526349601820","snapshotter":"overlayfs"}
  2024-04-18 02:42:16.612872681 +0000 UTC finch /containers/create {"id":"8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1","image":"localhost:49273/docker/library/alpine:latest","runtime":{"name":"io.containerd.runc.v2","options":{"type_url":"containerd.runc.v1.Options"}}}
  2024-04-18 02:42:16.667723037 +0000 UTC finch /snapshot/remove {"key":"/tmp/initialC3139841479","snapshotter":"overlayfs"}
  2024-04-18 02:42:16.749663969 +0000 UTC finch /tasks/create {"container_id":"8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1","bundle":"/run/containerd/io.containerd.runtime.v2.task/finch/8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1","rootfs":[{"type":"overlay","source":"overlay","options":["index=off","workdir=/var/lib/containerd/io.containerd.snapshotter.v1.overlayfs/snapshots/192/work","upperdir=/var/lib/containerd/io.containerd.snapshotter.v1.overlayfs/snapshots/192/fs","lowerdir=/var/lib/containerd/io.containerd.snapshotter.v1.overlayfs/snapshots/20/fs"]}],"io":{"stdout":"/run/containerd/fifo/1165971291/8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1-stdout","stderr":"/run/containerd/fifo/1165971291/8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1-stderr"},"pid":95754}
  2024-04-18 02:42:16.754736501 +0000 UTC finch /tasks/start {"container_id":"8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1","pid":95754}
  2024-04-18 02:42:16.756662516 +0000 UTC finch /tasks/exit {"container_id":"8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1","id":"8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1","pid":95754,"exited_at":{"seconds":1713408136,"nanos":756482205}}
  time="2024-04-18T02:42:17Z" level=warning msg="failed to inspect NetNS" error="failed to Statfs \"/proc/95754/ns/net\": no such file or directory" id=8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1
  localhost:49273/docker/library/alpine:latest
  ctr-test
  time="2024-04-18T02:42:18Z" level=warning msg="failed to inspect NetNS" error="failed to Statfs \"/proc/95754/ns/net\": no such file or directory" id=8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1
  3efcb40acce727ddba120f23d0f642421f743bca35f39e6625e436deae108c13
  8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1
  2024-04-18 02:42:20.095623863 +0000 UTC finch /tasks/delete {"container_id":"8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1","pid":95754,"exited_at":{"seconds":1713408136,"nanos":756482205}}
  2024-04-18 02:42:20.108607091 +0000 UTC finch /snapshot/remove {"key":"8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1","snapshotter":"overlayfs"}
  2024-04-18 02:42:20.11408024 +0000 UTC finch /containers/delete {"id":"8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1"}
  8fca1c568806a7e77bdb81356ec443d7b9fddd800e2d2aafba0105e3078ceaa1
  time="2024-04-18T02:42:20Z" level=fatal msg="exit status 255"
  [FAILED] in [AfterEach] - C:/Users/Administrator/go/pkg/mod/github.com/runfinch/common-tests@v0.7.21/command/command.go:122 @ 04/18/24 02:42:20.824
+ [FAILED] [9.439 seconds]
Finch Container Development E2E Tests inspect a container [AfterEach] should show the information of a container with --type=container flag
  [AfterEach] C:/Users/Administrator/go/pkg/mod/github.com/runfinch/common-tests@v0.7.21/tests/inspect.go:20
  [It] C:/Users/Administrator/go/pkg/mod/github.com/runfinch/common-tests@v0.7.21/tests/inspect.go:46

  [FAILED] Expected
      <int>: 1
  to match exit code:
      <int>: 0
  In [AfterEach] at: C:/Users/Administrator/go/pkg/mod/github.com/runfinch/common-tests@v0.7.21/command/command.go:122 @ 04/18/24 02:42:20.824
------------------------------

The part of the test that failed is not related to this changes.

Also, this CI did not fail in the workflows we ran in the past.

Therefore, could you please run the CI again when you have time, to see if the test failure is reproducible?
Note that the tests for the code added by this modification has passed.

@haytok
Copy link
Member Author

haytok commented Apr 18, 2024

Thanks for running the CI again!
The workflow that was failed (windows-e2e-tests (self-hosted, windows, amd64, test, test-e2e-container)) has passed, so CI has passed !!!

cmd/finch/virtual_machine_settings.go Outdated Show resolved Hide resolved
@Shubhranshu153
Copy link
Contributor

Thank you for the contribution.

@Shubhranshu153 Shubhranshu153 merged commit 8e809cc into runfinch:main Apr 19, 2024
22 checks passed
@haytok
Copy link
Member Author

haytok commented Apr 19, 2024

Thanks for review !!!!!

haytok added a commit to haytok/finch that referenced this pull request May 8, 2024
In my previous pull request, we added the functionality to change the
number of CPUs and memory size allocated to VMs.

  - runfinch#887

However, at that time, we did not add documentations for the finch vm
settings command.

Therefore, in this fix, we will add documentations for the finch vm
settings command.

Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
pendo324 pushed a commit that referenced this pull request May 21, 2024
In my previous pull request, we added the functionality to change the
number of CPUs and memory size allocated to VMs.

  - #887

However, at that time, we did not add documentations for the finch vm
settings command.

Therefore, in this fix, we will add documentations for the finch vm
settings command.

Issue #, if available: N/A

*Description of changes:* Details are described in this commit message.

*Testing done:* N/A



- [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.

Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants