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

runtimetest: add validation of cgroups #93

Merged

Conversation

Mashimiao
Copy link

Signed-off-by: Ma Shimiao [email protected]

@Mashimiao Mashimiao changed the title runtimetest: add validation of cgrup path runtimetest: add validation of cgrups path Jun 3, 2016
@Mashimiao Mashimiao changed the title runtimetest: add validation of cgrups path runtimetest: add validation of cgroups path Jun 3, 2016
func validateCgroupsPath(spec *rspec.Spec) error {
fmt.Println("validating cgroupsPath")
expectedPath := spec.Linux.CgroupsPath
actualPath, err := getThisCgroupPath("devices")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we blessing devices? What if the runtime is using unified cgroups?

Copy link
Author

Choose a reason for hiding this comment

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

Well, We need many default devices to create container. So cgroup device should be hard requirement.
The spec says "The Spec does not support split hierarchy[https://www.kernel.org/doc/Documentation/cgroup-v2.txt]. The cgroups will be created if they don't exist."
unified cgroups may not to be supported?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs some spec work before we can implement something reliable. I expect we'll want to make this comparison for unified cgroups if the container process belongs to the unified hierarchy, and with (some?) v1 cgroups if the container does not belong to the unified hierarchy.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Thu, Jun 02, 2016 at 10:31:32PM -0700, Ma Shimiao wrote:

  • actualPath, err := getThisCgroupPath("devices")

Well, We need many default devices to create container. So cgroup
device should be hard requirement.

But just because you create devices doesn't mean the user is setting
anything in linux.resources.devices 1.

The spec says "The Spec does not support split
hierarchy[https://www.kernel.org/doc/Documentation/cgroup-v2.txt]. The
cgroups will be created if they don't exist." unified cgroups may
not to be supported?

No, that wording is “we don't support per-controller paths” for the
reasons discussed in cgroup-v2.txt 2. It's not about limiting the
cgroup version.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your information. I misunderstood the referred link.
I will make more investigation and update the PR later.

@wking
Copy link
Contributor

wking commented Jun 3, 2016

On Thu, Jun 02, 2016 at 07:39:53PM -0700, Ma Shimiao wrote:

  • runtimetest: add validation of cgrup path

226e9a3 has the “cgrup” → “cgroup” typo in the commit-message still.

@Mashimiao Mashimiao force-pushed the runtime-test-cgroup-path-validation branch from 226e9a3 to 946b75e Compare June 3, 2016 05:36
@cyphar
Copy link
Member

cyphar commented Jun 8, 2016

I agree with @wking that the runtime-spec still needs some more work around cgroupPath as well as general cgroup handling. I'll work on a PR for this soon. But for the moment we should probably hold off on this PR.

@Mashimiao
Copy link
Author

Mashimiao commented Jun 8, 2016

@cyphar OK, I will not update the PR until we have a clear status of cgroupPath

@Mashimiao
Copy link
Author

ping @wking @cyphar @mrunalp

validateHostname,
validateRlimits,
validateMountsExist,
}

linuxValidations := []validation{
validateDefaultFS,
validateDefaultDevices,
// validateDefaultDevices,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why comment this out?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, this is a mistake

@cyphar
Copy link
Member

cyphar commented Aug 1, 2016

I don't understand any of the uses of strings.Replace with paths (nor strings.Contains). If you're trying to convert a path so that it no longer contains leading ../s then you can use libcontainer/utils.CleanPath for that purpose. It cleans both absolute and relative paths in a sane way.

As for replacing : with /, I don't get what you're trying to do (are you trying to generate a systemd slice path from a name?).

@wking
Copy link
Contributor

wking commented Aug 1, 2016

On Mon, Aug 01, 2016 at 01:52:15PM -0700, Aleksa Sarai wrote:

If you're trying to convert a path so that it no longer contains
leading ../s then you can use libcontainer/utils.CleanPath for
that purpose.

I'd rather not introduce a libcontainer dependency, because ocitools
is intended to test runC/libcontainer (among other runtimes). But in
this case, I don't see a need to clean ../ at all, since they are not
mentioned in the current runtime-spec.

I'd be ok landing language in runtime-spec about ‘..’ being an illegal
path component in cgroupsPath, but we don't have that language now, so
ocitools shouldn't do anything special for such paths.

@Mashimiao Mashimiao force-pushed the runtime-test-cgroup-path-validation branch 2 times, most recently from e26d31b to 7a670e8 Compare August 2, 2016 09:43
return fmt.Errorf("Cgroup path expected: %v, actual: %v", *expectedPath, actualPath)
}
} else {
if _, err := filepath.Rel(*expectedPath, actualPath); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Basically, this branch should either be a noop or should create a new container and compare the value. Those are the only two things we can safely do here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It currently warns about the MAY violation, which I think is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although I think Rel erroring isn't the right condition. We want to warn if the path returned by Rel starts with ...

Copy link
Contributor

Choose a reason for hiding this comment

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

And “starts with ..” should be “the first path segment (split on the path separator) is ..” not “the path string starts with the two characters ..”.

@Mashimiao Mashimiao added this to the v0.4.0 milestone Nov 1, 2017
@Mashimiao Mashimiao force-pushed the runtime-test-cgroup-path-validation branch from 7a670e8 to 5d65c3d Compare December 5, 2017 12:00
@Mashimiao
Copy link
Author

Test all cgroups except devices subsystem for croupv1

@Mashimiao Mashimiao changed the title runtimetest: add validation of cgroups path runtimetest: add validation of cgroups Dec 5, 2017
@Mashimiao
Copy link
Author

ping @opencontainers/runtime-tools-maintainers

@Mashimiao Mashimiao force-pushed the runtime-test-cgroup-path-validation branch 2 times, most recently from 1d98c84 to 3d31feb Compare December 5, 2017 12:29
@Mashimiao Mashimiao force-pushed the runtime-test-cgroup-path-validation branch from 3d31feb to 6351044 Compare December 5, 2017 13:44
}

r.SetID(uuid.NewV4().String())
stderr, err := r.Create()
Copy link
Member

Choose a reason for hiding this comment

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

no 'r.start'?

Copy link
Contributor

Choose a reason for hiding this comment

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

no 'r.start'?

What would you start? The bulk of the container is setup by the time create returns, and the runtime's dummy process is perfect for holding the container open while it is inspected from outside. So I don't think we want a start call in RuntimeOutsideValidate.

}

if f != nil {
if err := f("test"); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this hard-coded test. Don't we want to pass the bundle path, like we do for RuntimeInsideValidate?

Copy link
Author

Choose a reason for hiding this comment

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

No, here just to pass absolute cgroup path.
I will update PRs to support absolute and relative cgroup path validation

if err != nil {
return err
}
lbd, err := cg.GetBlockIOData(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think path should point to the bundle directory, which means we'll need something else for finding the cgroup path. I think we want two tests for absolute cgroupsPath:

Ma Shimiao added 2 commits December 6, 2017 11:15
@Mashimiao
Copy link
Author

Add support for relative cgrouppath test and add cgroup tests related with it


var (
// AbsCgroupPath is absolute path for container's cgroup mount
AbsCgroupPath = "/cgrouptest"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to put these two variables into the cgroups pkg?
Beside these, the PR looks good to me.

Copy link
Member

Choose a reason for hiding this comment

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

We can change it later if there is a better place to leave these.

@liangchenye
Copy link
Member

liangchenye commented Dec 13, 2017

LGTM

Approved with PullApprove

@zhouhao3
Copy link

zhouhao3 commented Dec 13, 2017

LGTM

Approved with PullApprove

@zhouhao3 zhouhao3 merged commit c3755c1 into opencontainers:master Dec 13, 2017
wking added a commit to wking/ocitools-v2 that referenced this pull request Apr 6, 2018
The previous values were giving me:

  container_linux.go:348: starting container process caused
    "process_linux.go:402: container init caused
    \"process_linux.go:367: setting cgroup config for procHooks
    process caused \\\"failed to write 56892210544640 to
    hugetlb.1GB.limit_in_bytes: open
    /sys/fs/cgroup/hugetlb/.../hugetlb.1GB.limit_in_bytes: permission
    denied\\\"\""

The previous values are originally from 432615a (add cgroup hugetlb
test for runtime, 2017-12-05, opencontainers#93), which doesn't motivate their
choice.  The new values are copy/pasted from the spec [1] (which
doesn't motivate its choice either ;).  I've kept something like
Alban's comment from 984dbc8 (Fix error messages in validation cgroup
tests, 2018-03-14, opencontainers#605) to at least explain where the number comes
from.

In testing with my local system, the issue seems to be pageSize and
not the limit value.  That seems to be supported by the kernel docs,
which have [2]:

	hugepages=	[HW,X86-32,IA-64] HugeTLB pages to allocate at boot.
	hugepagesz=	[HW,IA-64,PPC,X86-64] The size of the HugeTLB pages.
			On x86-64 and powerpc, this option can be specified
			multiple times interleaved with hugepages= to reserve
			huge pages of different sizes. Valid pages sizes on
			x86-64 are 2M (when the CPU supports "pse") and 1G
			(when the CPU supports the "pdpe1gb" cpuinfo flag).

My CPU supports both:

  $ cat /proc/cpuinfo | grep '^flags' | head -n1 | grep -o ' \(pse\|pdpe1gb\) '
   pse
   pdpe1gb

but I don't set hugepagesz, and I seem to only get 2M by default.  I
can get 1GB entries by booting with hugepagesz=1GB.  Longer-term, we
may want to auto-detect the value(s) currently enabled by the host
system, but for this commit I'm hard-coding 2MB.

[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.1/config-linux.md#example-8
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/kernel-parameters.txt?h=v4.16#n1336

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/ocitools-v2 that referenced this pull request Apr 6, 2018
The previous values were giving me:

  container_linux.go:348: starting container process caused
    "process_linux.go:402: container init caused
    \"process_linux.go:367: setting cgroup config for procHooks
    process caused \\\"failed to write 56892210544640 to
    hugetlb.1GB.limit_in_bytes: open
    /sys/fs/cgroup/hugetlb/.../hugetlb.1GB.limit_in_bytes: permission
    denied\\\"\""

The previous values are originally from 432615a (add cgroup hugetlb
test for runtime, 2017-12-05, opencontainers#93), which doesn't motivate their
choice.  The new values are copy/pasted from the spec [1] (which
doesn't motivate its choice either ;).  I've kept something like
Alban's comment from 984dbc8 (Fix error messages in validation cgroup
tests, 2018-03-14, opencontainers#605) to at least explain how the limit breaks
down.

In testing with my local system, the issue seems to be pageSize and
not the limit value.  That seems to be supported by the kernel docs,
which have [2]:

  hugepages=  [HW,X86-32,IA-64] HugeTLB pages to allocate at boot.
  hugepagesz= [HW,IA-64,PPC,X86-64] The size of the HugeTLB pages.
      On x86-64 and powerpc, this option can be specified
      multiple times interleaved with hugepages= to reserve
      huge pages of different sizes. Valid pages sizes on
      x86-64 are 2M (when the CPU supports "pse") and 1G
      (when the CPU supports the "pdpe1gb" cpuinfo flag).

My CPU supports both:

  $ cat /proc/cpuinfo | grep '^flags' | head -n1 | grep -o ' \(pse\|pdpe1gb\) '
   pse
   pdpe1gb

but I don't set hugepagesz, and I seem to only get 2M by default.  I
can get 1GB entries by booting with hugepagesz=1GB.  Longer-term, we
may want to auto-detect the value(s) currently enabled by the host
system, but for this commit I'm hard-coding 2MB.

[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.1/config-linux.md#example-8
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/kernel-parameters.txt?h=v4.16#n1336

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/ocitools-v2 that referenced this pull request Apr 7, 2018
The previous values were giving me:

  container_linux.go:348: starting container process caused
    "process_linux.go:402: container init caused
    \"process_linux.go:367: setting cgroup config for procHooks
    process caused \\\"failed to write 56892210544640 to
    hugetlb.1GB.limit_in_bytes: open
    /sys/fs/cgroup/hugetlb/.../hugetlb.1GB.limit_in_bytes: permission
    denied\\\"\""

The previous values are originally from 432615a (add cgroup hugetlb
test for runtime, 2017-12-05, opencontainers#93), which doesn't motivate their
choice.  The new values are copy/pasted from the spec [1] (which
doesn't motivate its choice either ;).  I've kept something like
Alban's comment from 984dbc8 (Fix error messages in validation cgroup
tests, 2018-03-14, opencontainers#605) to at least explain how the limit breaks
down.

In testing with my local system, the issue seems to be pageSize and
not the limit value.  That seems to be supported by the kernel docs,
which have [2]:

  hugepages=  [HW,X86-32,IA-64] HugeTLB pages to allocate at boot.
  hugepagesz= [HW,IA-64,PPC,X86-64] The size of the HugeTLB pages.
      On x86-64 and powerpc, this option can be specified
      multiple times interleaved with hugepages= to reserve
      huge pages of different sizes. Valid pages sizes on
      x86-64 are 2M (when the CPU supports "pse") and 1G
      (when the CPU supports the "pdpe1gb" cpuinfo flag).

My CPU supports both:

  $ cat /proc/cpuinfo | grep '^flags' | head -n1 | grep -o ' \(pse\|pdpe1gb\) '
   pse
   pdpe1gb

but I don't set hugepagesz, and I seem to only get 2M by default.  I
can get 1GB entries by booting with hugepagesz=1GB.  Longer-term, we
may want to auto-detect the value(s) currently enabled by the host
system, but for this commit I'm hard-coding 2MB.

[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.1/config-linux.md#example-8
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/kernel-parameters.txt?h=v4.16#n1336

Signed-off-by: W. Trevor King <[email protected]>
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.

5 participants