From 763bf18daa3687f06324650f7af2c910925f8872 Mon Sep 17 00:00:00 2001 From: Samuel Ortiz Date: Sun, 9 Dec 2018 19:40:17 +0100 Subject: [PATCH 1/3] virtcontainers: Remove the hypervisor init method We always combine the hypervisor init and createSandbox, because what we're trying to do is simply that: Set the hypervisor and have it create a sandbox. Instead of keeping a method with vague semantics, remove init and integrate the actual hypervisor setup phase into the createSandbox one. Signed-off-by: Samuel Ortiz --- virtcontainers/fc.go | 19 ++++--------------- virtcontainers/hypervisor.go | 4 +--- virtcontainers/mock_hypervisor.go | 16 ++++++---------- virtcontainers/mock_hypervisor_test.go | 15 +++------------ virtcontainers/qemu.go | 18 +++++++++++------- virtcontainers/qemu_test.go | 24 +++++++++++++++++++----- virtcontainers/sandbox.go | 6 +----- virtcontainers/vm.go | 6 +----- 8 files changed, 46 insertions(+), 62 deletions(-) diff --git a/virtcontainers/fc.go b/virtcontainers/fc.go index 573ad88d4..bc5e3bd37 100644 --- a/virtcontainers/fc.go +++ b/virtcontainers/fc.go @@ -126,15 +126,12 @@ func (fc *firecracker) trace(name string) (opentracing.Span, context.Context) { return span, ctx } -// -// init: initialize the firecracker hypervisor's structure. Doesn't -// actually do anything with firecracker itself, rather it just parses -// through and provides necessary details for its structs... -// -func (fc *firecracker) init(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error { +// For firecracker this call only sets the internal structure up. +// The sandbox will be created and started through startSandbox(). +func (fc *firecracker) createSandbox(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error { fc.ctx = ctx - span, _ := fc.trace("init") + span, _ := fc.trace("createSandbox") defer span.Finish() //TODO: check validity of the hypervisor config provided @@ -154,14 +151,6 @@ func (fc *firecracker) init(ctx context.Context, id string, hypervisorConfig *Hy return nil } -// for firecracker this call isn't necessary -func (fc *firecracker) createSandbox() error { - span, _ := fc.trace("createSandbox") - defer span.Finish() - - return nil -} - func (fc *firecracker) newFireClient() *client.Firecracker { span, _ := fc.trace("newFireClient") defer span.Finish() diff --git a/virtcontainers/hypervisor.go b/virtcontainers/hypervisor.go index bff95df32..0e2289bee 100644 --- a/virtcontainers/hypervisor.go +++ b/virtcontainers/hypervisor.go @@ -590,9 +590,7 @@ func RunningOnVMM(cpuInfoPath string) (bool, error) { // hypervisor is the virtcontainers hypervisor interface. // The default hypervisor implementation is Qemu. type hypervisor interface { - init(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error - - createSandbox() error + createSandbox(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error startSandbox() error waitSandbox(timeout int) error stopSandbox() error diff --git a/virtcontainers/mock_hypervisor.go b/virtcontainers/mock_hypervisor.go index 89b3687a8..d5ca20c2d 100644 --- a/virtcontainers/mock_hypervisor.go +++ b/virtcontainers/mock_hypervisor.go @@ -13,15 +13,6 @@ import ( type mockHypervisor struct { } -func (m *mockHypervisor) init(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error { - err := hypervisorConfig.valid() - if err != nil { - return err - } - - return nil -} - func (m *mockHypervisor) capabilities() capabilities { return capabilities{} } @@ -30,7 +21,12 @@ func (m *mockHypervisor) hypervisorConfig() HypervisorConfig { return HypervisorConfig{} } -func (m *mockHypervisor) createSandbox() error { +func (m *mockHypervisor) createSandbox(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error { + err := hypervisorConfig.valid() + if err != nil { + return err + } + return nil } diff --git a/virtcontainers/mock_hypervisor_test.go b/virtcontainers/mock_hypervisor_test.go index 2be8d30d1..e6007380f 100644 --- a/virtcontainers/mock_hypervisor_test.go +++ b/virtcontainers/mock_hypervisor_test.go @@ -11,7 +11,7 @@ import ( "testing" ) -func TestMockHypervisorInit(t *testing.T) { +func TestMockHypervisorCreateSandbox(t *testing.T) { var m *mockHypervisor sandbox := &Sandbox{ @@ -29,7 +29,7 @@ func TestMockHypervisorInit(t *testing.T) { ctx := context.Background() // wrong config - if err := m.init(ctx, sandbox.config.ID, &sandbox.config.HypervisorConfig, sandbox.storage); err == nil { + if err := m.createSandbox(ctx, sandbox.config.ID, &sandbox.config.HypervisorConfig, sandbox.storage); err == nil { t.Fatal() } @@ -39,16 +39,7 @@ func TestMockHypervisorInit(t *testing.T) { HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), } - // right config - if err := m.init(ctx, sandbox.config.ID, &sandbox.config.HypervisorConfig, sandbox.storage); err != nil { - t.Fatal(err) - } -} - -func TestMockHypervisorCreateSandbox(t *testing.T) { - var m *mockHypervisor - - if err := m.createSandbox(); err != nil { + if err := m.createSandbox(ctx, sandbox.config.ID, &sandbox.config.HypervisorConfig, sandbox.storage); err != nil { t.Fatal(err) } } diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go index 60b647689..97e9857c9 100644 --- a/virtcontainers/qemu.go +++ b/virtcontainers/qemu.go @@ -208,12 +208,9 @@ func (q *qemu) trace(name string) (opentracing.Span, context.Context) { return span, ctx } -// init intializes the Qemu structure. -func (q *qemu) init(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error { - // save - q.ctx = ctx - - span, _ := q.trace("init") +// setup sets the Qemu structure up. +func (q *qemu) setup(id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error { + span, _ := q.trace("setup") defer span.Finish() err := hypervisorConfig.valid() @@ -418,10 +415,17 @@ func (q *qemu) setupTemplate(knobs *govmmQemu.Knobs, memory *govmmQemu.Memory) g } // createSandbox is the Hypervisor sandbox creation implementation for govmmQemu. -func (q *qemu) createSandbox() error { +func (q *qemu) createSandbox(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error { + // Save the tracing context + q.ctx = ctx + span, _ := q.trace("createSandbox") defer span.Finish() + if err := q.setup(id, hypervisorConfig, storage); err != nil { + return err + } + machine, err := q.getQemuMachine() if err != nil { return err diff --git a/virtcontainers/qemu_test.go b/virtcontainers/qemu_test.go index 3bd09e3cd..73ad6adfc 100644 --- a/virtcontainers/qemu_test.go +++ b/virtcontainers/qemu_test.go @@ -70,7 +70,7 @@ func TestQemuKernelParameters(t *testing.T) { testQemuKernelParameters(t, params, expectedOut, false) } -func TestQemuInit(t *testing.T) { +func TestQemuCreateSandbox(t *testing.T) { qemuConfig := newQemuConfig() q := &qemu{} @@ -82,13 +82,20 @@ func TestQemuInit(t *testing.T) { }, } + // Create the hypervisor fake binary + testQemuPath := filepath.Join(testDir, testHypervisor) + _, err := os.Create(testQemuPath) + if err != nil { + t.Fatalf("Could not create hypervisor file %s: %v", testQemuPath, err) + } + // Create parent dir path for hypervisor.json parentDir := filepath.Join(runStoragePath, sandbox.id) if err := os.MkdirAll(parentDir, dirMode); err != nil { t.Fatalf("Could not create parent directory %s: %v", parentDir, err) } - if err := q.init(context.Background(), sandbox.id, &sandbox.config.HypervisorConfig, sandbox.storage); err != nil { + if err := q.createSandbox(context.Background(), sandbox.id, &sandbox.config.HypervisorConfig, sandbox.storage); err != nil { t.Fatal(err) } @@ -101,7 +108,7 @@ func TestQemuInit(t *testing.T) { } } -func TestQemuInitMissingParentDirFail(t *testing.T) { +func TestQemuCreateSandboxMissingParentDirFail(t *testing.T) { qemuConfig := newQemuConfig() q := &qemu{} @@ -113,14 +120,21 @@ func TestQemuInitMissingParentDirFail(t *testing.T) { }, } + // Create the hypervisor fake binary + testQemuPath := filepath.Join(testDir, testHypervisor) + _, err := os.Create(testQemuPath) + if err != nil { + t.Fatalf("Could not create hypervisor file %s: %v", testQemuPath, err) + } + // Ensure parent dir path for hypervisor.json does not exist. parentDir := filepath.Join(runStoragePath, sandbox.id) if err := os.RemoveAll(parentDir); err != nil { t.Fatal(err) } - if err := q.init(context.Background(), sandbox.id, &sandbox.config.HypervisorConfig, sandbox.storage); err != nil { - t.Fatalf("Qemu init() is not expected to fail because of missing parent directory for storage: %v", err) + if err := q.createSandbox(context.Background(), sandbox.id, &sandbox.config.HypervisorConfig, sandbox.storage); err != nil { + t.Fatalf("Qemu createSandbox() is not expected to fail because of missing parent directory for storage: %v", err) } } diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index a34203b12..90f1ea015 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -597,11 +597,7 @@ func newSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Factor } }() - if err = s.hypervisor.init(ctx, s.id, &sandboxConfig.HypervisorConfig, s.storage); err != nil { - return nil, err - } - - if err = s.hypervisor.createSandbox(); err != nil { + if err = s.hypervisor.createSandbox(ctx, s.id, &sandboxConfig.HypervisorConfig, s.storage); err != nil { return nil, err } diff --git a/virtcontainers/vm.go b/virtcontainers/vm.go index 147a876d5..e22fc1e69 100644 --- a/virtcontainers/vm.go +++ b/virtcontainers/vm.go @@ -118,11 +118,7 @@ func NewVM(ctx context.Context, config VMConfig) (*VM, error) { } }() - if err = hypervisor.init(ctx, id, &config.HypervisorConfig, &filesystem{}); err != nil { - return nil, err - } - - if err = hypervisor.createSandbox(); err != nil { + if err = hypervisor.createSandbox(ctx, id, &config.HypervisorConfig, &filesystem{}); err != nil { return nil, err } From cf22f402d80bd05ad00b5895d629d3358e334736 Mon Sep 17 00:00:00 2001 From: Samuel Ortiz Date: Mon, 10 Dec 2018 04:09:33 +0100 Subject: [PATCH 2/3] virtcontainers: Remove the hypervisor waitSandbox method We always call waitSandbox after we start the VM (startSandbox), so let's simplify the hypervisor interface and integrate waiting for the VM into startSandbox. This makes startSandbox a blocking call, but that is practically the case today. Fixes: #1009 Signed-off-by: Samuel Ortiz --- virtcontainers/fc.go | 16 ++++++---------- virtcontainers/hypervisor.go | 3 +-- virtcontainers/mock_hypervisor.go | 6 +----- virtcontainers/mock_hypervisor_test.go | 10 +--------- virtcontainers/qemu.go | 4 ++-- virtcontainers/sandbox.go | 6 +----- virtcontainers/vm.go | 7 ++----- 7 files changed, 14 insertions(+), 38 deletions(-) diff --git a/virtcontainers/fc.go b/virtcontainers/fc.go index bc5e3bd37..7942da958 100644 --- a/virtcontainers/fc.go +++ b/virtcontainers/fc.go @@ -335,7 +335,7 @@ func (fc *firecracker) fcStartVM() error { // startSandbox will start the hypervisor for the given sandbox. // In the context of firecracker, this will start the hypervisor, // for configuration, but not yet start the actual virtual machine -func (fc *firecracker) startSandbox() error { +func (fc *firecracker) startSandbox(timeout int) error { span, _ := fc.trace("startSandbox") defer span.Finish() @@ -375,7 +375,11 @@ func (fc *firecracker) startSandbox() error { } } - return fc.fcStartVM() + if err := fc.fcStartVM(); err != nil { + return err + } + + return fc.waitVMM(timeout) } func (fc *firecracker) createDiskPool() error { @@ -411,14 +415,6 @@ func (fc *firecracker) createDiskPool() error { return nil } -// waitSandbox will wait for the Sandbox's VM to be up and running. -func (fc *firecracker) waitSandbox(timeout int) error { - span, _ := fc.trace("waitSandbox") - defer span.Finish() - - return fc.waitVMM(timeout) -} - // stopSandbox will stop the Sandbox's VM. func (fc *firecracker) stopSandbox() (err error) { span, _ := fc.trace("stopSandbox") diff --git a/virtcontainers/hypervisor.go b/virtcontainers/hypervisor.go index 0e2289bee..6d5335917 100644 --- a/virtcontainers/hypervisor.go +++ b/virtcontainers/hypervisor.go @@ -591,8 +591,7 @@ func RunningOnVMM(cpuInfoPath string) (bool, error) { // The default hypervisor implementation is Qemu. type hypervisor interface { createSandbox(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error - startSandbox() error - waitSandbox(timeout int) error + startSandbox(timeout int) error stopSandbox() error pauseSandbox() error saveSandbox() error diff --git a/virtcontainers/mock_hypervisor.go b/virtcontainers/mock_hypervisor.go index d5ca20c2d..f23b3b876 100644 --- a/virtcontainers/mock_hypervisor.go +++ b/virtcontainers/mock_hypervisor.go @@ -30,11 +30,7 @@ func (m *mockHypervisor) createSandbox(ctx context.Context, id string, hyperviso return nil } -func (m *mockHypervisor) startSandbox() error { - return nil -} - -func (m *mockHypervisor) waitSandbox(timeout int) error { +func (m *mockHypervisor) startSandbox(timeout int) error { return nil } diff --git a/virtcontainers/mock_hypervisor_test.go b/virtcontainers/mock_hypervisor_test.go index e6007380f..75fd59c80 100644 --- a/virtcontainers/mock_hypervisor_test.go +++ b/virtcontainers/mock_hypervisor_test.go @@ -47,15 +47,7 @@ func TestMockHypervisorCreateSandbox(t *testing.T) { func TestMockHypervisorStartSandbox(t *testing.T) { var m *mockHypervisor - if err := m.startSandbox(); err != nil { - t.Fatal(err) - } -} - -func TestMockHypervisorWaitSandbox(t *testing.T) { - var m *mockHypervisor - - if err := m.waitSandbox(0); err != nil { + if err := m.startSandbox(vmStartTimeout); err != nil { t.Fatal(err) } } diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go index 97e9857c9..59d9dbbb1 100644 --- a/virtcontainers/qemu.go +++ b/virtcontainers/qemu.go @@ -534,7 +534,7 @@ func (q *qemu) createSandbox(ctx context.Context, id string, hypervisorConfig *H } // startSandbox will start the Sandbox's VM. -func (q *qemu) startSandbox() error { +func (q *qemu) startSandbox(timeout int) error { span, _ := q.trace("startSandbox") defer span.Finish() @@ -578,7 +578,7 @@ func (q *qemu) startSandbox() error { return fmt.Errorf("%s", strErr) } - return nil + return q.waitSandbox(timeout) } // waitSandbox will wait for the Sandbox's VM to be up and running. diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 90f1ea015..51a3a7f74 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -947,15 +947,11 @@ func (s *Sandbox) startVM() error { return nil } - return s.hypervisor.startSandbox() + return s.hypervisor.startSandbox(vmStartTimeout) }); err != nil { return err } - if err := s.hypervisor.waitSandbox(vmStartTimeout); err != nil { - return err - } - // In case of vm factory, network interfaces are hotplugged // after vm is started. if s.factory != nil { diff --git a/virtcontainers/vm.go b/virtcontainers/vm.go index e22fc1e69..43bffffe6 100644 --- a/virtcontainers/vm.go +++ b/virtcontainers/vm.go @@ -131,10 +131,7 @@ func NewVM(ctx context.Context, config VMConfig) (*VM, error) { } // 3. boot up guest vm - if err = hypervisor.startSandbox(); err != nil { - return nil, err - } - if err = hypervisor.waitSandbox(vmStartTimeout); err != nil { + if err = hypervisor.startSandbox(vmStartTimeout); err != nil { return nil, err } @@ -211,7 +208,7 @@ func (v *VM) Resume() error { // Start kicks off a configured VM. func (v *VM) Start() error { v.logger().Info("start vm") - return v.hypervisor.startSandbox() + return v.hypervisor.startSandbox(vmStartTimeout) } // Disconnect agent and proxy connections to a VM From a02fd5982caf307f11a694c660c141b17d3c6a05 Mon Sep 17 00:00:00 2001 From: Samuel Ortiz Date: Tue, 18 Dec 2018 17:02:17 +0100 Subject: [PATCH 3/3] virtcontainers: Remove code duplication in the test setup And add some additional log output for displaying the directories and files created when kicking the virtcontainers tests. Signed-off-by: Samuel Ortiz --- virtcontainers/virtcontainers_test.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/virtcontainers/virtcontainers_test.go b/virtcontainers/virtcontainers_test.go index d752f92f7..adf143b0b 100644 --- a/virtcontainers/virtcontainers_test.go +++ b/virtcontainers/virtcontainers_test.go @@ -81,27 +81,36 @@ func TestMain(m *testing.M) { panic(err) } + fmt.Printf("INFO: Creating virtcontainers test directory %s\n", testDir) err = os.MkdirAll(testDir, dirMode) if err != nil { fmt.Println("Could not create test directories:", err) os.Exit(1) } - _, err = os.Create(filepath.Join(testDir, testKernel)) + testQemuKernelPath = filepath.Join(testDir, testKernel) + testQemuInitrdPath = filepath.Join(testDir, testInitrd) + testQemuImagePath = filepath.Join(testDir, testImage) + testQemuPath = filepath.Join(testDir, testHypervisor) + + fmt.Printf("INFO: Creating virtcontainers test kernel %s\n", testQemuKernelPath) + _, err = os.Create(testQemuKernelPath) if err != nil { fmt.Println("Could not create test kernel:", err) os.RemoveAll(testDir) os.Exit(1) } - _, err = os.Create(filepath.Join(testDir, testImage)) + fmt.Printf("INFO: Creating virtcontainers test image %s\n", testQemuImagePath) + _, err = os.Create(testQemuImagePath) if err != nil { fmt.Println("Could not create test image:", err) os.RemoveAll(testDir) os.Exit(1) } - _, err = os.Create(filepath.Join(testDir, testHypervisor)) + fmt.Printf("INFO: Creating virtcontainers test hypervisor %s\n", testQemuPath) + _, err = os.Create(testQemuPath) if err != nil { fmt.Println("Could not create test hypervisor:", err) os.RemoveAll(testDir) @@ -127,11 +136,6 @@ func TestMain(m *testing.M) { sandboxFileState = filepath.Join(runStoragePath, testSandboxID, stateFile) sandboxFileLock = filepath.Join(runStoragePath, testSandboxID, lockFileName) - testQemuKernelPath = filepath.Join(testDir, testKernel) - testQemuInitrdPath = filepath.Join(testDir, testInitrd) - testQemuImagePath = filepath.Join(testDir, testImage) - testQemuPath = filepath.Join(testDir, testHypervisor) - testHyperstartCtlSocket = filepath.Join(testDir, "test_hyper.sock") testHyperstartTtySocket = filepath.Join(testDir, "test_tty.sock")