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

add new generate options for setting cpu/mem #175

Merged
merged 1 commit into from
Sep 6, 2016

Conversation

haiyanmeng
Copy link
Contributor

Signed-off-by: Haiyan Meng hmeng@redhat.com

@haiyanmeng
Copy link
Contributor Author

@mrunalp , PTAL.

@haiyanmeng
Copy link
Contributor Author

The test command is here:

ocitools generate --output config.json --cpu-shares 512 \
   --cpu-period 10000 \
   --cpu-quota 20000 \
   --realtime-runtime 35000000 \
   --realtime-period 343439999 \
   --cpus 1-3 \
   --mems 0 \
   --mem-limit 999000000 \
   --mem-reservation 6660000 \
   --mem-swap 1000000000 \
   --mem-kernel-limit 450000000 \
   --mem-kernel-tcp 3000000 \
   --mem-swappiness 30

@@ -234,6 +234,45 @@ inside of the container.
The special *PATH* `host` removes any existing UTS namespace from
the configuration.

**--cpu-shares**=CPUSHARES
Sets the CPU shares (relative weight (ratio) vs. other cgroups with cpu shares).
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having two descriptions to discuss, I'd rather just copy the runtime-spec wording verbatim here. For this setting, that's:

Specifies a relative share of CPU time available to the tasks in a cgroup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking , thanks for pointing out.
In fact, the current description I am using is from https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc1/specs-go/config.go#L272 .
Is there any plan on runtime-spec side to sync the descriptions on the config-linux.md and config.go?

Copy link
Contributor

Choose a reason for hiding this comment

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

On Tue, Aug 02, 2016 at 12:28:18PM -0700, hmeng-19 wrote:

In fact, the current description I am using is from
https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc1/specs-go/config.go#L272
. Is there any plan on runtime-spec side to sync the descriptions
on the config-linux.md and config.go?

Heh. I'm not aware of a plan, but I'll file a PR updating the Go
comments this evening if nobody beats me to it. And if you feel moved
to PR the update, don't wait for me ;).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking , go ahead with the runtime-spec PR to fix it.

@wking
Copy link
Contributor

wking commented Aug 2, 2016

On Tue, Aug 02, 2016 at 12:00:25PM -0700, hmeng-19 wrote:

The test command is here:

As ‘ocitools generate’ grows, it's probalby worth adding a
bats/sharness/… test suite locally to exercise all these options for
us. I think we've passed that tipping point already, and am happy to
PR something if ocitools maintainers agree (and have a test-suite
preference, otherwise I'll probably use sharness).

@haiyanmeng
Copy link
Contributor Author

@wking , @mrunalp , PTAL.

@@ -234,6 +234,45 @@ inside of the container.
The special *PATH* `host` removes any existing UTS namespace from
the configuration.

**--cpu-shares**=CPUSHARES
Copy link
Contributor

Choose a reason for hiding this comment

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

Options in this man page are currently alphabetized (since #44). That's an easier pattern to follow than grouping related options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking , will fix it. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may be worth using --linux-cpu-shares here to leave room for other platforms with their own interpretation of CPU shares. That will help avoid confusion when someone PRs --solaris-ncpus, etc. It's possible that there is common ground here and we could have a cross-platform --cpu-shares option, but if that's the case I'd rather have runtime-spec leading the push towards unification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrunalp , how to think about this?

@mrunalp
Copy link
Contributor

mrunalp commented Aug 2, 2016

@wking 👍 to a test harness. I don't care what it is as long as we have something 😉

@haiyanmeng
Copy link
Contributor Author

@wking , @mrunalp , PTAL.
Rename the options to be --linux-*;
Reorder the options in man/ocitools-generate.1.md.

@haiyanmeng
Copy link
Contributor Author

The new test command is:

ocitools generate --output config.json \
   --linux-cpu-shares 512 \
   --linux-cpu-period 10000 \
   --linux-cpu-quota 20000 \
   --linux-realtime-runtime 35000000 \
   --linux-realtime-period 343439999 \
   --linux-cpus 1-3 \
   --linux-mems 0 \
   --linux-mem-limit 999000000 \
   --linux-mem-reservation 6660000 \
   --linux-mem-swap 1000000000 \
   --linux-mem-kernel-limit 450000000 \
   --linux-mem-kernel-tcp 3000000 \
   --linux-mem-swappiness 30

@mrunalp
Copy link
Contributor

mrunalp commented Aug 5, 2016

Needs rebase?

@haiyanmeng
Copy link
Contributor Author

@mrunalp , rebased it. PTAL.

Sets the limit of memory usage in bytes.

**--linux-mem-reservation**=MEMRESERVATION
sets the soft limit of memory usage in bytes.
Copy link
Contributor

Choose a reason for hiding this comment

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

“sets” → “Sets”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it.

Specifies a relative share of CPU time available to the tasks in a cgroup.

**--linux-cpu-period**=CPUPERIOD
Specifies a period of time in microseconds for how regularly a cgroup's access to CPU resources should be reallocated (CFS scheduler only).
Copy link
Contributor

Choose a reason for hiding this comment

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

“CFS only” seems like we want a --host-specific guard to error out if someone tries to set it with --host-specific when they aren't using a CFS scheduler. But after reading over sched(7) and sched_setscheduler(2), I'm not quite sure how to do that. It looks like you check for a SCHED_OTHER policy, but sched_getscheduler is per-thread. Is the scheduler policy really set on a per-thread basis? Is a host-specific check just supposed to look for internal consistency (e.g. the config doesn't set both a CFS-only and a realtime-only parameter)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking , I think there are two ways we can use to determine the current scheduler.

  1. Read the kernel config file, refer to https://en.wikipedia.org/wiki/Completely_Fair_Scheduler:
[hmeng@localhost boot]$ grep CONFIG_SCHED_AUTOGROUP /boot/config-4.6.4-201.fc23.x86_64 
CONFIG_SCHED_AUTOGROUP=y
  1. check the /sys/fs/cgroup/cpu dir to see whether cpu.cfs_period_us exists or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Fri, Aug 05, 2016 at 11:58:48AM -0700, hmeng-19 wrote:

@wking , I think there are two ways we can use to determine the current scheduler.

  1. Read the kernel config file, refer to https://en.wikipedia.org/wiki/Completely_Fair_Scheduler:
[hmeng@localhost boot]$ grep CONFIG_SCHED_AUTOGROUP /boot/config-4.6.4-201.fc23.x86_64 
CONFIG_SCHED_AUTOGROUP=y
  1. check the /sys/fs/cgroup/cpu dir to see whether cpu.cfs_period_us exists or not.

These both sound like “does your kernel support that scheduler”, but
if scheduler policy is per-thread, then how do you know the container
will use that available scheduler (and not another available
scheduler)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking , @mrunalp , do correct me if I am wrong.
I always thought the scheduling policy is a system-level config. It is decided when you built your kernel. Right?

Copy link
Contributor

Choose a reason for hiding this comment

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

On Fri, Aug 05, 2016 at 08:25:27PM -0700, hmeng-19 wrote:

+--linux-cpu-period=CPUPERIOD

  • Specifies a period of time in microseconds for how regularly a cgroup's access to CPU resources should be reallocated (CFS scheduler only).

@wking , @mrunalp , do correct me if I am wrong.
I always thought the scheduling policy is a system-level config. It
is decided when you built your kernel. Right?

That was my impression before poking around in the man pages today.
But see also 1, which has:

Each process or thread is controlled by an associated scheduling
policy and priority…

And 2 has:

If at least one RT task is running, no other SCHED_OTHER task will
be allowed to run in any CPU.

which makes it fairly clear that you can have threads/processes with
different schedulers running concurrently. I'm not sure how that
works for the cgroup settings though. Maybe you can set them all
(both real-time and CFS parameters) and the kernel will apply the
appropriate limits to each task depending on its policy? In that
case, configs that specify settings for multiple schedulers are fair
game.

@mrunalp
Copy link
Contributor

mrunalp commented Aug 8, 2016

LGTM

@Mashimiao
Copy link

@hmeng-19 seems need rebase

Signed-off-by: Haiyan Meng <hmeng@redhat.com>
@haiyanmeng
Copy link
Contributor Author

@Mashimiao , @mrunalp , PTAL.

@mrunalp
Copy link
Contributor

mrunalp commented Sep 6, 2016

LGTM

@mrunalp mrunalp merged commit 089d971 into opencontainers:master Sep 6, 2016
wking pushed a commit to wking/ocitools-v2 that referenced this pull request Sep 6, 2016
Signed-off-by: Haiyan Meng <hmeng@redhat.com>

Backported to v1.0.0.rc1 from 092cb7c opencontainers#175 (cherry-pick applied
cleanly).

Signed-off-by: W. Trevor King <wking@tremily.us>
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