cleanup: remove defaultfs validaton

[Mashimiao]

According to spec, defaultFS is not MUST any more

Signed-off-by: Ma Shimiao [email protected]

[mrunalp]

LGTM

[Mashimiao]

ping @hqhq @liangchenye

[wking]

On Mon, Feb 13, 2017 at 11:38:02PM -0800, Ma Shimiao wrote: According to spec, defaultFS is not MUST any more

opencontainers/runtime-spec#666 changed the MUST to a SHOULD, but we should still be testing SHOULDs (like we do in [1]). I'd float some example code, but: * I'm not clear on how runtime-tools maintainers intend to handle SHOULD-level warnings. I'd recommend TAP diagnostics for warnings, and a strict mode (or whatever you want to call it, some other ideas in opencontainers/image-tools#66) to turn those diagnostics into pass/fail checks. In the meantime, “just print a warning to stdout” seems to be the best approach to land in an OCI master branch. * I'm not clear on who the newly SHOULD-level requirement is intended for. Just dropping the check (like you do in 28c2564) is closer to either opencontainers/runtime-spec#678 or opencontainers/runtime-spec#679, which would remove the SHOULD too. But the only feedback on those is two runtime-spec maintainers pushing back against the first. So I'm ok with just removing the check, but until something like opencontainers/runtime-spec#678 or opencontainers/runtime-spec#679 lands to remove the upstream SHOULD, I think we should open a runtime-tools issue about the untested SHOULD needing some tests. [1]: https://github.com/opencontainers/runtime-tools/blob/a652af160a151258b9f0d697a7ab064b4aa9c1cd/validate/validate.go#L496

[Mashimiao]

runtimetest validate whether container environment is set as SPEC and user required.
items marked as SHOULD exist or not do not affect whether a container environment is valid or not.
what we need to care about based on SPEC and config.json

[wking]

On Tue, Feb 28, 2017 at 12:33:51AM -0800, Ma Shimiao wrote: runtimetest validate whether container environment is set as SPEC and user required.

I agree that we want to test all MUST-level requirements. I don't think that means we need to ignore SHOULD-level requirements.

items marked as `SHOULD` exist or not do not affect whether a container environment is valid or not.

Agreed. But SHOULD-level recommendations are still important. From the RFC [1]: SHOULD This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course. So I'm pretty sure we want to check for SHOULD-level violations and warn users (“you're violating a SHOULD-level requirement. Are you sure you want to do that?”). There should be options to suppress those warnings (“I know what I'm doing”) and options to turn those warnings into errors (“I want to be very cautious”), but I don't think ignoring SHOULD-level requirements in the validator is a good idea. [1]: https://tools.ietf.org/html/rfc2119#section-3

[Mashimiao]

On 03/01/2017 02:41 AM, W. Trevor King wrote: So I'm pretty sure we want to check for SHOULD-level violations and warn users

I agree that we should warn user SHOULD-level validation. But SPEC do not clearly say runtime SHOULD supply those default filesystem, and it's also in Container Configuration document. So, I think whether config default filesystems or not depend on user choice. And we should put this validation into bundle-validation. How do you think about it?

[wking]

On Tue, Feb 28, 2017 at 05:58:48PM -0800, Ma Shimiao wrote: But SPEC do not clearly say runtime SHOULD supply those default filesystem, and it's also in Container Configuration document. So, I think whether config default filesystems or not depend on user choice. And we should put this validation into bundle-validation.

As I said in my initial comment [1], I'm ok with just removing the check, but until something like opencontainers/runtime-spec#678 or opencontainers/runtime-spec#679 lands to remove the upstream SHOULD, I think we should open a runtime-tools issue about the untested SHOULD needing some tests. It looks like both of those PRs have been closed, so I'm not really sure how to resolve the uncertainty in testing it. But leaving the SHOULD untested without any open issue or plans to test it in the future seems sloppy. [1]: #324 (comment)

[liangchenye]

open an issue to keep track of the 'SHOULD' problem.
#335

[liangchenye]

LGTM