Skip to content

[WIP]: Create a Plugin System for Lima #3573

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

Draft
wants to merge 49 commits into
base: master
Choose a base branch
from

Conversation

unsuman
Copy link
Contributor

@unsuman unsuman commented May 23, 2025

Important

This PR is only for preview purposes and not meant to be merged. Later, the commits can be cherry-picked and raised as separate small PRs.

This PR creates a Plugin System for drivers which can be embedded in the main binary or built as separate binaries, which then communicate with the main binary using gRPC. This work is done as part of my GSoC '25 task.

@AkihiroSuda AkihiroSuda added the gsoc/2025 Google Summer of Code 2025 label May 23, 2025
@unsuman unsuman force-pushed the feature/driver-plugin-system branch from 77877e0 to 7a082d2 Compare May 25, 2025 11:21
@@ -54,6 +56,9 @@ $ limactl create --set='.cpus = 2 | .memory = "2GiB"'
To see the template list:
$ limactl create --list-templates

To see the drivers list:
$ limactl create --list-drivers
Copy link
Member

Choose a reason for hiding this comment

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

The example currently just focuses on templates, not on drivers.

@@ -0,0 +1,118 @@
//go:build !darwin || no_vz
Copy link
Member

Choose a reason for hiding this comment

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

The package shouldn't be compiled at all on non-Darwin

@unsuman unsuman force-pushed the feature/driver-plugin-system branch from 3f1de89 to e53a368 Compare May 26, 2025 13:34
unsuman added 16 commits May 27, 2025 15:24
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
…plugin related functions

Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
…rnal drivers

Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
…internal plugins

Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
…rface

Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
…ement common error

Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
@unsuman unsuman force-pushed the feature/driver-plugin-system branch from e53a368 to 4630452 Compare May 27, 2025 10:28
unsuman added 2 commits May 27, 2025 16:12
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

This should be cmd/<EXECUTABLE NAME>/main.go

Copy link
Member

Choose a reason for hiding this comment

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

Makefile has to be updated to compile this by default.

@AkihiroSuda
Copy link
Member

How to test this ?

Tried go build -o ~/.local/libexec/lima/drivers/lima-driver-qemu ./pkg/driver/qemu/cmd , but ~/.local/bin/limactl did not recognize the driver.

@unsuman
Copy link
Contributor Author

unsuman commented Jun 11, 2025

How to test this ?

Tried go build -o ~/.local/libexec/lima/drivers/lima-driver-qemu ./pkg/driver/qemu/cmd , but ~/.local/bin/limactl did not recognize the driver.

The driver discovery through the directory is not working rn. I will work on it once I fix the GuestAgentConn() function.

For now, you can test the driver by setting the executable driver's path to LIMA_DRIVERS_PATH

unsuman added 7 commits June 11, 2025 15:22
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
…socket

Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
@@ -0,0 +1,9 @@
//go:build darwin && !no_vz
Copy link
Member

Choose a reason for hiding this comment

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

I like to avoid double-negation, when possible:

Suggested change
//go:build darwin && !no_vz
//go:build darwin && driver_vz

Then the Makefile will just have a DRIVER_TAGS=driver_qemu,driver_vz,driver_wsl setting that the user can override to only include the desired driver(s).

Copy link
Member

Choose a reason for hiding this comment

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

Also go:build darwin is already implied by the filename

func (d *BaseDriver) Stop(_ context.Context) error {
return nil
ChangeDisplayPassword(ctx context.Context, password string) error
GetDisplayConnection(ctx context.Context) (string, error)
Copy link
Member

Choose a reason for hiding this comment

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

In Go "getters" don't use a Get prefix. You know it is a getter because the method name doesn't include an action word (verb).

Suggested change
GetDisplayConnection(ctx context.Context) (string, error)
DisplayConnection(ctx context.Context) (string, error)

func (d *BaseDriver) ApplySnapshot(_ context.Context, _ string) error {
return errors.New("unimplemented")
}
GetInfo() Info
Copy link
Member

@jandubois jandubois Jun 19, 2025

Choose a reason for hiding this comment

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

I'm not going to comment on additional getters; they all should drop the Get prefix.

Suggested change
GetInfo() Info
Info() Info

func (d *BaseDriver) DeleteSnapshot(_ context.Context, _ string) error {
return errors.New("unimplemented")
// SetConfig sets the configuration for the instance.
SetConfig(inst *store.Instance, sshLocalPort int)
Copy link
Member

@jandubois jandubois Jun 19, 2025

Choose a reason for hiding this comment

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

I wonder if it makes sense to have a separate type (like the Kubernetes Completed* types):

type ConfiguredDriver struct {
    Driver
}

Then you could have a function to turn a Driver into a ConfiguredDriver:

Suggested change
SetConfig(inst *store.Instance, sshLocalPort int)
Configure(inst *store.Instance, sshLocalPort int) ConfiguredDriver

And all the functions/methods that would only work with configured driver would only accept that type instead.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to move this to cmd/lima-driver-qemu at the top level. There should be no main packages with a main function inside the pkg tree.

VSockPort int
VirtioPort string

DriverType driver.DriverType
Copy link
Member

Choose a reason for hiding this comment

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

It feels like a failure of the abstraction if the driver needs to know if it is an internal or external driver. I was hoping that this could be entirely handled by the remoting layer.

Copy link
Member

Choose a reason for hiding this comment

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

GuestAgentConn needs to know this, at least for now

Copy link
Member

Choose a reason for hiding this comment

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

I see this now. I hope we can fix this.

Copy link
Member

Choose a reason for hiding this comment

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

One way to deal with this would be to have both a GuestAgentConn() and ExternalGuestAgentConn() method on the driver, and the driverserver.GuestAgentConn() method in pkg/driver/external/server/methods.go would call s.driver.ExternalGuestAgentConn() instead of the internal version.

Maybe call it GuestAgentConnExternal() so both have the same prefix, idk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or can we change the method signature to GuestAgentConn() (net.Conn, type string) where on the pkg/driver/external/server/methods.go side we will check if the incoming connection is not a UDS, we will go with proxying out that connection?

Copy link
Member

Choose a reason for hiding this comment

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

change the method signature to GuestAgentConn() (net.Conn, type string)

That would be even better, if you can make it work. Because then the proxying is not part of the driver, but of the server code, where it really belongs.

I just think that the driver should be completely ignorant of whether it is running internal or external.

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Sorry, my earlier feedback from today was supposed to be part of this, but I continued to press the wrong button.

@@ -0,0 +1,18 @@
//go:build (darwin && amd64) || (darwin && arm64)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not just checking for darwin?

Suggested change
//go:build (darwin && amd64) || (darwin && arm64)
//go:build darwin

Copy link
Member

Choose a reason for hiding this comment

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

There is some inconsistency in the go:build line between the files in this directory. E.g. errors_darwin.go has

/go:build darwin && !no_vz

It looks like including/excluding the driver is controlled by importing pkg/driver/vz/register into main, so that file is really the only one that needs the go:build line. All the rest are not referenced and don't need it.

If we want to keep it for some kind of "belts & suspender" reasons, then all the files should have it.

I also think that we should rename all the file to remove the _darwin suffix from the filenames. It is no longer needed because the files will not be referenced on any other OS. Only the _arm64 suffix on the Rosetta code needs to stay.

Copy link
Member

Choose a reason for hiding this comment

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

It is no longer needed because the files will not be referenced on any other OS.

Still needed to allow running golangci-lint run ./..., govulncheck ./..., etc. on non-macOS ?

Copy link
Member

Choose a reason for hiding this comment

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

Still needed to allow running golangci-lint run ./..., govulncheck ./..., etc. on non-macOS ?

Yeah, probably true. But then it can be removed from the go:build line.

if *limaDriver == limayaml.WSL2 {
return wsl2.New(base)
// CreateTargetDriverInstance creates the appropriate driver for an instance.
func CreateTargetDriverInstance(inst *store.Instance, sshLocalPort int) (driver.Driver, error) {
Copy link
Member

Choose a reason for hiding this comment

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

We are overloading the word instance here, for drivers and machines.

I think my previous suggestion to use a different type for DriverInstance would work well here:

Suggested change
func CreateTargetDriverInstance(inst *store.Instance, sshLocalPort int) (driver.Driver, error) {
func CreateConfiguredDriver(inst *store.Instance, sshLocalPort int) (driver.ConfiguredDriver, error) {

Comment on lines 22 to 24
driver.SetConfig(inst, sshLocalPort)

return driver, nil
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
driver.SetConfig(inst, sshLocalPort)
return driver, nil
return driver.Configure(inst, sshLocalPort)

if err != nil {
return err
}
stdDriverDir := filepath.Join(homeDir, ".local", "libexec", "lima", "drivers")
Copy link
Member

Choose a reason for hiding this comment

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

I thought this was going to be /usr/local/libexec/lima and not a user-owned directory. We don't store anything except LIMA_HOME under the user's home directory.

Copy link
Member

Choose a reason for hiding this comment

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

To elaborate a bit, I expected this to use filepath.Dir(filepath.Dir(usrlocalsharelima.Dir()) as the prefix. Maybe there should be a usrlocalsharelima.Prefix() function for that.

Further, I don't know if we need a $PREFIX/libexec/lima/drivers directory, or if we can't just use $PREFIX/libexec since we filter on lima-driver-* filenames anyways.

At most we should be using ``$PREFIX/libexec/lima; the drivers` subdirectory is not needed.


info, err := os.Stat(path)
if err != nil {
logrus.Warnf("Error accessing driver path %s: %v", path, err)
Copy link
Member

Choose a reason for hiding this comment

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

For consistency we should always quote filenames. Especially if they are user-provided and may contain whitespace. But let's just always do it:

Suggested change
logrus.Warnf("Error accessing driver path %s: %v", path, err)
logrus.Warnf("Error accessing driver path %q: %v", path, err)

func (r *Registry) registerDriverFile(path string) {
base := filepath.Base(path)
if !strings.HasPrefix(base, "lima-driver-") {
fmt.Printf("Skipping %s: does not start with 'lima-driver-'\n", base)
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is debugging code; we don't want to print anything for ignored files.

}

name := strings.TrimPrefix(base, "lima-driver-")
name = strings.TrimSuffix(name, filepath.Ext(name))
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is fine; we don't need to allow . in driver names. Otherwise I would have said we only strip .exe suffix on Windows, but otherwise allow any characters.

Comment on lines +445 to +450
errorsAsStrings := make([]string, len(inst.Errors))
for i, err := range inst.Errors {
if err != nil {
errorsAsStrings[i] = err.Error()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this is going to work properly. E.g. if the error is later processed again with errors.Is(), then the result will always be false because we are not preserving the type.

Not sure if that is important, but it would require some reflection magic to make it work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep this for a later stage, then!

…rver-level

Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
@unsuman unsuman force-pushed the feature/driver-plugin-system branch from 76c670d to 5f4f779 Compare June 23, 2025 13:02
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
@unsuman unsuman force-pushed the feature/driver-plugin-system branch from 32d3b4f to b896fbf Compare June 23, 2025 16:14
unsuman added 2 commits June 24, 2025 23:45
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Makefile Outdated
@@ -167,7 +167,7 @@ binaries: limactl helpers guestagents \
################################################################################
# _output/bin
.PHONY: limactl lima helpers
limactl: _output/bin/limactl$(exe) lima
limactl: _output/bin/limactl$(exe) external-drivers lima
Copy link
Member

Choose a reason for hiding this comment

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

limactl shouldn't depend on external drivers

Copy link
Contributor Author

@unsuman unsuman Jun 26, 2025

Choose a reason for hiding this comment

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

If we remove external-drivers(now additional-drivers) from limactl as a dependency, then how will it get triggered?
How will this work?
make ADDITIONAL_DRIVERS="qemu" limactl -B

Copy link
Member

Choose a reason for hiding this comment

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

Same as additional-guestagents

Makefile Outdated
DRIVER_INSTALL_DIR := _output/libexec/lima

.PHONY: external-drivers
external-drivers:
Copy link
Member

@AkihiroSuda AkihiroSuda Jun 25, 2025

Choose a reason for hiding this comment

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

"additional-drivers" might be more consistent with "additional-guestagents"

LIMACTL_DRIVER_TAGS += external_wsl2
endif

GO_BUILDTAGS ?=
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
GO_BUILDTAGS ?=
GO_BUILD_TAGS ?=

@AkihiroSuda
Copy link
Member

Bug: The progress bar seems no longer shown when downloading an uncached image.
It is still shown for downloading the nerdctl archive, though.

$ limactl start
? Creating an instance "default" Proceed with the current configuration
INFO[0000] Starting new instance of external driver "vz" 
INFO[0000] Starting external driver at /Users/suda/.local/libexec/lima/lima-driver-vz 
INFO[0000] External driver vz started successfully       driver=vz
INFO[0000] Starting the instance "default" with VM driver "vz" 
INFO[0000] Reusing existing external driver "vz" instance 
INFO[0048] Attempting to download the nerdctl archive    arch=aarch64 digest="sha256:1b52f32b7d5bbf63005bceb6a3cacd237d2fa8f1d05bb590e8ce58731779b9ee" location="https://github.com/containerd/nerdctl/releases/download/v2.1.2/nerdctl-full-2.1.2-linux-arm64.tar.gz"
Downloading the nerdctl archive (nerdctl-full-2.1.2-linux-arm64.tar.gz)
224.67 MiB / 224.67 MiB [----------------------------------] 100.00% 27.42 MiB/s
INFO[0057] Downloaded the nerdctl archive from "https://github.com/containerd/nerdctl/releases/download/v2.1.2/nerdctl-full-2.1.2-linux-arm64.tar.gz" 
INFO[0057] [hostagent] Starting new instance of external driver "vz" 
INFO[0057] [hostagent] time="2025-06-26T11:41:36+09:00" level=info msg="Starting external driver at /Users/suda/.local/libexec/lima/lima-driver-vz" 
INFO[0057] [hostagent] time="2025-06-26T11:41:36+09:00" level=info msg="External driver vz started successfully" driver=vz 
INFO[0058] [hostagent] hostagent socket created at /Users/suda/.lima/default/ha.sock 
INFO[0058] SSH Local Port: 60022                        
INFO[0058] [hostagent] Waiting for the essential requirement 1 of 2: "ssh" 
INFO[0068] [hostagent] Waiting for the essential requirement 1 of 2: "ssh" 
INFO[0068] [hostagent] The essential requirement 1 of 2 is satisfied 
INFO[0068] [hostagent] Waiting for the essential requirement 2 of 2: "user session is ready for ssh" 
INFO[0068] [hostagent] The essential requirement 2 of 2 is satisfied 
INFO[0068] [hostagent] Waiting for the optional requirement 1 of 2: "systemd must be available" 
INFO[0068] [hostagent] time="2025-06-26T11:41:47+09:00" level=info msg="Getting guest agent connection" 
INFO[0068] [hostagent] Guest agent is running           
INFO[0068] [hostagent] The optional requirement 1 of 2 is satisfied 
INFO[0068] [hostagent] Not forwarding TCP 0.0.0.0:22    
INFO[0068] [hostagent] Waiting for the optional requirement 2 of 2: "containerd binaries to be installed" 
INFO[0068] [hostagent] Not forwarding TCP 127.0.0.54:53 
INFO[0068] [hostagent] Not forwarding TCP 127.0.0.53:53 
INFO[0068] [hostagent] Not forwarding TCP [::]:22       
INFO[0068] [hostagent] Not forwarding UDP 127.0.0.54:53 
INFO[0068] [hostagent] Not forwarding UDP 127.0.0.53:53 
INFO[0068] [hostagent] Not forwarding UDP 192.168.5.15:68 
INFO[0083] [hostagent] Forwarding TCP from 127.0.0.1:32833 to 127.0.0.1:32833 
INFO[0083] [hostagent] The optional requirement 2 of 2 is satisfied 
INFO[0083] [hostagent] Waiting for the guest agent to be running 
INFO[0083] [hostagent] Waiting for the final requirement 1 of 1: "boot scripts must have finished" 
INFO[0092] [hostagent] The final requirement 1 of 1 is satisfied 
INFO[0093] READY. Run `lima` to open the shell.         
INFO[0093] Stopping external driver vz                  
INFO[0093] External driver vz stopped successfully      
INFO[0093] External driver vz stopped successfully

Also, "Stopping external driver vz" might be confusing to users, as it sounds like shutting down the VM

unsuman added 2 commits June 27, 2025 01:14
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc/2025 Google Summer of Code 2025
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants