Add mems, cpus check and move common codes into utils

[Mashimiao]

No description provided.

[wking]

This (and UnitListValid) are both validators. Why not keep them in the validate package?

[Mashimiao]

On 03/11/2017 02:28 AM, W. Trevor King wrote: +// CapValid checks whether a capability is valid +func CapValid(c string, hostSpecific bool) error { This (and |UnitListValid|) are both validators. Why not keep them in the validate package?

As they also be used by generate, so I'm a little hesitant, and moved them into utils first as a try.

[wking]

As they also be used by generate...

I think generate importing validate is fine (although I wouldn't like it if validate imported generate). Moving validation functions into utils as generate needs them seems like it would destabilize the validate API without much gain.

[Mashimiao]

They were not member functions of Validator, I think this change will not destabilize the validate API.
And they really works as tiny tool function for validations and params check, let them to be public functions in Validation seems also not suitable. From this point, it's a good modification.

[Mashimiao]

ping @mrunalp @liangchenye

[liangchenye]

I prefer to change UnitListValid to a regular expression

var match = regexp.MustCompile
var cpuMemReg = match(`^([0-9]+|[0-9]+-[0-9]+)(,([0-9]+|[0-9]+-[0-9]+))*$`)

And we can use it in validation code:
cpuMemReg.MatchString(***)
I tested it, it works.

[wking]

These validity checks are not backed by the spec, and since opencontainers/runtime-spec#780 landed I think the spec is unlikely to add them in the future. Do runtime-tools maintainers intend to reach past runtime-spec and enforce limits that have been silently punted to the kernel (runtime-spec doesn't even have “MUST be a valid value for kernel interface [link]”)? If so, we'll need a validation level for “kernel limits not covered by runtime-spec”. If not, I think we can drop UnitListValid.

[Mashimiao]

Sorry, I don't know the history very well. Why do we remove these limits? These parameters are very related with kernel, if they are not accepted by kernel, that makes no sense, right?

[wking]

Why do we remove these limits? These parameters are very related with kernel, if they are not accepted by kernel, that makes no sense, right?

My impression is that runtime-spec maintainers feel that inlining kernel limits isn't particularly useful. The spec itself has no opinion on these limits, and just punts to the kernel (although I think the wording for the punts still needs work, e.g. opencontainers/runtime-spec#746).

But you could write a config with foo-bar as your linux.resources.cpu.cpus value, it would be valid vs. runtime-spec, and if you had a very strange kernel it would actually work for creating a new container. As far as runtime-spec (and our spec-targeting validation here) is concerned, that would be fine. Luckily, the kernel APIs are fairly stable, so config-authors don't have to worry too much about which kernel they're targeting when they figure out these values. And again, if we want to inline these checks with a “kernel limits not covered by runtime-spec” level, I'm fine with that.

[Mashimiao]

On 04/28/2017 07:42 PM, 梁辰晔 (Liang Chenye) wrote: |var match = regexp.MustCompile var cpuMemReg = match(`^([0-9]+|[0-9]+-[0-9]+)(,([0-9]+|[0-9]+-[0-9]+))*$`) |

For example, we set cpus ax min-max The Linux kernel requires min must less than max. If someone sets as 4-2 to cpus, this regexp can't check out such problem.

[liangchenye]

I feel that we need more validation works in 'generate' module.
so it seems better that 'generate' module is using 'validate' module other than sharing a common utils module with 'generate' module.

And another comment is, we are using great project 'syndtr/gocapability', it is already a common 'utils'. Maybe capValid should be upstreamed to it?

[Mashimiao]

@liangchenye I reconsidered your suggestion. As our CapValid has host-specific check, it's not so good to put it into gocapability Lib. But move it into validate module again is OK.
updated, PTAL

[wking]

I'm not sure this check (or your following mems check) is grounded in the spec, but if the runtime-tools maintainer consensus is that it is (or if they just want “unlikely to be supported by the kernel” warnings), then the validate/ changes from this PR belong in v0.3.0. I've also made a similar suggestion for #386 here, in case runtime-tools maintainers want to handle both instances the same way.

[Mashimiao]

I think this PR is not so valuable as I considered, so I decide to close it.
Will open it depends on our future needs.