From c41c9de839a501f718c2424754479e09e5303a10 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Mon, 13 Aug 2018 17:15:32 +0800 Subject: [PATCH 1/6] proxy: do not decode proxy config It is a well defined structure that needs no decoding. Signed-off-by: Peng Tao --- virtcontainers/proxy.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/virtcontainers/proxy.go b/virtcontainers/proxy.go index 4af852254f..f2f55f8a82 100644 --- a/virtcontainers/proxy.go +++ b/virtcontainers/proxy.go @@ -9,7 +9,6 @@ import ( "fmt" "path/filepath" - "github.com/mitchellh/mapstructure" "github.com/sirupsen/logrus" ) @@ -132,9 +131,9 @@ func newProxyConfig(sandboxConfig *SandboxConfig) (ProxyConfig, error) { case KataProxyType: fallthrough case CCProxyType: - if err := mapstructure.Decode(sandboxConfig.ProxyConfig, &config); err != nil { - return ProxyConfig{}, err - } + config = sandboxConfig.ProxyConfig + default: + return ProxyConfig{}, fmt.Errorf("unknown proxy type %v", sandboxConfig.ProxyType) } if config.Path == "" { From f39fa5d4896fd649b859290b2edbf353397bf589 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Tue, 14 Aug 2018 16:51:23 +0800 Subject: [PATCH 2/6] proxy: remove newProxyConfig The proxy config does not depend on proxy type. Let's not misture them. Signed-off-by: Peng Tao --- virtcontainers/cc_proxy.go | 9 +++++++-- virtcontainers/kata_proxy.go | 8 ++++++-- virtcontainers/proxy.go | 24 ++++-------------------- virtcontainers/proxy_test.go | 27 ++++++++------------------- 4 files changed, 25 insertions(+), 43 deletions(-) diff --git a/virtcontainers/cc_proxy.go b/virtcontainers/cc_proxy.go index ef6cce5eb0..3d66e1fd5c 100644 --- a/virtcontainers/cc_proxy.go +++ b/virtcontainers/cc_proxy.go @@ -6,6 +6,7 @@ package virtcontainers import ( + "fmt" "os/exec" ) @@ -14,8 +15,12 @@ type ccProxy struct { // start is the proxy start implementation for ccProxy. func (p *ccProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) { - config, err := newProxyConfig(sandbox.config) - if err != nil { + if sandbox.config == nil { + return -1, "", fmt.Errorf("Sandbox config cannot be nil") + } + + config := sandbox.config.ProxyConfig + if err := validateProxyConfig(config); err != nil { return -1, "", err } diff --git a/virtcontainers/kata_proxy.go b/virtcontainers/kata_proxy.go index fe4b01e9c7..e68686295b 100644 --- a/virtcontainers/kata_proxy.go +++ b/virtcontainers/kata_proxy.go @@ -25,6 +25,10 @@ func (p *kataProxy) consoleWatched() bool { // start is kataProxy start implementation for proxy interface. func (p *kataProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) { sandbox.Logger().Info("Starting regular Kata proxy rather than built-in") + if sandbox.config == nil { + return -1, "", fmt.Errorf("Sandbox config cannot be nil") + } + if sandbox.agent == nil { return -1, "", fmt.Errorf("No agent") } @@ -33,8 +37,8 @@ func (p *kataProxy) start(sandbox *Sandbox, params proxyParams) (int, string, er return -1, "", fmt.Errorf("AgentURL cannot be empty") } - config, err := newProxyConfig(sandbox.config) - if err != nil { + config := sandbox.config.ProxyConfig + if err := validateProxyConfig(config); err != nil { return -1, "", err } diff --git a/virtcontainers/proxy.go b/virtcontainers/proxy.go index f2f55f8a82..29256e7349 100644 --- a/virtcontainers/proxy.go +++ b/virtcontainers/proxy.go @@ -119,28 +119,12 @@ func newProxy(pType ProxyType) (proxy, error) { } } -// newProxyConfig returns a proxy config from a generic SandboxConfig handler, -// after it properly checked the configuration was valid. -func newProxyConfig(sandboxConfig *SandboxConfig) (ProxyConfig, error) { - if sandboxConfig == nil { - return ProxyConfig{}, fmt.Errorf("Sandbox config cannot be nil") +func validateProxyConfig(proxyConfig ProxyConfig) error { + if len(proxyConfig.Path) == 0 { + return fmt.Errorf("Proxy path cannot be empty") } - var config ProxyConfig - switch sandboxConfig.ProxyType { - case KataProxyType: - fallthrough - case CCProxyType: - config = sandboxConfig.ProxyConfig - default: - return ProxyConfig{}, fmt.Errorf("unknown proxy type %v", sandboxConfig.ProxyType) - } - - if config.Path == "" { - return ProxyConfig{}, fmt.Errorf("Proxy path cannot be empty") - } - - return config, nil + return nil } func defaultProxyURL(sandbox *Sandbox, socketType string) (string, error) { diff --git a/virtcontainers/proxy_test.go b/virtcontainers/proxy_test.go index b796d30780..4f72a2212d 100644 --- a/virtcontainers/proxy_test.go +++ b/virtcontainers/proxy_test.go @@ -154,15 +154,15 @@ func TestNewProxyFromUnknownProxyType(t *testing.T) { } } -func testNewProxyConfigFromSandboxConfig(t *testing.T, sandboxConfig SandboxConfig, expected ProxyConfig) { - result, err := newProxyConfig(&sandboxConfig) - if err != nil { +func testNewProxyFromSandboxConfig(t *testing.T, sandboxConfig SandboxConfig) { + if _, err := newProxy(sandboxConfig.ProxyType); err != nil { t.Fatal(err) } - if reflect.DeepEqual(result, expected) == false { - t.Fatalf("Got %+v\nExpecting %+v", result, expected) + if err := validateProxyConfig(sandboxConfig.ProxyConfig); err != nil { + t.Fatal(err) } + } var testProxyPath = "proxy-path" @@ -177,7 +177,7 @@ func TestNewProxyConfigFromCCProxySandboxConfig(t *testing.T) { ProxyConfig: proxyConfig, } - testNewProxyConfigFromSandboxConfig(t, sandboxConfig, proxyConfig) + testNewProxyFromSandboxConfig(t, sandboxConfig) } func TestNewProxyConfigFromKataProxySandboxConfig(t *testing.T) { @@ -190,22 +190,11 @@ func TestNewProxyConfigFromKataProxySandboxConfig(t *testing.T) { ProxyConfig: proxyConfig, } - testNewProxyConfigFromSandboxConfig(t, sandboxConfig, proxyConfig) -} - -func TestNewProxyConfigNilSandboxConfigFailure(t *testing.T) { - if _, err := newProxyConfig(nil); err == nil { - t.Fatal("Should fail because SandboxConfig provided is nil") - } + testNewProxyFromSandboxConfig(t, sandboxConfig) } func TestNewProxyConfigNoPathFailure(t *testing.T) { - sandboxConfig := &SandboxConfig{ - ProxyType: CCProxyType, - ProxyConfig: ProxyConfig{}, - } - - if _, err := newProxyConfig(sandboxConfig); err == nil { + if err := validateProxyConfig(ProxyConfig{}); err == nil { t.Fatal("Should fail because ProxyConfig has no Path") } } From 8f77c33d68b3fc066c59a2c6895b19916bd7c0b7 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Fri, 17 Aug 2018 16:44:10 +0800 Subject: [PATCH 3/6] proxy: decouple from sandbox A proxy is mostly associated with an agent. Decouple it from sandbox so that we can start it before linking vm with an actual sandbox. Signed-off-by: Peng Tao --- virtcontainers/cc_proxy.go | 24 ++---- virtcontainers/cc_proxy_test.go | 2 +- virtcontainers/hyperstart_agent.go | 8 +- virtcontainers/kata_agent.go | 19 +++-- virtcontainers/kata_builtin_proxy.go | 34 +++++--- virtcontainers/kata_proxy.go | 39 +++------ virtcontainers/kata_proxy_test.go | 2 +- virtcontainers/no_proxy.go | 11 ++- virtcontainers/no_proxy_test.go | 11 ++- virtcontainers/noop_proxy.go | 4 +- virtcontainers/proxy.go | 30 +++++-- virtcontainers/proxy_test.go | 123 ++++++++++++++++----------- 12 files changed, 177 insertions(+), 130 deletions(-) diff --git a/virtcontainers/cc_proxy.go b/virtcontainers/cc_proxy.go index 3d66e1fd5c..fbfa733f28 100644 --- a/virtcontainers/cc_proxy.go +++ b/virtcontainers/cc_proxy.go @@ -5,33 +5,27 @@ package virtcontainers -import ( - "fmt" - "os/exec" -) +import "os/exec" type ccProxy struct { } // start is the proxy start implementation for ccProxy. -func (p *ccProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) { - if sandbox.config == nil { - return -1, "", fmt.Errorf("Sandbox config cannot be nil") - } - - config := sandbox.config.ProxyConfig - if err := validateProxyConfig(config); err != nil { +func (p *ccProxy) start(params proxyParams) (int, string, error) { + if err := validateProxyParams(params); err != nil { return -1, "", err } + params.logger.Info("Starting cc proxy") + // construct the socket path the proxy instance will use - proxyURL, err := defaultProxyURL(sandbox, SocketTypeUNIX) + proxyURL, err := defaultProxyURL(params.id, SocketTypeUNIX) if err != nil { return -1, "", err } - args := []string{config.Path, "-uri", proxyURL} - if config.Debug { + args := []string{params.path, "-uri", proxyURL} + if params.debug { args = append(args, "-log", "debug") } @@ -43,7 +37,7 @@ func (p *ccProxy) start(sandbox *Sandbox, params proxyParams) (int, string, erro return cmd.Process.Pid, proxyURL, nil } -func (p *ccProxy) stop(sandbox *Sandbox, pid int) error { +func (p *ccProxy) stop(pid int) error { return nil } diff --git a/virtcontainers/cc_proxy_test.go b/virtcontainers/cc_proxy_test.go index ed19855271..d5b07f85c2 100644 --- a/virtcontainers/cc_proxy_test.go +++ b/virtcontainers/cc_proxy_test.go @@ -12,5 +12,5 @@ import ( func TestCCProxyStart(t *testing.T) { proxy := &ccProxy{} - testProxyStart(t, nil, proxy, CCProxyType) + testProxyStart(t, nil, proxy) } diff --git a/virtcontainers/hyperstart_agent.go b/virtcontainers/hyperstart_agent.go index 4c135b8bc4..c7ee02a3b9 100644 --- a/virtcontainers/hyperstart_agent.go +++ b/virtcontainers/hyperstart_agent.go @@ -390,7 +390,11 @@ func (h *hyper) startProxy(sandbox *Sandbox) error { } // Start the proxy here - pid, uri, err := h.proxy.start(sandbox, proxyParams{}) + pid, uri, err := h.proxy.start(proxyParams{ + id: sandbox.id, + path: sandbox.config.ProxyConfig.Path, + logger: h.Logger(), + }) if err != nil { return err } @@ -461,7 +465,7 @@ func (h *hyper) stopSandbox(sandbox *Sandbox) error { return err } - return h.proxy.stop(sandbox, h.state.ProxyPid) + return h.proxy.stop(h.state.ProxyPid) } // handleBlockVolumes handles volumes that are block device files, by diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go index 41e0d1541a..088ee4b155 100644 --- a/virtcontainers/kata_agent.go +++ b/virtcontainers/kata_agent.go @@ -477,13 +477,22 @@ func (k *kataAgent) startProxy(sandbox *Sandbox) error { return err } + consoleURL, err := sandbox.hypervisor.getSandboxConsole(sandbox.id) + if err != nil { + return err + } + proxyParams := proxyParams{ - agentURL: agentURL, - logger: k.Logger().WithField("sandbox", sandbox.id), + id: sandbox.id, + path: sandbox.config.ProxyConfig.Path, + agentURL: agentURL, + consoleURL: consoleURL, + logger: k.Logger().WithField("sandbox", sandbox.id), + debug: sandbox.config.ProxyConfig.Debug, } // Start the proxy here - pid, uri, err := k.proxy.start(sandbox, proxyParams) + pid, uri, err := k.proxy.start(proxyParams) if err != nil { return err } @@ -492,7 +501,7 @@ func (k *kataAgent) startProxy(sandbox *Sandbox) error { // then rollback to kill kata-proxy process defer func() { if err != nil && pid > 0 { - k.proxy.stop(sandbox, pid) + k.proxy.stop(pid) } }() @@ -602,7 +611,7 @@ func (k *kataAgent) stopSandbox(sandbox *Sandbox) error { return err } - return k.proxy.stop(sandbox, k.state.ProxyPid) + return k.proxy.stop(k.state.ProxyPid) } func (k *kataAgent) cleanupSandbox(sandbox *Sandbox) error { diff --git a/virtcontainers/kata_builtin_proxy.go b/virtcontainers/kata_builtin_proxy.go index ebd3cef0c3..58b7c6edd4 100644 --- a/virtcontainers/kata_builtin_proxy.go +++ b/virtcontainers/kata_builtin_proxy.go @@ -26,22 +26,36 @@ func (p *kataBuiltInProxy) consoleWatched() bool { return p.conn != nil } +func (p *kataBuiltInProxy) validateParams(params proxyParams) error { + if len(params.id) == 0 || len(params.agentURL) == 0 || len(params.consoleURL) == 0 { + return fmt.Errorf("Invalid proxy parameters %+v", params) + } + + if params.logger == nil { + return fmt.Errorf("Invalid proxy parameter: proxy logger is not set") + } + + return nil +} + // start is the proxy start implementation for kata builtin proxy. // It starts the console watcher for the guest. // It returns agentURL to let agent connect directly. -func (p *kataBuiltInProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) { - if p.consoleWatched() { - return -1, "", fmt.Errorf("kata builtin proxy running for sandbox %s", p.sandboxID) - } - - p.sandboxID = sandbox.id - console, err := sandbox.hypervisor.getSandboxConsole(sandbox.id) - if err != nil { +func (p *kataBuiltInProxy) start(params proxyParams) (int, string, error) { + if err := p.validateParams(params); err != nil { return -1, "", err } - err = p.watchConsole(consoleProtoUnix, console, params.logger) + if p.consoleWatched() { + return -1, "", fmt.Errorf("kata builtin proxy running for sandbox %s", params.id) + } + + params.logger.Info("Starting builtin kata proxy") + + p.sandboxID = params.id + err := p.watchConsole(consoleProtoUnix, params.consoleURL, params.logger) if err != nil { + p.sandboxID = "" return -1, "", err } @@ -49,7 +63,7 @@ func (p *kataBuiltInProxy) start(sandbox *Sandbox, params proxyParams) (int, str } // stop is the proxy stop implementation for kata builtin proxy. -func (p *kataBuiltInProxy) stop(sandbox *Sandbox, pid int) error { +func (p *kataBuiltInProxy) stop(pid int) error { if p.conn != nil { p.conn.Close() p.conn = nil diff --git a/virtcontainers/kata_proxy.go b/virtcontainers/kata_proxy.go index e68686295b..1ff9d01f14 100644 --- a/virtcontainers/kata_proxy.go +++ b/virtcontainers/kata_proxy.go @@ -6,7 +6,6 @@ package virtcontainers import ( - "fmt" "os/exec" "syscall" ) @@ -23,46 +22,28 @@ func (p *kataProxy) consoleWatched() bool { } // start is kataProxy start implementation for proxy interface. -func (p *kataProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) { - sandbox.Logger().Info("Starting regular Kata proxy rather than built-in") - if sandbox.config == nil { - return -1, "", fmt.Errorf("Sandbox config cannot be nil") - } - - if sandbox.agent == nil { - return -1, "", fmt.Errorf("No agent") - } - - if params.agentURL == "" { - return -1, "", fmt.Errorf("AgentURL cannot be empty") - } - - config := sandbox.config.ProxyConfig - if err := validateProxyConfig(config); err != nil { +func (p *kataProxy) start(params proxyParams) (int, string, error) { + if err := validateProxyParams(params); err != nil { return -1, "", err } + params.logger.Info("Starting regular Kata proxy rather than built-in") + // construct the socket path the proxy instance will use - proxyURL, err := defaultProxyURL(sandbox, SocketTypeUNIX) + proxyURL, err := defaultProxyURL(params.id, SocketTypeUNIX) if err != nil { return -1, "", err } args := []string{ - config.Path, + params.path, "-listen-socket", proxyURL, "-mux-socket", params.agentURL, - "-sandbox", sandbox.ID(), + "-sandbox", params.id, } - if config.Debug { - args = append(args, "-log", "debug") - console, err := sandbox.hypervisor.getSandboxConsole(sandbox.id) - if err != nil { - return -1, "", err - } - - args = append(args, "-agent-logs-socket", console) + if params.debug { + args = append(args, "-log", "debug", "-agent-logs-socket", params.consoleURL) } cmd := exec.Command(args[0], args[1:]...) @@ -74,7 +55,7 @@ func (p *kataProxy) start(sandbox *Sandbox, params proxyParams) (int, string, er } // stop is kataProxy stop implementation for proxy interface. -func (p *kataProxy) stop(sandbox *Sandbox, pid int) error { +func (p *kataProxy) stop(pid int) error { // Signal the proxy with SIGTERM. return syscall.Kill(pid, syscall.SIGTERM) } diff --git a/virtcontainers/kata_proxy_test.go b/virtcontainers/kata_proxy_test.go index 0e202d7a57..d14fc7bcb5 100644 --- a/virtcontainers/kata_proxy_test.go +++ b/virtcontainers/kata_proxy_test.go @@ -13,5 +13,5 @@ func TestKataProxyStart(t *testing.T) { agent := &kataAgent{} proxy := &kataProxy{} - testProxyStart(t, agent, proxy, KataProxyType) + testProxyStart(t, agent, proxy) } diff --git a/virtcontainers/no_proxy.go b/virtcontainers/no_proxy.go index 33664a56bf..ef97e4a30d 100644 --- a/virtcontainers/no_proxy.go +++ b/virtcontainers/no_proxy.go @@ -23,8 +23,13 @@ type noProxy struct { } // start is noProxy start implementation for proxy interface. -func (p *noProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) { - sandbox.Logger().Info("No proxy started because of no-proxy implementation") +func (p *noProxy) start(params proxyParams) (int, string, error) { + if params.logger == nil { + return -1, "", fmt.Errorf("proxy logger is not set") + } + + params.logger.Info("No proxy started because of no-proxy implementation") + if params.agentURL == "" { return -1, "", fmt.Errorf("AgentURL cannot be empty") } @@ -33,7 +38,7 @@ func (p *noProxy) start(sandbox *Sandbox, params proxyParams) (int, string, erro } // stop is noProxy stop implementation for proxy interface. -func (p *noProxy) stop(sandbox *Sandbox, pid int) error { +func (p *noProxy) stop(pid int) error { return nil } diff --git a/virtcontainers/no_proxy_test.go b/virtcontainers/no_proxy_test.go index 4b1bd1b63a..5bf6df765c 100644 --- a/virtcontainers/no_proxy_test.go +++ b/virtcontainers/no_proxy_test.go @@ -10,14 +10,13 @@ import ( ) func TestNoProxyStart(t *testing.T) { - sandbox := &Sandbox{ - agent: newAgent(NoopAgentType), - } - p := &noProxy{} agentURL := "agentURL" - pid, vmURL, err := p.start(sandbox, proxyParams{agentURL: agentURL}) + pid, vmURL, err := p.start(proxyParams{ + agentURL: agentURL, + logger: testDefaultLogger, + }) if err != nil { t.Fatal(err) } @@ -34,7 +33,7 @@ func TestNoProxyStart(t *testing.T) { func TestNoProxyStop(t *testing.T) { p := &noProxy{} - if err := p.stop(&Sandbox{}, 0); err != nil { + if err := p.stop(0); err != nil { t.Fatal(err) } } diff --git a/virtcontainers/noop_proxy.go b/virtcontainers/noop_proxy.go index 473d8a63ae..5a80f7b60d 100644 --- a/virtcontainers/noop_proxy.go +++ b/virtcontainers/noop_proxy.go @@ -13,13 +13,13 @@ var noopProxyURL = "noopProxyURL" // register is the proxy start implementation for testing purpose. // It does nothing. -func (p *noopProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) { +func (p *noopProxy) start(params proxyParams) (int, string, error) { return 0, noopProxyURL, nil } // stop is the proxy stop implementation for testing purpose. // It does nothing. -func (p *noopProxy) stop(sandbox *Sandbox, pid int) error { +func (p *noopProxy) stop(pid int) error { return nil } diff --git a/virtcontainers/proxy.go b/virtcontainers/proxy.go index 29256e7349..050980c3ea 100644 --- a/virtcontainers/proxy.go +++ b/virtcontainers/proxy.go @@ -22,8 +22,12 @@ type ProxyConfig struct { // proxyParams is the structure providing specific parameters needed // for the execution of the proxy binary. type proxyParams struct { - agentURL string - logger *logrus.Entry + id string + path string + agentURL string + consoleURL string + logger *logrus.Entry + debug bool } // ProxyType describes a proxy type. @@ -119,6 +123,18 @@ func newProxy(pType ProxyType) (proxy, error) { } } +func validateProxyParams(p proxyParams) error { + if len(p.path) == 0 || len(p.id) == 0 || len(p.agentURL) == 0 || len(p.consoleURL) == 0 { + return fmt.Errorf("Invalid proxy parameters %+v", p) + } + + if p.logger == nil { + return fmt.Errorf("Invalid proxy parameter: proxy logger is not set") + } + + return nil +} + func validateProxyConfig(proxyConfig ProxyConfig) error { if len(proxyConfig.Path) == 0 { return fmt.Errorf("Proxy path cannot be empty") @@ -127,10 +143,10 @@ func validateProxyConfig(proxyConfig ProxyConfig) error { return nil } -func defaultProxyURL(sandbox *Sandbox, socketType string) (string, error) { +func defaultProxyURL(id, socketType string) (string, error) { switch socketType { case SocketTypeUNIX: - socketPath := filepath.Join(runStoragePath, sandbox.id, "proxy.sock") + socketPath := filepath.Join(runStoragePath, id, "proxy.sock") return fmt.Sprintf("unix://%s", socketPath), nil case SocketTypeVSOCK: // TODO Build the VSOCK default URL @@ -146,13 +162,13 @@ func isProxyBuiltIn(pType ProxyType) bool { // proxy is the virtcontainers proxy interface. type proxy interface { - // start launches a proxy instance for the specified sandbox, returning + // start launches a proxy instance with specified parameters, returning // the PID of the process and the URL used to connect to it. - start(sandbox *Sandbox, params proxyParams) (int, string, error) + start(params proxyParams) (int, string, error) // stop terminates a proxy instance after all communications with the // agent inside the VM have been properly stopped. - stop(sandbox *Sandbox, pid int) error + stop(pid int) error //check if the proxy has watched the vm console. consoleWatched() bool diff --git a/virtcontainers/proxy_test.go b/virtcontainers/proxy_test.go index 4f72a2212d..4a396e9e9c 100644 --- a/virtcontainers/proxy_test.go +++ b/virtcontainers/proxy_test.go @@ -13,9 +13,12 @@ import ( "reflect" "testing" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" ) +var testDefaultLogger = logrus.WithField("proxy", "test") + func testSetProxyType(t *testing.T, value string, expected ProxyType) { var proxyType ProxyType @@ -206,7 +209,7 @@ func testDefaultProxyURL(expectedURL string, socketType string, sandboxID string id: sandboxID, } - url, err := defaultProxyURL(sandbox, socketType) + url, err := defaultProxyURL(sandbox.id, socketType) if err != nil { return err } @@ -242,7 +245,7 @@ func TestDefaultProxyURLUnknown(t *testing.T) { } } -func testProxyStart(t *testing.T, agent agent, proxy proxy, proxyType ProxyType) { +func testProxyStart(t *testing.T, agent agent, proxy proxy) { assert := assert.New(t) assert.NotNil(proxy) @@ -252,7 +255,6 @@ func testProxyStart(t *testing.T, agent agent, proxy proxy, proxyType ProxyType) defer os.RemoveAll(tmpdir) type testData struct { - sandbox *Sandbox params proxyParams expectedURI string expectError bool @@ -263,60 +265,43 @@ func testProxyStart(t *testing.T, agent agent, proxy proxy, proxyType ProxyType) expectedURI := fmt.Sprintf("unix://%s", expectedSocketPath) data := []testData{ - {&Sandbox{}, proxyParams{}, "", true}, + {proxyParams{}, "", true}, { - &Sandbox{ - config: &SandboxConfig{ - ProxyType: "invalid", - }, - }, - proxyParams{}, - "", true, - }, - { - &Sandbox{ - config: &SandboxConfig{ - ProxyType: proxyType, - // invalid - no path - ProxyConfig: ProxyConfig{}, - }, - }, - proxyParams{}, - "", true, - }, - { - &Sandbox{ - config: &SandboxConfig{ - ProxyType: proxyType, - ProxyConfig: ProxyConfig{ - Path: invalidPath, - }, - }, - }, - proxyParams{}, - "", true, - }, - - { - &Sandbox{ - id: testSandboxID, - agent: agent, - config: &SandboxConfig{ - ProxyType: proxyType, - ProxyConfig: ProxyConfig{ - Path: "echo", - }, - }, - }, + // no path proxyParams{ - agentURL: "agentURL", + id: "foobar", + agentURL: "agentURL", + consoleURL: "consoleURL", + logger: testDefaultLogger, + }, + "", true, + }, + { + // invalid path + proxyParams{ + id: "foobar", + path: invalidPath, + agentURL: "agentURL", + consoleURL: "consoleURL", + logger: testDefaultLogger, + }, + "", true, + }, + { + // good case + proxyParams{ + id: testSandboxID, + path: "echo", + agentURL: "agentURL", + consoleURL: "consoleURL", + logger: testDefaultLogger, }, expectedURI, false, }, } for _, d := range data { - pid, uri, err := proxy.start(d.sandbox, d.params) + pid, uri, err := proxy.start(d.params) if d.expectError { assert.Error(err) continue @@ -327,3 +312,43 @@ func testProxyStart(t *testing.T, agent agent, proxy proxy, proxyType ProxyType) assert.Equal(d.expectedURI, uri) } } + +func TestValidateProxyConfig(t *testing.T) { + assert := assert.New(t) + + config := ProxyConfig{} + err := validateProxyConfig(config) + assert.Error(err) + + config.Path = "foobar" + err = validateProxyConfig(config) + assert.Nil(err) +} + +func TestValidateProxyParams(t *testing.T) { + assert := assert.New(t) + + p := proxyParams{} + err := validateProxyParams(p) + assert.Error(err) + + p.path = "foobar" + err = validateProxyParams(p) + assert.Error(err) + + p.id = "foobar1" + err = validateProxyParams(p) + assert.Error(err) + + p.agentURL = "foobar2" + err = validateProxyParams(p) + assert.Error(err) + + p.consoleURL = "foobar3" + err = validateProxyParams(p) + assert.Error(err) + + p.logger = &logrus.Entry{} + err = validateProxyParams(p) + assert.Nil(err) +} From 4738d4e87ae486da40cf1c7a646acecd0b434100 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Fri, 17 Aug 2018 18:38:49 +0800 Subject: [PATCH 4/6] agent: add setProxy/getAgentURL interface Callers can use setProxy to ask agent to use an existing proxy. agent is modified to rely on its state.URL to tell if an its proxy is a valid one. And startProxy will skip a valid proxy since it is already started. Signed-off-by: Peng Tao --- virtcontainers/agent.go | 6 +++ virtcontainers/hyperstart_agent.go | 59 +++++++++++++++++++++-- virtcontainers/hyperstart_agent_test.go | 23 +++++++++ virtcontainers/kata_agent.go | 58 ++++++++++++++++++++-- virtcontainers/kata_agent_test.go | 32 ++++++++++++ virtcontainers/kata_builtin_proxy.go | 4 +- virtcontainers/kata_builtin_proxy_test.go | 50 +++++++++++++++++++ virtcontainers/noop_agent.go | 10 ++++ virtcontainers/noop_agent_test.go | 21 ++++++++ 9 files changed, 253 insertions(+), 10 deletions(-) create mode 100644 virtcontainers/kata_builtin_proxy_test.go diff --git a/virtcontainers/agent.go b/virtcontainers/agent.go index 29d9862bd8..f5ada6ada6 100644 --- a/virtcontainers/agent.go +++ b/virtcontainers/agent.go @@ -145,6 +145,12 @@ type agent interface { // start the proxy startProxy(sandbox *Sandbox) error + // set to use an existing proxy + setProxy(sandbox *Sandbox, proxy proxy, pid int, url string) error + + // get agent url + getAgentURL() (string, error) + // createSandbox will tell the agent to perform necessary setup for a Sandbox. createSandbox(sandbox *Sandbox) error diff --git a/virtcontainers/hyperstart_agent.go b/virtcontainers/hyperstart_agent.go index c7ee02a3b9..35d7964200 100644 --- a/virtcontainers/hyperstart_agent.go +++ b/virtcontainers/hyperstart_agent.go @@ -389,20 +389,34 @@ func (h *hyper) startProxy(sandbox *Sandbox) error { return nil } + if h.state.URL != "" { + h.Logger().WithFields(logrus.Fields{ + "sandbox": sandbox.id, + "proxy-pid": h.state.ProxyPid, + "proxy-url": h.state.URL, + }).Infof("proxy already started") + return nil + } + // Start the proxy here pid, uri, err := h.proxy.start(proxyParams{ id: sandbox.id, path: sandbox.config.ProxyConfig.Path, + debug: sandbox.config.ProxyConfig.Debug, logger: h.Logger(), }) if err != nil { return err } + defer func() { + if err != nil { + h.proxy.stop(pid) + } + }() + // Fill agent state with proxy information, and store them. - h.state.ProxyPid = pid - h.state.URL = uri - if err := sandbox.storage.storeAgentState(sandbox.id, h.state); err != nil { + if err = h.setProxy(sandbox, h.proxy, pid, uri); err != nil { return err } @@ -465,7 +479,18 @@ func (h *hyper) stopSandbox(sandbox *Sandbox) error { return err } - return h.proxy.stop(h.state.ProxyPid) + if err := h.proxy.stop(h.state.ProxyPid); err != nil { + return err + } + + h.state.ProxyPid = -1 + h.state.URL = "" + if err := sandbox.storage.storeAgentState(sandbox.id, h.state); err != nil { + // ignore error + h.Logger().WithError(err).WithField("sandbox", sandbox.id).Error("failed to clean up agent state") + } + + return nil } // handleBlockVolumes handles volumes that are block device files, by @@ -936,3 +961,29 @@ func (h *hyper) reseedRNG(data []byte) error { // hyperstart-agent does not support reseeding return nil } + +func (h *hyper) getAgentURL() (string, error) { + // hyperstart-agent does not support getting agent url + return "", nil +} + +func (h *hyper) setProxy(sandbox *Sandbox, proxy proxy, pid int, url string) error { + if url == "" { + return fmt.Errorf("invalid empty proxy url") + } + + if h.state.URL != "" && h.state.URL != url { + h.proxy.stop(h.state.ProxyPid) + } + + h.proxy = proxy + h.state.ProxyPid = pid + h.state.URL = url + if sandbox != nil { + if err := sandbox.storage.storeAgentState(sandbox.id, h.state); err != nil { + return err + } + } + + return nil +} diff --git a/virtcontainers/hyperstart_agent_test.go b/virtcontainers/hyperstart_agent_test.go index 41b5faf9e1..c17f997c25 100644 --- a/virtcontainers/hyperstart_agent_test.go +++ b/virtcontainers/hyperstart_agent_test.go @@ -238,3 +238,26 @@ func TestHyperListRoutes(t *testing.T) { _, err := h.listRoutes() assert.Nil(err) } + +func TestHyperSetProxy(t *testing.T) { + assert := assert.New(t) + + h := &hyper{} + p := &ccProxy{} + s := &Sandbox{storage: &filesystem{}} + + err := h.setProxy(s, p, 0, "") + assert.Error(err) + + err = h.setProxy(s, p, 0, "foobar") + assert.Error(err) +} + +func TestHyperGetAgentUrl(t *testing.T) { + assert := assert.New(t) + h := &hyper{} + + url, err := h.getAgentURL() + assert.Nil(err) + assert.Empty(url) +} diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go index 088ee4b155..f26a084962 100644 --- a/virtcontainers/kata_agent.go +++ b/virtcontainers/kata_agent.go @@ -471,6 +471,15 @@ func (k *kataAgent) startProxy(sandbox *Sandbox) error { return nil } + if k.state.URL != "" { + k.Logger().WithFields(logrus.Fields{ + "sandbox": sandbox.id, + "proxy-pid": k.state.ProxyPid, + "proxy-url": k.state.URL, + }).Infof("proxy already started") + return nil + } + // Get agent socket path to provide it to the proxy. agentURL, err := k.agentURL() if err != nil { @@ -500,15 +509,13 @@ func (k *kataAgent) startProxy(sandbox *Sandbox) error { // If error occurs after kata-proxy process start, // then rollback to kill kata-proxy process defer func() { - if err != nil && pid > 0 { + if err != nil { k.proxy.stop(pid) } }() // Fill agent state with proxy information, and store them. - k.state.ProxyPid = pid - k.state.URL = uri - if err = sandbox.storage.storeAgentState(sandbox.id, k.state); err != nil { + if err = k.setProxy(sandbox, k.proxy, pid, uri); err != nil { return err } @@ -521,6 +528,35 @@ func (k *kataAgent) startProxy(sandbox *Sandbox) error { return nil } +func (k *kataAgent) getAgentURL() (string, error) { + return k.agentURL() +} + +func (k *kataAgent) setProxy(sandbox *Sandbox, proxy proxy, pid int, url string) error { + if url == "" { + var err error + if url, err = k.agentURL(); err != nil { + return err + } + } + + // Are we setting the same proxy again? + if k.proxy != nil && k.state.URL != "" && k.state.URL != url { + k.proxy.stop(k.state.ProxyPid) + } + + k.proxy = proxy + k.state.ProxyPid = pid + k.state.URL = url + if sandbox != nil { + if err := sandbox.storage.storeAgentState(sandbox.id, k.state); err != nil { + return err + } + } + + return nil +} + func (k *kataAgent) startSandbox(sandbox *Sandbox) error { span, _ := k.trace("startSandbox") defer span.Finish() @@ -611,7 +647,19 @@ func (k *kataAgent) stopSandbox(sandbox *Sandbox) error { return err } - return k.proxy.stop(k.state.ProxyPid) + if err := k.proxy.stop(k.state.ProxyPid); err != nil { + return err + } + + // clean up agent state + k.state.ProxyPid = -1 + k.state.URL = "" + if err := sandbox.storage.storeAgentState(sandbox.id, k.state); err != nil { + // ignore error + k.Logger().WithError(err).WithField("sandbox", sandbox.id).Error("failed to clean up agent state") + } + + return nil } func (k *kataAgent) cleanupSandbox(sandbox *Sandbox) error { diff --git a/virtcontainers/kata_agent_test.go b/virtcontainers/kata_agent_test.go index 305bfabf81..043f49d79f 100644 --- a/virtcontainers/kata_agent_test.go +++ b/virtcontainers/kata_agent_test.go @@ -795,3 +795,35 @@ func TestAgentNetworkOperation(t *testing.T) { _, err = k.listRoutes() assert.Nil(err) } + +func TestKataAgentSetProxy(t *testing.T) { + assert := assert.New(t) + + k := &kataAgent{} + p := &kataBuiltInProxy{} + s := &Sandbox{storage: &filesystem{}} + + err := k.setProxy(s, p, 0, "") + assert.Error(err) + + err = k.setProxy(s, p, 0, "foobar") + assert.Error(err) +} + +func TestKataGetAgentUrl(t *testing.T) { + assert := assert.New(t) + + k := &kataAgent{} + err := k.generateVMSocket("foobar", KataAgentConfig{}) + assert.Nil(err) + url, err := k.getAgentURL() + assert.Nil(err) + assert.NotEmpty(url) + + err = k.generateVMSocket("foobar", KataAgentConfig{UseVSock: true}) + assert.Nil(err) + url, err = k.getAgentURL() + assert.Nil(err) + assert.NotEmpty(url) + +} diff --git a/virtcontainers/kata_builtin_proxy.go b/virtcontainers/kata_builtin_proxy.go index 58b7c6edd4..ef18d0765b 100644 --- a/virtcontainers/kata_builtin_proxy.go +++ b/virtcontainers/kata_builtin_proxy.go @@ -14,6 +14,8 @@ import ( "github.com/sirupsen/logrus" ) +var buildinProxyConsoleProto = consoleProtoUnix + // This is a kata builtin proxy implementation of the proxy interface. Kata proxy // functionality is implemented inside the virtcontainers library. type kataBuiltInProxy struct { @@ -53,7 +55,7 @@ func (p *kataBuiltInProxy) start(params proxyParams) (int, string, error) { params.logger.Info("Starting builtin kata proxy") p.sandboxID = params.id - err := p.watchConsole(consoleProtoUnix, params.consoleURL, params.logger) + err := p.watchConsole(buildinProxyConsoleProto, params.consoleURL, params.logger) if err != nil { p.sandboxID = "" return -1, "", err diff --git a/virtcontainers/kata_builtin_proxy_test.go b/virtcontainers/kata_builtin_proxy_test.go new file mode 100644 index 0000000000..32e9b95b1d --- /dev/null +++ b/virtcontainers/kata_builtin_proxy_test.go @@ -0,0 +1,50 @@ +// Copyright (c) 2018 HyperHQ Inc. +// +// SPDX-License-Identifier: Apache-2.0 +// + +package virtcontainers + +import ( + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" +) + +func TestKataBuiltinProxy(t *testing.T) { + assert := assert.New(t) + + p := kataBuiltInProxy{} + + params := proxyParams{} + + err := p.validateParams(params) + assert.NotNil(err) + + params.id = "foobarproxy" + err = p.validateParams(params) + assert.NotNil(err) + + params.agentURL = "foobaragent" + err = p.validateParams(params) + assert.NotNil(err) + + params.consoleURL = "foobarconsole" + err = p.validateParams(params) + assert.NotNil(err) + + params.logger = logrus.WithField("proxy", params.id) + err = p.validateParams(params) + assert.Nil(err) + + buildinProxyConsoleProto = "foobarproto" + _, _, err = p.start(params) + assert.NotNil(err) + assert.Empty(p.sandboxID) + + err = p.stop(0) + assert.Nil(err) + + assert.False(p.consoleWatched()) +} diff --git a/virtcontainers/noop_agent.go b/virtcontainers/noop_agent.go index 27eaea301e..e98bcd74fe 100644 --- a/virtcontainers/noop_agent.go +++ b/virtcontainers/noop_agent.go @@ -187,3 +187,13 @@ func (n *noopAgent) getSharePath(id string) string { func (n *noopAgent) reseedRNG(data []byte) error { return nil } + +// getAgentURL is the Noop agent url getter. It returns nothing. +func (n *noopAgent) getAgentURL() (string, error) { + return "", nil +} + +// setProxy is the Noop agent proxy setter. It does nothing. +func (n *noopAgent) setProxy(sandbox *Sandbox, proxy proxy, pid int, url string) error { + return nil +} diff --git a/virtcontainers/noop_agent_test.go b/virtcontainers/noop_agent_test.go index 144c8dff6c..163e443226 100644 --- a/virtcontainers/noop_agent_test.go +++ b/virtcontainers/noop_agent_test.go @@ -9,6 +9,8 @@ package virtcontainers import ( "context" "testing" + + "github.com/stretchr/testify/assert" ) func testCreateNoopContainer() (*Sandbox, *Container, error) { @@ -251,3 +253,22 @@ func TestNoopAgentListRoutes(t *testing.T) { t.Fatal("listRoutes failed") } } + +func TestNoopAgentRSetProxy(t *testing.T) { + n := &noopAgent{} + p := &noopProxy{} + s := &Sandbox{} + err := n.setProxy(s, p, 0, "") + if err != nil { + t.Fatal("set proxy failed") + } +} + +func TestNoopGetAgentUrl(t *testing.T) { + assert := assert.New(t) + n := &noopAgent{} + + url, err := n.getAgentURL() + assert.Nil(err) + assert.Empty(url) +} From 07c1f18e5116f6f511609055b28bd7b013f8232c Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Tue, 28 Aug 2018 12:02:06 +0800 Subject: [PATCH 5/6] factory: start proxy after create new VM The PR moves ahead the start of proxy process for vm factory so that it waits for both vm and proxy to be up at the same time. This saves about 300ms for new container creation in my local test machine. Fixes: #683 Signed-off-by: Peng Tao --- virtcontainers/factory/base/base.go | 2 +- virtcontainers/factory/cache/cache.go | 4 +- virtcontainers/factory/cache/cache_test.go | 3 +- virtcontainers/factory/direct/direct.go | 4 +- virtcontainers/factory/direct/direct_test.go | 3 +- virtcontainers/factory/factory.go | 44 ++++--- virtcontainers/factory/factory_test.go | 15 +-- virtcontainers/factory/template/template.go | 35 +++-- .../factory/template/template_test.go | 18 ++- virtcontainers/noop_proxy_test.go | 26 ++++ virtcontainers/sandbox.go | 14 +- virtcontainers/vm.go | 122 +++++++++++++++--- virtcontainers/vm_test.go | 1 + 13 files changed, 226 insertions(+), 65 deletions(-) create mode 100644 virtcontainers/noop_proxy_test.go diff --git a/virtcontainers/factory/base/base.go b/virtcontainers/factory/base/base.go index 98f5b27678..cdd05dde91 100644 --- a/virtcontainers/factory/base/base.go +++ b/virtcontainers/factory/base/base.go @@ -21,7 +21,7 @@ type FactoryBase interface { Config() vc.VMConfig // GetBaseVM returns a paused VM created by the base factory. - GetBaseVM(ctx context.Context) (*vc.VM, error) + GetBaseVM(ctx context.Context, config vc.VMConfig) (*vc.VM, error) // CloseFactory closes the base factory. CloseFactory(ctx context.Context) diff --git a/virtcontainers/factory/cache/cache.go b/virtcontainers/factory/cache/cache.go index edc130223e..7a4a093fad 100644 --- a/virtcontainers/factory/cache/cache.go +++ b/virtcontainers/factory/cache/cache.go @@ -37,7 +37,7 @@ func New(ctx context.Context, count uint, b base.FactoryBase) base.FactoryBase { c.wg.Add(1) go func() { for { - vm, err := b.GetBaseVM(ctx) + vm, err := b.GetBaseVM(ctx, c.Config()) if err != nil { c.wg.Done() c.CloseFactory(ctx) @@ -63,7 +63,7 @@ func (c *cache) Config() vc.VMConfig { } // GetBaseVM returns a base VM from cache factory's base factory. -func (c *cache) GetBaseVM(ctx context.Context) (*vc.VM, error) { +func (c *cache) GetBaseVM(ctx context.Context, config vc.VMConfig) (*vc.VM, error) { vm, ok := <-c.cacheCh if ok { return vm, nil diff --git a/virtcontainers/factory/cache/cache_test.go b/virtcontainers/factory/cache/cache_test.go index 800d3eb1b6..4139f6930c 100644 --- a/virtcontainers/factory/cache/cache_test.go +++ b/virtcontainers/factory/cache/cache_test.go @@ -27,6 +27,7 @@ func TestTemplateFactory(t *testing.T) { vmConfig := vc.VMConfig{ HypervisorType: vc.MockHypervisor, AgentType: vc.NoopAgentType, + ProxyType: vc.NoopProxyType, HypervisorConfig: hyperConfig, } @@ -39,7 +40,7 @@ func TestTemplateFactory(t *testing.T) { assert.Equal(f.Config(), vmConfig) // GetBaseVM - _, err := f.GetBaseVM(ctx) + _, err := f.GetBaseVM(ctx, vmConfig) assert.Nil(err) // CloseFactory diff --git a/virtcontainers/factory/direct/direct.go b/virtcontainers/factory/direct/direct.go index 1cc59f52e5..6ae891679b 100644 --- a/virtcontainers/factory/direct/direct.go +++ b/virtcontainers/factory/direct/direct.go @@ -28,8 +28,8 @@ func (d *direct) Config() vc.VMConfig { } // GetBaseVM create a new VM directly. -func (d *direct) GetBaseVM(ctx context.Context) (*vc.VM, error) { - vm, err := vc.NewVM(ctx, d.config) +func (d *direct) GetBaseVM(ctx context.Context, config vc.VMConfig) (*vc.VM, error) { + vm, err := vc.NewVM(ctx, config) if err != nil { return nil, err } diff --git a/virtcontainers/factory/direct/direct_test.go b/virtcontainers/factory/direct/direct_test.go index 58be358a09..20a2548747 100644 --- a/virtcontainers/factory/direct/direct_test.go +++ b/virtcontainers/factory/direct/direct_test.go @@ -26,6 +26,7 @@ func TestTemplateFactory(t *testing.T) { vmConfig := vc.VMConfig{ HypervisorType: vc.MockHypervisor, AgentType: vc.NoopAgentType, + ProxyType: vc.NoopProxyType, HypervisorConfig: hyperConfig, } @@ -38,7 +39,7 @@ func TestTemplateFactory(t *testing.T) { assert.Equal(f.Config(), vmConfig) // GetBaseVM - _, err := f.GetBaseVM(ctx) + _, err := f.GetBaseVM(ctx, vmConfig) assert.Nil(err) // CloseFactory diff --git a/virtcontainers/factory/factory.go b/virtcontainers/factory/factory.go index 94b6359580..b0d308f4e8 100644 --- a/virtcontainers/factory/factory.go +++ b/virtcontainers/factory/factory.go @@ -29,10 +29,6 @@ type Config struct { VMConfig vc.VMConfig } -func (f *Config) validate() error { - return f.VMConfig.Valid() -} - type factory struct { base base.FactoryBase } @@ -50,7 +46,7 @@ func NewFactory(ctx context.Context, config Config, fetchOnly bool) (vc.Factory, span, _ := trace(ctx, "NewFactory") defer span.Finish() - err := config.validate() + err := config.VMConfig.Valid() if err != nil { return nil, err } @@ -93,13 +89,15 @@ func (f *factory) log() *logrus.Entry { return factoryLogger.WithField("subsystem", "factory") } -func resetHypervisorConfig(config *vc.HypervisorConfig) { - config.NumVCPUs = 0 - config.MemorySize = 0 - config.BootToBeTemplate = false - config.BootFromTemplate = false - config.MemoryPath = "" - config.DevicesStatePath = "" +func resetHypervisorConfig(config *vc.VMConfig) { + config.HypervisorConfig.NumVCPUs = 0 + config.HypervisorConfig.MemorySize = 0 + config.HypervisorConfig.BootToBeTemplate = false + config.HypervisorConfig.BootFromTemplate = false + config.HypervisorConfig.MemoryPath = "" + config.HypervisorConfig.DevicesStatePath = "" + config.ProxyType = vc.NoopProxyType + config.ProxyConfig = vc.ProxyConfig{} } // It's important that baseConfig and newConfig are passed by value! @@ -113,8 +111,8 @@ func checkVMConfig(config1, config2 vc.VMConfig) error { } // check hypervisor config details - resetHypervisorConfig(&config1.HypervisorConfig) - resetHypervisorConfig(&config2.HypervisorConfig) + resetHypervisorConfig(&config1) + resetHypervisorConfig(&config2) if !reflect.DeepEqual(config1, config2) { return fmt.Errorf("hypervisor config does not match, base: %+v. new: %+v", config1, config2) @@ -129,13 +127,25 @@ func (f *factory) checkConfig(config vc.VMConfig) error { return checkVMConfig(config, baseConfig) } +func (f *factory) validateNewVMConfig(config vc.VMConfig) error { + if len(config.AgentType.String()) == 0 { + return fmt.Errorf("Missing agent type") + } + + if len(config.ProxyType.String()) == 0 { + return fmt.Errorf("Missing proxy type") + } + + return config.Valid() +} + // GetVM returns a working blank VM created by the factory. func (f *factory) GetVM(ctx context.Context, config vc.VMConfig) (*vc.VM, error) { span, _ := trace(ctx, "GetVM") defer span.Finish() hypervisorConfig := config.HypervisorConfig - err := config.Valid() + err := f.validateNewVMConfig(config) if err != nil { f.log().WithError(err).Error("invalid hypervisor config") return nil, err @@ -144,11 +154,11 @@ func (f *factory) GetVM(ctx context.Context, config vc.VMConfig) (*vc.VM, error) err = f.checkConfig(config) if err != nil { f.log().WithError(err).Info("fallback to direct factory vm") - return direct.New(ctx, config).GetBaseVM(ctx) + return direct.New(ctx, config).GetBaseVM(ctx, config) } f.log().Info("get base VM") - vm, err := f.base.GetBaseVM(ctx) + vm, err := f.base.GetBaseVM(ctx, config) if err != nil { f.log().WithError(err).Error("failed to get base VM") return nil, err diff --git a/virtcontainers/factory/factory_test.go b/virtcontainers/factory/factory_test.go index c20f1f40a8..6340b22ae8 100644 --- a/virtcontainers/factory/factory_test.go +++ b/virtcontainers/factory/factory_test.go @@ -94,23 +94,21 @@ func TestFactorySetLogger(t *testing.T) { func TestVMConfigValid(t *testing.T) { assert := assert.New(t) - config := Config{} - - err := config.validate() - assert.Error(err) - testDir, _ := ioutil.TempDir("", "vmfactory-tmp-") - config.VMConfig = vc.VMConfig{ + config := vc.VMConfig{ HypervisorType: vc.MockHypervisor, AgentType: vc.NoopAgentType, + ProxyType: vc.NoopProxyType, HypervisorConfig: vc.HypervisorConfig{ KernelPath: testDir, ImagePath: testDir, }, } - err = config.validate() + f := factory{} + + err := f.validateNewVMConfig(config) assert.Nil(err) } @@ -165,8 +163,9 @@ func TestFactoryGetVM(t *testing.T) { } vmConfig := vc.VMConfig{ HypervisorType: vc.MockHypervisor, - AgentType: vc.NoopAgentType, HypervisorConfig: hyperConfig, + AgentType: vc.NoopAgentType, + ProxyType: vc.NoopProxyType, } ctx := context.Background() diff --git a/virtcontainers/factory/template/template.go b/virtcontainers/factory/template/template.go index 7daf8a76f9..beea512d23 100644 --- a/virtcontainers/factory/template/template.go +++ b/virtcontainers/factory/template/template.go @@ -23,6 +23,10 @@ type template struct { config vc.VMConfig } +var templateProxyType = vc.KataBuiltInProxyType +var templateWaitForAgent = 2 * time.Second +var templateWaitForMigration = 1 * time.Second + // Fetch finds and returns a pre-built template factory. // TODO: save template metadata and fetch from storage. func Fetch(config vc.VMConfig) (base.FactoryBase, error) { @@ -63,8 +67,8 @@ func (t *template) Config() vc.VMConfig { } // GetBaseVM creates a new paused VM from the template VM. -func (t *template) GetBaseVM(ctx context.Context) (*vc.VM, error) { - return t.createFromTemplateVM(ctx) +func (t *template) GetBaseVM(ctx context.Context, config vc.VMConfig) (*vc.VM, error) { + return t.createFromTemplateVM(ctx, config) } // CloseFactory cleans up the template VM. @@ -100,6 +104,8 @@ func (t *template) createTemplateVM(ctx context.Context) error { config.HypervisorConfig.BootFromTemplate = false config.HypervisorConfig.MemoryPath = t.statePath + "/memory" config.HypervisorConfig.DevicesStatePath = t.statePath + "/state" + // template vm uses builtin proxy + config.ProxyType = templateProxyType vm, err := vc.NewVM(ctx, config) if err != nil { @@ -107,28 +113,41 @@ func (t *template) createTemplateVM(ctx context.Context) error { } defer vm.Stop() - err = vm.Pause() - if err != nil { + if err = vm.Disconnect(); err != nil { return err } - err = vm.Save() - if err != nil { + // Sleep a bit to let the agent grpc server clean up + // When we close connection to the agent, it needs sometime to cleanup + // and restart listening on the communication( serial or vsock) port. + // That time can be saved if we sleep a bit to wait for the agent to + // come around and start listening again. The sleep is only done when + // creating new vm templates and saves time for every new vm that are + // created from template, so it worth the invest. + time.Sleep(templateWaitForAgent) + + if err = vm.Pause(); err != nil { + return err + } + + if err = vm.Save(); err != nil { return err } // qemu QMP does not wait for migration to finish... - time.Sleep(1 * time.Second) + time.Sleep(templateWaitForMigration) return nil } -func (t *template) createFromTemplateVM(ctx context.Context) (*vc.VM, error) { +func (t *template) createFromTemplateVM(ctx context.Context, c vc.VMConfig) (*vc.VM, error) { config := t.config config.HypervisorConfig.BootToBeTemplate = false config.HypervisorConfig.BootFromTemplate = true config.HypervisorConfig.MemoryPath = t.statePath + "/memory" config.HypervisorConfig.DevicesStatePath = t.statePath + "/state" + config.ProxyType = c.ProxyType + config.ProxyConfig = c.ProxyConfig return vc.NewVM(ctx, config) } diff --git a/virtcontainers/factory/template/template_test.go b/virtcontainers/factory/template/template_test.go index 21e23a48ba..09e9cd325a 100644 --- a/virtcontainers/factory/template/template_test.go +++ b/virtcontainers/factory/template/template_test.go @@ -10,6 +10,7 @@ import ( "io/ioutil" "os" "testing" + "time" "github.com/stretchr/testify/assert" @@ -19,6 +20,9 @@ import ( func TestTemplateFactory(t *testing.T) { assert := assert.New(t) + templateWaitForMigration = 1 * time.Microsecond + templateWaitForAgent = 1 * time.Microsecond + testDir, _ := ioutil.TempDir("", "vmfactory-tmp-") hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, @@ -26,8 +30,9 @@ func TestTemplateFactory(t *testing.T) { } vmConfig := vc.VMConfig{ HypervisorType: vc.MockHypervisor, - AgentType: vc.NoopAgentType, HypervisorConfig: hyperConfig, + AgentType: vc.NoopAgentType, + ProxyType: vc.NoopProxyType, } ctx := context.Background() @@ -39,7 +44,7 @@ func TestTemplateFactory(t *testing.T) { assert.Equal(f.Config(), vmConfig) // GetBaseVM - _, err := f.GetBaseVM(ctx) + _, err := f.GetBaseVM(ctx, vmConfig) assert.Nil(err) // Fetch @@ -62,9 +67,16 @@ func TestTemplateFactory(t *testing.T) { assert.Nil(err) err = tt.createTemplateVM(ctx) + assert.Error(err) + + _, err = f.GetBaseVM(ctx, vmConfig) assert.Nil(err) - _, err = tt.GetBaseVM(ctx) + templateProxyType = vc.NoopProxyType + err = tt.createTemplateVM(ctx) + assert.Nil(err) + + _, err = f.GetBaseVM(ctx, vmConfig) assert.Nil(err) // CloseFactory diff --git a/virtcontainers/noop_proxy_test.go b/virtcontainers/noop_proxy_test.go new file mode 100644 index 0000000000..6801a81d4f --- /dev/null +++ b/virtcontainers/noop_proxy_test.go @@ -0,0 +1,26 @@ +// Copyright (c) 2018 HyperHQ Inc. +// +// SPDX-License-Identifier: Apache-2.0 +// + +package virtcontainers + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNoopProxy(t *testing.T) { + n := &noopProxy{} + assert := assert.New(t) + + _, url, err := n.start(proxyParams{}) + assert.Nil(err) + assert.Equal(url, noopProxyURL) + + err = n.stop(0) + assert.Nil(err) + + assert.False(n.consoleWatched()) +} diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index d9aed435b8..50ae41490c 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -1104,6 +1104,8 @@ func (s *Sandbox) startVM() error { HypervisorConfig: s.config.HypervisorConfig, AgentType: s.config.AgentType, AgentConfig: s.config.AgentConfig, + ProxyType: s.config.ProxyType, + ProxyConfig: s.config.ProxyConfig, }) if err != nil { return err @@ -1593,9 +1595,9 @@ func (s *Sandbox) HotplugAddDevice(device api.Device, devType config.DeviceType) if _, err := s.hypervisor.hotplugAddDevice(dev, vfioDev); err != nil { s.Logger(). WithFields(logrus.Fields{ - "sandboxid": s.id, - "vfio device ID": dev.ID, - "vfio device BDF": dev.BDF, + "sandbox": s.id, + "vfio-device-ID": dev.ID, + "vfio-device-BDF": dev.BDF, }).WithError(err).Error("failed to hotplug VFIO device") return err } @@ -1630,9 +1632,9 @@ func (s *Sandbox) HotplugRemoveDevice(device api.Device, devType config.DeviceTy if _, err := s.hypervisor.hotplugRemoveDevice(dev, vfioDev); err != nil { s.Logger().WithError(err). WithFields(logrus.Fields{ - "sandboxid": s.id, - "vfio device ID": dev.ID, - "vfio device BDF": dev.BDF, + "sandbox": s.id, + "vfio-device-ID": dev.ID, + "vfio-device-BDF": dev.BDF, }).Error("failed to hot unplug VFIO device") return err } diff --git a/virtcontainers/vm.go b/virtcontainers/vm.go index 2c8bf6bc66..dd3dc67a31 100644 --- a/virtcontainers/vm.go +++ b/virtcontainers/vm.go @@ -21,6 +21,10 @@ type VM struct { hypervisor hypervisor agent agent + proxy proxy + proxyPid int + proxyURL string + cpu uint32 memory uint32 @@ -34,6 +38,9 @@ type VMConfig struct { AgentType AgentType AgentConfig interface{} + + ProxyType ProxyType + ProxyConfig ProxyConfig } // Valid check VMConfig validity. @@ -41,8 +48,56 @@ func (c *VMConfig) Valid() error { return c.HypervisorConfig.valid() } +func setupProxy(h hypervisor, agent agent, config VMConfig, id string) (int, string, proxy, error) { + consoleURL, err := h.getSandboxConsole(id) + if err != nil { + return -1, "", nil, err + } + agentURL, err := agent.getAgentURL() + if err != nil { + return -1, "", nil, err + } + + // default to kata builtin proxy + proxyType := config.ProxyType + if len(proxyType.String()) == 0 { + proxyType = KataBuiltInProxyType + } + proxy, err := newProxy(proxyType) + if err != nil { + return -1, "", nil, err + } + + proxyParams := proxyParams{ + id: id, + path: config.ProxyConfig.Path, + agentURL: agentURL, + consoleURL: consoleURL, + logger: virtLog.WithField("vm", id), + debug: config.ProxyConfig.Debug, + } + pid, url, err := proxy.start(proxyParams) + if err != nil { + virtLog.WithFields(logrus.Fields{ + "vm": id, + "proxy type": config.ProxyType, + "params": proxyParams, + }).WithError(err).Error("failed to start proxy") + return -1, "", nil, err + } + + return pid, url, proxy, nil +} + // NewVM creates a new VM based on provided VMConfig. func NewVM(ctx context.Context, config VMConfig) (*VM, error) { + var ( + proxy proxy + pid int + url string + ) + + // 1. setup hypervisor hypervisor, err := newHypervisor(config.HypervisorType) if err != nil { return nil, err @@ -54,11 +109,11 @@ func NewVM(ctx context.Context, config VMConfig) (*VM, error) { id := uuid.Generate().String() - virtLog.WithField("vm id", id).WithField("config", config).Info("create new vm") + virtLog.WithField("vm", id).WithField("config", config).Info("create new vm") defer func() { if err != nil { - virtLog.WithField("vm id", id).WithError(err).Error("failed to create new vm") + virtLog.WithField("vm", id).WithError(err).Error("failed to create new vm") } }() @@ -70,37 +125,48 @@ func NewVM(ctx context.Context, config VMConfig) (*VM, error) { return nil, err } + // 2. setup agent agent := newAgent(config.AgentType) - agentConfig := newAgentConfig(config.AgentType, config.AgentConfig) - // do not keep connection for temp agent - if c, ok := agentConfig.(KataAgentConfig); ok { - c.LongLiveConn = false - } vmSharePath := buildVMSharePath(id) - err = agent.configure(hypervisor, id, vmSharePath, true, agentConfig) + err = agent.configure(hypervisor, id, vmSharePath, isProxyBuiltIn(config.ProxyType), config.AgentConfig) if err != nil { return nil, err } + // 3. boot up guest vm if err = hypervisor.startSandbox(); err != nil { return nil, err } + if err = hypervisor.waitSandbox(vmStartTimeout); err != nil { + return nil, err + } defer func() { if err != nil { - virtLog.WithField("vm id", id).WithError(err).Info("clean up vm") + virtLog.WithField("vm", id).WithError(err).Info("clean up vm") hypervisor.stopSandbox() } }() + // 4. setup proxy + pid, url, proxy, err = setupProxy(hypervisor, agent, config, id) + if err != nil { + return nil, err + } + defer func() { + if err != nil { + virtLog.WithField("vm", id).WithError(err).Info("clean up proxy") + proxy.stop(pid) + } + }() + if err = agent.setProxy(nil, proxy, pid, url); err != nil { + return nil, err + } + + // 5. check agent aliveness // VMs booted from template are paused, do not check if !config.HypervisorConfig.BootFromTemplate { - err = hypervisor.waitSandbox(vmStartTimeout) - if err != nil { - return nil, err - } - - virtLog.WithField("vm id", id).Info("check agent status") + virtLog.WithField("vm", id).Info("check agent status") err = agent.check() if err != nil { return nil, err @@ -111,6 +177,9 @@ func NewVM(ctx context.Context, config VMConfig) (*VM, error) { id: id, hypervisor: hypervisor, agent: agent, + proxy: proxy, + proxyPid: pid, + proxyURL: url, cpu: config.HypervisorConfig.NumVCPUs, memory: config.HypervisorConfig.MemorySize, }, nil @@ -121,7 +190,7 @@ func buildVMSharePath(id string) string { } func (v *VM) logger() logrus.FieldLogger { - return virtLog.WithField("vm id", v.id) + return virtLog.WithField("vm", v.id) } // Pause pauses a VM. @@ -148,9 +217,24 @@ func (v *VM) Start() error { return v.hypervisor.startSandbox() } +// Disconnect agent and proxy connections to a VM +func (v *VM) Disconnect() error { + v.logger().Info("kill vm") + + if err := v.agent.disconnect(); err != nil { + v.logger().WithError(err).Error("failed to disconnect agent") + } + if err := v.proxy.stop(v.proxyPid); err != nil { + v.logger().WithError(err).Error("failed to stop proxy") + } + + return nil +} + // Stop stops a VM process. func (v *VM) Stop() error { v.logger().Info("kill vm") + return v.hypervisor.stopSandbox() } @@ -227,8 +311,14 @@ func (v *VM) assignSandbox(s *Sandbox) error { "vmSockDir": vmSockDir, "sbSharePath": sbSharePath, "sbSockDir": sbSockDir, + "proxy-pid": v.proxyPid, + "proxy-url": v.proxyURL, }).Infof("assign vm to sandbox %s", s.id) + if err := s.agent.setProxy(s, v.proxy, v.proxyPid, v.proxyURL); err != nil { + return err + } + // First make sure the symlinks do not exist os.RemoveAll(sbSharePath) os.RemoveAll(sbSockDir) diff --git a/virtcontainers/vm_test.go b/virtcontainers/vm_test.go index 8ecbd5c7da..fa5894c850 100644 --- a/virtcontainers/vm_test.go +++ b/virtcontainers/vm_test.go @@ -20,6 +20,7 @@ func TestNewVM(t *testing.T) { config := VMConfig{ HypervisorType: MockHypervisor, AgentType: NoopAgentType, + ProxyType: NoopProxyType, } hyperConfig := HypervisorConfig{ KernelPath: testDir, From d75841ef23802e63a9103227ac243fb8a7c5ae2b Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Wed, 12 Sep 2018 11:27:26 +0800 Subject: [PATCH 6/6] ut: add more UTs Let's make codecov happier;) Signed-off-by: Peng Tao --- virtcontainers/cc_proxy_test.go | 12 +++++++ virtcontainers/factory/factory_test.go | 13 ++++++-- .../factory/template/template_test.go | 15 +++++++-- virtcontainers/no_proxy_test.go | 31 +++++++++---------- virtcontainers/vm_test.go | 23 ++++++++++++++ 5 files changed, 73 insertions(+), 21 deletions(-) diff --git a/virtcontainers/cc_proxy_test.go b/virtcontainers/cc_proxy_test.go index d5b07f85c2..faa32e891d 100644 --- a/virtcontainers/cc_proxy_test.go +++ b/virtcontainers/cc_proxy_test.go @@ -7,6 +7,8 @@ package virtcontainers import ( "testing" + + "github.com/stretchr/testify/assert" ) func TestCCProxyStart(t *testing.T) { @@ -14,3 +16,13 @@ func TestCCProxyStart(t *testing.T) { testProxyStart(t, nil, proxy) } + +func TestCCProxy(t *testing.T) { + proxy := &ccProxy{} + assert := assert.New(t) + + err := proxy.stop(0) + assert.Nil(err) + + assert.False(proxy.consoleWatched()) +} diff --git a/virtcontainers/factory/factory_test.go b/virtcontainers/factory/factory_test.go index 6340b22ae8..1b1f7888b4 100644 --- a/virtcontainers/factory/factory_test.go +++ b/virtcontainers/factory/factory_test.go @@ -98,8 +98,6 @@ func TestVMConfigValid(t *testing.T) { config := vc.VMConfig{ HypervisorType: vc.MockHypervisor, - AgentType: vc.NoopAgentType, - ProxyType: vc.NoopProxyType, HypervisorConfig: vc.HypervisorConfig{ KernelPath: testDir, ImagePath: testDir, @@ -109,6 +107,14 @@ func TestVMConfigValid(t *testing.T) { f := factory{} err := f.validateNewVMConfig(config) + assert.NotNil(err) + + config.AgentType = vc.NoopAgentType + err = f.validateNewVMConfig(config) + assert.NotNil(err) + + config.ProxyType = vc.NoopProxyType + err = f.validateNewVMConfig(config) assert.Nil(err) } @@ -168,6 +174,9 @@ func TestFactoryGetVM(t *testing.T) { ProxyType: vc.NoopProxyType, } + err := vmConfig.Valid() + assert.Nil(err) + ctx := context.Background() // direct factory diff --git a/virtcontainers/factory/template/template_test.go b/virtcontainers/factory/template/template_test.go index 09e9cd325a..a5021f6107 100644 --- a/virtcontainers/factory/template/template_test.go +++ b/virtcontainers/factory/template/template_test.go @@ -35,6 +35,9 @@ func TestTemplateFactory(t *testing.T) { ProxyType: vc.NoopProxyType, } + err := vmConfig.Valid() + assert.Nil(err) + ctx := context.Background() // New @@ -44,7 +47,7 @@ func TestTemplateFactory(t *testing.T) { assert.Equal(f.Config(), vmConfig) // GetBaseVM - _, err := f.GetBaseVM(ctx, vmConfig) + _, err = f.GetBaseVM(ctx, vmConfig) assert.Nil(err) // Fetch @@ -53,6 +56,8 @@ func TestTemplateFactory(t *testing.T) { config: vmConfig, } + assert.Equal(tt.Config(), vmConfig) + err = tt.checkTemplateVM() assert.Error(err) @@ -69,13 +74,19 @@ func TestTemplateFactory(t *testing.T) { err = tt.createTemplateVM(ctx) assert.Error(err) + templateProxyType = vc.NoopProxyType + _, err = tt.GetBaseVM(ctx, vmConfig) + assert.Nil(err) + _, err = f.GetBaseVM(ctx, vmConfig) assert.Nil(err) - templateProxyType = vc.NoopProxyType err = tt.createTemplateVM(ctx) assert.Nil(err) + _, err = tt.GetBaseVM(ctx, vmConfig) + assert.Nil(err) + _, err = f.GetBaseVM(ctx, vmConfig) assert.Nil(err) diff --git a/virtcontainers/no_proxy_test.go b/virtcontainers/no_proxy_test.go index 5bf6df765c..52375eeaf0 100644 --- a/virtcontainers/no_proxy_test.go +++ b/virtcontainers/no_proxy_test.go @@ -7,33 +7,30 @@ package virtcontainers import ( "testing" + + "github.com/stretchr/testify/assert" ) func TestNoProxyStart(t *testing.T) { p := &noProxy{} + assert := assert.New(t) agentURL := "agentURL" + _, _, err := p.start(proxyParams{ + agentURL: agentURL, + }) + assert.NotNil(err) + pid, vmURL, err := p.start(proxyParams{ agentURL: agentURL, logger: testDefaultLogger, }) - if err != nil { - t.Fatal(err) - } + assert.Nil(err) + assert.Equal(vmURL, agentURL) + assert.Equal(pid, 0) - if vmURL != agentURL { - t.Fatalf("Got URL %q, expecting %q", vmURL, agentURL) - } + err = p.stop(0) + assert.Nil(err) - if pid != 0 { - t.Fatal("Failure since returned PID should be 0") - } -} - -func TestNoProxyStop(t *testing.T) { - p := &noProxy{} - - if err := p.stop(0); err != nil { - t.Fatal(err) - } + assert.False(p.consoleWatched()) } diff --git a/virtcontainers/vm_test.go b/virtcontainers/vm_test.go index fa5894c850..40a9729d6b 100644 --- a/virtcontainers/vm_test.go +++ b/virtcontainers/vm_test.go @@ -44,6 +44,8 @@ func TestNewVM(t *testing.T) { assert.Nil(err) err = vm.Start() assert.Nil(err) + err = vm.Disconnect() + assert.Nil(err) err = vm.Save() assert.Nil(err) err = vm.Stop() @@ -87,3 +89,24 @@ func TestVMConfigValid(t *testing.T) { err = config.Valid() assert.Nil(err) } + +func TestSetupProxy(t *testing.T) { + assert := assert.New(t) + + config := VMConfig{ + HypervisorType: MockHypervisor, + AgentType: NoopAgentType, + } + + hypervisor := &mockHypervisor{} + agent := &noopAgent{} + + // wrong proxy type + config.ProxyType = "invalidProxyType" + _, _, _, err := setupProxy(hypervisor, agent, config, "foobar") + assert.NotNil(err) + + config.ProxyType = NoopProxyType + _, _, _, err = setupProxy(hypervisor, agent, config, "foobar") + assert.Nil(err) +}