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/defs-linux: change weight type to uint16 #898

Merged
merged 1 commit into from
Dec 18, 2019

Conversation

0x0916
Copy link
Contributor

@0x0916 0x0916 commented Jul 14, 2017

Signed-off-by: 0x0916 w@laoqinren.net

Signed-off-by: 0x0916 <w@laoqinren.net>
@dqminh
Copy link
Contributor

dqminh commented Jul 17, 2017

LGTM

Approved with PullApprove

@stevvooe
Copy link

This change is probably not backwards compatible and against best practices. Do you have any documentation citing this as a uint16?

cc @tonistiigi @crosbymichael

@0x0916
Copy link
Contributor Author

0x0916 commented Jul 18, 2017

@stevvooe Please refer to https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#block-io

also, the kernel document: https://www.kernel.org/doc/Documentation/cgroup-v1/blkio-controller.txt

- blkio.weight
	- Specifies per cgroup weight. This is default weight of the group
	  on all the devices until and unless overridden by per device rule.
	  (See blkio.weight_device).
	  Currently allowed range of weights is from 10 to 1000.

@stevvooe
Copy link

@0x0916 How does that counter any of the concerns? The kernel API doesn't mention that this is restricted to a uint16. The specification currently calls for an integer and this requires a uint16. If others were expecting this to take something outside of a uint16, it would be considered backwards incompatible.

It is unlikely that this will break anyone, but I'd urge caution.

@dqminh
Copy link
Contributor

dqminh commented Jul 18, 2017

@stevvooe this should not break anyone, it also matches the type in spec-go which has been uint16 since forever. I think this is more like fixing a typo.

@stevvooe
Copy link

@dqminh Didn't we recently learn our lesson about liberal use of unsigned integers? The only use case of unsigned integers is when they map a field to a specific bit width. Unsigned integers should not be used to validate that something is positive or within a certain range of values. That is the job of validation.

@wking
Copy link
Contributor

wking commented Oct 4, 2017

To help this PR along, here are the current types for weight in the current master (3417098):

The weight entry in defs-linux.json (which is what this PR is touching as of 574182a) is also consumed by the leafWeight entries:

$ git describe
v1.0.0-39-g3417098
$ git grep -B2 definitions/weight schema/
schema/config-linux.json-                            "weight": {
schema/config-linux.json-                                "id": "https://opencontainers.org/schema/bundle/linux/resources/blockIO/weight",
schema/config-linux.json:                                "$ref": "defs-linux.json#/definitions/weight"
--
schema/config-linux.json-                            "leafWeight": {
schema/config-linux.json-                                "id": "https://opencontainers.org/schema/bundle/linux/resources/blockIO/leafWeight",
schema/config-linux.json:                                "$ref": "defs-linux.json#/definitions/weight"
--
schema/defs-linux.json-                    "properties": {
schema/defs-linux.json-                        "weight": {
schema/defs-linux.json:                            "$ref": "#/definitions/weight"
schema/defs-linux.json-                        },
schema/defs-linux.json-                        "leafWeight": {
schema/defs-linux.json:                            "$ref": "#/definitions/weight"

And leafWeight is also a unit16 in master:

$ git grep 'leafWeight.*OPTIONAL' config-linux.md
config-linux.md:* **`leafWeight`** *(uint16, OPTIONAL)* …
config-linux.md:    * **`leafWeight`** *(uint16, OPTIONAL)* …
$ git grep 'leafWeight' specs-go/
specs-go/config.go:     LeafWeight *uint16 `json:"leafWeight,omitempty"`
specs-go/config.go:     LeafWeight *uint16 `json:"leafWeight,omitempty"`

So the only thing in that whole chain that is not uint16 (modulo pointer) is the defs-linux.json entry being fixed in 574182a.

@stevvooe brings up #876, and I have no problem with runtime-spec maintainers deciding that weight and/or leafWeight should really have been int64 (or whatever). But whatever they have chosen as the type in the normative Markdown spec should be reflected in the informative, provided as-is JSON Schema and Go types provided alongside the spec. 574182a is making things consistent assuming the runtime-spec maintainers want to stick with the Markdown uint16. If they decide to switch to a different type, we'd need a separate PR. But just leaving things as they stand in master leaves the JSON Schema out of sync with the normative Markdown for no purpose.

@vbatts
Copy link
Member

vbatts commented Dec 17, 2019

house keeping: as this is in just the jsonschema and matches what the docs say, i'm cool with it

LGTM

Approved with PullApprove

@tianon
Copy link
Member

tianon commented Dec 18, 2019

LGTM

This also matches the Go implementation, which is convincing IMO.

Approved with PullApprove

@tianon tianon merged commit bab266e into opencontainers:master Dec 18, 2019
@vbatts vbatts mentioned this pull request Jan 9, 2020
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.

7 participants