(re)introduce some of the compose v2 attributes#78
(re)introduce some of the compose v2 attributes#78ndeloof merged 1 commit intocompose-spec:masterfrom
Conversation
EricHripko
left a comment
EricHripko
left a comment
There was a problem hiding this comment.
Neat 👍 Was reading through this and spotted a pair of typos - so decided to chip in
EricHripko
left a comment
EricHripko
left a comment
There was a problem hiding this comment.
Almost there - got a few comments inline 💬 I've also noticed that build.shm_size, blkio_limit and blkio_weight don't seem to have descriptions in this PR. Should they be documented too?
| "oom_score_adj": {"type": "integer", "minimum": -1000, "maximum": 1000}, | ||
| "pid": {"type": ["string", "null"]}, | ||
|
|
||
| "pids_limit": {"type": ["number", "string"]}, |
There was a problem hiding this comment.
Can we put this under resources?
There was a problem hiding this comment.
We are discussing re-introducing support for V2 schema attributes, this doesn't prevent another discussion about deprecating some of them and introduce a better way to define equivalent feature
| "items": {"$ref": "#/definitions/blkio_limit"} | ||
| }, | ||
| "device_read_iops": { | ||
| "type": "array", | ||
| "items": {"$ref": "#/definitions/blkio_limit"} | ||
| }, | ||
| "device_write_bps": { | ||
| "type": "array", | ||
| "items": {"$ref": "#/definitions/blkio_limit"} | ||
| }, | ||
| "device_write_iops": { | ||
| "type": "array", | ||
| "items": {"$ref": "#/definitions/blkio_limit"} |
There was a problem hiding this comment.
We should consider re-designing this, e.g. to group these options per device
devices:
- device: <device>
iops: <x>
bps: <x>There was a problem hiding this comment.
I also think so.
We also should define an oci (or linux or any comparable name) for cgroup related stuff and raw linux kernel features exposed by the OCI runtime
| "cpu_count": {"type": "integer", "minimum": 0}, | ||
| "cpu_percent": {"type": "integer", "minimum": 0, "maximum": 100}, | ||
| "cpu_shares": {"type": ["number", "string"]}, | ||
| "cpu_quota": {"type": ["number", "string"]}, | ||
| "cpu_period": {"type": ["number", "string"]}, | ||
| "cpu_rt_period": {"type": ["number", "string"]}, | ||
| "cpu_rt_runtime": {"type": ["number", "string"]}, | ||
| "cpus": {"type": "number", "minimum": 0}, | ||
| "cpuset": {"type": "string"}, |
There was a problem hiding this comment.
There's an overlap between some of these and resource.cpus.
Some of them are really hard to use, and must be specified "together" (cpu-period and cpu-quota, but also cpuset-cpus and cpuset-mem). Ideally we would have a better design for that to make such things logically grouped, so that they will be set together https://docs.docker.com/engine/reference/run/#cpu-period-constraint
| "group_add": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": ["string", "number"] | ||
| }, | ||
| "uniqueItems": true | ||
| }, |
There was a problem hiding this comment.
Need to review how these (should) interact with user; user can have user:group, which sets the primary group, and this sets auxiliary groups. For swarm services we renamed this to groups
--group list Set one or more supplementary user groups for the container
| weight_device: | ||
| - path: /dev/sda | ||
| weight: 400 | ||
| device_read_bps: | ||
| - path: /dev/sdb | ||
| rate: '12mb' |
There was a problem hiding this comment.
See my other comment; these settings are per device; ideally we would flip the configuration to be grouped per device. These configs are also non-portable (devices will be different per environment, which makes it not very suitable for a compose-file that is to be shared between developers / environments)
| ### oom_kill_disable | ||
|
|
||
| If `oom_kill_disable` is set Compose implementation MUST configure the platform so it won't kill the container in case | ||
| of memory starvation. |
There was a problem hiding this comment.
There's been some discussion about this option overall; it's really dangerous (process can never be killed, which means the host will be killed before this container). I'd be very cautious bringing this back.
oom_score_adj would already achieve this in 99% of the situations. (-1000 is "unkillable")
There was a problem hiding this comment.
again, we bring it back to the spec because it existed in v2 and we want to give implementation a chance to parse any compose file and manage such attribute the way they want (can just ignore with a warning). We can discuss about deprecating any dangerous feature in another issue
|
|
||
| * `always`: Compose implementations SHOULD always pull the image from the registry. | ||
| * `never`: Compose implementations SHOULD NOT pull the image from a registry and SHOULD rely on the platform cached image. If there is no cached image, a failure MUST be reported. | ||
| * `if_not_present`: Compose implementations SHOULD pull the image only if it's not available in the platform cache.This SHOULD be the default option for Compose implementations without build support. |
There was a problem hiding this comment.
This is the equivalent of --pull=missing (docker/cli#1498)? Was this how it was named in the v2 format, or renamed from what we had previously?
There was a problem hiding this comment.
I guess it was isnpired by Kubernetes IfNotPresent pull policy
There was a problem hiding this comment.
note: this one was introduced previously, only moved to use alphabetic ordering on attributes
Signed-off-by: Nicolas De Loof <[email protected]>
This field was ported from v2 and marked as deprecated in compose-spec#78, but still useful for Kata and gVisor users. Close compose-spec#101 Signed-off-by: Akihiro Suda <[email protected]>
This field was ported from v2 and marked as deprecated in compose-spec#78, but still useful for Kata and gVisor users. Close compose-spec#101 Signed-off-by: Akihiro Suda <[email protected]>
5 participants
What this PR does / why we need it:
Reintroduce attributes from compose 2.x file format:
Which issue(s) this PR fixes:
#12