-
Notifications
You must be signed in to change notification settings - Fork 673
[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
base: master
Are you sure you want to change the base?
Conversation
77877e0
to
7a082d2
Compare
cmd/limactl/start.go
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
pkg/driver/vz/vz_driver_others.go
Outdated
@@ -0,0 +1,118 @@ | |||
//go:build !darwin || no_vz |
There was a problem hiding this comment.
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
3f1de89
to
e53a368
Compare
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>
e53a368
to
4630452
Compare
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
pkg/driver/qemu/cmd/main.go
Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
How to test this ? Tried |
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 |
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>
cmd/limactl/main_darwin.go
Outdated
@@ -0,0 +1,9 @@ | |||
//go:build darwin && !no_vz |
There was a problem hiding this comment.
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:
//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).
There was a problem hiding this comment.
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
pkg/driver/driver.go
Outdated
func (d *BaseDriver) Stop(_ context.Context) error { | ||
return nil | ||
ChangeDisplayPassword(ctx context.Context, password string) error | ||
GetDisplayConnection(ctx context.Context) (string, error) |
There was a problem hiding this comment.
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).
GetDisplayConnection(ctx context.Context) (string, error) | |
DisplayConnection(ctx context.Context) (string, error) |
pkg/driver/driver.go
Outdated
func (d *BaseDriver) ApplySnapshot(_ context.Context, _ string) error { | ||
return errors.New("unimplemented") | ||
} | ||
GetInfo() Info |
There was a problem hiding this comment.
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.
GetInfo() Info | |
Info() Info |
pkg/driver/driver.go
Outdated
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) |
There was a problem hiding this comment.
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
:
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.
There was a problem hiding this comment.
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.
pkg/driver/qemu/qemu_driver.go
Outdated
VSockPort int | ||
VirtioPort string | ||
|
||
DriverType driver.DriverType |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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) |
There was a problem hiding this comment.
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
?
//go:build (darwin && amd64) || (darwin && arm64) | |
//go:build darwin |
pkg/driver/vz/disk.go
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
pkg/driverutil/instance.go
Outdated
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) { |
There was a problem hiding this comment.
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:
func CreateTargetDriverInstance(inst *store.Instance, sshLocalPort int) (driver.Driver, error) { | |
func CreateConfiguredDriver(inst *store.Instance, sshLocalPort int) (driver.ConfiguredDriver, error) { |
pkg/driverutil/instance.go
Outdated
driver.SetConfig(inst, sshLocalPort) | ||
|
||
return driver, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
driver.SetConfig(inst, sshLocalPort) | |
return driver, nil | |
return driver.Configure(inst, sshLocalPort) |
pkg/registry/registry.go
Outdated
if err != nil { | ||
return err | ||
} | ||
stdDriverDir := filepath.Join(homeDir, ".local", "libexec", "lima", "drivers") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
pkg/registry/registry.go
Outdated
|
||
info, err := os.Stat(path) | ||
if err != nil { | ||
logrus.Warnf("Error accessing driver path %s: %v", path, err) |
There was a problem hiding this comment.
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:
logrus.Warnf("Error accessing driver path %s: %v", path, err) | |
logrus.Warnf("Error accessing driver path %q: %v", path, err) |
pkg/registry/registry.go
Outdated
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) |
There was a problem hiding this comment.
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.
pkg/registry/registry.go
Outdated
} | ||
|
||
name := strings.TrimPrefix(base, "lima-driver-") | ||
name = strings.TrimSuffix(name, filepath.Ext(name)) |
There was a problem hiding this comment.
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.
errorsAsStrings := make([]string, len(inst.Errors)) | ||
for i, err := range inst.Errors { | ||
if err != nil { | ||
errorsAsStrings[i] = err.Error() | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
76c670d
to
5f4f779
Compare
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
32d3b4f
to
b896fbf
Compare
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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 ?= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
GO_BUILDTAGS ?= | |
GO_BUILD_TAGS ?= |
Bug: The progress bar seems no longer shown when downloading an uncached image.
Also, "Stopping external driver vz" might be confusing to users, as it sounds like shutting down the VM |
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
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.