Skip to content
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

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/limactl/stop.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func stopAction(cmd *cobra.Command, args []string) error {
if force {
instance.StopForcibly(inst)
} else {
err = instance.StopGracefully(inst)
err = instance.StopGracefully(inst, false)
}
// TODO: should we also reconcile networks if graceful stop returned an error?
if err == nil {
Expand Down
7 changes: 7 additions & 0 deletions pkg/driver/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ type Driver interface {

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

// RuntimeConfig accepts config containing changes to the runtime configuration, and returns the updated runtime configuration.
RuntimeConfig(_ context.Context, config interface{}) (interface{}, error)
}

type BaseDriver struct {
Expand Down Expand Up @@ -149,3 +152,7 @@ func (d *BaseDriver) GuestAgentConn(_ context.Context) (net.Conn, error) {
// use the unix socket forwarded by host agent
return nil, nil
}

func (d *BaseDriver) RuntimeConfig(_ context.Context, _ interface{}) (interface{}, error) {
return nil, fmt.Errorf("unimplemented")
}
35 changes: 35 additions & 0 deletions pkg/hostagent/api/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -16,6 +18,7 @@ import (
type HostAgentClient interface {
HTTPClient() *http.Client
Info(context.Context) (*api.Info, error)
DriverConfig(context.Context, interface{}) (interface{}, error)
Copy link
Member

Choose a reason for hiding this comment

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

Would more clear as:

// Get current driver config
DriveConfig(context.Context) (interface{}, error)

// Patch runtime config and return updated config
PatchDriveConfig(context.Context, interface{}) (interface{}, error)

}

// NewHostAgentClient creates a client.
Expand Down Expand Up @@ -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)
}
Copy link
Member

Choose a reason for hiding this comment

The 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
}
30 changes: 30 additions & 0 deletions pkg/hostagent/api/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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))
Copy link
Member

Choose a reason for hiding this comment

The 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.

}
4 changes: 4 additions & 0 deletions pkg/hostagent/hostagent.go
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,10 @@ func (a *HostAgent) Info(_ context.Context) (*hostagentapi.Info, error) {
return info, nil
}

func (a *HostAgent) DriverRuntimeConfig(ctx context.Context, config interface{}) (interface{}, error) {
return a.driver.RuntimeConfig(ctx, config)
}

func (a *HostAgent) startHostAgentRoutines(ctx context.Context) error {
if *a.instConfig.Plain {
logrus.Info("Running in plain mode. Mounts, port forwarding, containerd, etc. will be ignored. Guest agent will not be running.")
Expand Down
26 changes: 25 additions & 1 deletion pkg/instance/stop.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe save bool or saveMachineState bool?

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.")
Copy link
Member

Choose a reason for hiding this comment

The 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 {
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 "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)
Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

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

Doing input validation first is better and more clear, something like:

if saveOnStop && inst.VMType != limayaml.VZ {
    return fmt.Errorf(...)
}

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 {
Expand Down
31 changes: 31 additions & 0 deletions pkg/vz/vz_driver_darwin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
}
Copy link
Member

Choose a reason for hiding this comment

The 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:

save_restore.Validate(instance) error

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)
Expand Down