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

cmd/runtimetest/main.go: Unify validateMountsExist and validateMountsOrder #456

Conversation

wking
Copy link
Contributor

@wking wking commented Aug 30, 2017

Also increase the error message detail and continue through the remaining mounts instead of breaking on the first missing/misordered mount. Based on previous discussion here and here. With this commit, a configuration like:

  "mounts": [
    {
      "destination": "/tmp",
      "type": "tmpfs",
      "source": "none"
    },
    {
      "destination": "/tmp",
      "type": "tmpfs",
      "source": "none"
    },
    {
      "destination": "/dev",
      "type": "devtmpfs",
      "source": "devtmpfs"
    }
  ]

and mountinfo like:

$ grep -n '/dev \|/tmp ' /proc/self/mountinfo
2:19 17 0:6 / /dev rw,nosuid,relatime - devtmpfs devtmpfs rw,size=10240k,nr_inodes=2043951,mode=755
25:41 17 0:38 / /tmp rw,relatime - tmpfs none rw

will generate errors like:

* mounts[1] {/tmp tmpfs none []} does not exist
* mounts[2] {/dev devtmpfs devtmpfs []} is system mount 1, while mounts[0] {/tmp tmpfs none []} is system mount 24

Grep reports 2 and 25 because it's counting from one, and runtimetest reports 1 and 24 because it's counting from zero.

Before this commit, the error was just:

* Mounts[1] /tmp is not mounted in order

@wking wking force-pushed the combine-mount-order-and-existence-checks branch from 0fa2a3b to 401f3b6 Compare August 30, 2017 23:36
@wking
Copy link
Contributor Author

wking commented Aug 30, 2017

@Mashimiao had expected this unified approach to be long and complicated, but it ended up dropping over 20 lines even with the extra bookkeeping for configSys and highestMatchedConfig (which are used in the improved error messages).

@wking wking force-pushed the combine-mount-order-and-existence-checks branch from 401f3b6 to 60ab7ad Compare August 30, 2017 23:44
@Mashimiao
Copy link

The changes are good, but I don't think something like system mount 1 really make sense. we don't need to care about what it really mounted number, we just need to care about if it is mounted in order.
And system mount is not easy understand.
I think mounts[2] {/dev devtmpfs devtmpfs []} MUST be mounted in order after mounts[0] {/tmp tmpfs none []}, but not may be better.

…Order

Also increase the error message detail and continue through the
remaining mounts instead of breaking on the first missing/misordered
mount.  Based on previous discussion in [1,2].  With this commit,
a configuration like:

  "mounts": [
    {
      "destination": "/tmp",
      "type": "tmpfs",
      "source": "none"
    },
    {
      "destination": "/tmp",
      "type": "tmpfs",
      "source": "none"
    },
    {
      "destination": "/dev",
      "type": "devtmpfs",
      "source": "devtmpfs"
    }
  ]

and mountinfo like:

  $ grep -n '/dev \|/tmp ' /proc/self/mountinfo
  2:19 17 0:6 / /dev rw,nosuid,relatime - devtmpfs devtmpfs rw,size=10240k,nr_inodes=2043951,mode=755
  25:41 17 0:38 / /tmp rw,relatime - tmpfs none rw

will generate errors like:

  * mounts[1] {/tmp tmpfs none []} does not exist
  * mounts[2] {/dev devtmpfs devtmpfs []} mounted before mounts[0] {/tmp tmpfs none []}

Before this commit, the error was just:

  * Mounts[1] /tmp is not mounted in order

I'd prefer errors like:

  * mounts[1] {/tmp tmpfs none []} does not exist
  * mounts[2] {/dev devtmpfs devtmpfs []} is system mount 1, while mounts[0] {/tmp tmpfs none []} is system mount 24

where grep reports 2 and 25 because it's counting from one, and
runtimetest reports 1 and 24 because it's counting from zero, but Ma
prefers to not mention the system-mount order [3].

[1]: opencontainers#444 (comment)
[2]: opencontainers#444 (comment)
[3]: opencontainers#456 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking force-pushed the combine-mount-order-and-existence-checks branch from 60ab7ad to bcb5379 Compare August 31, 2017 18:35
@wking
Copy link
Contributor Author

wking commented Aug 31, 2017 via email

@Mashimiao
Copy link

Mashimiao commented Sep 1, 2017

LGTM

Approved with PullApprove

1 similar comment
@liangchenye
Copy link
Member

liangchenye commented Sep 1, 2017

LGTM

Approved with PullApprove

@zhouhao3 zhouhao3 merged commit 06bd267 into opencontainers:master Sep 1, 2017
@wking wking deleted the combine-mount-order-and-existence-checks branch September 6, 2017 16:33
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.

4 participants