add support for cpuset.mems #9536

hqhq · 2014-12-06T01:11:31Z

No description provided.

crosbymichael · 2014-12-06T02:00:49Z

Thanks, i'll update docker with the libcontainer change soon.

crosbymichael · 2014-12-06T02:01:51Z

runconfig/parse.go

We cannot remove this as it will break existing people using this but we can add more values.
flCpusetCpus = cmd.String([]string{"#-cpuset", "-cpuset-cpus"}, "", "CPUs in which to allow execution (0-3, 0,1)")

You are right, I'll change it soon.

hqhq · 2014-12-08T19:50:51Z

@crosbymichael please review again, I also fixed the tarsum test problem caused by this commit.

hqhq · 2014-12-25T02:39:47Z

ping.

LK4D4 · 2014-12-25T02:56:13Z

Isn't this breaking change for API?

hqhq · 2014-12-25T03:20:09Z

@LK4D4 which API do you mean?

LK4D4 · 2014-12-25T04:12:08Z

@hqhq Docker http API, for which in docs you changed Cpusets for CpusetCpus and CpusetMems

hqhq · 2014-12-25T08:10:06Z

Yes, but that's internal date, right? the only point that will affect user is the
command option, which is pointed out by crosbymichael, I have updated
to code to leave the option --cpuset as is was, only marked as deprecated.

SvenDowideit · 2014-12-29T06:59:41Z

The new flags need to be documented in the run and create sections of both cli.md and the man pages, and some further details are needed in run.md

hqhq · 2014-12-31T08:01:39Z

@SvenDowideit Thanks for your review, I did miss that, I have updated patches as you said,
but for run.md, I think cpuset.mems can be as simple as cpuset,cpus, what further details
did you mean to be added?

hqhq · 2014-12-31T09:58:59Z

update: add a patch for test case.

SvenDowideit · 2015-01-02T04:20:34Z

docs/man/docker-create.1.md

mmm, i think you will need to tell the reader what an MEMs is - CPU possibly not - @fredlf ?

ok, I'll add that, thanks.

Nah, I don't think we need to define CPU. But yes, we need to define MEMs.

SvenDowideit · 2015-01-02T04:21:45Z

@hqhq in run.md, we should also give the user an example, and explain what it does.

LK4D4 · 2015-01-02T20:29:49Z

@hqhq Docker API still used by users.

hqhq · 2015-01-03T03:37:24Z

@LK4D4 Hmmm.... what's your suggestion then? Should we keep using Cpuset
and just add CpusetMems?
Or do we have some deprecated tags for API?

LK4D4 · 2015-01-03T04:57:21Z

@hqhq I don't know actually :) There is way to check API version, but I'm not sure how this can help. Only way I see - deprecation in docs :) Maybe @crosbymichael knows better way

hqhq · 2015-01-05T06:17:57Z

@crosbymichael @LK4D4 what do you think if we go this way:

add CpusetCpus which has the same function as Cpuset, and keep using Cpuset;
add deprecated notify mechanism for docker http APIs, and mark Cpuset as deprecated;
remove Cpuset after a period of time;

LK4D4 · 2015-01-05T17:03:26Z

@hqhq I am okay with this. But we're not very good with removing things :) Maybe in 2.0.

fredlf · 2015-01-09T00:23:04Z

Agree with @SvenDowideit that we need an example in run.md and should define MEMs.

hqhq · 2015-01-12T01:52:12Z

@SvenDowideit @fredlf @LK4D4 @crosbymichael
Sorry for the delay, rebased and updated, please review again, thanks.

LK4D4 · 2015-01-12T03:02:22Z

docs/sources/reference/api/docker_remote_api_v1.12.md

Not sure that it is good idea to change docs for previous versions. ping @SvenDowideit

You are right, I think we should not change docs for previous version.
For now, change v1.16 and v1.17 is good enought.

hqhq · 2015-01-12T10:39:11Z

updated, remove the change of previous version of docs.

LK4D4 · 2015-01-12T16:41:38Z

@hqhq I'm not sure about 1.16 too, seems like this version was already released.

hqhq · 2015-01-20T06:08:38Z

No it won't break that, config.Cpuset is still used, you can see the changelog
in my first patch, that's how compatibility works for now.

LK4D4 · 2015-01-20T06:26:56Z

@hqhq sorry if I doesn't get, but here:
hqhq@bfea793#diff-6ce9e79ddb91a3f06352abe1f2c72ecbR280
I see that now config.Cpuset is not passed to execdriver. So, if I understand right then Cpuset is still present in config but not used for container setup. Maybe I just don't understand what's going on :) ping @crosbymichael

hqhq · 2015-01-20T06:36:31Z

Yes, it is not passed to execdriver, config.Cpuset is only set for showing
users the config info, because the value is the same as hostconfig.CpusetCpus,
so in other parts of docker code, we use hostconfig.CpusetCpus instead.

One exception is that when users use CpusetCpus in jaon for their http APIs,
we won't setup config.Cpuset because the user won't expect that.

LK4D4 · 2015-01-20T06:46:14Z

@hqhq I'm talking about case when use use Cpuset in json. There is no code which setups CpusetCpus from this, it was before in merge, but now I don't see it :/

As we will add CpusetMems, CpusetCpus is definitely a better name, but some users are already using Cpuset in their http APIs, so we need time to change it, right now we just make it compatible. A significant change is that we add CpusetCpus in hostConfig, not Config, because it's host dependent. For compatibility, the main idea is keep using Cpuset in Config Struct, and make it has the same value as CpusetCpus, but not always, some scenarios: - Users use --cpuset in docker command, it can setup cpuset.cpus and can get Cpuset field from docker inspect or other http API which will get config info. - Users use --cpuset-cpus in docker command, ditto. - Users use Cpuset field in their http APIs, ditto. - Users use CpusetCpus field in their http APIs, they won't get Cpuset field in Config info, because by then, they should already know what happens to Cpuset. Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>

Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>

hqhq · 2015-01-20T09:34:17Z

@LK4D4 Yep, that's the point I've missed in this version, sorry about that,
since we don't put CpusetCpus in config, so merge function is no longer
the right place, I put the sync in ContainerHostConfigFromJob, when
container start, CpusetCpus will get the right value if people still use Cpuset.

Update:

add sync for CpusetCpus and Cpuset, when people use Cpuset in json,
it will still be workable.

LK4D4 · 2015-01-20T17:57:11Z

Now seems legit :)

crosbymichael · 2015-01-20T19:17:41Z

@LK4D4 @unclejack shouldn't we wait until we can move all the resource config options to the hostconfig instead of having 6 on Config and 2 on hostconfig?

unclejack · 2015-01-20T19:23:28Z

@crosbymichael Fair enough, I'll remove my approval.

crosbymichael · 2015-01-20T19:51:12Z

This is something that we will have to do(carry the pr) because @hqhq has already helped alot and has been awesome on expanding cgroup support for docker.

@hqhq sorry for the confusion with this PR. Cgroup resource options have been on the Config since forever. They are host specific options so being on a config makes images not portable as this can be committed to an image. HostConfig is the correct place for this but I just want to make sure when we make the switch we don't have any regressions and we move everything so that it's not confusing to the user, as these structs are also used in the API.

Let us discuss a little more about a game plan for these resource settings and we can do the work if any more changes are required.

Thanks again!!

hqhq · 2015-01-21T02:01:53Z

@crosbymichael agreed, we should stop all the confusion of cgroup resource options, but this might be done step by step, and I think this PR is a good place to start.

What we need to do now is to confirm a proper way to do the compatibility work, don't have any regressions and make sure people can know how to use it the right way. So we can copy it to all other options. And I'd like to do this work :)

What do you think? BTW, do you think the compatibility work in this PR is good enough for all other options?

crosbymichael · 2015-01-21T18:20:08Z

@hqhq that would be awesome if you are interested in doing this work.

I think at the API request level we should do the copy from Config to HostConfig for compatibility then null out the fields on Config.

Then if you just move all the cgroup resource options to hostconfig and make sure it's copied at the API level it would be good to go.

Thanks for your help on this!

SvenDowideit · 2015-01-27T00:22:41Z

TBH, the text of the docs is now sufficiently detailed, that an example isn't going to be useful - great job! If we were to add an example, it would draw more attention to it from readers that do not have a NUMA system, and we should not make them (aka the majority) jealous :)

Docs LGTM - @fredlf @jamtur01

jamtur01 · 2015-01-27T00:29:40Z

Should there be something in the run.md docs? Otherwise LGTM.

hqhq · 2015-01-27T03:39:07Z

Thanks for all your reviews, let's wait for PR #10298 right now.

fredlf · 2015-01-28T22:31:44Z

docs/man/docker-create.1.md

four, processes, Docker

fredlf · 2015-01-28T22:34:43Z

Copy edits, otherwise LGTM. Agree with Sven, example no longer really needed.

crosbymichael · 2015-02-06T21:42:25Z

We will close this one for now in favor of the work being done in #10298 to allow this change to be merged.

unclejack · 2015-04-06T14:43:09Z

@crosbymichael Can we reopen this PR now? #10298 has been merged.

hqhq · 2015-04-07T10:54:23Z

@unclejack codes are quite different, I just send a new PR to support cpuset.mems, #12139, please take a look.

crosbymichael reviewed Dec 6, 2014
View reviewed changes

hqhq force-pushed the hq_add_cpuset_mems branch from af842a5 to eb752a8 Compare December 8, 2014 19:48

crosbymichael mentioned this pull request Dec 9, 2014

Update libcontainer dec8 #9574

Closed

jamtur01 added the /project/doc label Dec 9, 2014

crosbymichael added this to the 1.5.0 milestone Dec 17, 2014

hqhq force-pushed the hq_add_cpuset_mems branch from eb752a8 to 9e27075 Compare December 31, 2014 07:56

SvenDowideit reviewed Jan 2, 2015
View reviewed changes

hqhq force-pushed the hq_add_cpuset_mems branch from 09efe67 to 368f585 Compare January 12, 2015 01:49

LK4D4 reviewed Jan 12, 2015
View reviewed changes

hqhq force-pushed the hq_add_cpuset_mems branch from 368f585 to 441e945 Compare January 12, 2015 10:37

hqhq added 3 commits January 20, 2015 15:10

add support for cpuset.mems

5d32cf5

Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>

cgroup: add test case for cpuset.mems

0319b1a

Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>

hqhq force-pushed the hq_add_cpuset_mems branch from 19049b3 to 0319b1a Compare January 20, 2015 09:28

crosbymichael modified the milestone: 1.5.0 Jan 21, 2015

SvenDowideit mentioned this pull request Jan 27, 2015

move resource options from Config to hostConfig #10298

Merged

fredlf reviewed Jan 28, 2015
View reviewed changes

docs/man/docker-create.1.md

Copy link

Contributor

fredlf Jan 28, 2015

Choose a reason for hiding this comment

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

four, processes, Docker

crosbymichael closed this Feb 6, 2015

hqhq mentioned this pull request Apr 7, 2015

add support for cpuset.mems #12139

Merged

hqhq deleted the hq_add_cpuset_mems branch April 16, 2015 00:55

add support for cpuset.mems #9536

add support for cpuset.mems #9536

Uh oh!

Conversation

hqhq commented Dec 6, 2014

Uh oh!

crosbymichael commented Dec 6, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq commented Dec 8, 2014

Uh oh!

hqhq commented Dec 25, 2014

Uh oh!

LK4D4 commented Dec 25, 2014

Uh oh!

hqhq commented Dec 25, 2014

Uh oh!

LK4D4 commented Dec 25, 2014

Uh oh!

hqhq commented Dec 25, 2014

Uh oh!

SvenDowideit commented Dec 29, 2014

Uh oh!

hqhq commented Dec 31, 2014

Uh oh!

hqhq commented Dec 31, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SvenDowideit commented Jan 2, 2015

Uh oh!

LK4D4 commented Jan 2, 2015

Uh oh!

hqhq commented Jan 3, 2015

Uh oh!

LK4D4 commented Jan 3, 2015

Uh oh!

hqhq commented Jan 5, 2015

Uh oh!

LK4D4 commented Jan 5, 2015

Uh oh!

fredlf commented Jan 9, 2015

Uh oh!

hqhq commented Jan 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq commented Jan 12, 2015

Uh oh!

LK4D4 commented Jan 12, 2015

Uh oh!

hqhq commented Jan 20, 2015

Uh oh!

LK4D4 commented Jan 20, 2015

Uh oh!

hqhq commented Jan 20, 2015

Uh oh!

LK4D4 commented Jan 20, 2015

Uh oh!

hqhq commented Jan 20, 2015

Uh oh!

LK4D4 commented Jan 20, 2015

Uh oh!

crosbymichael commented Jan 20, 2015

Uh oh!

unclejack commented Jan 20, 2015

Uh oh!

crosbymichael commented Jan 20, 2015

Uh oh!

hqhq commented Jan 21, 2015

Uh oh!