Skip to content

Commit 129197c

Browse files
sipsmasipsma
authored
Merge pull request firecracker-microvm#223 from sipsma/fifoname
Give task and exec FIFO dirs unique names in VM.
2 parents 977e620 + dd471ba commit 129197c

File tree

2 files changed

+85
-42
lines changed

2 files changed

+85
-42
lines changed

‎agent/service.go‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -113,11 +113,17 @@ func unmarshalExtraData(marshalled *types.Any) (*proto.ExtraData, error) {
113113
return extraData, nil
114114
}
115115

116+
func fifoName(taskID, execID string) string {
117+
return fmt.Sprintf("exec-%s-task-%s", execID, taskID)
118+
}
119+
116120
// Create creates a new initial process and container using runc
117121
func (ts *TaskService) Create(requestCtx context.Context, req *taskAPI.CreateTaskRequest) (*taskAPI.CreateTaskResponse, error) {
118122
defer logPanicAndDie(log.G(requestCtx))
123+
taskID := req.ID
124+
execID := "" // the exec ID of the initial process in a task is an empty string by containerd convention
119125

120-
logger := log.G(requestCtx).WithFields(logrus.Fields{"id": req.ID, "bundle": req.Bundle})
126+
logger := log.G(requestCtx).WithField("TaskID", taskID).WithField("ExecID", execID)
121127
logger.Info("create")
122128

123129
extraData, err := unmarshalExtraData(req.Options)
@@ -129,7 +135,7 @@ func (ts *TaskService) Create(requestCtx context.Context, req *taskAPI.CreateTas
129135
req.Options = extraData.RuncOptions
130136

131137
// Override the bundle dir and rootfs paths, which were set on the Host and thus not valid here in the Guest
132-
bundleDir := bundle.Dir(filepath.Join(containerRootDir, req.ID))
138+
bundleDir := bundle.Dir(filepath.Join(containerRootDir, taskID))
133139
err = bundleDir.Create()
134140
if err != nil {
135141
return nil, errors.Wrap(err, "failed to create bundle dir")
@@ -187,7 +193,7 @@ func (ts *TaskService) Create(requestCtx context.Context, req *taskAPI.CreateTas
187193
ioConnectorSet = vm.NewNullIOProxy()
188194
} else {
189195
// Override the incoming stdio FIFOs, which have paths from the host that we can't use
190-
fifoSet, err := cio.NewFIFOSetInDir(bundleDir.RootPath(), req.ID, req.Terminal)
196+
fifoSet, err := cio.NewFIFOSetInDir(bundleDir.RootPath(), fifoName(taskID, execID), req.Terminal)
191197
if err != nil {
192198
logger.WithError(err).Error("failed opening stdio FIFOs")
193199
return nil, errors.Wrap(err, "failed to open stdio FIFOs")
@@ -362,7 +368,11 @@ func (ts *TaskService) Kill(requestCtx context.Context, req *taskAPI.KillRequest
362368
// Exec runs an additional process inside the container
363369
func (ts *TaskService) Exec(requestCtx context.Context, req *taskAPI.ExecProcessRequest) (*types.Empty, error) {
364370
defer logPanicAndDie(log.G(requestCtx))
365-
logger := log.G(requestCtx).WithField("TaskID", req.ID).WithField("ExecID", req.ExecID)
371+
372+
taskID := req.ID
373+
execID := req.ExecID
374+
375+
logger := log.G(requestCtx).WithField("TaskID", taskID).WithField("ExecID", execID)
366376
logger.Debug("exec")
367377

368378
extraData, err := unmarshalExtraData(req.Spec)
@@ -373,15 +383,15 @@ func (ts *TaskService) Exec(requestCtx context.Context, req *taskAPI.ExecProcess
373383
// Just provide runc the options it knows about, not our wrapper
374384
req.Spec = extraData.RuncOptions
375385

376-
bundleDir := bundle.Dir(filepath.Join(containerRootDir, req.ID))
386+
bundleDir := bundle.Dir(filepath.Join(containerRootDir, taskID))
377387

378388
var ioConnectorSet vm.IOProxy
379389

380390
if vm.IsAgentOnlyIO(req.Stdout, logger) {
381391
ioConnectorSet = vm.NewNullIOProxy()
382392
} else {
383393
// Override the incoming stdio FIFOs, which have paths from the host that we can't use
384-
fifoSet, err := cio.NewFIFOSetInDir(bundleDir.RootPath(), req.ExecID, req.Terminal)
394+
fifoSet, err := cio.NewFIFOSetInDir(bundleDir.RootPath(), fifoName(taskID, execID), req.Terminal)
385395
if err != nil {
386396
logger.WithError(err).Error("failed opening stdio FIFOs")
387397
return nil, errors.Wrap(err, "failed to open stdio FIFOs")

‎runtime/service_integ_test.go‎

Lines changed: 69 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,6 @@ func TestMultipleVMs_Isolated(t *testing.T) {
267267
go func(containerID int) {
268268
defer containerWg.Done()
269269
containerName := fmt.Sprintf("container-%d-%d", vmID, containerID)
270-
execName := fmt.Sprintf("exec-%d-%d", vmID, containerID)
271270
snapshotName := fmt.Sprintf("snapshot-%d-%d", vmID, containerID)
272271

273272
// spawn a container that just prints the VM's eth0 mac address (which we have set uniquely per VM)
@@ -297,47 +296,82 @@ func TestMultipleVMs_Isolated(t *testing.T) {
297296
taskExitCh, err := newTask.Wait(ctx)
298297
require.NoError(t, err, "failed to wait on task for container %s", containerName)
299298

300-
var execStdout bytes.Buffer
301-
var execStderr bytes.Buffer
302-
303-
newExec, err := newTask.Exec(ctx, execName, &specs.Process{
304-
Args: []string{"/bin/readlink", "/proc/self/ns/mnt"},
305-
Cwd: "/",
306-
}, cio.NewCreator(cio.WithStreams(nil, &execStdout, &execStderr)))
307-
require.NoError(t, err, "failed to exec %s", execName)
308-
309-
execExitCh, err := newExec.Wait(ctx)
310-
require.NoError(t, err, "failed to wait on exec %s", execName)
311-
312299
err = newTask.Start(ctx)
313300
require.NoError(t, err, "failed to start task for container %s", containerName)
314301

315-
err = newExec.Start(ctx)
316-
require.NoError(t, err, "failed to start exec %s", execName)
317-
318-
// First wait for the exec to exit, getting what it saw as its mnt namespace from the stdout
319-
// so that we can make sure its the same namespace as the task
320-
select {
321-
case exitStatus := <-execExitCh:
322-
_, err = client.TaskService().DeleteProcess(ctx, &tasks.DeleteProcessRequest{
323-
ContainerID: containerName,
324-
ExecID: execName,
325-
})
326-
require.NoError(t, err, "failed to delete exec %q", execName)
327-
328-
// if there was anything on stderr, print it to assist debugging
329-
stderrOutput := execStderr.String()
330-
if len(stderrOutput) != 0 {
331-
fmt.Printf("stderr output from exec %q: %q", execName, stderrOutput)
302+
// Create a few execs for the task, including one with the same ID as the taskID (to provide
303+
// regression coverage for a bug related to using the same task and exec ID).
304+
//
305+
// Save each of their stdout buffers, which will later be compared to ensure they each have
306+
// the same output.
307+
//
308+
// The output of the exec is the mount namespace in which it found itself executing. This
309+
// will be compared with the mount namespace the task is executing to ensure they are the same.
310+
// This is a rudimentary way of asserting that each exec was created in the expected task.
311+
execIDs := []string{fmt.Sprintf("exec-%d-%d", vmID, containerID), containerName}
312+
execStdouts := make(chan string, len(execIDs))
313+
var execWg sync.WaitGroup
314+
for _, execID := range execIDs {
315+
execWg.Add(1)
316+
go func(execID string) {
317+
defer execWg.Done()
318+
var execStdout bytes.Buffer
319+
var execStderr bytes.Buffer
320+
321+
newExec, err := newTask.Exec(ctx, execID, &specs.Process{
322+
Args: []string{"/bin/readlink", "/proc/self/ns/mnt"},
323+
Cwd: "/",
324+
}, cio.NewCreator(cio.WithStreams(nil, &execStdout, &execStderr)))
325+
require.NoError(t, err, "failed to exec %s", execID)
326+
327+
execExitCh, err := newExec.Wait(ctx)
328+
require.NoError(t, err, "failed to wait on exec %s", execID)
329+
330+
err = newExec.Start(ctx)
331+
require.NoError(t, err, "failed to start exec %s", execID)
332+
333+
select {
334+
case exitStatus := <-execExitCh:
335+
_, err = client.TaskService().DeleteProcess(ctx, &tasks.DeleteProcessRequest{
336+
ContainerID: containerName,
337+
ExecID: execID,
338+
})
339+
require.NoError(t, err, "failed to delete exec %q", execID)
340+
341+
// if there was anything on stderr, print it to assist debugging
342+
stderrOutput := execStderr.String()
343+
if len(stderrOutput) != 0 {
344+
fmt.Printf("stderr output from exec %q: %q", execID, stderrOutput)
345+
}
346+
347+
mntNS := strings.TrimSpace(execStdout.String())
348+
require.NotEmptyf(t, mntNS, "no stdout output for task %q exec %q", containerName, execID)
349+
execStdouts <- mntNS
350+
351+
require.Equal(t, uint32(0), exitStatus.ExitCode())
352+
case <-ctx.Done():
353+
require.Fail(t, "context cancelled",
354+
"context cancelled while waiting for exec %s to exit, err: %v", execID, ctx.Err())
355+
}
356+
}(execID)
357+
}
358+
execWg.Wait()
359+
close(execStdouts)
360+
361+
// Verify each exec had the same stdout and use that value as the mount namespace that will be compared
362+
// against that of the task below.
363+
var execMntNS string
364+
for execStdout := range execStdouts {
365+
if execMntNS == "" {
366+
// This is the first iteration of loop; we do a check that execStdout is not "" via require.NotEmptyf
367+
// in the execID loop above.
368+
execMntNS = execStdout
332369
}
333370

334-
require.Equal(t, uint32(0), exitStatus.ExitCode())
335-
case <-ctx.Done():
336-
require.Fail(t, "context cancelled",
337-
"context cancelled while waiting for exec %s to exit, err: %v", execName, ctx.Err())
371+
require.Equal(t, execMntNS, execStdout, "execs in same task unexpectedly have different outputs")
338372
}
339373

340-
// Now kill the task and verify it was in the right VM and has the same mnt namespace as its exec
374+
// Now kill the task and verify it was in the right VM and has the same mnt namespace as its execs
341375
err = newTask.Kill(ctx, syscall.SIGKILL)
342376
require.NoError(t, err, "failed to kill task %q", containerName)
343377

@@ -361,7 +395,6 @@ func TestMultipleVMs_Isolated(t *testing.T) {
361395
require.Equal(t, vmIDtoMacAddr(uint(vmID)), printedVMID, "unexpected VMID output from container %q", containerName)
362396

363397
taskMntNS := strings.TrimSpace(stdoutLines[1])
364-
execMntNS := strings.TrimSpace(execStdout.String())
365398
require.Equal(t, execMntNS, taskMntNS, "unexpected mnt NS output from container %q", containerName)
366399

367400
case <-ctx.Done():

0 commit comments

Comments
 (0)