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

schema/config-linux: add min/max limit for resource memory and cpu #533

Conversation

Mashimiao
Copy link

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

"$ref": "defs.json#/definitions/uint64Pointer"
"type": "integer",
"minimum": 1000,
"maximum": 1000000
Copy link
Member

Choose a reason for hiding this comment

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

Where do these magic numbers come from? If they come from the kernel, please document them in the config-linux.md file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Mashimiao Mashimiao force-pushed the schema-add-limit-config-linux branch 2 times, most recently from 52b0e19 to 89fd54d Compare August 19, 2016 01:52
@vbatts
Copy link
Member

vbatts commented Aug 29, 2016

@cyphar thoughts on this now?

@tianon
Copy link
Member

tianon commented Nov 4, 2016

friendly ping @cyphar ❤️ 🙏

@cyphar
Copy link
Member

cyphar commented Nov 5, 2016

I'm confused what this will look like as an actual Go structure. Will it be an interface{}?

@wking
Copy link
Contributor

wking commented Nov 5, 2016

On Fri, Nov 04, 2016 at 08:01:49PM -0700, Aleksa Sarai wrote:

I'm confused what this will look like as an actual Go structure.

They'll be *uint64, just like they are now. The current uint64Pointer
definition is just like what @Mashimiao is adding here 1, except
with different limits on the integer case 2.

@crosbymichael
Copy link
Member

crosbymichael commented Nov 7, 2016

How is this going to used at the system level?

@wking
Copy link
Contributor

wking commented Nov 7, 2016

On Mon, Nov 07, 2016 at 09:16AM -0800,, Michael Crosby wrote:

How is this going to used at the system level?

Is that “who consumes the JSON Schema”? There's currently schema/validate.go in this repo which lets anyone check their JSON against the official schema. And I've floated opencontainers/runtime-tools#197 to lean on those JSON Schemas for runtime-tools' validation. I'd expect folks building a new config or checking a config they've just received from someone else will want to validate that config before attempting to launch a container from it.

@Mashimiao
Copy link
Author

@opencontainers/runtime-spec-maintainers PTAL

@crosbymichael
Copy link
Member

This would be validating system settings. My feeling is that we don't need to spec out the kernel, its not our job.

REJECT

from me

@wking
Copy link
Contributor

wking commented Feb 2, 2017

This would be validating system settings. My feeling is that we don't need to spec out the kernel, its not our job.

I don't think we're ready for a hard line on that issue, since we currently have a fair amount of in-spec kernel documentation (e.g. here, here, here, …). But maybe @jlbutler will get us pointed more clearly in his reroll of #673.

And if we want to punt docs to the kernel, we should probably require that linux.resources.cpu.period have the same semantics as cpu.cfs_period_us. Otherwise it is not clear what we're punting to.

@Mashimiao
Copy link
Author

@crosbymichael we also validate range of blkio.weight in schema

@Mashimiao Mashimiao force-pushed the schema-add-limit-config-linux branch from 89fd54d to c315e34 Compare February 3, 2017 09:07
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@Mashimiao Mashimiao force-pushed the schema-add-limit-config-linux branch from c315e34 to a8076eb Compare February 3, 2017 09:08
@mrunalp
Copy link
Contributor

mrunalp commented Feb 22, 2017

Agree with @crosbymichael, Let's not add limits for values.

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.

8 participants