add validation for nestedVirtualization, rosetta, and mountType #3127

olamilekan000 · 2025-01-19T04:03:40Z

change validates config values for `nestedVirtualization`, `rosetta`, and `mountType` 
when a user starts or tries to create a VM.

Fixes #3126

olamilekan000 · 2025-01-19T09:47:11Z

@AkihiroSuda kindly review. Also, I am not sure why unit tests are failing for windows, go version 1.22 and 1.23, even though everything looks fine.

AkihiroSuda · 2025-01-20T02:54:57Z

https://github.com/lima-vm/lima/actions/runs/12853756300/job/35837296730?pr=3127

=== RUN   TestValidateRosetta
time="2025-01-19T12:59:05Z" level=debug msg="ResolveVMType: resolved VMType \"qemu\" (explicitly specified in []*LimaYAML{o,y,d}[1])"
    validate_test.go:202: assertion failed: expected an error, got nil
--- FAIL: TestValidateRosetta (0.00s)

=== RUN   TestValidateMountTypeOS
time="2025-01-19T12:59:05Z" level=debug msg="ResolveVMType: resolved VMType \"qemu\" (default for GOOS=\"linux\")"
time="2025-01-19T12:59:05Z" level=debug msg="ResolveVMType: resolved VMType \"qemu\" (default for GOOS=\"linux\")"
time="2025-01-19T12:59:05Z" level=warning msg="`mountType: virtiofs` on Linux is experimental"
    validate_test.go:272: assertion failed: expected an error, got nil
--- FAIL: TestValidateMountTypeOS (0.01s)

olamilekan000 · 2025-01-20T03:36:27Z

I have tried to replicate it locally but it passes. Not sure what I'm missing

olamilekan000 · 2025-01-20T05:53:07Z

@AkihiroSuda Found and fixed the issue. Thanks

jandubois · 2025-01-20T06:11:47Z

@olamilekan000 I don't know if all your pushes where necessary to deal with CI problems, but please be aware that each full CI run costs about $6 due to our use of large GitHub runners for macOS, and this PR had like 18 force-pushes¹. That's ok if the problem is only reproducible in CI, but otherwise please try to resolve problems locally first.

I have not checked, maybe the macOS jobs where skipped due to earlier compile/lint failures, in which case the cost will have been minimal. ↩

jandubois

I've only reviewed the validate changes, not the tests.

There may be more conditions that the validation could check for, but I'm out of time for now to think about it more.

pkg/limayaml/validate.go

olamilekan000 · 2025-01-20T08:18:20Z

@olamilekan000 I don't know if all your pushes where necessary to deal with CI problems, but please be aware that each full CI run costs about $6 due to our use of large GitHub runners for macOS, and this PR had like 18 force-pushes1. That's ok if the problem is only reproducible in CI, but otherwise please try to resolve problems locally first.

Footnotes

I have not checked, maybe the macOS jobs where skipped due to earlier compile/lint failures, in which case the cost will have been minimal. ↩

Thanks for bringing this to my attention. The reason for that many pushes was due to me trying to figure why the test passes locally but fails with the CI. Apologies for the inconvenience

Signed-off-by: olalekan odukoya <[email protected]>

jandubois

There are still invalid setting combinations that are not covered by tests.

It is a bit unfortunate that complete test coverage can only be achieved by running the test on each supported os and arch.

I think it would be better if the validation code had global variables for os and arch that can then be overridden in the tests to check for all scenarios with just a single local test run. But that should be a separate PR.

jandubois · 2025-02-04T19:48:01Z

pkg/limayaml/validate.go

+		REVSSHFS: true,
+		NINEP:    true,
+		VIRTIOFS: true,
+		WSLMount: true,


You should have a test that on Windows the only valid mountType is WSLMount.

jandubois · 2025-02-04T19:49:59Z

pkg/limayaml/validate.go

@@ -432,6 +463,9 @@ func validateNetwork(y *LimaYAML) error {
 				return fmt.Errorf("field `%s.macAddress` must be a 48 bit (6 bytes) MAC address; actual length of %q is %d bytes", field, nw.MACAddress, len(hw))
 			}
 		}
+		if nw.VZNAT != nil && *nw.VZNAT && *y.VMType != VZ {
+			return fmt.Errorf("field `%s.vzNAT` needs vmType set to %q; got %q", field, VZ, *y.VMType)


Similar errors should use similar wording. So always use requires or always needs (check what existing errors already use), and don't mix and match.

Suggested change

return fmt.Errorf("field `%s.vzNAT` needs vmType set to %q; got %q", field, VZ, *y.VMType)

return fmt.Errorf("field `%s.vzNAT` requires vmType %q; got %q", field, VZ, *y.VMType)

jandubois · 2025-02-04T19:53:36Z

pkg/limayaml/validate.go

@@ -372,6 +372,37 @@ func Validate(y *LimaYAML, warn bool) error {
 		}
 	}

+	if y.Rosetta.Enabled != nil && *y.Rosetta.Enabled && *y.VMType != VZ {


The check should also make sure the arch is aarch64. Theoretically it should also check that the OS is darwin, but you can set the vmType to VZ unless you are on macOS, so that should already be covered.

jandubois · 2025-02-04T19:55:52Z

pkg/limayaml/validate_test.go

+	assert.NilError(t, err)
+
+	err = Validate(y, false)
+	assert.NilError(t, err)


I would have expected this to fail on Linux and Windows, which don't support the VZ vm type. If it doesn't fail validation on Linux, then that needs to be added.

jandubois · 2025-02-04T19:56:57Z

pkg/limayaml/validate_test.go

+	assert.NilError(t, err)
+
+	err = Validate(y, false)
+	if runtime.GOOS == "darwin" && IsNativeArch(AARCH64) {


On Windows this should return a validation error that vm type QEMU is not supported.

jandubois · 2025-02-04T19:57:33Z

pkg/limayaml/validate_test.go

+	y, err = Load([]byte(rosettaDisabled+"\n"+images), "lima.yaml")
+	assert.NilError(t, err)
+
+	err = Validate(y, false)


Again, expected to fail validation on Windows.

I'm stopping the review here; this continues to be an issue for the rest of the tests.

Instead of testing for the specific failure on e.g. Windows, I think you should probably skip the whole tests on platforms where they are expected to fail for different reasons than the one you are testing.

olamilekan000 force-pushed the add-validation-for-lima-config-values branch 9 times, most recently from 14780e8 to f458f0e Compare January 19, 2025 05:37

olamilekan000 force-pushed the add-validation-for-lima-config-values branch from f458f0e to 7772fb5 Compare January 19, 2025 12:58

olamilekan000 force-pushed the add-validation-for-lima-config-values branch 12 times, most recently from 5dc7b37 to 9c6b92d Compare January 20, 2025 05:52

jandubois requested changes Jan 20, 2025

View reviewed changes

olamilekan000 force-pushed the add-validation-for-lima-config-values branch from 9c6b92d to e5c21f4 Compare January 20, 2025 08:44

olamilekan000 requested a review from jandubois January 20, 2025 08:46

olamilekan000 force-pushed the add-validation-for-lima-config-values branch 3 times, most recently from 4cba40b to 4ec0da9 Compare January 20, 2025 09:04

change adds validation for lima config values on start or create command

a27ec96

Signed-off-by: olalekan odukoya <[email protected]>

olamilekan000 force-pushed the add-validation-for-lima-config-values branch from 4ec0da9 to a27ec96 Compare January 21, 2025 23:25

jandubois requested changes Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add validation for nestedVirtualization, rosetta, and mountType #3127

add validation for nestedVirtualization, rosetta, and mountType #3127

olamilekan000 commented Jan 19, 2025

olamilekan000 commented Jan 19, 2025

AkihiroSuda commented Jan 20, 2025

olamilekan000 commented Jan 20, 2025

olamilekan000 commented Jan 20, 2025

jandubois commented Jan 20, 2025

jandubois left a comment

olamilekan000 commented Jan 20, 2025

Footnotes

jandubois left a comment

jandubois Feb 4, 2025

jandubois Feb 4, 2025

jandubois Feb 4, 2025

jandubois Feb 4, 2025

jandubois Feb 4, 2025

jandubois Feb 4, 2025

	return fmt.Errorf("field `%s.vzNAT` needs vmType set to %q; got %q", field, VZ, *y.VMType)
	return fmt.Errorf("field `%s.vzNAT` requires vmType %q; got %q", field, VZ, *y.VMType)

add validation for nestedVirtualization, rosetta, and mountType #3127

Are you sure you want to change the base?

add validation for nestedVirtualization, rosetta, and mountType #3127

Conversation

olamilekan000 commented Jan 19, 2025

olamilekan000 commented Jan 19, 2025

AkihiroSuda commented Jan 20, 2025

olamilekan000 commented Jan 20, 2025

olamilekan000 commented Jan 20, 2025

jandubois commented Jan 20, 2025

Footnotes

jandubois left a comment

Choose a reason for hiding this comment

olamilekan000 commented Jan 20, 2025

Footnotes

jandubois left a comment

Choose a reason for hiding this comment

jandubois Feb 4, 2025

Choose a reason for hiding this comment

jandubois Feb 4, 2025

Choose a reason for hiding this comment

jandubois Feb 4, 2025

Choose a reason for hiding this comment

jandubois Feb 4, 2025

Choose a reason for hiding this comment

jandubois Feb 4, 2025

Choose a reason for hiding this comment

jandubois Feb 4, 2025

Choose a reason for hiding this comment