Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Commit db8500d

Browse files
Random-LiuRandom-Liu
authored
Merge pull request #892 from Random-Liu/fix-volume-mount-order
Sort volume mount.
2 parents 67c0b3e + 063f815 commit db8500d

4 files changed

Lines changed: 140 additions & 18 deletions

File tree

‎pkg/server/container_create.go‎

Lines changed: 52 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package server
1919
import (
2020
"os"
2121
"path/filepath"
22+
"sort"
2223
"strconv"
2324
"strings"
2425
"time"
@@ -351,8 +352,8 @@ func (c *criService) generateContainerSpec(id string, sandboxID string, sandboxP
351352
return nil, errors.Wrapf(err, "failed to init selinux options %+v", securityContext.GetSelinuxOptions())
352353
}
353354

354-
// Add extra mounts first so that CRI specified mounts can override.
355-
mounts := append(extraMounts, config.GetMounts()...)
355+
// Merge extra mounts and CRI mounts.
356+
mounts := mergeMounts(config.GetMounts(), extraMounts)
356357
if err := c.addOCIBindMounts(&g, mounts, mountLabel); err != nil {
357358
return nil, errors.Wrapf(err, "failed to set OCI bind mounts %+v", mounts)
358359
}
@@ -616,13 +617,40 @@ func setOCIDevicesPrivileged(g *generate.Generator) error {
616617

617618
// addOCIBindMounts adds bind mounts.
618619
func (c *criService) addOCIBindMounts(g *generate.Generator, mounts []*runtime.Mount, mountLabel string) error {
620+
// Sort mounts in number of parts. This ensures that high level mounts don't
621+
// shadow other mounts.
622+
sort.Sort(orderedMounts(mounts))
623+
619624
// Mount cgroup into the container as readonly, which inherits docker's behavior.
620625
g.AddMount(runtimespec.Mount{
621626
Source: "cgroup",
622627
Destination: "/sys/fs/cgroup",
623628
Type: "cgroup",
624629
Options: []string{"nosuid", "noexec", "nodev", "relatime", "ro"},
625630
})
631+
632+
// Copy all mounts from default mounts, except for
633+
// - mounts overriden by supplied mount;
634+
// - all mounts under /dev if a supplied /dev is present.
635+
mountSet := make(map[string]struct{})
636+
for _, m := range mounts {
637+
mountSet[filepath.Clean(m.ContainerPath)] = struct{}{}
638+
}
639+
defaultMounts := g.Mounts()
640+
g.ClearMounts()
641+
for _, m := range defaultMounts {
642+
dst := filepath.Clean(m.Destination)
643+
if _, ok := mountSet[dst]; ok {
644+
// filter out mount overridden by a supplied mount
645+
continue
646+
}
647+
if _, mountDev := mountSet["/dev"]; mountDev && strings.HasPrefix(dst, "/dev/") {
648+
// filter out everything under /dev if /dev is a supplied mount
649+
continue
650+
}
651+
g.AddMount(m)
652+
}
653+
626654
for _, mount := range mounts {
627655
dst := mount.GetContainerPath()
628656
src := mount.GetHostPath()
@@ -841,10 +869,6 @@ func defaultRuntimeSpec(id string) (*runtimespec.Spec, error) {
841869
if mount.Destination == "/run" {
842870
continue
843871
}
844-
// CRI plugin handles `/dev/shm` itself.
845-
if mount.Destination == "/dev/shm" {
846-
continue
847-
}
848872
mounts = append(mounts, mount)
849873
}
850874
spec.Mounts = mounts
@@ -988,3 +1012,25 @@ func generateUserString(username string, uid, gid *runtime.Int64Value) (string,
9881012
}
9891013
return userstr, nil
9901014
}
1015+
1016+
// mergeMounts merge CRI mounts with extra mounts. If a mount destination
1017+
// is mounted by both a CRI mount and an extra mount, the CRI mount will
1018+
// be kept.
1019+
func mergeMounts(criMounts, extraMounts []*runtime.Mount) []*runtime.Mount {
1020+
var mounts []*runtime.Mount
1021+
mounts = append(mounts, criMounts...)
1022+
// Copy all mounts from extra mounts, except for mounts overriden by CRI.
1023+
for _, e := range extraMounts {
1024+
found := false
1025+
for _, c := range criMounts {
1026+
if filepath.Clean(e.ContainerPath) == filepath.Clean(c.ContainerPath) {
1027+
found = true
1028+
break
1029+
}
1030+
}
1031+
if !found {
1032+
mounts = append(mounts, e)
1033+
}
1034+
}
1035+
return mounts
1036+
}

‎pkg/server/container_create_test.go‎

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package server
1919
import (
2020
"path/filepath"
2121
"reflect"
22+
"strings"
2223
"testing"
2324

2425
"github.com/containerd/containerd/contrib/apparmor"
@@ -311,26 +312,50 @@ func TestContainerSpecWithExtraMounts(t *testing.T) {
311312
Readonly: false,
312313
}
313314
config.Mounts = append(config.Mounts, mountInConfig)
314-
extraMount := &runtime.Mount{
315-
ContainerPath: "test-container-path",
316-
HostPath: "test-host-path-extra",
317-
Readonly: true,
315+
extraMounts := []*runtime.Mount{
316+
{
317+
ContainerPath: "test-container-path",
318+
HostPath: "test-host-path-extra",
319+
Readonly: true,
320+
},
321+
{
322+
ContainerPath: "/sys",
323+
HostPath: "test-sys-extra",
324+
Readonly: false,
325+
},
326+
{
327+
ContainerPath: "/dev",
328+
HostPath: "test-dev-extra",
329+
Readonly: false,
330+
},
318331
}
319-
spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, []*runtime.Mount{extraMount})
332+
spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, extraMounts)
320333
require.NoError(t, err)
321334
specCheck(t, testID, testSandboxID, testPid, spec)
322-
var mounts []runtimespec.Mount
335+
var mounts, sysMounts, devMounts []runtimespec.Mount
323336
for _, m := range spec.Mounts {
324337
if m.Destination == "test-container-path" {
325338
mounts = append(mounts, m)
339+
} else if m.Destination == "/sys" {
340+
sysMounts = append(sysMounts, m)
341+
} else if strings.HasPrefix(m.Destination, "/dev") {
342+
devMounts = append(devMounts, m)
326343
}
327344
}
328-
t.Logf("Extra mounts should come first")
329-
require.Len(t, mounts, 2)
330-
assert.Equal(t, "test-host-path-extra", mounts[0].Source)
331-
assert.Contains(t, mounts[0].Options, "ro")
332-
assert.Equal(t, "test-host-path", mounts[1].Source)
333-
assert.Contains(t, mounts[1].Options, "rw")
345+
t.Logf("CRI mount should override extra mount")
346+
require.Len(t, mounts, 1)
347+
assert.Equal(t, "test-host-path", mounts[0].Source)
348+
assert.Contains(t, mounts[0].Options, "rw")
349+
350+
t.Logf("Extra mount should override default mount")
351+
require.Len(t, sysMounts, 1)
352+
assert.Equal(t, "test-sys-extra", sysMounts[0].Source)
353+
assert.Contains(t, sysMounts[0].Options, "rw")
354+
355+
t.Logf("Dev mount should override all default dev mounts")
356+
require.Len(t, devMounts, 1)
357+
assert.Equal(t, "test-dev-extra", devMounts[0].Source)
358+
assert.Contains(t, devMounts[0].Options, "rw")
334359
}
335360

336361
func TestContainerAndSandboxPrivileged(t *testing.T) {

‎pkg/server/helpers.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package server
1919
import (
2020
"encoding/json"
2121
"fmt"
22+
"os"
2223
"path"
2324
"path/filepath"
2425
"regexp"
@@ -472,3 +473,30 @@ func toRuntimeAuthConfig(a criconfig.AuthConfig) *runtime.AuthConfig {
472473
IdentityToken: a.IdentityToken,
473474
}
474475
}
476+
477+
// mounts defines how to sort runtime.Mount.
478+
// This is the same with the Docker implementation:
479+
// https://github.com/moby/moby/blob/17.05.x/daemon/volumes.go#L26
480+
type orderedMounts []*runtime.Mount
481+
482+
// Len returns the number of mounts. Used in sorting.
483+
func (m orderedMounts) Len() int {
484+
return len(m)
485+
}
486+
487+
// Less returns true if the number of parts (a/b/c would be 3 parts) in the
488+
// mount indexed by parameter 1 is less than that of the mount indexed by
489+
// parameter 2. Used in sorting.
490+
func (m orderedMounts) Less(i, j int) bool {
491+
return m.parts(i) < m.parts(j)
492+
}
493+
494+
// Swap swaps two items in an array of mounts. Used in sorting
495+
func (m orderedMounts) Swap(i, j int) {
496+
m[i], m[j] = m[j], m[i]
497+
}
498+
499+
// parts returns the number of parts in the destination of a mount. Used in sorting.
500+
func (m orderedMounts) parts(i int) int {
501+
return strings.Count(filepath.Clean(m[i].ContainerPath), string(os.PathSeparator))
502+
}

‎pkg/server/helpers_test.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ limitations under the License.
1717
package server
1818

1919
import (
20+
"sort"
2021
"testing"
2122

2223
"github.com/containerd/containerd"
@@ -25,6 +26,7 @@ import (
2526
imagedigest "github.com/opencontainers/go-digest"
2627
"github.com/stretchr/testify/assert"
2728
"golang.org/x/net/context"
29+
runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
2830

2931
criconfig "github.com/containerd/cri/pkg/config"
3032
"github.com/containerd/cri/pkg/util"
@@ -190,3 +192,24 @@ func TestGetRuntimeConfigFromContainerInfo(t *testing.T) {
190192
})
191193
}
192194
}
195+
196+
func TestOrderedMounts(t *testing.T) {
197+
mounts := []*runtime.Mount{
198+
{ContainerPath: "/a/b/c"},
199+
{ContainerPath: "/a/b"},
200+
{ContainerPath: "/a/b/c/d"},
201+
{ContainerPath: "/a"},
202+
{ContainerPath: "/b"},
203+
{ContainerPath: "/b/c"},
204+
}
205+
expected := []*runtime.Mount{
206+
{ContainerPath: "/a"},
207+
{ContainerPath: "/b"},
208+
{ContainerPath: "/a/b"},
209+
{ContainerPath: "/b/c"},
210+
{ContainerPath: "/a/b/c"},
211+
{ContainerPath: "/a/b/c/d"},
212+
}
213+
sort.Stable(orderedMounts(mounts))
214+
assert.Equal(t, expected, mounts)
215+
}

0 commit comments

Comments
 (0)