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.

Sign up for GitHub

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

Jump to bottom

Improve setupSystemd, grab mount options from the host #8127

Merged
merged 1 commit into from
Oct 30, 2020
Merged

Improve setupSystemd, grab mount options from the host #8127

merged 1 commit into from
Oct 30, 2020

Conversation

andylibrian
Copy link
Contributor

For systemd with cgroupsv1, the mount options is currenlty hard coded:

[]string{"bind", "nodev", "noexec", "nosuid", "rprivate"}

As discussed in PR #7640, we want to grab the mount options from the host.

fixes #7661

I'm not quite sure if there is any existing test that can hit cgroupsv1 only, to cover this scenario specifically.

rhatdan
rhatdan approved these changes Oct 25, 2020
View reviewed changes
libpod/container_internal_linux.go Outdated
@@ -38,6 +38,7 @@ import (
"github.com/containers/storage/pkg/archive"
"github.com/containers/storage/pkg/idtools"
securejoin "github.com/cyphar/filepath-securejoin"
"github.com/moby/sys/mountinfo"
Copy link
Member

Choose a reason for hiding this comment

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

Could you use "github.com/containers/storage/pkg/mount"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sure. I wasn't aware of that. I've just pushed the update. Let me know if I should squash the commits.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andylibrian, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2020
@andylibrian andylibrian force-pushed the grab-systemd-mount-flags-from-the-host-7661 branch from cf9e5a7 to 52cb6d9 Compare October 25, 2020 12:45
mheon
mheon reviewed Oct 25, 2020
View reviewed changes
libpod/container_internal_linux.go Outdated
}

if hostMount == nil {
return errors.New("/sys/fs/cgroup/systemd is not mounted on the host")
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this errors.Wrapf(define.ErrInternal, "/sys/fs/cgroup/systemd is not mounted on the host") - we generally try to have all errors from Libpod wrap one of our defined errors whenever we can.

@mheon
Copy link
Member

mheon commented Oct 25, 2020

One nit otherwise LGTM

@rhatdan
Copy link
Member

rhatdan commented Oct 25, 2020

Fix @mheon issue and LGTM

@andylibrian
Copy link
Contributor Author

I've just applied @mheon suggestion

giuseppe
giuseppe reviewed Oct 26, 2020
View reviewed changes
libpod/container_internal_linux.go Outdated
Comment on lines 672 to 688
hostMounts, err := mount.GetMounts()
if err != nil {
return err
}

var hostMount *mount.Info
for _, m := range hostMounts {
if m != nil && m.Mountpoint == "/sys/fs/cgroup/systemd" {
hostMount = m
break
}
}

if hostMount == nil {
return errors.Wrapf(define.ErrInternal, "/sys/fs/cgroup/systemd is not mounted on the host")
}

Copy link
Member

Choose a reason for hiding this comment

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

this is a bit expensive.

Could we just use statfs("/sys/fs/cgroup/systemd") and read f_flags from there?

@rhatdan
Copy link
Member

rhatdan commented Oct 27, 2020

@andylibrian Could you make the change @giuseppe requested so we can get this merged.

@andylibrian
Copy link
Contributor Author

I've just pushed an update to use statfs to read the mount options. With statfs we need to build the options from flags like this:

		if statfs.Flags&unix.MS_NODEV == unix.MS_NODEV {
			mountOptions = append(mountOptions, "nodev")
		}
		if statfs.Flags&unix.MS_NOEXEC == unix.MS_NOEXEC {
			mountOptions = append(mountOptions, "noexec")
		}
		if statfs.Flags&unix.MS_NOSUID == unix.MS_NOSUID {
			mountOptions = append(mountOptions, "nosuid")
		}
		if statfs.Flags&unix.MS_RDONLY == unix.MS_RDONLY {
			mountOptions = append(mountOptions, "ro")
		}

This works on ubuntu 20.04.1 LTS (Focal Fossa) but I'm not sure if there is any more flags needed for other cases.

@rhatdan @giuseppe

mheon
mheon reviewed Oct 29, 2020
View reviewed changes
libpod/container_internal_linux.go
systemdMnt := spec.Mount{
Destination: "/sys/fs/cgroup/systemd",
Type: "bind",
Source: "/sys/fs/cgroup/systemd",
Options: []string{"bind", "nodev", "noexec", "nosuid", "rprivate"},
Options: mountOptions,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should probably force rprivate even if not used by the host - @giuseppe @rhatdan Agree?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. We should always add rprivate

giuseppe
giuseppe reviewed Oct 30, 2020
View reviewed changes
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

just a minor comment, could you please squash your patches into one?

libpod/container_internal_linux.go Outdated
@@ -668,11 +668,30 @@ func (c *Container) setupSystemd(mounts []spec.Mount, g generate.Generator) erro
}
g.AddMount(systemdMnt)
} else {
var statfs unix.Statfs_t
if err := unix.Statfs("/sys/fs/cgroup/systemd", &statfs); err != nil {
return errors.Wrapf(err, "/sys/fs/cgroup/systemd is not mounted on the host")
Copy link
Member

Choose a reason for hiding this comment

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

if it is not mounted, could we just use a set of default mount options?

Copy link
Member

Choose a reason for hiding this comment

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

Does that make sense given that we're trying to mount the same directory into the container? It seems like we ought to fail if we can't actually mount the cgroups in.

@andylibrian andylibrian force-pushed the grab-systemd-mount-flags-from-the-host-7661 branch from 701b64d to 6779c1c Compare October 30, 2020 13:53
@andylibrian
Copy link
Contributor Author

I've just pushed the update as suggested, squashed into a single commit as requested. There is still a discussion about whether we should throw an error or fallback to default mount options in case systemd is not mounted.

@mheon
Copy link
Member

mheon commented Oct 30, 2020

I think we're good to merge as-is; LGTM

@mheon
Copy link
Member

mheon commented Oct 30, 2020

@containers/podman-maintainers PTAL

vrothberg
vrothberg reviewed Oct 30, 2020
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@rhatdan
Copy link
Member

rhatdan commented Oct 30, 2020

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 30, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2020
@rhatdan
Copy link
Member

rhatdan commented Oct 30, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 30, 2020
@openshift-merge-robot openshift-merge-robot merged commit 7a68db3 into containers:master Oct 30, 2020
@andylibrian andylibrian deleted the grab-systemd-mount-flags-from-the-host-7661 branch October 31, 2020 03:24
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg left review comments

@mheon mheon mheon left review comments

@rhatdan rhatdan rhatdan approved these changes

@giuseppe giuseppe giuseppe left review comments

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Grab mount flags for systemd work from the host mount
7 participants
@andylibrian @openshift-ci-robot @mheon @rhatdan @giuseppe @vrothberg @openshift-merge-robot