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

Additional module options for enhanced control over VM configuration. #10

Merged
merged 3 commits into from
Feb 8, 2025

Conversation

quinneden
Copy link
Contributor

This pull request includes several changes to enhance the configurability and modularity of the VM. The most significant changes include additional options for config.rosetta-builder, refactoring the package outputs in flake.nix, and updating package.nix to support the additional options implemented in module.nix.

New module options:

  • cores: specifies number of cpu cores for the Lima instance, as well as the maximum number of jobs allowed for the builder in the config.nix.buildMachines specification. Default is 8.
  • diskSize: specifies the size of the disk created for the instance, default is "100GiB".
  • memory: specifies the amount of memory to allocate, default is "6GiB".
  • enableRosetta: enable/disable Rosetta 2 for the VM, true by default. I'd like to hear your thoughts on this one as it sort of contradicts the idea of this project (and the name), but I thought it might be beneficial to provide the option to disable it if it's not needed by the user, as running Rosetta 2 causes a slight performance decrease for the VM (lima-vm/lima#1269).
  • port: specify SSH port for the instance, default is 31122.

Major changes:

  • flake.nix: Removed packages.aarch64-linux.on-demand, since onDemand is passed as an override directly to the derivation that builds the image, so having an explicit output for each is unnecessary.
  • module.nix: Added imageWithFinalConfig which calls packages.aarch64-linux.image, passing the values of the options set in the module as overrides. The vmYaml function now sets the values of disk, memory, cpus, ssh.localPort and rosetta.enabled to that of their respective module option instead of being hard-coded. The script for the launchd daemon has also been modified so that changes to the values of any module options that set parameter values in vmYaml will cause the existing Lima instance to be deleted and a new one will be created using the updated vmYaml.

Minor changes:

  • package.nix: Improved the readability of the configuration by using with lib and restructuring the code blocks.
  • flake.lock: Updated flake inputs.
  • constants.nix: remove debug as it is now defined by config.rosetta-builder.debug.

@kibiz0r
Copy link

kibiz0r commented Jan 30, 2025

@quinneden That's wild, I just ran into this OOM error and started delving into how I might increase the memory allocated to the Lima VM and concluded I'd have to fork the repo... and then I saw this PR, only 15 minutes old.

Currently re-running in case I squeak past the memory constraint, but if it fails again I'll give this a shot. Many thanks!

Edit: Worked like a charm. Increased memory size and even disabled Rosetta for a bit of extra oomph, and it went smooth!

@quinneden
Copy link
Contributor Author

@quinneden That's wild, I just ran into this OOM error and started delving into how I might increase the memory allocated to the Lima VM and concluded I'd have to fork the repo... and then I saw this PR, only 15 minutes old.

Currently re-running in case I squeak past the memory constraint, but if it fails again I'll give this a shot. Many thanks!

Edit: Worked like a charm. Increased memory size and even disabled Rosetta for a bit of extra oomph, and it went smooth!

That's great! I'm glad to hear that it went smoothly. :)

@cpick
Copy link
Owner

cpick commented Jan 31, 2025

Thank you for the pull request. I’m traveling for the next day, but am interested to check it out.

Copy link
Owner

@cpick cpick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this pull request! I like the direction it takes this project and it's teaching me some things about Nix.

Minor changes mentioned below. If I find a few minutes I'll make the changes myself and merge (and happy to further discuss any of them if there are contrary opinions).

On the various formatting changes: is there a linter/Nix formatter that you use? I'm relatively new to the language and trying to learn (or, worse, police) style is exhausting. I would love something that can handle that automatically?

flake.nix Show resolved Hide resolved
module.nix Show resolved Hide resolved
module.nix Show resolved Hide resolved
module.nix Show resolved Hide resolved
module.nix Show resolved Hide resolved
module.nix Show resolved Hide resolved
module.nix Show resolved Hide resolved
package.nix Show resolved Hide resolved
package.nix Show resolved Hide resolved
package.nix Show resolved Hide resolved
@quinneden
Copy link
Contributor Author

Thank you for this pull request! I like the direction it takes this project and it's teaching me some things about Nix.

Minor changes mentioned above. If I find a few minutes I'll make the changes myself and merge (and happy to further discuss any of them if there are contrary opinions).

On the various formatting changes: is there a linter/Nix formatter that you use? I'm relatively new to the language and trying to learn (or, worse, police) style is exhausting. I would love something that can handle that automatically?

Currently away from home, I’ll read these more in depth in a few hrs but regarding the formatting, I prefer nixfmt-rfc-style. It’s not currently stable but will be the official formatting standard. I also just find it to be the easiest to read. However, I formatted the code in this pr with Alejandra because it looked like maybe you were using it already.

@cpick
Copy link
Owner

cpick commented Feb 8, 2025

I'll be taking a crack at addressing some of my comments in my module-options branch.

cpick pushed a commit that referenced this pull request Feb 8, 2025
cpick pushed a commit that referenced this pull request Feb 8, 2025
In case the `default` ever changes for some reason.
Resolves:
#10 (comment)
cpick pushed a commit that referenced this pull request Feb 8, 2025
Remove references to "Lima" (as it's an implementation detail) and add
an example for `onDemand`.

Resolves:
#10 (comment)
#10 (comment)
cpick pushed a commit that referenced this pull request Feb 8, 2025
Make it clear that debuging provides potentially-dangerous access to the
VM.  (It auto-logs in to a user that can `sudo` to root.)

Resolves:
#10 (comment)
cpick pushed a commit that referenced this pull request Feb 8, 2025
cpick pushed a commit that referenced this pull request Feb 8, 2025
This may be reinstated in the future (it may provide better performance,
see linked comment), but simplify the module for now.

Resolves:
#10 (comment)
cpick pushed a commit that referenced this pull request Feb 8, 2025
Prevent potential insecure configurations.  For now any flexibility that
users request will be added as specific, known-safe options.  If that
becomes too onerous this option may be reinstated (with warnings and/or
a more ominous option name).

Resolves:
#10 (comment)
cpick pushed a commit that referenced this pull request Feb 8, 2025
Make it clear that debuging provides potentially-dangerous access to the
VM.  (Any user able to SSH into the VM could tamper with builds.  See
linked comment.)

More context:
#10 (comment)
cpick pushed a commit that referenced this pull request Feb 8, 2025
Prevent the module from being configured in an insecure manner.
This may be reversed in the future.

Resolves:
#10 (comment)
@cpick cpick closed this in 6d74454 Feb 8, 2025
@cpick cpick merged commit 0ee6138 into cpick:main Feb 8, 2025
@cpick
Copy link
Owner

cpick commented Feb 8, 2025

I merged with additional changes that addressed my review comments.
I'm open to feedback if you disagree with some of my modifications, please let me know!

@quinneden
Copy link
Contributor Author

I merged with additional changes that addressed my review comments. I'm open to feedback if you disagree with some of my modifications, please let me know!

Ok great! I apologize I never got a chance to look these over last night, had a personal situation come up but I’m excited to look when I can and I’m happy to have this merged!

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.

3 participants