Merge pull request #1010 from sameo/topic/hypervisor-interface

Hypervisor interface simplification

virtcontainers/fc.go

@@ -126,15 +126,12 @@ func (fc *firecracker) trace(name string) (opentracing.Span, context.Context) {
 	return span, ctx
 }
 
-//
+// For firecracker this call only sets the internal structure up.
-// init: initialize the firecracker hypervisor's structure. Doesn't
+// The sandbox will be created and started through startSandbox().
-//  actually do anything with firecracker itself, rather it just parses
+func (fc *firecracker) createSandbox(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error {
-//  through and provides necessary details for its structs...
-//
-func (fc *firecracker) init(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()
@@ -346,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()
 
@@ -386,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 {
@@ -422,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")

virtcontainers/hypervisor.go

@@ -590,11 +590,8 @@ 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(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error
-
+	startSandbox(timeout int) error
-	createSandbox() error
-	startSandbox() error
-	waitSandbox(timeout int) error
 	stopSandbox() error
 	pauseSandbox() error
 	saveSandbox() error

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,15 +21,16 @@ 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
 }
 
-func (m *mockHypervisor) startSandbox() error {
+func (m *mockHypervisor) startSandbox(timeout int) error {
-	return nil
-}
-
-func (m *mockHypervisor) waitSandbox(timeout int) error {
 	return nil
 }
 

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.createSandbox(ctx, sandbox.config.ID, &sandbox.config.HypervisorConfig, sandbox.storage); err != nil {
-	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 {
 		t.Fatal(err)
 	}
 }
@@ -56,15 +47,7 @@ func TestMockHypervisorCreateSandbox(t *testing.T) {
 func TestMockHypervisorStartSandbox(t *testing.T) {
 	var m *mockHypervisor
 
-	if err := m.startSandbox(); err != nil {
+	if err := m.startSandbox(vmStartTimeout); err != nil {
-		t.Fatal(err)
-	}
-}
-
-func TestMockHypervisorWaitSandbox(t *testing.T) {
-	var m *mockHypervisor
-
-	if err := m.waitSandbox(0); err != nil {
 		t.Fatal(err)
 	}
 }

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.
+// setup sets the Qemu structure up.
-func (q *qemu) init(ctx context.Context, id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error {
+func (q *qemu) setup(id string, hypervisorConfig *HypervisorConfig, storage resourceStorage) error {
-	// save
+	span, _ := q.trace("setup")
-	q.ctx = ctx
-
-	span, _ := q.trace("init")
 	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
@@ -530,7 +534,7 @@ func (q *qemu) createSandbox() error {
 }
 
 // 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()
 
@@ -574,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.

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 {
+	if err := q.createSandbox(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)
+		t.Fatalf("Qemu createSandbox() is not expected to fail because of missing parent directory for storage: %v", err)
 	}
 }
 

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 {
+	if err = s.hypervisor.createSandbox(ctx, s.id, &sandboxConfig.HypervisorConfig, s.storage); err != nil {
-		return nil, err
-	}
-
-	if err = s.hypervisor.createSandbox(); err != nil {
 		return nil, err
 	}
 
@@ -951,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 {

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")
 

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 {
+	if err = hypervisor.createSandbox(ctx, id, &config.HypervisorConfig, &filesystem{}); err != nil {
-		return nil, err
-	}
-
-	if err = hypervisor.createSandbox(); err != nil {
 		return nil, err
 	}
 
@@ -135,10 +131,7 @@ func NewVM(ctx context.Context, config VMConfig) (*VM, error) {
 	}
 
 	// 3. boot up guest vm
-	if err = hypervisor.startSandbox(); err != nil {
+	if err = hypervisor.startSandbox(vmStartTimeout); err != nil {
-		return nil, err
-	}
-	if err = hypervisor.waitSandbox(vmStartTimeout); err != nil {
 		return nil, err
 	}
 
@@ -215,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