Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When calling volume driver Mount, send opaque ID #21015

Merged
merged 1 commit into from
May 5, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions container/container_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/Sirupsen/logrus"
"github.com/docker/docker/pkg/chrootarchive"
"github.com/docker/docker/pkg/stringid"
"github.com/docker/docker/pkg/symlink"
"github.com/docker/docker/pkg/system"
"github.com/docker/docker/utils"
Expand Down Expand Up @@ -181,11 +182,17 @@ func (container *Container) CopyImagePathContent(v volume.Volume, destination st
return err
}

path, err := v.Mount()
id := stringid.GenerateNonCryptoID()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a non-cryptographically strong ID? Presumably uniqueness is important.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally use non-crypto for these sort of IDs (even container ID's are non-crypto). These are going to be relatively short-lived and not likely to conflict (and not catastrophic if they happen to).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be pretty catastrophic for me (the bug that got us here in the
first place) if there were duplicates. Can we just ensure no other
containers have the ID and re-roll if needed?

-Erik

On 8 Mar 2016, at 12:25, Brian Goff wrote:

@@ -485,16 +486,18 @@ func (container *Container)
CopyImagePathContent(v volume.Volume, destination st
return err
}

  • path, err := v.Mount()
  • id := stringid.GenerateNonCryptoID()

We generally use non-crypto for these sort of IDs (even container ID's
are non-crypto). These are going to be relatively short-lived and not
likely to conflict (and not catastrophic if they happen to).


Reply to this email directly or view it on GitHub:
https://github.com/docker/docker/pull/21015/files#r55421645

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but non-cryptographically doesn't say non-unique, only "possibly predictable", or am I mistaken here? i.e. a UUID may be predictable, thus "non-crypto", but still is unique

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, we could never guarantee this across hosts anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I didn't mean that, sorry if I was implying otherwise. just ensure that no two existing mounts have the same ID and I'll be fine at least, I think.

path, err := v.Mount(id)
if err != nil {
return err
}
defer v.Unmount()

defer func() {
if err := v.Unmount(id); err != nil {
logrus.Warnf("error while unmounting volume %s: %v", v.Name(), err)
}
}()
return copyExistingContents(rootfs, path)
}

Expand Down Expand Up @@ -328,9 +335,10 @@ func (container *Container) UnmountVolumes(forceSyscall bool, volumeEventLog fun
}

if volumeMount.Volume != nil {
if err := volumeMount.Volume.Unmount(); err != nil {
if err := volumeMount.Volume.Unmount(volumeMount.ID); err != nil {
return err
}
volumeMount.ID = ""

attributes := map[string]string{
"driver": volumeMount.Volume.DriverName(),
Expand Down
10 changes: 8 additions & 2 deletions docs/extend/plugins_volume.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ Respond with a string error if an error occurred.
**Request**:
```json
{
"Name": "volume_name"
"Name": "volume_name",
"ID": "b87d7442095999a92b65b3d9691e697b61713829cc0ffd1bb72e4ccd51aa4d6c"
}
```

Expand All @@ -124,6 +125,8 @@ name. This is called once per container start. If the same volume_name is reques
more than once, the plugin may need to keep track of each new mount request and provision
at the first mount request and deprovision at the last corresponding unmount request.

`ID` is a unqiue ID for the caller that is requesting the mount.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo "unqiue"


**Response**:
```json
{
Expand Down Expand Up @@ -162,14 +165,17 @@ available, and/or a string error if an error occurred.
**Request**:
```json
{
"Name": "volume_name"
"Name": "volume_name",
"ID": "b87d7442095999a92b65b3d9691e697b61713829cc0ffd1bb72e4ccd51aa4d6c"
}
```

Indication that Docker no longer is using the named volume. This is called once
per container stop. Plugin may deduce that it is safe to deprovision it at
this point.

`ID` is a unqiue ID for the caller that is requesting the mount.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo "unqiue"


**Response**:
```json
{
Expand Down
15 changes: 15 additions & 0 deletions integration-cli/docker_cli_external_volume_driver_unix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
type pluginRequest struct {
Name string
Opts map[string]string
ID string
}

type pluginResp struct {
Expand Down Expand Up @@ -204,6 +205,11 @@ func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
return
}

if err := ioutil.WriteFile(filepath.Join(p, "mountID"), []byte(pr.ID), 0644); err != nil {
send(w, err)
return
}

send(w, &pluginResp{Mountpoint: p})
})

Expand Down Expand Up @@ -476,3 +482,12 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverPathCalls(c *check.C
c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
c.Assert(s.ec.paths, checker.Equals, 1)
}

func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverMountID(c *check.C) {
err := s.d.StartWithBusybox()
c.Assert(err, checker.IsNil)

out, err := s.d.Cmd("run", "--rm", "-v", "external-volume-test:/tmp/external-volume-test", "--volume-driver", "test-external-volume-driver", "busybox:latest", "cat", "/tmp/external-volume-test/test")
c.Assert(err, checker.IsNil, check.Commentf(out))
c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
}
9 changes: 8 additions & 1 deletion pkg/plugins/pluginrpc-gen/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,17 @@ var templFuncs = template.FuncMap{
"marshalType": marshalType,
"isErr": isErr,
"lower": strings.ToLower,
"title": strings.Title,
"title": title,
"tag": buildTag,
}

func title(s string) string {
if strings.ToLower(s) == "id" {
return "ID"
}
return strings.Title(s)
}

var generatedTempl = template.Must(template.New("rpc_cient").Funcs(templFuncs).Parse(`
// generated code - DO NOT EDIT
{{ range $k, $v := .BuildTags }}
Expand Down
8 changes: 4 additions & 4 deletions volume/drivers/adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,14 @@ func (a *volumeAdapter) CachedPath() string {
return a.eMount
}

func (a *volumeAdapter) Mount() (string, error) {
func (a *volumeAdapter) Mount(id string) (string, error) {
var err error
a.eMount, err = a.proxy.Mount(a.name)
a.eMount, err = a.proxy.Mount(a.name, id)
return a.eMount, err
}

func (a *volumeAdapter) Unmount() error {
err := a.proxy.Unmount(a.name)
func (a *volumeAdapter) Unmount(id string) error {
err := a.proxy.Unmount(a.name, id)
if err == nil {
a.eMount = ""
}
Expand Down
4 changes: 2 additions & 2 deletions volume/drivers/extpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ type volumeDriver interface {
// Get the mountpoint of the given volume
Path(name string) (mountpoint string, err error)
// Mount the given volume and return the mountpoint
Mount(name string) (mountpoint string, err error)
Mount(name, id string) (mountpoint string, err error)
// Unmount the given volume
Unmount(name string) (err error)
Unmount(name, id string) (err error)
// List lists all the volumes known to the driver
List() (volumes list, err error)
// Get retrieves the volume with the requested name
Expand Down
8 changes: 6 additions & 2 deletions volume/drivers/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,20 +97,22 @@ func (pp *volumeDriverProxy) Path(name string) (mountpoint string, err error) {

type volumeDriverProxyMountRequest struct {
Name string
ID string
}

type volumeDriverProxyMountResponse struct {
Mountpoint string
Err string
}

func (pp *volumeDriverProxy) Mount(name string) (mountpoint string, err error) {
func (pp *volumeDriverProxy) Mount(name string, id string) (mountpoint string, err error) {
var (
req volumeDriverProxyMountRequest
ret volumeDriverProxyMountResponse
)

req.Name = name
req.ID = id
if err = pp.Call("VolumeDriver.Mount", req, &ret); err != nil {
return
}
Expand All @@ -126,19 +128,21 @@ func (pp *volumeDriverProxy) Mount(name string) (mountpoint string, err error) {

type volumeDriverProxyUnmountRequest struct {
Name string
ID string
}

type volumeDriverProxyUnmountResponse struct {
Err string
}

func (pp *volumeDriverProxy) Unmount(name string) (err error) {
func (pp *volumeDriverProxy) Unmount(name string, id string) (err error) {
var (
req volumeDriverProxyUnmountRequest
ret volumeDriverProxyUnmountResponse
)

req.Name = name
req.ID = id
if err = pp.Call("VolumeDriver.Unmount", req, &ret); err != nil {
return
}
Expand Down
4 changes: 2 additions & 2 deletions volume/drivers/proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func TestVolumeRequestError(t *testing.T) {
t.Fatalf("Unexpected error: %v\n", err)
}

_, err = driver.Mount("volume")
_, err = driver.Mount("volume", "123")
if err == nil {
t.Fatal("Expected error, was nil")
}
Expand All @@ -77,7 +77,7 @@ func TestVolumeRequestError(t *testing.T) {
t.Fatalf("Unexpected error: %v\n", err)
}

err = driver.Unmount("volume")
err = driver.Unmount("volume", "123")
if err == nil {
t.Fatal("Expected error, was nil")
}
Expand Down
4 changes: 2 additions & 2 deletions volume/local/local.go
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ func (v *localVolume) Path() string {
}

// Mount implements the localVolume interface, returning the data location.
func (v *localVolume) Mount() (string, error) {
func (v *localVolume) Mount(id string) (string, error) {
v.m.Lock()
defer v.m.Unlock()
if v.opts != nil {
Expand All @@ -303,7 +303,7 @@ func (v *localVolume) Mount() (string, error) {
}

// Umount is for satisfying the localVolume interface and does not do anything in this driver.
func (v *localVolume) Unmount() error {
func (v *localVolume) Unmount(id string) error {
v.m.Lock()
defer v.m.Unlock()
if v.opts != nil {
Expand Down
8 changes: 4 additions & 4 deletions volume/local/local_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,12 @@ func TestCreateWithOpts(t *testing.T) {
}
v := vol.(*localVolume)

dir, err := v.Mount()
dir, err := v.Mount("1234")
if err != nil {
t.Fatal(err)
}
defer func() {
if err := v.Unmount(); err != nil {
if err := v.Unmount("1234"); err != nil {
t.Fatal(err)
}
}()
Expand Down Expand Up @@ -225,14 +225,14 @@ func TestCreateWithOpts(t *testing.T) {
}

// test double mount
if _, err := v.Mount(); err != nil {
if _, err := v.Mount("1234"); err != nil {
t.Fatal(err)
}
if v.active.count != 2 {
t.Fatalf("Expected active mount count to be 2, got %d", v.active.count)
}

if err := v.Unmount(); err != nil {
if err := v.Unmount("1234"); err != nil {
t.Fatal(err)
}
if v.active.count != 1 {
Expand Down
8 changes: 4 additions & 4 deletions volume/testutils/testutils.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ func (NoopVolume) DriverName() string { return "noop" }
func (NoopVolume) Path() string { return "noop" }

// Mount mounts the volume in the container
func (NoopVolume) Mount() (string, error) { return "noop", nil }
func (NoopVolume) Mount(_ string) (string, error) { return "noop", nil }

// Unmount unmounts the volume from the container
func (NoopVolume) Unmount() error { return nil }
func (NoopVolume) Unmount(_ string) error { return nil }

// Status proivdes low-level details about the volume
func (NoopVolume) Status() map[string]interface{} { return nil }
Expand All @@ -48,10 +48,10 @@ func (f FakeVolume) DriverName() string { return f.driverName }
func (FakeVolume) Path() string { return "fake" }

// Mount mounts the volume in the container
func (FakeVolume) Mount() (string, error) { return "fake", nil }
func (FakeVolume) Mount(_ string) (string, error) { return "fake", nil }

// Unmount unmounts the volume from the container
func (FakeVolume) Unmount() error { return nil }
func (FakeVolume) Unmount(_ string) error { return nil }

// Status proivdes low-level details about the volume
func (FakeVolume) Status() map[string]interface{} { return nil }
Expand Down
14 changes: 11 additions & 3 deletions volume/volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"os"
"runtime"
"strings"

"github.com/docker/docker/pkg/stringid"
)

// DefaultDriverName is the driver name used for the driver
Expand Down Expand Up @@ -35,9 +37,9 @@ type Volume interface {
Path() string
// Mount mounts the volume and returns the absolute path to
// where it can be consumed.
Mount() (string, error)
Mount(id string) (string, error)
// Unmount unmounts the volume when it is no longer in use.
Unmount() error
Unmount(id string) error
// Status returns low-level status information about a volume
Status() map[string]interface{}
}
Expand All @@ -64,13 +66,19 @@ type MountPoint struct {
// Use a pointer here so we can tell if the user set this value explicitly
// This allows us to error out when the user explicitly enabled copy but we can't copy due to the volume being populated
CopyData bool `json:"-"`
// ID is the opaque ID used to pass to the volume driver.
// This should be set by calls to `Mount` and unset by calls to `Unmount`
ID string
}

// Setup sets up a mount point by either mounting the volume if it is
// configured, or creating the source directory if supplied.
func (m *MountPoint) Setup() (string, error) {
if m.Volume != nil {
return m.Volume.Mount()
if m.ID == "" {
m.ID = stringid.GenerateNonCryptoID()
}
return m.Volume.Mount(m.ID)
}
if len(m.Source) > 0 {
if _, err := os.Stat(m.Source); err != nil {
Expand Down