-
Notifications
You must be signed in to change notification settings - Fork 615
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
vz: implement auto save/restore #2900
base: master
Are you sure you want to change the base?
Changes from 1 commit
4fc95cd
349ccd1
929c7e5
c5305c0
1d533a7
de78015
7f4df6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,11 @@ package client | |
// Apache License 2.0 | ||
|
||
import ( | ||
"bytes" | ||
"context" | ||
"encoding/json" | ||
"fmt" | ||
"io" | ||
"net/http" | ||
|
||
"github.com/lima-vm/lima/pkg/hostagent/api" | ||
|
@@ -16,6 +18,7 @@ import ( | |
type HostAgentClient interface { | ||
HTTPClient() *http.Client | ||
Info(context.Context) (*api.Info, error) | ||
DriverConfig(context.Context, interface{}) (interface{}, error) | ||
} | ||
|
||
// NewHostAgentClient creates a client. | ||
|
@@ -62,3 +65,35 @@ func (c *client) Info(ctx context.Context) (*api.Info, error) { | |
} | ||
return &info, nil | ||
} | ||
|
||
func (c *client) DriverConfig(ctx context.Context, config interface{}) (interface{}, error) { | ||
u := fmt.Sprintf("http://%s/%s/driver/config", c.dummyHost, c.version) | ||
method := "GET" | ||
var body io.Reader | ||
if config != nil { | ||
method = "PATCH" | ||
b, err := json.Marshal(config) | ||
if err != nil { | ||
return nil, err | ||
} | ||
body = bytes.NewBuffer(b) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing both GET and PATCH in the same function is confusing, both for understanding this code, and both for code using this function. If we want to avoid duplicate code for sending a request and getting a response, move the common code to a helper. |
||
req, err := http.NewRequestWithContext(ctx, method, u, body) | ||
if err != nil { | ||
return nil, err | ||
} | ||
req.Header.Set("Content-Type", "application/json") | ||
resp, err := c.Do(req) | ||
if err != nil { | ||
return nil, err | ||
} | ||
defer resp.Body.Close() | ||
if err := httpclientutil.Successful(resp); err != nil { | ||
return nil, err | ||
} | ||
dec := json.NewDecoder(resp.Body) | ||
if err := dec.Decode(&config); err != nil { | ||
return nil, err | ||
} | ||
return &config, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,36 @@ func (b *Backend) GetInfo(w http.ResponseWriter, r *http.Request) { | |
_, _ = w.Write(m) | ||
} | ||
|
||
// DriverConfig is the handler for GET /v1/driver/config and PATCH /v1/driver/config. | ||
func (b *Backend) DriverConfig(w http.ResponseWriter, r *http.Request) { | ||
ctx := r.Context() | ||
ctx, cancel := context.WithCancel(ctx) | ||
defer cancel() | ||
var config interface{} | ||
if r.Method == http.MethodPatch { | ||
defer r.Body.Close() | ||
if err := json.NewDecoder(r.Body).Decode(&config); err != nil { | ||
b.onError(w, err, http.StatusBadRequest) | ||
return | ||
} | ||
} | ||
config, err := b.Agent.DriverRuntimeConfig(ctx, config) | ||
if err != nil { | ||
b.onError(w, err, http.StatusInternalServerError) | ||
return | ||
} | ||
m, err := json.Marshal(config) | ||
if err != nil { | ||
b.onError(w, err, http.StatusInternalServerError) | ||
return | ||
} | ||
w.Header().Set("Content-Type", "application/json") | ||
w.WriteHeader(http.StatusOK) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't OK the default? |
||
_, _ = w.Write(m) | ||
} | ||
|
||
func AddRoutes(r *http.ServeMux, b *Backend) { | ||
r.Handle("/v1/info", http.HandlerFunc(b.GetInfo)) | ||
r.Handle("GET /v1/driver/config", http.HandlerFunc(b.DriverConfig)) | ||
r.Handle("PATCH /v1/driver/config", http.HandlerFunc(b.DriverConfig)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name of the new handler is not consistent with existing GetInfo. GetConfig and PatchConfig would be consistent. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,18 +9,42 @@ import ( | |
"strings" | ||
"time" | ||
|
||
hostagentclient "github.com/lima-vm/lima/pkg/hostagent/api/client" | ||
hostagentevents "github.com/lima-vm/lima/pkg/hostagent/events" | ||
"github.com/lima-vm/lima/pkg/limayaml" | ||
"github.com/lima-vm/lima/pkg/osutil" | ||
"github.com/lima-vm/lima/pkg/store" | ||
"github.com/lima-vm/lima/pkg/store/filenames" | ||
"github.com/sirupsen/logrus" | ||
) | ||
|
||
func StopGracefully(inst *store.Instance) error { | ||
func StopGracefully(inst *store.Instance, saveOnStop bool) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe |
||
if inst.Status != store.StatusRunning { | ||
return fmt.Errorf("expected status %q, got %q (maybe use `limactl stop -f`?)", store.StatusRunning, inst.Status) | ||
} | ||
|
||
if saveOnStop && inst.Saved { | ||
logrus.Warn("saved VZ machine state is found. It will be overwritten by the new one.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why warning? this is expected condition, nothing to warn about. Use Debug? |
||
} | ||
|
||
if inst.VMType == limayaml.VZ { | ||
haSock := filepath.Join(inst.Dir, filenames.HostAgentSock) | ||
haClient, err := hostagentclient.NewHostAgentClient(haSock) | ||
if err != nil { | ||
logrus.WithError(err).Error("Failed to create a host agent client") | ||
} | ||
ctx, cancel := context.WithTimeout(context.TODO(), 3*time.Second) | ||
defer cancel() | ||
disableSaveOnStopConfig := struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this "disableSaveOnStopConfig"? why not patch? |
||
SaveOnStop bool `json:"saveOnStop"` | ||
}{SaveOnStop: saveOnStop} | ||
_, err = haClient.DriverConfig(ctx, disableSaveOnStopConfig) | ||
if err != nil { | ||
return fmt.Errorf("failed to disable saveOnStop: %w", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we treating this as disable when the value can be true? |
||
} | ||
} else if saveOnStop { | ||
return fmt.Errorf("save is not supported for %q", inst.VMType) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing input validation first is better and more clear, something like:
This keeps the happy path very clear, and it is easy to very that we did not forget to handle all abnormal cases. |
||
} | ||
begin := time.Now() // used for logrus propagation | ||
logrus.Infof("Sending SIGINT to hostagent process %d", inst.HostAgentPID) | ||
if err := osutil.SysKill(inst.HostAgentPID, osutil.SigInt); err != nil { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,14 +8,17 @@ import ( | |
"fmt" | ||
"net" | ||
"path/filepath" | ||
"runtime" | ||
"time" | ||
|
||
"github.com/Code-Hex/vz/v3" | ||
"github.com/mitchellh/mapstructure" | ||
|
||
"github.com/sirupsen/logrus" | ||
|
||
"github.com/lima-vm/lima/pkg/driver" | ||
"github.com/lima-vm/lima/pkg/limayaml" | ||
"github.com/lima-vm/lima/pkg/osutil" | ||
"github.com/lima-vm/lima/pkg/reflectutil" | ||
"github.com/lima-vm/lima/pkg/store/filenames" | ||
) | ||
|
@@ -204,6 +207,34 @@ func (l *LimaVzDriver) RunGUI() error { | |
return fmt.Errorf("RunGUI is not supported for the given driver '%s' and display '%s'", "vz", *l.Instance.Config.Video.Display) | ||
} | ||
|
||
func (l *LimaVzDriver) RuntimeConfig(_ context.Context, config interface{}) (interface{}, error) { | ||
if config == nil { | ||
return l.config, nil | ||
} | ||
var newConfig LimaVzDriverRuntimeConfig | ||
err := mapstructure.Decode(config, &newConfig) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if newConfig.SaveOnStop { | ||
if runtime.GOARCH != "arm64" { | ||
return nil, fmt.Errorf("saveOnStop is not supported on %s", runtime.GOARCH) | ||
} else if runtime.GOOS != "darwin" { | ||
return nil, fmt.Errorf("saveOnStop is not supported on %s", runtime.GOOS) | ||
} else if macOSProductVersion, err := osutil.ProductVersion(); err != nil { | ||
return nil, fmt.Errorf("failed to get macOS product version: %w", err) | ||
} else if macOSProductVersion.Major < 14 { | ||
return nil, fmt.Errorf("saveOnStop is not supported on macOS %d", macOSProductVersion.Major) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a copy of the block validating the config in a previous commit. Maybe we can have one place doing this check:
Then we can call it in both places, |
||
logrus.Info("VZ RuntimeConfiguration changed: SaveOnStop is enabled") | ||
l.config.SaveOnStop = true | ||
} else { | ||
logrus.Info("VZ RuntimeConfiguration changed: SaveOnStop is disabled") | ||
l.config.SaveOnStop = false | ||
} | ||
return l.config, nil | ||
} | ||
|
||
func (l *LimaVzDriver) Stop(_ context.Context) error { | ||
if l.config.SaveOnStop { | ||
machineStatePath := filepath.Join(l.Instance.Dir, filenames.VzMachineState) | ||
|
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.
Would more clear as: