Skip to content

Conversation

@wking
Copy link
Contributor

@wking wking commented May 20, 2016

These landed as CheckMounts in 647e355 (bundle validate update to
0.3.0, 2016-02-23, #20), but both checks are too strict.

The first (destination exists in the rootfs) errors on valid cases
like:

"mounts": [
  {
    "source": "users",
    "destination": "/home",
    "type": "bind"
  },
  {
    "source": "none",
    "destination": "/home/wking",
    "type": "tmpfs"
  }
]

Where the source users directory already contained a wking
subdirectory. So by the time the tmpfs was setup, the destination
directory would exist, but at validation time (without having run the
bind mount) the tmpfs destination directory would not exist.

The second (destination is a directory) errors on valid cases like:

"mounts": [
  {
    "source": "/etc/resolv.conf",
    "destination": "/etc/resolv.conf",
    "type": "bind"
  }
]

because binding files to files works. In a shell:

# touch test
# mount --bind /etc/resolv.conf test
# umount test

However binding directories to files does not work:

# mount --bind /etc test
mount: mount point /tmp/test is not a directory

Figuring out which mount configurations are valid and which aren't may
be possible, but I'm pretty sure it's more trouble than we want to get
into. There may be room for other mount tests (e.g. comparing type
against /proc/filesystems as a host-specific test), but I'm leaving
those to subsequent pull requests.

@wking
Copy link
Contributor Author

wking commented May 20, 2016

And somewhat related to “more trouble than we want to get into” is opencontainers/runtime-spec#418, which allows the runtime to not validate every possible thing before asking the kernel to do it ;). And in ocitools validate we're tasked with validating the bundle config against the runtime spec, but we don't have to get down into validating the bundle config against kernel constraints unless we want to.

@mrunalp
Copy link
Contributor

mrunalp commented May 20, 2016

I am okay removing this check but would like to hear @liangchenye opinion as well.

@liangchenye
Copy link
Member

Looks good to me too. @mrunalp @wking

@mrunalp
Copy link
Contributor

mrunalp commented May 23, 2016

Needs rebase.

@wking
Copy link
Contributor Author

wking commented May 23, 2016

On Mon, May 23, 2016 at 09:26:19AM -0700, Mrunal Patel wrote:

Needs rebase.

Rebased with 2eaec20 → 3b9d616.

@mrunalp
Copy link
Contributor

mrunalp commented May 23, 2016

github is still complaining about conflicts.

These landed as CheckMounts in 647e355 (bundle validate update to
0.3.0, 2016-02-23, opencontainers#20), but both checks are too strict.

The first (destination exists in the rootfs) errors on valid cases
like:

  "mounts": [
    {
      "source": "users",
      "destination": "/home",
      "type": "bind"
    },
    {
      "source": "none",
      "destination": "/home/wking",
      "type": "tmpfs"
    }
  ]

Where the source 'users' directory already contained a 'wking'
subdirectory.  So by the time the tmpfs was setup, the destination
directory would exist, but at validation time (without having run the
bind mount) the tmpfs destination directory would not exist.

The second (destination is a directory) errors on valid cases like:

  "mounts": [
    {
      "source": "/etc/resolv.conf",
      "destination": "/etc/resolv.conf",
      "type": "bind"
    }
  ]

because binding files to files works.  In a shell:

  # touch test
  # mount --bind /etc/resolv.conf test
  # umount test

However binding directories to files does not work:

  # mount --bind /etc test
  mount: mount point /tmp/test is not a directory

Figuring out which mount configurations are valid and which aren't may
be possible, but I'm pretty sure it's more trouble than we want to get
into.  There may be room for other mount tests (e.g. comparing 'type'
against /proc/filesystems as a host-specific test), but I'm leaving
those to subsequent pull requests.

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented May 23, 2016

On Mon, May 23, 2016 at 09:31:19AM -0700, Mrunal Patel wrote:

github is still complaining about conflicts.

Odd, I'd pushed the commit 1. I just bumped the commit date to get
a new hash and pushed db4632b, and GitHub doesn't seem to have noticed
that either. Maybe just merge this one from the command line? Or I
can open a new PR?

@mrunalp
Copy link
Contributor

mrunalp commented May 23, 2016

@wking Can you open a new PR?

@wking wking mentioned this pull request May 23, 2016
@wking
Copy link
Contributor Author

wking commented May 23, 2016

On Mon, May 23, 2016 at 09:57:12AM -0700, Mrunal Patel wrote:

@wking Can you open a new PR?

Filed in #74.

@wking wking force-pushed the drop-mount-checks branch 2 times, most recently from 3b9d616 to db4632b Compare May 23, 2016 17:24
@mrunalp
Copy link
Contributor

mrunalp commented May 23, 2016

LGTM. Looks like github was probably slow in updating merge status.

@mrunalp mrunalp merged commit 4675b7d into opencontainers:master May 23, 2016
@wking wking deleted the drop-mount-checks branch June 17, 2016 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants