fix blkio related validation by Mashimiao · Pull Request #545 · opencontainers/runtime-spec

Mashimiao · 2016-08-29T04:02:50Z

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

vbatts · 2016-08-29T14:53:36Z

@cyphar are these values that you say needed to stay as pointers?

cyphar · 2016-08-29T15:01:25Z

I'm fairly sure they do need to remain as pointers. Though, IIRC @wking mentioned something about this a few days ago (saying that there are some open issues about the pointer as dont-touch-this semantics).

wking · 2016-08-29T18:30:39Z

On Mon, Aug 29, 2016 at 08:01:27AM -0700, Aleksa Sarai wrote:

I'm fairly sure they do need to remain as pointers. Though, IIRC
@wking mentioned something about this a few days ago (saying that
there are some open issues about the pointer as dont-touch-this
semantics).

Pointers make it easy to say “leave it alone”, but they don't give you
a clear way to say “explicitly set this to unlimited” 1.

But in this case, blkioThrottleReadBpsDevice and friends are “array or
null”, and the array entries are not pointers. So I'm fine with
that part of 5d47014.

The uint16 → int16 change for major/minor matches the spec in
config-linux.md and blockIODevice in the Go types. But I don't see a
reason to allow negative values there, so we may want to consolidate
around uints instead.

And the Major/Minor touched by 5d47014 are not pointers, but the
versions used in the spec's cgroup entries are optional 2. So we'll
probably need majorPointer and minorPointer (although currently
linux.resources.devices isn't represented in the JSON Schema at all).

Mashimiao · 2016-09-29T08:08:17Z

ping @vbatts any comments?
This is based on the current specs-go. I think we can pass this if there is no plan to modify specs-go recently.
We need to validate based on the latest specs-go.

Mashimiao · 2016-12-19T03:07:20Z

@opencontainers/runtime-spec-maintainers PTAL

hqhq · 2016-12-19T06:00:40Z

schema/config-linux.json

                            "blkioWeight": {
                                "id": "https://opencontainers.org/schema/bundle/linux/resources/blockIO/blkioWeight",
-                                "$ref": "defs-linux.json#/definitions/blkioWeightPointer"
+                                "$ref": "defs-linux.json#/definitions/blkioWeight"


I see blkioWeight is still pointer in specs-go.

I have modified blkioWeight to uint16Pointer as it is typed in spec-go, so blkioWeightPointer is not needed any more

ok, I see, thanks.

hqhq · 2016-12-19T08:00:26Z

LGTM

Mashimiao · 2016-12-23T04:43:45Z

@vbatts @mrunalp PTAL

mrunalp · 2017-01-11T22:48:45Z

@hqhq does this still make sense after your recent PR about negative values?

wking · 2017-01-12T00:15:19Z

schema/config-linux.json

+                                        }
+                                    },
+                                    {
+                                        "type": "null"


We probably don't want the null alternative since null alternatives for arrays were just rejected in #555.

fixed, thanks.

wking · 2017-01-12T00:18:29Z

On Wed, Jan 11, 2017 at 02:48:46PM -0800, Mrunal Patel wrote: @hqhq does this still make sense after your recent PR about negative values?

That's a reference to #648.

Mashimiao · 2017-01-12T01:40:49Z

@hqhq @wking @mrunalp @vbatts PTAL

hqhq · 2017-01-12T01:50:59Z

@mrunalp #648 didn't affect blkio fields, this PR looks more like cleanup with a minor fix, so it still looks good to me.

LGTM

wking · 2017-01-12T01:51:22Z

schema/defs-linux.json

        "blkioWeight": {
-            "type": "integer",
-            "minimum": 10,
-            "maximum": 1000


I think we want to keep this.

Yeah, it seems keep this is better, updated.

Mashimiao · 2017-01-12T06:22:07Z

According to #555 remove null option for array.
@hqhq @wking @mrunalp @vbatts PTAL

wking · 2017-01-12T06:41:45Z

schema/config-linux.json

-                ]
+                "type": "array",
+                    "items": {
+                     "$ref": "defs-linux.json#/definitions/Device"


Inconsistent indent (try cd schema && make fmt).

fixed, thanks

wking · 2017-01-12T06:45:48Z

schema/config-linux.json

                                "id": "https://opencontainers.org/schema/bundle/linux/resources/blockIO/blkioWeightDevice",
                                "type": "array",
                                "items": {
-                                    "$ref": "defs-linux.json#/definitions/blockIODeviceWeightPointer"


As of 61e0da0, you still need to remove the definition for this from defs-linux.json.

fixed, thanks

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

wking · 2017-01-12T07:07:53Z

Through 58832f9 looks good to me.

Mashimiao · 2017-01-23T01:47:11Z

@hqhq @wking @mrunalp @vbatts PTAL

wking · 2017-01-23T04:28:39Z

On Sun, Jan 22, 2017 at 05:47:12PM -0800, Ma Shimiao wrote: @hqhq @wking @mrunalp @vbatts PTAL

This PR is still at 58832f9, which still looks good to me [1] ;). [1]: #545 (comment)

hqhq · 2017-01-23T06:22:10Z

LGTM

crosbymichael · 2017-01-23T18:32:22Z

LGTM

Mashimiao force-pushed the fix-blockio-related-validation branch from 5d47014 to bd8e3a5 Compare December 19, 2016 03:04

hqhq reviewed Dec 19, 2016

View reviewed changes

wking reviewed Jan 12, 2017

View reviewed changes

Mashimiao force-pushed the fix-blockio-related-validation branch from bd8e3a5 to 055f3fa Compare January 12, 2017 01:37

wking reviewed Jan 12, 2017

View reviewed changes

Mashimiao force-pushed the fix-blockio-related-validation branch from 055f3fa to 61e0da0 Compare January 12, 2017 06:20

wking reviewed Jan 12, 2017

View reviewed changes

fix blkio related validation

a04cce9

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

Mashimiao force-pushed the fix-blockio-related-validation branch from 61e0da0 to 58832f9 Compare January 12, 2017 06:59

config-linux: remove unneeded null for array

58832f9

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

Mashimiao mentioned this pull request Jan 23, 2017

schema: Drop pointers and nulls #662

Merged

crosbymichael merged commit d5a1269 into opencontainers:master Jan 23, 2017

vbatts mentioned this pull request Jan 27, 2017

Bump version for 1.0.0-rc4 #672

Merged

Conversation

Mashimiao commented Aug 29, 2016

Uh oh!

vbatts commented Aug 29, 2016

Uh oh!

cyphar commented Aug 29, 2016

Uh oh!

wking commented Aug 29, 2016

Uh oh!

Mashimiao commented Sep 29, 2016

Uh oh!

Mashimiao commented Dec 19, 2016

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!

hqhq commented Dec 19, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mashimiao commented Dec 23, 2016

Uh oh!

mrunalp commented Jan 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Jan 12, 2017 via email

Uh oh!

Mashimiao commented Jan 12, 2017

Uh oh!

hqhq commented Jan 12, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mashimiao commented Jan 12, 2017

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!

Choose a reason for hiding this comment

Uh oh!

wking commented Jan 12, 2017 via email

Uh oh!

Mashimiao commented Jan 23, 2017

Uh oh!

wking commented Jan 23, 2017 via email

Uh oh!

hqhq commented Jan 23, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crosbymichael commented Jan 23, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hqhq commented Dec 19, 2016 •

edited by caniszczyk

Loading

hqhq commented Jan 12, 2017 •

edited by caniszczyk

Loading

hqhq commented Jan 23, 2017 •

edited by caniszczyk

Loading

crosbymichael commented Jan 23, 2017 •

edited by caniszczyk

Loading