-
Notifications
You must be signed in to change notification settings - Fork 706
refactor: decouple driver-specific settings for internal drivers #3901
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
Conversation
c3a8755
to
e887f1c
Compare
258c9b2
to
5fc24e4
Compare
c2e8e60
to
aa26f46
Compare
Still draft? |
I'll open it shortly, testing some more things. |
aa26f46
to
0a74ee7
Compare
… drivers Signed-off-by: Ansuman Sahoo <[email protected]> experiment with wsl2 in ha Signed-off-by: Ansuman Sahoo <[email protected]> boot scripts for wsl2 Signed-off-by: Ansuman Sahoo <[email protected]>
0a74ee7
to
2076ff1
Compare
return []string{"10", "30", "50", "100", "200"}, cobra.ShellCompDirectiveNoFileComp | ||
}) | ||
|
||
flags.String("vm-type", "", commentPrefix+"Virtual machine type") |
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.
--vm-type
is not guaranteed to work as an edit flag.
e.g., this will probably not work
limactl create --vm-type qemu
limactl edit --vm-type vz
Probably we need a warning when --vm-type
is specified as an edit flag for an existing instance, but it can be worked out later.
VirtioPort string `json:"virtioPort"` | ||
InstanceDir string `json:"instanceDir,omitempty"` | ||
DriverName string `json:"driverName"` | ||
CanRunGUI bool `json:"canRunGui,omitempty"` |
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.
Shouldn't this be in a "DriverFeatures" ?
Can be fixed later
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.
Thanks
Is copying |
args.RosettaEnabled = *instConfig.VMOpts.VZ.Rosetta.Enabled | ||
} | ||
if instConfig.VMOpts.VZ.Rosetta.BinFmt != nil { | ||
args.RosettaEnabled = *instConfig.VMOpts.VZ.Rosetta.BinFmt |
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.
- args.RosettaEnabled = *instConfig.VMOpts.VZ.Rosetta.BinFmt
+ args.RosettaBinFmt = *instConfig.VMOpts.VZ.Rosetta.BinFmt
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.
opened #3921
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.
Thanks
lima-vm#3901 (comment) Signed-off-by: Norio Nomura <[email protected]>
Linking the discussion: https://cloud-native.slack.com/archives/C08KKELMQ92/p1754425219854189?thread_ts=1754414764.412809&cid=C08KKELMQ92 (Aug 6) |
return 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.
These remnant of ResolveVMType
could be removed.
I'll open a PR.
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 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.
Thanks a lot🙇♂️
This PR broke the SSHStatus, reverting to the previous setup when always returning localhost for all instances. i.e. not a pure refactoring |
Note
This is the second PR out of the three PRs, which depends on #3900. Merging all will close #3769.
Key Changes
AcceptConfig()
andFillConfig()
, respectively, for all three drivers. These actions are performed before configuring the driver(driverutil/vm.go
).driver.InspectStatus()
) and providing boot scripts(driver.BootScripts()
) are all done at the driver level.InspectStatus()
has been implemented by all the drivers(~wsl2. Others return""
because their status is figured out through PID files). VM-specific boot scripts(vz and wsl2), only wsl2 is left to be decoupled(already done in refactor: move driver specific code to drivers #3770, but having some problems connecting to hostagent through ssh).Register()
andUnregister()
from the driver interface. RenamedInitialize()
toCreate()
and added its counterpartDelete()
. Also added two driver featuresDynamicSSHAddress
andSkipSocketForwarding
, these were added only for WSL2(not in this PR but soon if that error gets fixed) and future drivers.