The only .PHONY entry we *need* is for schema/validate, since that's a
real file but we haven't told Make about its real dependencies (which
involve complicated Go lookups). I'm personally in favor of using
.PHONY for all targets that aren't on-disk files, because it hints to
readers that the rule is not generating a file at the target. But
there has been resistance to adding .PHONY entries to all such cases
(e.g. [1,2]), so this commit brings us around to a
internally-consistent "only use .PHONY when you always need it"
position.
That means that, for example, users who create files named 'clean'
will turn 'clean' the target into a no-op, but runtime-spec
maintainers are ok with that.
[1]: https://github.com/opencontainers/runtime-spec/pull/791#issuecomment-300369882
[2]: https://github.com/opencontainers/runtime-spec/pull/791#issuecomment-300612827
Signed-off-by: W. Trevor King <wking@tremily.us>
The only discussion related to this is in [1,2], where the
relationship between oomScoreAdj and disableOOMKiller is raised. But
since 429f936 (Adding cgroups path to the Spec, 2015-09-02, #137)
resources has been tied to cgroups, and oomScoreAdj is not about
cgroups. For example, we currently have (in config-linux.md):
You can configure a container's cgroups via the resources field of
the Linux configuration.
I suggested we move the property from linux.resources.oomScoreAdj to
linux.oomScoreAdj so config authors and runtimes don't have to worry
about what cgroupsPath means if the only entry in resources is
oomScoreAdj. Michael responded with [4]:
If anything it should probably go on the process
So that's what this commit does.
I've gone with the four-space indents here to keep Pandoc happy (see
7795661 (runtime.md: Fix sub-bullet indentation, 2016-06-08, #495),
but have left the existing entries in this list unchanged to reduce
churn.
[1]: https://github.com/opencontainers/runtime-spec/pull/236
[2]: https://github.com/opencontainers/runtime-spec/pull/292
[3]: https://github.com/opencontainers/runtime-spec/pull/137
[4]: https://github.com/opencontainers/runtime-spec/issues/782#issuecomment-299990075
Signed-off-by: W. Trevor King <wking@tremily.us>
And fill in some known-good and known-bad examples. We can make this
as detailed as we want, but this commit just adds enough to know that:
* The full-file spec examples are valid.
* The JSON Schema can distinguish valid examples from invalid JSON.
This will help catch JSON Schema typos like those being addressed by
[1].
[1]: https://github.com/opencontainers/runtime-spec/pull/784
Signed-off-by: W. Trevor King <wking@tremily.us>
In order to increase the granularity of CPU resource control, change
the CPU Percent (0-100) resource setting to CPU Maximum (0-10000)
Signed-off-by: Darren Stahl <darst@microsoft.com>
Before this commit, linux.seccomp.sycalls was required, but we didn't
require an entry in the array. That means '"syscalls": []' would be
technically valid, and I'm pretty sure that's not what we want.
If it makes sense to have a seccomp property that does not need
syscalls entries, then syscalls should be optional (which is what this
commit is doing).
If it does not makes sense to have an empty/unset syscalls then it
should be required and have a minimum length of one.
Before 652323c (improve seccomp format to be more expressive,
2017-01-13, #657), syscalls was omitempty (and therefore more
optional-feeling, although there was no real Markdown spec for seccomp
before 3ca5c6c, config-linux.md: fix seccomp, 2017-03-02, #706, so
it's hard to know). This commit has gone with OPTIONAL, because a
seccomp config which only sets defaultAction seems potentially valid.
The SCMP_ACT_KILL example is prompted by:
On Tue, Apr 25, 2017 at 01:32:26PM -0700, David Lyle wrote [1]:
> Technically, OPTIONAL is the right value, but unless you specify the
> default action for seccomp to be SCMP_ACT_ALLOW the result will be
> an error at run time.
>
> I would suggest an additional clarification to this fact in
> config-linux.md would be very helpful if marking syscall as
> OPTIONAL.
I've phrased the example more conservatively, because I'm not sure
that SCMP_ACT_ALLOW is the only possible value to avoid an error. For
example, perhaps a SCMP_ACT_TRACE default with an empty syscalls array
would not die on the first syscall. The point of the example is to
remind config authors that without a useful syscalls array, the
default value is very important ;).
Also add the previously-missing 'required' property to the seccomp
JSON Schema entry.
[1]: https://github.com/opencontainers/runtime-spec/pull/768#issuecomment-297156102
Signed-off-by: W. Trevor King <wking@tremily.us>
The only non-phony target (where the target name matches the output
file) is 'validate', but we need .PHONY there because the Go
dependencies are not represented in the Makefile. This commit adds
the missing .PHONY declarations to the other targets, which truly are
phony.
Signed-off-by: W. Trevor King <wking@tremily.us>
If the timeout value was zero, the hook would always error, and there
doesn't seem to be much point to that. And I'm not sure what negative
timeouts would mean. By adding this restriction, we do not limit
useful hook entries, and we give the validation code grounds to warn
users attempting to validate configs which are poorly defined or have
useless hook entries.
I'd like to remove the pointer from the Go type to comply with our
anti-pointer zero-value style (style.md) now that Go's zero-value is
clearly invalid. However, there has been maintainer resistance to
removing the pointer [1] (although I don't think this is consistent
with previous maintainer statements that we don't need pointers when
the zero value is invalid [2]). In order to land the normative spec
change, this commit keeps the current *int for Hook.Timeout and punts
a consistent policy to future work.
[1]: https://github.com/opencontainers/runtime-spec/pull/764#discussion_r111607155
[2]: https://github.com/opencontainers/runtime-spec/pull/653#issuecomment-272253010
Signed-off-by: W. Trevor King <wking@tremily.us>
I expect the (undocumented) intention here is to iterate through
'names' and call seccomp_rule_add(3) or similar for each name. In
that case, an empty 'names' makes the whole syscall entry a no-op, and
with this commit we can warn users who are validating such configs.
If, on the other hand, we were comfortable with no-op syscall entries,
we'd want to make 'names' OPTIONAL.
Warning folks who accidentally empty (or don't set) 'names' seems more
useful to me, and doesn't restrict the useful config space, so that's
what I've gone with in this commit.
minItems is documented in [1], and there is an example of its use in
[2]:
"options": {
"type": "array",
"minItems": 1,
"items": { "type": "string" },
"uniqueItems": true
},
[1]: https://tools.ietf.org/html/draft-wright-json-schema-validation-00#section-5.11
[2]: http://json-schema.org/example2.html
Signed-off-by: W. Trevor King <wking@tremily.us>
The:
"type": [
"string"
]
syntax added in 652323cd (improve seccomp format to be more
expressive, 2017-01-13, #657) is not valid:
$ ./validate ./config-schema.json <../config.json
The document is not valid. see errors :
- linux.seccomp.syscalls.0.names: Invalid type. Expected: string, given: array
Signed-off-by: W. Trevor King <wking@tremily.us>
This partially revert #648 , after a second thought, I think we
should use specs value the same as kernel API input, see:
https://github.com/opencontainers/runtime-spec/issues/692#issuecomment-281889852
For memory and hugetlb limits *.limit_in_bytes, cgroup APIs take the values
as string, but the parsed values are unsigned long, see:
https://github.com/torvalds/linux/blob/v4.10/mm/page_counter.c#L175-L193
For `cpu.cfs_quota_us` and `cpu.rt_runtime_us`, cgroup APIs take the input
value as signed long long, while `cpu.cfs_period_us` and `cpu.rt_periof_us`
take the input value as unsigned long long.
Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>
These are optional on multiple platforms and should be left up to the
runtime/host system for validation.
Closes#470
Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
this commit contains:
* add explicit usage message to validate
* schemaPath was overrided by filepath.Abs(), schemaLoader would not get
* the abs path.
* check local scheme and document file path with os.Stat()
Signed-off-by: Deng Guangxing <dengguangxing@huawei.com>
Maintainers feel (and I agree) that there's no point in explicitly
allowing a null value when callers can simply leave the property unset
[1]. This commit removes all references to "pointer" and "null" from
the JSON Schema to support that decision. While optional properties
may sometimes be represented as pointer types in Go [2], optional
properties should be represented in JSON Schema by not including the
properties in the 'required' array.
[1]: https://github.com/opencontainers/runtime-spec/pull/555#issuecomment-272020515
[2]: style.md "Optional settings should not have pointer Go types"
Signed-off-by: W. Trevor King <wking@tremily.us>
W. Trevor King
Michael Crosby
Tianon Gravi
Darren Stahl
zhouhao
Mrunal Patel
v1.0.0.batts
Qiang Huang
Ma Shimiao
grantseltzer
Deng Guangxing