Skip to content

Refactor: Decouple Qemu Internal from pkg/instance #3377

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 3, 2025
Merged

Refactor: Decouple Qemu Internal from pkg/instance #3377

merged 1 commit into from
May 3, 2025

Conversation

unsuman
Copy link
Contributor

@unsuman unsuman commented Mar 24, 2025 •
edited
Loading

WIP

This PR fixes #1746.

AkihiroSuda
AkihiroSuda reviewed Mar 25, 2025
View reviewed changes
pkg/driver/driver.go Outdated
@@ -71,6 +71,8 @@ type Driver interface {

// GuestAgentConn returns the guest agent connection, or nil (if forwarded by ssh).
GuestAgentConn(_ context.Context) (net.Conn, error)

CheckBinarySignature(_ context.Context, arch 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.

This should be probably just merged to the existing Validate() method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AkihiroSuda I did what you suggested. What about this?

the "github.com/lima-vm/lima/pkg/qemu" is also present in cmd/limactl/disk.go file and "github.com/lima-vm/lima/pkg/qemu/imgutil" in pkg/store/disk.go file

Copy link
Member

Choose a reason for hiding this comment

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

The disk image operation seems another aspect and can be visited later.

Rather we should concentrate on decoupling pkg/limayaml from the VM drivers.
Quite complicated though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AkihiroSuda I went through the VM drivers code to decouple limayaml. I see primarily constants like limayaml.VIRTIOFS, limayaml.WSLMount and functions like limayaml.MACAddress(), limayaml.FirstUsernetIndex() etc. What should be my approach here?

Copy link
Member

Choose a reason for hiding this comment

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

limayaml.VIRTIOFS, limayaml.WSLMount

Eventually these should be defined in VM drivers, probably in another PR.
VIRTIOFS will be duplicated in qemu, vz, and potentially more drivers

limayaml.MACAddress(), limayaml.FirstUsernetIndex() etc.

No need to split from limayaml, as they do not depend on the implementation details of the VM drivers

Copy link
Member

Choose a reason for hiding this comment

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

Relevant:

AkihiroSuda
AkihiroSuda reviewed Mar 25, 2025
View reviewed changes
pkg/qemu/qemu_driver.go Outdated
@@ -50,6 +50,25 @@ func New(driver *driver.BaseDriver) *LimaQemuDriver {
}

func (l *LimaQemuDriver) Validate() error {
// Ask the user to sign the qemu binary with the "com.apple.security.hypervisor" if needed.
// Workaround for https://github.com/lima-vm/lima/issues/1742
if runtime.GOOS == "darwin" && l.BaseDriver.Instance.VMType == limayaml.QEMU {
Copy link
Member

Choose a reason for hiding this comment

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

VMType == QEMU is always true here

Copy link
Member

Choose a reason for hiding this comment

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

Please also squash the commits

AkihiroSuda
AkihiroSuda reviewed Mar 25, 2025
View reviewed changes
pkg/qemu/qemu_driver.go Outdated
@@ -411,25 +430,6 @@ func (l *LimaQemuDriver) GuestAgentConn(ctx context.Context) (net.Conn, error) {
return dialContext, err
}

func (l *LimaQemuDriver) CheckBinarySignature(_ context.Context, arch 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.

For readability you can just keep this function with privatization (Check... -> check...), and call it from Validate()

@unsuman unsuman force-pushed the fix/decouple-vm-internals branch from 31adc47 to e1b3f85 Compare March 25, 2025 12:16
@unsuman unsuman force-pushed the fix/decouple-vm-internals branch from e1b3f85 to aa4d9e9 Compare April 10, 2025 16:24
@AkihiroSuda
Copy link
Member

Still draft?

@unsuman
Copy link
Contributor Author

unsuman commented May 1, 2025

Still draft?

#3377 (comment)

Im little confused here what should be my approach? Any guidance is much appreciated then I can proceed with the PR.

@unsuman
Copy link
Contributor Author

unsuman commented May 1, 2025

@AkihiroSuda Are you implying to decouple *limayaml.LimaYAML from the qemu.go file?

type Config struct {
	Name         string
	InstanceDir  string
	LimaYAML     *limayaml.LimaYAML
	SSHLocalPort int
	SSHAddress   string
}

@AkihiroSuda
Copy link
Member

qemu.go may continue to refer to limayaml, but limayaml shouldn't contain the details of qemu

@unsuman unsuman force-pushed the fix/decouple-vm-internals branch 4 times, most recently from f624ab0 to 960d2a3 Compare May 2, 2025 14:37
@unsuman
Copy link
Contributor Author

unsuman commented May 2, 2025

@AkihiroSuda I've tried to move the cpuType to vmOpts.qemu. Should I also move the DefaultCPUType() logic to pkg/qemu and import it from there into pkg/limayaml for now, and is there anything else that I should focus on refactoring?

@AkihiroSuda
Copy link
Member

@AkihiroSuda I've tried to move the cpuType to vmOpts.qemu. Should I also move the DefaultCPUType() logic to pkg/qemu and import it from there into pkg/limayaml for now, and is there anything else that I should focus on refactoring?

pkg/limayaml shouldn't depend on pkg/qemu.
The default cpuType in vmOpts.qemu should be just empty, and resolved when it is passed to qemu.
I suggest working on this in a separate PR.

@unsuman
Copy link
Contributor Author

unsuman commented May 3, 2025

@AkihiroSuda I've tried to move the cpuType to vmOpts.qemu. Should I also move the DefaultCPUType() logic to pkg/qemu and import it from there into pkg/limayaml for now, and is there anything else that I should focus on refactoring?

pkg/limayaml shouldn't depend on pkg/qemu. The default cpuType in vmOpts.qemu should be just empty, and resolved when it is passed to qemu. I suggest working on this in a separate PR.

Is there any more work remaining on this PR, such as decoupling qemu from the disk operations, or should I open it for review?

@unsuman unsuman force-pushed the fix/decouple-vm-internals branch from 960d2a3 to 8b9e9ad Compare May 3, 2025 07:49
@unsuman
decouple qemu internal from pkg/instance & pkg/limayaml
3322d95 
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
@unsuman unsuman force-pushed the fix/decouple-vm-internals branch from 8b9e9ad to 3322d95 Compare May 3, 2025 07:51
AkihiroSuda
AkihiroSuda approved these changes May 3, 2025
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Seems ready to merge as-is

@AkihiroSuda AkihiroSuda added this to the v1.1.0 milestone May 3, 2025
@unsuman unsuman marked this pull request as ready for review May 3, 2025 11:16
@AkihiroSuda AkihiroSuda merged commit d7ece61 into lima-vm:master May 3, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees
No one assigned
Labels
component/qemu QEMU
Projects
None yet
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.

Decouple pkg/start (and others) from VM driver internals
2 participants
@unsuman @AkihiroSuda