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

Mount test #81

Merged
merged 1 commit into from
Jun 7, 2016
Merged

Mount test #81

merged 1 commit into from
Jun 7, 2016

Conversation

liangchenye
Copy link
Member

validate the runtime mount information by using docker/pkg/mount
and validate the rootfs readonly setting

}

func validateMounts(spec *rspec.Spec) error {
fmt.Println("validating mounts")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably be using a logging framework for these so folks can adjust the verbosity.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use logrus which is already imported.

@@ -0,0 +1 @@
[code from](github.com/docker/docker/pkg/mount)
Copy link
Member

@cyphar cyphar Jun 2, 2016

Choose a reason for hiding this comment

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

Minor thing: If we're going to start pulling code from other repositories we should mention the license (I know it's the same as this project but it's a different project). We should also mention the commit ID of when we pulled it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, LICENCE.md and commit ID are added.

@liangchenye liangchenye force-pushed the mount branch 2 times, most recently from d6b98b3 to 49ecdec Compare June 2, 2016 09:11
@liangchenye
Copy link
Member Author

Updated, PTAL @mrunalp @wking

@wking
Copy link
Contributor

wking commented Jun 2, 2016

On Thu, Jun 02, 2016 at 02:13:36AM -0700, 梁辰晔 (Liang Chenye) wrote:

Updated, PTAL @mrunalp @wking

3f334dc49ecdec is forking and vendoring docker/pkg/mount? Can't
we get the Docker maintainers to agree on a more neutral place for
shared development? It seems inefficient to fork.

@liangchenye
Copy link
Member Author

liangchenye commented Jun 3, 2016

3f334dc49ecdec is forking and vendoring docker/pkg/mount? Can't
we get the Docker maintainers to agree on a more neutral place for
shared development? It seems inefficient to fork.

Yes, it is forking and vendoring docker/pkg/mount and does not take much time actually.
A neutral place is an elegant solution, the disadvantage is it still brings unnecessary code and I worried it will take a long time to convince Docker maintainers.


func validateMountsExistence(spec *rspec.Spec) error {
fmt.Println("validating mounts existence")
infos, err := mount.GetMounts()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: infos -> mounts ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The returned struct of mount.GetMounts() is named of 'Info", so I use infos previously.
Now it changes to mountInfos.

@mrunalp
Copy link
Contributor

mrunalp commented Jun 6, 2016

The check for mount options doesn't work for strictatime. It is passed as MS_STRICTATIME to the /dev mount syscall as it errors out when passed as an option. We should probably skip checking mount options or only check for ones that actually are visible in the mount output.

@wking
Copy link
Contributor

wking commented Jun 6, 2016

On Mon, Jun 06, 2016 at 02:38:39PM -0700, Mrunal Patel wrote:

It is passed as MS_STRICTATIME to the /dev mount syscall as it
errors out when passed as an option…

Just to clarify, that distinction is ‘mountflags’ vs. ‘data’ in
mount(2). Does the “doesn't work in ‘data’” restriction apply to only
a subset of filesystem-independent mount options 1?

@mrunalp
Copy link
Contributor

mrunalp commented Jun 7, 2016

@wking Yes, it mountflags vs data. And yes I would think that the filesystem-independent options may or may not be visible in the output.

Signed-off-by: liang chenye <liangchenye@huawei.com>
@liangchenye
Copy link
Member Author

Remove 'option' checking for now for three reasons:

  1. some flags like 'strictatime' are invisible but not all flags are not invisible.
    I did not see any rule about this in the mount page or document.
  2. options like 'newinstance' is invisible too
    Also I did not see any explanation about it.
  3. data like 'a=b' is could be detected, but the output format is a little different (mode=666 vs mode=0666). It will be complicated and may not cover all the differences.

@mrunalp
Copy link
Contributor

mrunalp commented Jun 7, 2016

LGTM. We need to remove cgroups mount from runc config which is non-standard. I'll create a follow-on PR for that.

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