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

Json schema and examples #370

Merged
merged 2 commits into from
Apr 12, 2016

Conversation

vbatts
Copy link
Member

@vbatts vbatts commented Apr 6, 2016

Fixes #344

{
"hostID": 1000,
"containerID": 0,
"size": 10
Copy link
Contributor

Choose a reason for hiding this comment

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

32k will be a better example for the mappings.

Several fields needed the correct typing, and updates for recent changes.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@vbatts
Copy link
Member Author

vbatts commented Apr 7, 2016

updated. PTAL.

@wking
Copy link
Contributor

wking commented Apr 7, 2016

On Thu, Apr 07, 2016 at 07:14:17AM -0700, Vincent Batts wrote:

updated. PTAL.

It looks like fc6670d1e2a72d addresses all the points raised so
far, except for the version issue which we aren't sure how to handle
1.

There are still some zero values that don't make sense (e.g. in
linux.resources.cpu.period, but 2 has “The minimum quota allowed for
the quota or period is 1ms.”).

@vbatts
Copy link
Member Author

vbatts commented Apr 7, 2016

@wking fair. Though it also says that a -1 to quotas indicates no restriction, though we have it as a uint64. That does not line up.

I've updated the example.

"memory": {
"limit": 536870912,
"reservation": 0,
"swap": 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps make the swap same as the limit?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

On Fri, Apr 8, 2016 at 1:06 PM, Mrunal Patel notifications@github.com
wrote:

In config.md
#370 (comment)
:

  •            ]
    
  •        },
    
  •        "pids": {
    
  •            "limit": 32771
    
  •        },
    
  •        "hugepageLimits": [
    
  •            {
    
  •                "pageSize": "2MB",
    
  •                "limit": 9223372036854772000
    
  •            }
    
  •        ],
    
  •        "oomScoreAdj": 100,
    
  •        "memory": {
    
  •            "limit": 536870912,
    
  •            "reservation": 0,
    
  •            "swap": 0,
    

Perhaps make the swap same as the limit?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/runtime-spec/pull/370/files/e8ba1d05e1347edc6771a8133a4c729db6df3a50#r59057195

"swappiness": 0
},
"cpu": {
"shares": 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

To pick another suspicious example, it looks like 0 here is “never run this if anyone else a share value” (docs here). Maybe follow the kernel docs and use 1024?

@mrunalp
Copy link
Contributor

mrunalp commented Apr 11, 2016

@vbatts Could you address the latest comments from @wking ?

@vbatts
Copy link
Member Author

vbatts commented Apr 11, 2016

@mrunalp done.
PTAL.

@@ -267,9 +267,9 @@ The following parameters can be specified to setup the controller:

```json
"memory": {
"limit": 0,
"limit": 536870912,
"reservation": 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably not a good example of a soft-limit ;).

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

now I have a question. @mrunalp or @crosbymichael, does this reservation correspond to memory.soft_limit_in_bytes? if so, that can take a string like 1G or 256M. So is this uint64 value just the number of bytes for a soft limit?

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, it is supposed to be 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.

If we were to be pedantic, I'd rename all the fields to include the units. For example, reservation becomes reservationBytes

Copy link
Contributor

Choose a reason for hiding this comment

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

It is in bytes.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed. and add in bytes to the field docs

Copy link
Contributor

Choose a reason for hiding this comment

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

On Mon, Apr 11, 2016 at 03:52:52PM -0700, Vish Kannan wrote:

@@ -267,9 +267,9 @@ The following parameters can be specified to setup the controller:

     "memory": {
-        "limit": 0,
+        "limit": 536870912,
         "reservation": 0,

If we were to be pedantic, I'd rename all the fields to include the
units. For example, reservation becomes reservationBytes

Ugh :p. I think that's what docs are for ;).

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree. API should be user-friendly and readable. Having to keep
referring to docs while programming against an API is not a good user
experience. Simple changes like including units in the field names will
save a lot of time in the long run.

On Mon, Apr 11, 2016 at 4:01 PM, W. Trevor King notifications@github.com
wrote:

In config-linux.md
#370 (comment)
:

@@ -267,9 +267,9 @@ The following parameters can be specified to setup the controller:

     "memory": {
-        "limit": 0,
+        "limit": 536870912,
         "reservation": 0,

On Mon, Apr 11, 2016 at 03:52:52PM -0700, Vish Kannan wrote: > @@ -267,9
+267,9 @@ The following parameters can be specified to setup the
controller: > > ```json > "memory": { > - "limit": 0, > + "limit":
536870912, > "reservation": 0, If we were to be pedantic, I'd rename all
the fields to include the units. For example,reservationbecomes
`reservationBytes`
Ugh :p. I think that's what docs are for ;).


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/runtime-spec/pull/370/files/c1612af9b80dd828f8fccea3bfca5300281d37b9#r59296588

Copy link
Contributor

Choose a reason for hiding this comment

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

On Mon, Apr 11, 2016 at 04:08:14PM -0700, Vish Kannan wrote:

@@ -267,9 +267,9 @@ The following parameters can be specified to setup the controller:

     "memory": {
-        "limit": 0,
+        "limit": 536870912,
         "reservation": 0,

I disagree. API should be user-friendly and readable. Having to keep
referring to docs while programming against an API is not a good
user experience.

Some ideas don't pack down nicely into a JSON key, so I think not
referring to docs while programming against an API is a bad idea. If
the only interesting thing to say is “this is in bytes”, then reading
the docs won't take very long ;). That said, the kernel devs are with
you on this one (since they chose limit_in_bytes),

@wking
Copy link
Contributor

wking commented Apr 11, 2016

a341686c1612af look good to me, although it's still pretty easy to
find apparently-meaningless example values 1. I thought part of the
purpose of this PR was fixing these meaningless examples (see also
#348), but we can obviously push some of that to follow-up PRs /
issues if we don't want to check them now.

@mrunalp
Copy link
Contributor

mrunalp commented Apr 11, 2016

I think we can do cleanups in follow on PRs. This LGTM overall.

@mrunalp
Copy link
Contributor

mrunalp commented Apr 11, 2016

@crosbymichael @vishh PTAL

@vishh
Copy link
Contributor

vishh commented Apr 11, 2016

LGTM

* "complete" JSON example
* fix a couple of values
* fix a missing comma

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@mrunalp mrunalp merged commit 6734c7a into opencontainers:master Apr 12, 2016
@vbatts vbatts deleted the json_schema_and_examples branch April 12, 2016 02:09
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Apr 12, 2016
Through 6734c7a (Merge pull request opencontainers#370 from
vbatts/json_schema_and_examples, 2016-04-11).

The only unlisted changes to master were a brief run with ffjson
(opencontainers#343, opencontainers#351), but that was pulled out due to gccgo issues in opencontainers#363.

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Apr 12, 2016
Through 6734c7a (Merge pull request opencontainers#370 from
vbatts/json_schema_and_examples, 2016-04-11).

The only unlisted changes to master were a brief run with ffjson
(opencontainers#343, opencontainers#351), but that was pulled out due to gccgo issues in opencontainers#363.

Signed-off-by: W. Trevor King <wking@tremily.us>
vbatts pushed a commit to vbatts/oci-runtime-spec that referenced this pull request Apr 12, 2016
Through 6734c7a (Merge pull request opencontainers#370 from
vbatts/json_schema_and_examples, 2016-04-11).

The only unlisted changes to master were a brief run with ffjson
(opencontainers#343, opencontainers#351), but that was pulled out due to gccgo issues in opencontainers#363.

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Apr 20, 2016
To match where they're defined in the JSON Schema [1].  The old
location is from d4e7326 (config: JSON examples, 2016-04-06, opencontainers#370),
and seems to have been accidental.

[1]: https://github.com/opencontainers/runtime-spec/blob/0982071b288ddddc1ae84d21c4bd682c96942f5c/schema/schema-linux.json#L21-L48
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Apr 20, 2016
To match where they're defined in the JSON Schema [1].  The old
location is from d4e7326 (config: JSON examples, 2016-04-06, opencontainers#370),
and seems to have been accidental.

[1]: https://github.com/opencontainers/runtime-spec/blob/0982071b288ddddc1ae84d21c4bd682c96942f5c/schema/schema-linux.json#L21-L48

Signed-off-by: W. Trevor King <wking@tremily.us>
Mashimiao pushed a commit to Mashimiao/specs that referenced this pull request Aug 19, 2016
Through 6734c7a (Merge pull request opencontainers#370 from
vbatts/json_schema_and_examples, 2016-04-11).

The only unlisted changes to master were a brief run with ffjson
(opencontainers#343, opencontainers#351), but that was pulled out due to gccgo issues in opencontainers#363.

Signed-off-by: W. Trevor King <wking@tremily.us>
Mashimiao pushed a commit to Mashimiao/specs that referenced this pull request Aug 19, 2016
To match where they're defined in the JSON Schema [1].  The old
location is from d4e7326 (config: JSON examples, 2016-04-06, opencontainers#370),
and seems to have been accidental.

[1]: https://github.com/opencontainers/runtime-spec/blob/0982071b288ddddc1ae84d21c4bd682c96942f5c/schema/schema-linux.json#L21-L48

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.

None yet

4 participants