From 711783f547cb679c698c2081a5a110648282cda6 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Fri, 10 Nov 2017 16:10:35 -0800 Subject: [PATCH 1/4] agent: hyperstart: Tear down properly the pod By using stopPod() to call into DESTROYPOD command, we make sure the pod inside the VM is properly torn down. Signed-off-by: Sebastien Boeuf --- hyperstart.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/hyperstart.go b/hyperstart.go index 2c7fbea7..2b33da58 100644 --- a/hyperstart.go +++ b/hyperstart.go @@ -440,6 +440,15 @@ func (h *hyper) startPod(pod Pod) error { // stopPod is the agent Pod stopping implementation for hyperstart. func (h *hyper) stopPod(pod Pod) error { + proxyCmd := hyperstartProxyCmd{ + cmd: hyperstart.DestroyPod, + message: nil, + } + + if _, err := h.proxy.sendCmd(proxyCmd); err != nil { + return err + } + return nil } From eeacf8b837c114b2dfeaf880d59bf69c2e67bf6b Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Thu, 16 Nov 2017 12:01:13 -0800 Subject: [PATCH 2/4] qemu: Fix disconnectCh issue and simplify it The fact that waitPod() implementation for QEMU tries to connect to the VM from a loop, means that every time QMPStart() returns, the disconnect channel is closed. We need a new channel to be setup every time we call QMPStart() again. Also, this commit takes care of removing disconnectCh field from qmpChannel structure since this is not used. Signed-off-by: Sebastien Boeuf --- qemu.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/qemu.go b/qemu.go index 40c3c03a..c88f33ae 100644 --- a/qemu.go +++ b/qemu.go @@ -32,11 +32,10 @@ import ( ) type qmpChannel struct { - ctx context.Context - path string - disconnectCh chan struct{} - wg sync.WaitGroup - qmp *ciaoQemu.QMP + ctx context.Context + path string + wg sync.WaitGroup + qmp *ciaoQemu.QMP } // QemuState keeps Qemu's state @@ -762,7 +761,6 @@ func (q *qemu) waitPod(timeout int) error { return fmt.Errorf("Invalid timeout %ds", timeout) } - disconnectCh := make(chan struct{}) cfg := ciaoQemu.QMPConfig{Logger: newQMPLogger()} var qmp *ciaoQemu.QMP @@ -771,6 +769,7 @@ func (q *qemu) waitPod(timeout int) error { timeStart := time.Now() for { + disconnectCh := make(chan struct{}) qmp, ver, err = ciaoQemu.QMPStart(q.qmpMonitorCh.ctx, q.qmpMonitorCh.path, cfg, disconnectCh) if err == nil { break @@ -803,11 +802,11 @@ func (q *qemu) waitPod(timeout int) error { // stopPod will stop the Pod's VM. func (q *qemu) stopPod() error { cfg := ciaoQemu.QMPConfig{Logger: newQMPLogger()} - q.qmpControlCh.disconnectCh = make(chan struct{}) + disconnectCh := make(chan struct{}) const timeout = time.Duration(10) * time.Second q.Logger().Info("Stopping Pod") - qmp, _, err := ciaoQemu.QMPStart(q.qmpControlCh.ctx, q.qmpControlCh.path, cfg, q.qmpControlCh.disconnectCh) + qmp, _, err := ciaoQemu.QMPStart(q.qmpControlCh.ctx, q.qmpControlCh.path, cfg, disconnectCh) if err != nil { q.Logger().WithError(err).Error("Failed to connect to QEMU instance") return err @@ -825,7 +824,7 @@ func (q *qemu) stopPod() error { // Wait for the VM disconnection notification select { - case <-q.qmpControlCh.disconnectCh: + case <-disconnectCh: break case <-time.After(timeout): return fmt.Errorf("Did not receive the VM disconnection notification (timeout %ds)", timeout) From 60266030cbd0ab60dc8408373f620e709718b024 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Fri, 10 Nov 2017 16:13:32 -0800 Subject: [PATCH 3/4] qemu: Don't wait for the end of the VM This patch brings in a real improvement about the time it takes to delete the pod. This is because it does not wait anymore for the end of the VM. Indeed, as long as we have called into QMP to shutdown the VM, this is not Clear Containers responsibility if the VM cannot be stopped for any reason. It basically should not happen. Signed-off-by: Sebastien Boeuf --- qemu.go | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/qemu.go b/qemu.go index c88f33ae..dab0fe95 100644 --- a/qemu.go +++ b/qemu.go @@ -803,7 +803,6 @@ func (q *qemu) waitPod(timeout int) error { func (q *qemu) stopPod() error { cfg := ciaoQemu.QMPConfig{Logger: newQMPLogger()} disconnectCh := make(chan struct{}) - const timeout = time.Duration(10) * time.Second q.Logger().Info("Stopping Pod") qmp, _, err := ciaoQemu.QMPStart(q.qmpControlCh.ctx, q.qmpControlCh.path, cfg, disconnectCh) @@ -818,19 +817,7 @@ func (q *qemu) stopPod() error { return err } - if err := qmp.ExecuteQuit(q.qmpMonitorCh.ctx); err != nil { - return err - } - - // Wait for the VM disconnection notification - select { - case <-disconnectCh: - break - case <-time.After(timeout): - return fmt.Errorf("Did not receive the VM disconnection notification (timeout %ds)", timeout) - } - - return nil + return qmp.ExecuteQuit(q.qmpMonitorCh.ctx) } func (q *qemu) togglePausePod(pause bool) error { From eb39def4c12dd3f44640ba49a5aa8eed84c641c0 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Tue, 14 Nov 2017 12:10:24 -0800 Subject: [PATCH 4/4] qemu: Provide unique QMP sockets names Because the previous commit introduced some improvements by not waiting for the end of the VM, it also introduced a side effect issue. When a pod is deleted, the caller can consider this pod ID as not used anymore, meaning it can start a new pod with the same ID. The problem being the new QMP sockets will be created, but if the previous VM terminates after this action, it will remove the freshly created sockets because they will have the exact same names in the exact same directory. The way to solve this issue is to generate a unique UUID that we can concatenate with the socket name to make it unique. There are some requirements to meet about the QMP socket path length this patch is also handling. Signed-off-by: Sebastien Boeuf --- pod.go | 13 ++++------ qemu.go | 73 +++++++++++++++++++++++++++++++++------------------- qemu_test.go | 36 ++++++++++++++++++++++++-- 3 files changed, 86 insertions(+), 36 deletions(-) diff --git a/pod.go b/pod.go index 8a6ac516..28d79241 100644 --- a/pod.go +++ b/pod.go @@ -581,10 +581,6 @@ func createPod(podConfig PodConfig) (*Pod, error) { return nil, err } - if err := p.storage.storeHypervisorState(p.id, p.hypervisor.getState()); err != nil { - return nil, err - } - return p, nil } @@ -629,10 +625,6 @@ func doFetchPod(podConfig PodConfig) (*Pod, error) { wg: &sync.WaitGroup{}, } - if err := p.hypervisor.init(p); err != nil { - return nil, err - } - containers, err := newContainers(p, podConfig.Containers) if err != nil { return nil, err @@ -644,6 +636,11 @@ func doFetchPod(podConfig PodConfig) (*Pod, error) { return nil, err } + if err := p.hypervisor.init(p); err != nil { + p.storage.deletePodResources(p.id, nil) + return nil, err + } + if err := p.hypervisor.createPod(podConfig); err != nil { p.storage.deletePodResources(p.id, nil) return nil, err diff --git a/qemu.go b/qemu.go index dab0fe95..8c3d09bc 100644 --- a/qemu.go +++ b/qemu.go @@ -21,7 +21,6 @@ import ( "fmt" "os" "path/filepath" - "regexp" "strings" "sync" "time" @@ -41,6 +40,7 @@ type qmpChannel struct { // QemuState keeps Qemu's state type QemuState struct { Bridges []Bridge + UUID string } // qemu is an Hypervisor interface implementation for the Linux qemu hypervisor. @@ -82,6 +82,8 @@ const ( const qmpCapErrMsg = "Failed to negoatiate QMP capabilities" +const qmpSockPathSizeLimit = 107 + // Mapping between machine types and QEMU binary paths. var qemuPaths = map[string]string{ QemuPCLite: "/usr/bin/qemu-lite-system-x86_64", @@ -470,25 +472,6 @@ func (q *qemu) appendImage(devices []ciaoQemu.Device, podConfig PodConfig) ([]ci return devices, nil } -func (q *qemu) forceUUIDFormat(str string) string { - re := regexp.MustCompile(`[^[0-9,a-f,A-F]]*`) - hexStr := re.ReplaceAllLiteralString(str, ``) - - slice := []byte(hexStr) - sliceLen := len(slice) - - var uuidSlice uuid.UUID - uuidLen := len(uuidSlice) - - if sliceLen > uuidLen { - copy(uuidSlice[:], slice[:uuidLen]) - } else { - copy(uuidSlice[:], slice) - } - - return uuidSlice.String() -} - func (q *qemu) getMachine(name string) (ciaoQemu.Machine, error) { for _, m := range supportedQemuMachines { if m.Type == name { @@ -542,16 +525,23 @@ func (q *qemu) init(pod *Pod) error { q.config = pod.config.HypervisorConfig q.pod = pod - if err = pod.storage.fetchHypervisorState(pod.id, &q.state); err != nil { + if err := pod.storage.fetchHypervisorState(pod.id, &q.state); err != nil { q.Logger().Debug("Creating bridges") q.state.Bridges = NewBridges(q.config.DefaultBridges, q.config.HypervisorMachineType) + + q.Logger().Debug("Creating UUID") + q.state.UUID = uuid.Generate().String() + + if err := pod.storage.storeHypervisorState(pod.id, q.state); err != nil { + return err + } } - if err = q.buildPath(); err != nil { + if err := q.buildPath(); err != nil { return err } - if err = q.buildKernelParams(q.config); err != nil { + if err := q.buildKernelParams(q.config); err != nil { return err } @@ -613,6 +603,23 @@ func (q *qemu) setMemoryResources(podConfig PodConfig) (ciaoQemu.Memory, error) return memory, nil } +func (q *qemu) qmpSocketPath(socketName string) (string, error) { + parentDirPath := filepath.Join(runStoragePath, q.pod.id) + if len(parentDirPath) > qmpSockPathSizeLimit { + return "", fmt.Errorf("Parent directory path %q is too long "+ + "(%d characters), could not add any path for the QMP socket", + parentDirPath, len(parentDirPath)) + } + + path := fmt.Sprintf("%s/%s-%s", parentDirPath, q.state.UUID, socketName) + + if len(path) > qmpSockPathSizeLimit { + return path[:qmpSockPathSizeLimit], nil + } + + return path, nil +} + // createPod is the Hypervisor pod creation implementation for ciaoQemu. func (q *qemu) createPod(podConfig PodConfig) error { var devices []ciaoQemu.Device @@ -668,14 +675,28 @@ func (q *qemu) createPod(podConfig PodConfig) error { DriftFix: "slew", } + if q.state.UUID == "" { + return fmt.Errorf("UUID should not be empty") + } + + monitorSockPath, err := q.qmpSocketPath(monitorSocket) + if err != nil { + return err + } + q.qmpMonitorCh = qmpChannel{ ctx: context.Background(), - path: fmt.Sprintf("%s/%s/%s", runStoragePath, podConfig.ID, monitorSocket), + path: monitorSockPath, + } + + controlSockPath, err := q.qmpSocketPath(controlSocket) + if err != nil { + return err } q.qmpControlCh = qmpChannel{ ctx: context.Background(), - path: fmt.Sprintf("%s/%s/%s", runStoragePath, podConfig.ID, controlSocket), + path: controlSockPath, } qmpSockets := []ciaoQemu.QMPSocket{ @@ -717,7 +738,7 @@ func (q *qemu) createPod(podConfig PodConfig) error { qemuConfig := ciaoQemu.Config{ Name: fmt.Sprintf("pod-%s", podConfig.ID), - UUID: q.forceUUIDFormat(podConfig.ID), + UUID: q.state.UUID, Path: q.path, Ctx: q.qmpMonitorCh.ctx, Machine: machine, diff --git a/qemu_test.go b/qemu_test.go index ea254e4d..d2ab7663 100644 --- a/qemu_test.go +++ b/qemu_test.go @@ -384,8 +384,17 @@ func TestQemuInit(t *testing.T) { }, } - err := q.init(pod) - if err != nil { + // Create parent dir path for hypervisor.json + parentDir := filepath.Join(runStoragePath, pod.id) + if err := os.MkdirAll(parentDir, dirMode); err != nil { + t.Fatalf("Could not create parent directory %s: %v", parentDir, err) + } + + if err := q.init(pod); err != nil { + t.Fatal(err) + } + + if err := os.RemoveAll(parentDir); err != nil { t.Fatal(err) } @@ -405,6 +414,29 @@ func TestQemuInit(t *testing.T) { } } +func TestQemuInitMissingParentDirFail(t *testing.T) { + qemuConfig := newQemuConfig() + q := &qemu{} + + pod := &Pod{ + id: "testPod", + storage: &filesystem{}, + config: &PodConfig{ + HypervisorConfig: qemuConfig, + }, + } + + // Ensure parent dir path for hypervisor.json does not exist. + parentDir := filepath.Join(runStoragePath, pod.id) + if err := os.RemoveAll(parentDir); err != nil { + t.Fatal(err) + } + + if err := q.init(pod); err == nil { + t.Fatal("Qemu init() expected to fail because of missing parent directory for storage") + } +} + func TestQemuSetCPUResources(t *testing.T) { vcpus := 1