From 94df315de82212c28357652a083c24d2accc29f5 Mon Sep 17 00:00:00 2001 From: Mike Brown Date: Thu, 22 Mar 2018 14:49:21 -0500 Subject: [PATCH 1/2] adds volatile state directory to the fs plan for cntrs/pods/fifo Signed-off-by: Mike Brown --- cri.go | 6 +- docs/config.md | 2 +- docs/crictl.md | 3 +- pkg/config/config.go | 2 + pkg/server/container_create.go | 28 ++++++-- pkg/server/container_create_test.go | 20 +++--- pkg/server/container_execsync.go | 4 +- pkg/server/container_remove.go | 7 +- pkg/server/helpers.go | 37 ++++++---- pkg/server/restart.go | 103 +++++++++++++--------------- pkg/server/sandbox_remove.go | 9 ++- pkg/server/sandbox_run.go | 34 ++++++--- pkg/server/sandbox_run_test.go | 31 ++++++--- pkg/server/sandbox_stop.go | 5 +- pkg/server/service_test.go | 6 +- 15 files changed, 178 insertions(+), 119 deletions(-) diff --git a/cri.go b/cri.go index 57320c32a..b51b284de 100644 --- a/cri.go +++ b/cri.go @@ -63,13 +63,11 @@ func initCRIService(ic *plugin.InitContext) (interface{}, error) { ctx := ic.Context pluginConfig := ic.Config.(*criconfig.PluginConfig) c := criconfig.Config{ - PluginConfig: *pluginConfig, - // This is a hack. We assume that containerd root directory - // is one level above plugin directory. - // TODO(random-liu): Expose containerd config to plugin. + PluginConfig: *pluginConfig, ContainerdRootDir: filepath.Dir(ic.Root), ContainerdEndpoint: ic.Address, RootDir: ic.Root, + StateDir: ic.State, } log.G(ctx).Infof("Start cri plugin with config %+v", c) diff --git a/docs/config.md b/docs/config.md index 9b377251c..91a8b910d 100644 --- a/docs/config.md +++ b/docs/config.md @@ -67,4 +67,4 @@ The explanation and default value of each configuration item are as follows: [plugins.cri.registry.mirrors] [plugins.cri.registry.mirrors."docker.io"] endpoint = ["https://registry-1.docker.io", ] -``` \ No newline at end of file +``` diff --git a/docs/crictl.md b/docs/crictl.md index 8e7b7cf00..8d0578af2 100644 --- a/docs/crictl.md +++ b/docs/crictl.md @@ -189,7 +189,8 @@ $ crictl info "statsCollectPeriod": 10, "containerdRootDir": "/var/lib/containerd", "containerdEndpoint": "/run/containerd/containerd.sock", - "rootDir": "/var/lib/containerd/io.containerd.grpc.v1.cri" + "rootDir": "/var/lib/containerd/io.containerd.grpc.v1.cri", + "stateDir": "/run/containerd/io.containerd.grpc.v1.cri", }, "golang": "go1.10" } diff --git a/pkg/config/config.go b/pkg/config/config.go index c4de6ee91..5c24faf4d 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -96,6 +96,8 @@ type Config struct { // RootDir is the root directory path for managing cri plugin files // (metadata checkpoint etc.) RootDir string `json:"rootDir"` + // StateDir is the root directory path for managing volatile pod/container data + StateDir string `json:"stateDir"` } // DefaultConfig returns default configurations of cri plugin. diff --git a/pkg/server/container_create.go b/pkg/server/container_create.go index 61037c2cb..be9a79890 100644 --- a/pkg/server/container_create.go +++ b/pkg/server/container_create.go @@ -134,7 +134,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta logrus.Debugf("Use OCI %+v for container %q", ociRuntime, id) // Create container root directory. - containerRootDir := getContainerRootDir(c.config.RootDir, id) + containerRootDir := c.getContainerRootDir(id) if err = c.os.MkdirAll(containerRootDir, 0755); err != nil { return nil, errors.Wrapf(err, "failed to create container root directory %q", containerRootDir) @@ -148,12 +148,26 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta } } }() + volatileContainerRootDir := c.getVolatileContainerRootDir(id) + if err = c.os.MkdirAll(volatileContainerRootDir, 0755); err != nil { + return nil, errors.Wrapf(err, "failed to create volatile container root directory %q", + volatileContainerRootDir) + } + defer func() { + if retErr != nil { + // Cleanup the volatile container root directory. + if err = c.os.RemoveAll(volatileContainerRootDir); err != nil { + logrus.WithError(err).Errorf("Failed to remove volatile container root directory %q", + volatileContainerRootDir) + } + } + }() // Create container volumes mounts. volumeMounts := c.generateVolumeMounts(containerRootDir, config.GetMounts(), &image.ImageSpec.Config) // Generate container runtime spec. - mounts := c.generateContainerMounts(getSandboxRootDir(c.config.RootDir, sandboxID), config) + mounts := c.generateContainerMounts(sandboxID, config) spec, err := c.generateContainerSpec(id, sandboxID, sandboxPid, config, sandboxConfig, &image.ImageSpec.Config, append(mounts, volumeMounts...)) if err != nil { @@ -188,7 +202,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta } containerIO, err := cio.NewContainerIO(id, - cio.WithNewFIFOs(containerRootDir, config.GetTty(), config.GetStdin())) + cio.WithNewFIFOs(volatileContainerRootDir, config.GetTty(), config.GetStdin())) if err != nil { return nil, errors.Wrap(err, "failed to create container io") } @@ -416,13 +430,13 @@ func (c *criService) generateVolumeMounts(containerRootDir string, criMounts []* // generateContainerMounts sets up necessary container mounts including /dev/shm, /etc/hosts // and /etc/resolv.conf. -func (c *criService) generateContainerMounts(sandboxRootDir string, config *runtime.ContainerConfig) []*runtime.Mount { +func (c *criService) generateContainerMounts(sandboxID string, config *runtime.ContainerConfig) []*runtime.Mount { var mounts []*runtime.Mount securityContext := config.GetLinux().GetSecurityContext() if !isInCRIMounts(etcHosts, config.GetMounts()) { mounts = append(mounts, &runtime.Mount{ ContainerPath: etcHosts, - HostPath: getSandboxHosts(sandboxRootDir), + HostPath: c.getSandboxHosts(sandboxID), Readonly: securityContext.GetReadonlyRootfs(), }) } @@ -432,13 +446,13 @@ func (c *criService) generateContainerMounts(sandboxRootDir string, config *runt if !isInCRIMounts(resolvConfPath, config.GetMounts()) { mounts = append(mounts, &runtime.Mount{ ContainerPath: resolvConfPath, - HostPath: getResolvPath(sandboxRootDir), + HostPath: c.getResolvPath(sandboxID), Readonly: securityContext.GetReadonlyRootfs(), }) } if !isInCRIMounts(devShm, config.GetMounts()) { - sandboxDevShm := getSandboxDevShm(sandboxRootDir) + sandboxDevShm := c.getSandboxDevShm(sandboxID) if securityContext.GetNamespaceOptions().GetIpc() == runtime.NamespaceMode_NODE { sandboxDevShm = devShm } diff --git a/pkg/server/container_create_test.go b/pkg/server/container_create_test.go index 73b346fb2..1baeb5d47 100644 --- a/pkg/server/container_create_test.go +++ b/pkg/server/container_create_test.go @@ -497,7 +497,7 @@ func TestGenerateVolumeMounts(t *testing.T) { } func TestGenerateContainerMounts(t *testing.T) { - testSandboxRootDir := "test-sandbox-root" + const testSandboxID = "test-id" for desc, test := range map[string]struct { criMounts []*runtime.Mount securityContext *runtime.LinuxContainerSecurityContext @@ -510,17 +510,17 @@ func TestGenerateContainerMounts(t *testing.T) { expectedMounts: []*runtime.Mount{ { ContainerPath: "/etc/hosts", - HostPath: testSandboxRootDir + "/hosts", + HostPath: filepath.Join(testRootDir, sandboxesDir, testSandboxID, "hosts"), Readonly: true, }, { ContainerPath: resolvConfPath, - HostPath: testSandboxRootDir + "/resolv.conf", + HostPath: filepath.Join(testRootDir, sandboxesDir, testSandboxID, "resolv.conf"), Readonly: true, }, { ContainerPath: "/dev/shm", - HostPath: testSandboxRootDir + "/shm", + HostPath: filepath.Join(testStateDir, sandboxesDir, testSandboxID, "shm"), Readonly: false, }, }, @@ -530,17 +530,17 @@ func TestGenerateContainerMounts(t *testing.T) { expectedMounts: []*runtime.Mount{ { ContainerPath: "/etc/hosts", - HostPath: testSandboxRootDir + "/hosts", + HostPath: filepath.Join(testRootDir, sandboxesDir, testSandboxID, "hosts"), Readonly: false, }, { ContainerPath: resolvConfPath, - HostPath: testSandboxRootDir + "/resolv.conf", + HostPath: filepath.Join(testRootDir, sandboxesDir, testSandboxID, "resolv.conf"), Readonly: false, }, { ContainerPath: "/dev/shm", - HostPath: testSandboxRootDir + "/shm", + HostPath: filepath.Join(testStateDir, sandboxesDir, testSandboxID, "shm"), Readonly: false, }, }, @@ -552,12 +552,12 @@ func TestGenerateContainerMounts(t *testing.T) { expectedMounts: []*runtime.Mount{ { ContainerPath: "/etc/hosts", - HostPath: testSandboxRootDir + "/hosts", + HostPath: filepath.Join(testRootDir, sandboxesDir, testSandboxID, "hosts"), Readonly: false, }, { ContainerPath: resolvConfPath, - HostPath: testSandboxRootDir + "/resolv.conf", + HostPath: filepath.Join(testRootDir, sandboxesDir, testSandboxID, "resolv.conf"), Readonly: false, }, { @@ -597,7 +597,7 @@ func TestGenerateContainerMounts(t *testing.T) { }, } c := newTestCRIService() - mounts := c.generateContainerMounts(testSandboxRootDir, config) + mounts := c.generateContainerMounts(testSandboxID, config) assert.Equal(t, test.expectedMounts, mounts, desc) } } diff --git a/pkg/server/container_execsync.go b/pkg/server/container_execsync.go index bf4efffd5..cf7f1e668 100644 --- a/pkg/server/container_execsync.go +++ b/pkg/server/container_execsync.go @@ -116,12 +116,12 @@ func (c *criService) execInContainer(ctx context.Context, id string, opts execOp } execID := util.GenerateID() logrus.Debugf("Generated exec id %q for container %q", execID, id) - rootDir := getContainerRootDir(c.config.RootDir, id) + volatileRootDir := c.getVolatileContainerRootDir(id) var execIO *cio.ExecIO process, err := task.Exec(ctx, execID, pspec, func(id string) (containerdio.IO, error) { var err error - execIO, err = cio.NewExecIO(id, rootDir, opts.tty, opts.stdin != nil) + execIO, err = cio.NewExecIO(id, volatileRootDir, opts.tty, opts.stdin != nil) return execIO, err }, ) diff --git a/pkg/server/container_remove.go b/pkg/server/container_remove.go index 83fec6522..e05d79618 100644 --- a/pkg/server/container_remove.go +++ b/pkg/server/container_remove.go @@ -76,11 +76,16 @@ func (c *criService) RemoveContainer(ctx context.Context, r *runtime.RemoveConta return nil, errors.Wrapf(err, "failed to delete container checkpoint for %q", id) } - containerRootDir := getContainerRootDir(c.config.RootDir, id) + containerRootDir := c.getContainerRootDir(id) if err := system.EnsureRemoveAll(containerRootDir); err != nil { return nil, errors.Wrapf(err, "failed to remove container root directory %q", containerRootDir) } + volatileContainerRootDir := c.getVolatileContainerRootDir(id) + if err := system.EnsureRemoveAll(volatileContainerRootDir); err != nil { + return nil, errors.Wrapf(err, "failed to remove volatile container root directory %q", + volatileContainerRootDir) + } c.containerStore.Delete(id) diff --git a/pkg/server/helpers.go b/pkg/server/helpers.go index 06e43a257..b09f28151 100644 --- a/pkg/server/helpers.go +++ b/pkg/server/helpers.go @@ -156,29 +156,42 @@ func getCgroupsPath(cgroupsParent, id string, systemdCgroup bool) string { } // getSandboxRootDir returns the root directory for managing sandbox files, -// e.g. named pipes. -func getSandboxRootDir(rootDir, id string) string { - return filepath.Join(rootDir, sandboxesDir, id) +// e.g. hosts files. +func (c *criService) getSandboxRootDir(id string) string { + return filepath.Join(c.config.RootDir, sandboxesDir, id) } -// getContainerRootDir returns the root directory for managing container files. -func getContainerRootDir(rootDir, id string) string { - return filepath.Join(rootDir, containersDir, id) +// getVolatileSandboxRootDir returns the root directory for managing volatile sandbox files, +// e.g. named pipes. +func (c *criService) getVolatileSandboxRootDir(id string) string { + return filepath.Join(c.config.StateDir, sandboxesDir, id) +} + +// getContainerRootDir returns the root directory for managing container files, +// e.g. state checkpoint. +func (c *criService) getContainerRootDir(id string) string { + return filepath.Join(c.config.RootDir, containersDir, id) +} + +// getVolatileContainerRootDir returns the root directory for managing volatile container files, +// e.g. named pipes. +func (c *criService) getVolatileContainerRootDir(id string) string { + return filepath.Join(c.config.StateDir, containersDir, id) } // getSandboxHosts returns the hosts file path inside the sandbox root directory. -func getSandboxHosts(sandboxRootDir string) string { - return filepath.Join(sandboxRootDir, "hosts") +func (c *criService) getSandboxHosts(id string) string { + return filepath.Join(c.getSandboxRootDir(id), "hosts") } // getResolvPath returns resolv.conf filepath for specified sandbox. -func getResolvPath(sandboxRoot string) string { - return filepath.Join(sandboxRoot, "resolv.conf") +func (c *criService) getResolvPath(id string) string { + return filepath.Join(c.getSandboxRootDir(id), "resolv.conf") } // getSandboxDevShm returns the shm file path inside the sandbox root directory. -func getSandboxDevShm(sandboxRootDir string) string { - return filepath.Join(sandboxRootDir, "shm") +func (c *criService) getSandboxDevShm(id string) string { + return filepath.Join(c.getVolatileSandboxRootDir(id), "shm") } // getNetworkNamespace returns the network namespace of a process. diff --git a/pkg/server/restart.go b/pkg/server/restart.go index 9912a5854..d37f8a287 100644 --- a/pkg/server/restart.go +++ b/pkg/server/restart.go @@ -78,8 +78,9 @@ func (c *criService) recover(ctx context.Context) error { return errors.Wrap(err, "failed to list containers") } for _, container := range containers { - containerDir := getContainerRootDir(c.config.RootDir, container.ID()) - cntr, err := loadContainer(ctx, container, containerDir) + containerDir := c.getContainerRootDir(container.ID()) + volatileContainerDir := c.getVolatileContainerRootDir(container.ID()) + cntr, err := loadContainer(ctx, container, containerDir, volatileContainerDir) if err != nil { logrus.WithError(err).Errorf("Failed to load container %q", container.ID()) continue @@ -113,21 +114,42 @@ func (c *criService) recover(ctx context.Context) error { // we can't even get metadata, we should cleanup orphaned sandbox/container directories // with best effort. - // Cleanup orphaned sandbox directories without corresponding containerd container. - if err := cleanupOrphanedSandboxDirs(sandboxes, filepath.Join(c.config.RootDir, "sandboxes")); err != nil { - return errors.Wrap(err, "failed to cleanup orphaned sandbox directories") + // Cleanup orphaned sandbox and container directories without corresponding containerd container. + for _, cleanup := range []struct { + cntrs []containerd.Container + base string + errMsg string + }{ + { + cntrs: sandboxes, + base: filepath.Join(c.config.RootDir, sandboxesDir), + errMsg: "failed to cleanup orphaned sandbox directories", + }, + { + cntrs: sandboxes, + base: filepath.Join(c.config.StateDir, sandboxesDir), + errMsg: "failed to cleanup orphaned volatile sandbox directories", + }, + { + cntrs: containers, + base: filepath.Join(c.config.RootDir, containersDir), + errMsg: "failed to cleanup orphaned container directories", + }, + { + cntrs: containers, + base: filepath.Join(c.config.StateDir, containersDir), + errMsg: "failed to cleanup orphaned volatile container directories", + }, + } { + if err := cleanupOrphanedIDDirs(cleanup.cntrs, cleanup.base); err != nil { + return errors.Wrap(err, cleanup.errMsg) + } } - - // Cleanup orphaned container directories without corresponding containerd container. - if err := cleanupOrphanedContainerDirs(containers, filepath.Join(c.config.RootDir, "containers")); err != nil { - return errors.Wrap(err, "failed to cleanup orphaned container directories") - } - return nil } // loadContainer loads container from containerd and status checkpoint. -func loadContainer(ctx context.Context, cntr containerd.Container, containerDir string) (containerstore.Container, error) { +func loadContainer(ctx context.Context, cntr containerd.Container, containerDir, volatileContainerDir string) (containerstore.Container, error) { id := cntr.ID() var container containerstore.Container // Load container metadata. @@ -197,7 +219,7 @@ func loadContainer(ctx context.Context, cntr containerd.Container, containerDir // containerd got restarted during that. In that case, we still // treat the container as `CREATED`. containerIO, err = cio.NewContainerIO(id, - cio.WithNewFIFOs(containerDir, meta.Config.GetTty(), meta.Config.GetStdin()), + cio.WithNewFIFOs(volatileContainerDir, meta.Config.GetTty(), meta.Config.GetStdin()), ) if err != nil { return container, errors.Wrap(err, "failed to create container io") @@ -448,59 +470,30 @@ func loadImages(ctx context.Context, cImages []containerd.Image, return images, nil } -func cleanupOrphanedSandboxDirs(cntrs []containerd.Container, sandboxesRoot string) error { - // Cleanup orphaned sandbox directories. - dirs, err := ioutil.ReadDir(sandboxesRoot) +func cleanupOrphanedIDDirs(cntrs []containerd.Container, base string) error { + // Cleanup orphaned id directories. + dirs, err := ioutil.ReadDir(base) if err != nil && !os.IsNotExist(err) { - return errors.Wrapf(err, "failed to read pod sandboxes directory %q", sandboxesRoot) + return errors.Wrap(err, "failed to read base directory") } - cntrsMap := make(map[string]containerd.Container) + idsMap := make(map[string]containerd.Container) for _, cntr := range cntrs { - cntrsMap[cntr.ID()] = cntr + idsMap[cntr.ID()] = cntr } for _, d := range dirs { if !d.IsDir() { - logrus.Warnf("Invalid file %q found in pod sandboxes directory", d.Name()) + logrus.Warnf("Invalid file %q found in base directory %q", d.Name(), base) continue } - if _, ok := cntrsMap[d.Name()]; ok { - // Do not remove sandbox directory if corresponding container is found. + if _, ok := idsMap[d.Name()]; ok { + // Do not remove id directory if corresponding container is found. continue } - sandboxDir := filepath.Join(sandboxesRoot, d.Name()) - if err := system.EnsureRemoveAll(sandboxDir); err != nil { - logrus.WithError(err).Warnf("Failed to remove pod sandbox directory %q", sandboxDir) + dir := filepath.Join(base, d.Name()) + if err := system.EnsureRemoveAll(dir); err != nil { + logrus.WithError(err).Warnf("Failed to remove id directory %q", dir) } else { - logrus.Debugf("Cleanup orphaned pod sandbox directory %q", sandboxDir) - } - } - return nil -} - -func cleanupOrphanedContainerDirs(cntrs []containerd.Container, containersRoot string) error { - // Cleanup orphaned container directories. - dirs, err := ioutil.ReadDir(containersRoot) - if err != nil && !os.IsNotExist(err) { - return errors.Wrapf(err, "failed to read containers directory %q", containersRoot) - } - cntrsMap := make(map[string]containerd.Container) - for _, cntr := range cntrs { - cntrsMap[cntr.ID()] = cntr - } - for _, d := range dirs { - if !d.IsDir() { - logrus.Warnf("Invalid file %q found in containers directory", d.Name()) - continue - } - if _, ok := cntrsMap[d.Name()]; ok { - // Do not remove container directory if corresponding container is found. - continue - } - containerDir := filepath.Join(containersRoot, d.Name()) - if err := system.EnsureRemoveAll(containerDir); err != nil { - logrus.WithError(err).Warnf("Failed to remove container directory %q", containerDir) - } else { - logrus.Debugf("Cleanup orphaned container directory %q", containerDir) + logrus.Debugf("Cleanup orphaned id directory %q", dir) } } return nil diff --git a/pkg/server/sandbox_remove.go b/pkg/server/sandbox_remove.go index 4f8e0a858..3d3849a8d 100644 --- a/pkg/server/sandbox_remove.go +++ b/pkg/server/sandbox_remove.go @@ -72,12 +72,17 @@ func (c *criService) RemovePodSandbox(ctx context.Context, r *runtime.RemovePodS } } - // Cleanup the sandbox root directory. - sandboxRootDir := getSandboxRootDir(c.config.RootDir, id) + // Cleanup the sandbox root directories. + sandboxRootDir := c.getSandboxRootDir(id) if err := system.EnsureRemoveAll(sandboxRootDir); err != nil { return nil, errors.Wrapf(err, "failed to remove sandbox root directory %q", sandboxRootDir) } + volatileSandboxRootDir := c.getVolatileSandboxRootDir(id) + if err := system.EnsureRemoveAll(volatileSandboxRootDir); err != nil { + return nil, errors.Wrapf(err, "failed to remove volatile sandbox root directory %q", + volatileSandboxRootDir) + } // Delete sandbox container. if err := sandbox.Container.Delete(ctx, containerd.WithSnapshotCleanup); err != nil { diff --git a/pkg/server/sandbox_run.go b/pkg/server/sandbox_run.go index 237290709..de0b42bee 100644 --- a/pkg/server/sandbox_run.go +++ b/pkg/server/sandbox_run.go @@ -189,8 +189,8 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox } }() - // Create sandbox container root directory. - sandboxRootDir := getSandboxRootDir(c.config.RootDir, id) + // Create sandbox container root directories. + sandboxRootDir := c.getSandboxRootDir(id) if err := c.os.MkdirAll(sandboxRootDir, 0755); err != nil { return nil, errors.Wrapf(err, "failed to create sandbox root directory %q", sandboxRootDir) @@ -204,14 +204,28 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox } } }() + volatileSandboxRootDir := c.getVolatileSandboxRootDir(id) + if err := c.os.MkdirAll(volatileSandboxRootDir, 0755); err != nil { + return nil, errors.Wrapf(err, "failed to create volatile sandbox root directory %q", + volatileSandboxRootDir) + } + defer func() { + if retErr != nil { + // Cleanup the volatile sandbox root directory. + if err := c.os.RemoveAll(volatileSandboxRootDir); err != nil { + logrus.WithError(err).Errorf("Failed to remove volatile sandbox root directory %q", + volatileSandboxRootDir) + } + } + }() // Setup sandbox /dev/shm, /etc/hosts and /etc/resolv.conf. - if err = c.setupSandboxFiles(sandboxRootDir, config); err != nil { + if err = c.setupSandboxFiles(id, config); err != nil { return nil, errors.Wrapf(err, "failed to setup sandbox files") } defer func() { if retErr != nil { - if err = c.unmountSandboxFiles(sandboxRootDir, config); err != nil { + if err = c.unmountSandboxFiles(id, config); err != nil { logrus.WithError(err).Errorf("Failed to unmount sandbox files in %q", sandboxRootDir) } @@ -398,9 +412,9 @@ func (c *criService) generateSandboxContainerSpec(id string, config *runtime.Pod // setupSandboxFiles sets up necessary sandbox files including /dev/shm, /etc/hosts // and /etc/resolv.conf. -func (c *criService) setupSandboxFiles(rootDir string, config *runtime.PodSandboxConfig) error { +func (c *criService) setupSandboxFiles(id string, config *runtime.PodSandboxConfig) error { // TODO(random-liu): Consider whether we should maintain /etc/hosts and /etc/resolv.conf in kubelet. - sandboxEtcHosts := getSandboxHosts(rootDir) + sandboxEtcHosts := c.getSandboxHosts(id) if err := c.os.CopyFile(etcHosts, sandboxEtcHosts, 0644); err != nil { return errors.Wrapf(err, "failed to generate sandbox hosts file %q", sandboxEtcHosts) } @@ -414,7 +428,7 @@ func (c *criService) setupSandboxFiles(rootDir string, config *runtime.PodSandbo return errors.Wrapf(err, "failed to parse sandbox DNSConfig %+v", dnsConfig) } } - resolvPath := getResolvPath(rootDir) + resolvPath := c.getResolvPath(id) if resolvContent == "" { // copy host's resolv.conf to resolvPath err = c.os.CopyFile(resolvConfPath, resolvPath, 0644) @@ -434,7 +448,7 @@ func (c *criService) setupSandboxFiles(rootDir string, config *runtime.PodSandbo return errors.Wrapf(err, "host %q is not available for host ipc", devShm) } } else { - sandboxDevShm := getSandboxDevShm(rootDir) + sandboxDevShm := c.getSandboxDevShm(id) if err := c.os.MkdirAll(sandboxDevShm, 0700); err != nil { return errors.Wrap(err, "failed to create sandbox shm") } @@ -475,9 +489,9 @@ func parseDNSOptions(servers, searches, options []string) (string, error) { // remove these files. Unmount should *NOT* return error when: // 1) The mount point is already unmounted. // 2) The mount point doesn't exist. -func (c *criService) unmountSandboxFiles(rootDir string, config *runtime.PodSandboxConfig) error { +func (c *criService) unmountSandboxFiles(id string, config *runtime.PodSandboxConfig) error { if config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetIpc() != runtime.NamespaceMode_NODE { - if err := c.os.Unmount(getSandboxDevShm(rootDir), unix.MNT_DETACH); err != nil && !os.IsNotExist(err) { + if err := c.os.Unmount(c.getSandboxDevShm(id), unix.MNT_DETACH); err != nil && !os.IsNotExist(err) { return err } } diff --git a/pkg/server/sandbox_run_test.go b/pkg/server/sandbox_run_test.go index c5a355a6c..cac8637e9 100644 --- a/pkg/server/sandbox_run_test.go +++ b/pkg/server/sandbox_run_test.go @@ -18,6 +18,7 @@ package server import ( "os" + "path/filepath" "testing" cni "github.com/containerd/go-cni" @@ -177,7 +178,7 @@ func TestGenerateSandboxContainerSpec(t *testing.T) { } func TestSetupSandboxFiles(t *testing.T) { - testRootDir := "test-sandbox-root" + const testID = "test-id" for desc, test := range map[string]struct { dnsConfig *runtime.DNSConfig ipcMode runtime.NamespaceMode @@ -189,13 +190,17 @@ func TestSetupSandboxFiles(t *testing.T) { { Name: "CopyFile", Arguments: []interface{}{ - "/etc/hosts", testRootDir + "/hosts", os.FileMode(0644), + "/etc/hosts", + filepath.Join(testRootDir, sandboxesDir, testID, "hosts"), + os.FileMode(0644), }, }, { Name: "CopyFile", Arguments: []interface{}{ - "/etc/resolv.conf", testRootDir + "/resolv.conf", os.FileMode(0644), + "/etc/resolv.conf", + filepath.Join(testRootDir, sandboxesDir, testID, "resolv.conf"), + os.FileMode(0644), }, }, { @@ -215,13 +220,16 @@ func TestSetupSandboxFiles(t *testing.T) { { Name: "CopyFile", Arguments: []interface{}{ - "/etc/hosts", testRootDir + "/hosts", os.FileMode(0644), + "/etc/hosts", + filepath.Join(testRootDir, sandboxesDir, testID, "hosts"), + os.FileMode(0644), }, }, { Name: "WriteFile", Arguments: []interface{}{ - testRootDir + "/resolv.conf", []byte(`search 114.114.114.114 + filepath.Join(testRootDir, sandboxesDir, testID, "resolv.conf"), + []byte(`search 114.114.114.114 nameserver 8.8.8.8 options timeout:1 `), os.FileMode(0644), @@ -239,19 +247,24 @@ options timeout:1 { Name: "CopyFile", Arguments: []interface{}{ - "/etc/hosts", testRootDir + "/hosts", os.FileMode(0644), + "/etc/hosts", + filepath.Join(testRootDir, sandboxesDir, testID, "hosts"), + os.FileMode(0644), }, }, { Name: "CopyFile", Arguments: []interface{}{ - "/etc/resolv.conf", testRootDir + "/resolv.conf", os.FileMode(0644), + "/etc/resolv.conf", + filepath.Join(testRootDir, sandboxesDir, testID, "resolv.conf"), + os.FileMode(0644), }, }, { Name: "MkdirAll", Arguments: []interface{}{ - testRootDir + "/shm", os.FileMode(0700), + filepath.Join(testStateDir, sandboxesDir, testID, "shm"), + os.FileMode(0700), }, }, { @@ -273,7 +286,7 @@ options timeout:1 }, }, } - c.setupSandboxFiles(testRootDir, cfg) + c.setupSandboxFiles(testID, cfg) calls := c.os.(*ostesting.FakeOS).GetCalls() assert.Len(t, calls, len(test.expectedCalls)) for i, expected := range test.expectedCalls { diff --git a/pkg/server/sandbox_stop.go b/pkg/server/sandbox_stop.go index ce9dca26b..eb00c8d55 100644 --- a/pkg/server/sandbox_stop.go +++ b/pkg/server/sandbox_stop.go @@ -81,9 +81,8 @@ func (c *criService) StopPodSandbox(ctx context.Context, r *runtime.StopPodSandb logrus.Infof("TearDown network for sandbox %q successfully", id) - sandboxRoot := getSandboxRootDir(c.config.RootDir, id) - if err := c.unmountSandboxFiles(sandboxRoot, sandbox.Config); err != nil { - return nil, errors.Wrapf(err, "failed to unmount sandbox files in %q", sandboxRoot) + if err := c.unmountSandboxFiles(id, sandbox.Config); err != nil { + return nil, errors.Wrap(err, "failed to unmount sandbox files") } // Only stop sandbox container when it's running. diff --git a/pkg/server/service_test.go b/pkg/server/service_test.go index eb0d79eed..0bc81e670 100644 --- a/pkg/server/service_test.go +++ b/pkg/server/service_test.go @@ -28,7 +28,8 @@ import ( ) const ( - testRootDir = "/test/rootfs" + testRootDir = "/test/root" + testStateDir = "/test/state" // Use an image id as test sandbox image to avoid image name resolve. // TODO(random-liu): Change this to image name after we have complete image // management unit test framework. @@ -40,7 +41,8 @@ const ( func newTestCRIService() *criService { return &criService{ config: criconfig.Config{ - RootDir: testRootDir, + RootDir: testRootDir, + StateDir: testStateDir, PluginConfig: criconfig.PluginConfig{ SandboxImage: testSandboxImage, }, From f4c9ef2647e3016f8689542272ea4b4dbcadfca3 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Fri, 23 Mar 2018 23:31:19 +0000 Subject: [PATCH 2/2] Add symlink follow into unmount util. Signed-off-by: Lantao Liu --- pkg/os/os.go | 25 ++++++++++++++++++------- pkg/os/testing/fake_os.go | 38 ++++++++++++++++++++++++++------------ pkg/server/sandbox_run.go | 8 ++++++-- pkg/store/sandbox/netns.go | 15 ++++----------- 4 files changed, 54 insertions(+), 32 deletions(-) diff --git a/pkg/os/os.go b/pkg/os/os.go index 128ffb8f9..f0e7a3271 100644 --- a/pkg/os/os.go +++ b/pkg/os/os.go @@ -25,6 +25,7 @@ import ( containerdmount "github.com/containerd/containerd/mount" "github.com/containerd/fifo" "github.com/docker/docker/pkg/mount" + "github.com/docker/docker/pkg/symlink" "golang.org/x/net/context" "golang.org/x/sys/unix" ) @@ -37,6 +38,7 @@ type OS interface { OpenFifo(ctx context.Context, fn string, flag int, perm os.FileMode) (io.ReadWriteCloser, error) Stat(name string) (os.FileInfo, error) ResolveSymbolicLink(name string) (string, error) + FollowSymlinkInScope(path, scope string) (string, error) CopyFile(src, dest string, perm os.FileMode) error WriteFile(filename string, data []byte, perm os.FileMode) error Mount(source string, target string, fstype string, flags uintptr, data string) error @@ -79,6 +81,11 @@ func (RealOS) ResolveSymbolicLink(path string) (string, error) { return filepath.EvalSymlinks(path) } +// FollowSymlinkInScope will call symlink.FollowSymlinkInScope. +func (RealOS) FollowSymlinkInScope(path, scope string) (string, error) { + return symlink.FollowSymlinkInScope(path, scope) +} + // CopyFile will copy src file to dest file func (RealOS) CopyFile(src, dest string, perm os.FileMode) error { in, err := os.Open(src) @@ -107,17 +114,21 @@ func (RealOS) Mount(source string, target string, fstype string, flags uintptr, return unix.Mount(source, target, fstype, flags, data) } -// Unmount will call unix.Unmount to unmount the file. The function doesn't -// return error if target is not mounted. +// Unmount will call Unmount to unmount the file. func (RealOS) Unmount(target string, flags int) error { - // TODO(random-liu): Follow symlink to make sure the result is correct. - if mounted, err := mount.Mounted(target); err != nil || !mounted { - return err - } - return unix.Unmount(target, flags) + return Unmount(target, flags) } // LookupMount gets mount info of a given path. func (RealOS) LookupMount(path string) (containerdmount.Info, error) { return containerdmount.Lookup(path) } + +// Unmount will call unix.Unmount to unmount the file. The function doesn't +// return error if target is not mounted. +func Unmount(target string, flags int) error { + if mounted, err := mount.Mounted(target); err != nil || !mounted { + return err + } + return unix.Unmount(target, flags) +} diff --git a/pkg/os/testing/fake_os.go b/pkg/os/testing/fake_os.go index fdba1d2a7..f33cbbd4f 100644 --- a/pkg/os/testing/fake_os.go +++ b/pkg/os/testing/fake_os.go @@ -40,18 +40,19 @@ type CalledDetail struct { // of the real call. type FakeOS struct { sync.Mutex - MkdirAllFn func(string, os.FileMode) error - RemoveAllFn func(string) error - OpenFifoFn func(context.Context, string, int, os.FileMode) (io.ReadWriteCloser, error) - StatFn func(string) (os.FileInfo, error) - ResolveSymbolicLinkFn func(string) (string, error) - CopyFileFn func(string, string, os.FileMode) error - WriteFileFn func(string, []byte, os.FileMode) error - MountFn func(source string, target string, fstype string, flags uintptr, data string) error - UnmountFn func(target string, flags int) error - LookupMountFn func(path string) (containerdmount.Info, error) - calls []CalledDetail - errors map[string]error + MkdirAllFn func(string, os.FileMode) error + RemoveAllFn func(string) error + OpenFifoFn func(context.Context, string, int, os.FileMode) (io.ReadWriteCloser, error) + StatFn func(string) (os.FileInfo, error) + ResolveSymbolicLinkFn func(string) (string, error) + FollowSymlinkInScopeFn func(string, string) (string, error) + CopyFileFn func(string, string, os.FileMode) error + WriteFileFn func(string, []byte, os.FileMode) error + MountFn func(source string, target string, fstype string, flags uintptr, data string) error + UnmountFn func(target string, flags int) error + LookupMountFn func(path string) (containerdmount.Info, error) + calls []CalledDetail + errors map[string]error } var _ osInterface.OS = &FakeOS{} @@ -176,6 +177,19 @@ func (f *FakeOS) ResolveSymbolicLink(path string) (string, error) { return path, nil } +// FollowSymlinkInScope is a fake call that invokes FollowSymlinkInScope or returns its input +func (f *FakeOS) FollowSymlinkInScope(path, scope string) (string, error) { + f.appendCalls("FollowSymlinkInScope", path, scope) + if err := f.getError("FollowSymlinkInScope"); err != nil { + return "", err + } + + if f.FollowSymlinkInScopeFn != nil { + return f.FollowSymlinkInScopeFn(path, scope) + } + return path, nil +} + // CopyFile is a fake call that invokes CopyFileFn or just return nil. func (f *FakeOS) CopyFile(src, dest string, perm os.FileMode) error { f.appendCalls("CopyFile", src, dest, perm) diff --git a/pkg/server/sandbox_run.go b/pkg/server/sandbox_run.go index de0b42bee..e27ec8bc1 100644 --- a/pkg/server/sandbox_run.go +++ b/pkg/server/sandbox_run.go @@ -491,8 +491,12 @@ func parseDNSOptions(servers, searches, options []string) (string, error) { // 2) The mount point doesn't exist. func (c *criService) unmountSandboxFiles(id string, config *runtime.PodSandboxConfig) error { if config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetIpc() != runtime.NamespaceMode_NODE { - if err := c.os.Unmount(c.getSandboxDevShm(id), unix.MNT_DETACH); err != nil && !os.IsNotExist(err) { - return err + path, err := c.os.FollowSymlinkInScope(c.getSandboxDevShm(id), "/") + if err != nil { + return errors.Wrap(err, "failed to follow symlink") + } + if err := c.os.Unmount(path, unix.MNT_DETACH); err != nil && !os.IsNotExist(err) { + return errors.Wrapf(err, "failed to unmount %q", path) } } return nil diff --git a/pkg/store/sandbox/netns.go b/pkg/store/sandbox/netns.go index 1fc98ee00..8ec4c1de5 100644 --- a/pkg/store/sandbox/netns.go +++ b/pkg/store/sandbox/netns.go @@ -21,10 +21,11 @@ import ( "sync" cnins "github.com/containernetworking/plugins/pkg/ns" - "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/symlink" "github.com/pkg/errors" "golang.org/x/sys/unix" + + osinterface "github.com/containerd/cri/pkg/os" ) // ErrClosedNetNS is the error returned when network namespace is closed. @@ -81,7 +82,6 @@ func (n *NetNS) Remove() error { } if n.restored { path := n.ns.Path() - // TODO(random-liu): Add util function for unmount. // Check netns existence. if _, err := os.Stat(path); err != nil { if os.IsNotExist(err) { @@ -93,15 +93,8 @@ func (n *NetNS) Remove() error { if err != nil { return errors.Wrap(err, "failed to follow symlink") } - mounted, err := mount.Mounted(path) - if err != nil { - return errors.Wrap(err, "failed to check netns mounted") - } - if mounted { - err := unix.Unmount(path, unix.MNT_DETACH) - if err != nil && !os.IsNotExist(err) { - return errors.Wrap(err, "failed to umount netns") - } + if err := osinterface.Unmount(path, unix.MNT_DETACH); err != nil && !os.IsNotExist(err) { + return errors.Wrap(err, "failed to umount netns") } if err := os.RemoveAll(path); err != nil { return errors.Wrap(err, "failed to remove netns")