rebase the specerror framework ; complete rfc code of config.md #458

liangchenye · 2017-08-31T12:29:43Z

Signed-off-by: Liang Chenye [email protected]

liangchenye · 2017-08-31T12:31:07Z

I'll rebase it after #455.

wking

Instead of:

ProcessCWDAbs represents the error code of absolute path test of process cwd

can we inline the 1.0.0 spec language?

ProcessCWDAbs represents the "This value MUST be an absolute path" process.cwd requirement.

The Go comments are not printed as part of the test output, but inlining spec language gives Go devs clarity until opencontainers/runtime-spec#634 is re-opened and addressed.

And while we're cleaning this up, I'd recommend Version or OCIVersion where you currently have SpecVersion, because you don't currently prefix other codes with Spec.

wking · 2017-08-31T18:49:17Z

error/runtime_spec.go

+	// Process
+	// ProcessOnStart represents the error code of process property test when 'start' is called
+	ProcessOnStart
+	// ProcessIngnoreConsoleSize represents the error code of console size test when terminal is `false` or unset


nit: you seem to go back and forth between backticks (like your false here) and single quotes (like your 'start' two lines up). It would be nice to pick one and stick with it. Go's comment → HTML converter doesn't render Markdown or have a way to get <code>…</code> spans, so I'd recommend using single quotes. But I'll be happy as long as we're consistent one way or the other.

wking · 2017-08-31T19:01:22Z

error/runtime_spec.go

+	// PosixHookPathAbs represents the error code of absolute path test of posix hook path
+	PosixHookPathAbs
+	// PosixHookTimeout represents the error code of hook timeout test
+	PosixHookTimeout


I'd recommend more specific names for future-proofing. For example, PosixHookTimeoutPositive for “If set, timeout MUST be greater than zero”.

wking · 2017-08-31T19:10:56Z

error/runtime_spec.go

@@ -30,7 +30,7 @@ const (
 	RootOnNonHyperV
 	// RootOnHyperV represents the error code of root setting test on hyper-v containers
 	RootOnHyperV
-	// PathFormatOnWindows represents the error code of the path format test on Window
+	// PathFormatOnWindows represents the error code of the path format test on Windows
 	PathFormatOnWindows
 	// PathName represents the error code of the path name test
 	PathName


This could be more specific, e.g. PosixRootPathConvention or PosixRootPathRootfs, because it's a POSIX-only SHOULD about root.path suggesting “the conventional rootfs”.

wking · 2017-08-31T19:13:00Z

error/runtime_spec.go

@@ -41,6 +41,73 @@ const (
 	// ReadonlyOnWindows represents the error code of readonly setting test on Windows
 	ReadonlyOnWindows


This could be a more specific WindowsRootReadonlyFalse, and the earlier ReadonlyFilesystem could be RootReadonlyImplemented or some such, to distinguish from other root.readonly issues like using a non-boolean value.

wking · 2017-08-31T19:16:45Z

error/runtime_spec.go

+	// MountDestinationAbs represents the error code of absolute path test of mount destination
+	MountDestinationAbs
+	// MountNotNestedOnWindows represents the error code of nested mount point on Windows
+	MountNotNestedOnWindows


I recommend consistent platform namespacing. You have an OnWindows suffix here, but have other entries with a platform prefix (e.g. WindowsPlatform, LinuxOomScoreAdj, PosixHookPathAbs). I recommend a template like:

{platform, if platform-specific}{JSON path in camelCase}{something about what's being tested}

which would make this one WindowsMountsDestinationNotNested.

wking · 2017-08-31T19:20:32Z

error/runtime_spec.go

+	// RlimitHardMax represents the error code of rlimit hard test
+	RlimitHardMax
+	// RlimitDuplicated represents the error code of rlimit duplicate type test
+	RlimitDuplicated


I don't see an entry in your rlimits codes for “For each entry in rlimits, a getrlimit(3) on type MUST succeed”. That would be something like PosixProcessRlimitsTypeGet.

wking · 2017-08-31T19:29:36Z

error/runtime_spec.go

+	// PosixHookStateOverStdin represents the error code of `state` information test if it was passed to hook process
+	PosixHookStateOverStdin
+	// PosixHookPrestartOrder represents the error code of order test of pre-start hook
+	PosixHookPrestartOrder


Is this a subcase of PosixHookCalledInOrder? Or is this intended for “The pre-start hooks MUST be called after the start operation is called but before the user-specified program command is executed”? I don't see a need for the former, and think a better name for the latter would be PosixHooksPrestartTiming or something so we can distinguish between the order of one hook relative to another and the order of the hooks as a whole relative to the surrounding lifecycle.

wking · 2017-08-31T19:31:20Z

error/runtime_spec.go

+
+	// Annotations
+	// AnnotationKeyValue represents the error code of annotations key-value map test
+	AnnotationKeyValue


There are several RFC 2119 conditions for annotations, but you only have one code here.

I saw they were quoted, should we remove the this quote from the spec?

Keys MUST be strings. Keys MUST NOT be an empty string. Keys SHOULD be named using a reverse domain notation - e.g. `com.example.myKey`. Keys using the `org.opencontainers` namespace are reserved and MUST NOT be used by subsequent specifications. Implementations that are reading/processing this configuration file MUST NOT generate an error if they encounter an unknown annotation key. Values MUST be strings. Values MAY be an empty string.

I saw they were quoted…

I see some backticks in the spec you reference, but no quotes. And backticks are just markup, they don't affect the number of RFC 2119 conditions contained in the text.

wking · 2017-08-31T20:00:50Z

error/runtime_spec.go

+
+	// Extensibility
+	// IgnoreUnknownProperties represents the error code of unknown properties test
+	IgnoreUnknownProperties


There are two MUSTs for this in the spec, but I've filed opencontainers/runtime-spec#916 to collapse them to one.

liangchenye · 2017-09-01T03:07:15Z

@wking I'll use the inline language. It is always difficult for me to find suitable English word to describe a spec error.
Ideally, if we have more specific opencontainers/runtime-spec#634, we can generate the spec error code from spec files automatically.

wking · 2017-09-01T04:44:02Z

On Fri, Sep 01, 2017 at 03:07:19AM +0000, 梁辰晔 (Liang Chenye) wrote: Ideally, if we have more specific opencontainers/runtime-spec#634, we can generate the spec error code from spec files automatically.

Yeah, that would be good. But I'd much rather have gotten them via opencontainers/runtime-spec#628 and am not interested in maintaining per-sentence link anchors by hand (the approach taken with opencontainers/runtime-spec#634). If someone else wants to step up and add those to the spec, then great, but I'll be very surprised if it happens in the next three months ;).

liangchenye · 2017-09-01T14:08:39Z

I wrote a simple tool today to help make life easier:
https://github.com/liangchenye/markdown-to-json
So far, the output seems good.
https://github.com/liangchenye/markdown-to-json/blob/master/testdata/config.output
Just for fun!

liangchenye · 2017-09-02T06:38:24Z

We have several RootPath so we need to change them by hand.
But we can map the error code to the string automatically, (The value SHOULD be the conventional...), so it would be easy to sync the spec errors with the runtime-spec.

   RootPath
	// RootPath represents "The value SHOULD be the conventional `rootfs`."
        RootPath
	// RootPath represents "A directory MUST exist at the path declared by the field."

liangchenye · 2017-09-05T14:02:15Z

updated base on https://github.com/liangchenye/markdown-to-json/blob/master/testdata/config.output
The current naming rules are:

Full Property Name + Condition + Expected
Take RootPathOnPosixConvention for example,
RootPath is the full property, 'OnPosix' is the condition, 'Convention' is the expected result.
Make it shorter
Proc -- Process for example.
Polish the name
Now following most of @wking 's suggestion, like 'Timing'...

The ref and the comment are generated automatically by tool.

Mashimiao · 2017-09-06T03:52:40Z

awesome!

liangchenye · 2017-09-07T12:58:50Z

The https://github.com/liangchenye/markdown-to-json tool is updated, I try to make as useful as i18n tool (make update-po).

wking · 2017-09-07T18:53:30Z

specerror/error.go

+	RootPathExist:              {Level: rfc2119.Must, Reference: rootRef},
+	RootReadonlyImplement:      {Level: rfc2119.Must, Reference: rootRef},
+	RootReadonlyOnWindowsFalse: {Level: rfc2119.Must, Reference: rootRef},
+	// Mounts


The constants now have namespaced names (e.g. Mounts*), so there's no need for comments like // Mounts, your earlier // Root, etc. There may still be a use for the file-level comments (// Bundle.md and // Config.md) with the current names, but in order to avoid collisions it may be better to include that level in the constant names too (ConfigFileExistence → BundleConfigFileExistence, RootOnWindowsRequired → ConfigRootOnWindowsRequired, etc.).

wking · 2017-09-15T02:21:05Z

Based on #465, this should go on the v0.3.0 milestone.

Mashimiao · 2017-09-15T02:28:23Z

yes, marked

The spec isn't particuarly clear on this, saying [1]: * Linux: valid values are defined in the [`getrlimit(2)`][getrlimit.2] man page, such as `RLIMIT_MSGQUEUE`. * Solaris: valid values are defined in the [`getrlimit(3)`][getrlimit.3] man page, such as `RLIMIT_CORE`. and [2]: For each entry in `rlimits`, a [`getrlimit(3)`][getrlimit.3] on `type` MUST succeed. It doesn't say: Linux: The value MUST be listed in the getrlimit(2) man page... and it doesn't require the runtime to support the values listed in the man page [3,4]. So there are three sets: * Values listed in the man page * Values supported by the host kernel * Values supported by the runtime And as the spec stands, these sets are only weakly coupled, and any of them could be a sub- or superset of any other. In practice, I expect the sets to all coincide, with the kernel occasionally adding or removing values, and the man page and runtimes trailing along behind. To address that, this commit weakens the previous hard error to a SHOULD-level error. The PosixProcRlimitsValueError constant is new to this commit, because the spec contains neither a MUST nor a SHOULD for this condition, although I expect a SHOULD-level suggestion was implied by [1]. The posixProcRef constant is cherry-picked from 27503c5 (complete spec errors of config.md, 2017-09-05, opencontainers#458). [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L168-L169 [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L168-L169 [3]: opencontainers/runtime-spec#813 [4]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L463 Signed-off-by: W. Trevor King <[email protected]>

The spec isn't particuarly clear on this, saying [1]: * Linux: valid values are defined in the [`getrlimit(2)`][getrlimit.2] man page, such as `RLIMIT_MSGQUEUE`. * Solaris: valid values are defined in the [`getrlimit(3)`][getrlimit.3] man page, such as `RLIMIT_CORE`. and [2]: For each entry in `rlimits`, a [`getrlimit(3)`][getrlimit.3] on `type` MUST succeed. It doesn't say: Linux: The value MUST be listed in the getrlimit(2) man page... and it doesn't require the runtime to support the values listed in the man page [3,4]. So there are three sets: * Values listed in the man page * Values supported by the host kernel * Values supported by the runtime And as the spec stands, these sets are only weakly coupled, and any of them could be a sub- or superset of any other. In practice, I expect the sets to all coincide, with the kernel occasionally adding or removing values, and the man page and runtimes trailing along behind. To address that, this commit weakens the previous hard error to a SHOULD-level error. The PosixProcRlimitsValueError constant is new to this commit, because the spec contains neither a MUST nor a SHOULD for this condition, although I expect a SHOULD-level suggestion was implied by [1]. The posixProcRef constant is cherry-picked from 27503c5 (complete spec errors of config.md, 2017-09-05, opencontainers#458). Also make CheckRlimits a no-op on Windows, because the spec does not define process.rlimits for that OS [5]. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L168-L169 [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L168-L169 [3]: opencontainers/runtime-spec#813 [4]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L463 [5]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#posix-process Signed-off-by: W. Trevor King <[email protected]>

liangchenye · 2017-10-09T07:41:23Z

I got near 200 RFC errors and found it is difficult to maintain all of them in one file.
So I'll add different go files to match 'config.md', 'config-linux.md' and the etc...

liangchenye · 2017-10-17T10:17:21Z

rewrite the spec error framework
the error.go just provide the basic template and common function.
provide a 'register' function so each .go (generate from .md) could register
its RFC codes.
complete the config.go
this is originally get from markdown-to-json tool
add small part rfc codes of bundle.md/runtime.md

PTAL @wking @Mashimiao @q384566678

wking · 2017-10-17T14:07:29Z

specerror/error.go

-		return fmt.Sprintf(referenceTemplate, version, "runtime.md#create"), nil
+var ociErrors = map[Code]errorTemplate{}
+
+func registOCIError(code Code, level rfc2119.Level, ref func(versiong string) (string, error)) {


This package is just about runtime-spec requirement, so this can be register. No need to have OCI or error in the name, because they're covered by the package path.

I‘ll change it tomorrow morning.

wking · 2017-10-17T14:08:27Z

specerror/error.go

+
+func registOCIError(code Code, level rfc2119.Level, ref func(versiong string) (string, error)) {
+	if _, ok := ociErrors[code]; ok {
+		panic(fmt.Sprintf("should not regist a same code twice: %s", code))


Maybe: regist a -> register the.

wking · 2017-10-17T14:10:38Z

specerror/bundle.go

@@ -0,0 +1,31 @@
+// Package specerror implements runtime-spec-specific tooling for
+// tracking RFC 2119 violations.


To stay DRY, I think you only want this comment in error.go.

wking · 2017-10-17T14:12:32Z

specerror/config-linux.go

+// define error codes
+const (
+	// DefaultFilesystems represents "The following filesystems SHOULD be made available in each container's filesystem:"
+	DefaultFilesystems = "The following filesystems SHOULD be made available in each container's filesystem:"


Is this really our only config-linux.md requirement? Either way, I'd rather get this PR landed, because the change feels to big to wait until it's complete and perfect.

no，these items were already recorded in runtime tools implementation. I add them first. The whole rfc errors will come later.

Signed-off-by: Liang Chenye <[email protected]>

liangchenye · 2017-10-18T01:19:30Z

updated, rename registOCIError to register.

zhouhao3 · 2017-10-18T01:49:54Z

LGTM

Mashimiao · 2017-10-18T02:21:16Z

LGTM

wking reviewed Aug 31, 2017

View reviewed changes

liangchenye force-pushed the unittest branch from 39cb55c to f1b5d4f Compare September 5, 2017 13:54

liangchenye force-pushed the unittest branch from f1b5d4f to 27503c5 Compare September 5, 2017 14:07

wking reviewed Sep 7, 2017

View reviewed changes

Mashimiao added this to the v0.3.0 milestone Sep 15, 2017

wking mentioned this pull request Sep 21, 2017

validate: Soften unrecognized rlimit types to SHOULD violations #481

Merged

liangchenye force-pushed the unittest branch from 27503c5 to 42fde59 Compare September 26, 2017 10:02

liangchenye force-pushed the unittest branch 3 times, most recently from 655cb94 to ce4fbd0 Compare October 17, 2017 10:00

liangchenye changed the title ~~[WIP] complete rfc code of config.md~~ rebase the specerror framework ; complete rfc code of config.md Oct 17, 2017

wking reviewed Oct 17, 2017

View reviewed changes

liangchenye force-pushed the unittest branch from ce4fbd0 to bef654c Compare October 18, 2017 01:13

add specerror framework; complete rfc errors of bundle.md and config.md

b446e38

Signed-off-by: Liang Chenye <[email protected]>

liangchenye force-pushed the unittest branch from bef654c to b446e38 Compare October 18, 2017 01:16

liangchenye mentioned this pull request Oct 18, 2017

ROADMAP for runtime-tools #465

Open

wking approved these changes Oct 18, 2017

View reviewed changes

Mashimiao merged commit 66b64a9 into opencontainers:master Oct 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rebase the specerror framework ; complete rfc code of config.md #458

rebase the specerror framework ; complete rfc code of config.md #458

liangchenye commented Aug 31, 2017

liangchenye commented Aug 31, 2017

wking left a comment

wking Aug 31, 2017

wking Aug 31, 2017

wking Aug 31, 2017

wking Aug 31, 2017

wking Aug 31, 2017

wking Aug 31, 2017

wking Aug 31, 2017

wking Aug 31, 2017

liangchenye Sep 1, 2017

wking Sep 1, 2017

wking Aug 31, 2017

liangchenye commented Sep 1, 2017

wking commented Sep 1, 2017 via email

liangchenye commented Sep 1, 2017

liangchenye commented Sep 2, 2017

liangchenye commented Sep 5, 2017

Mashimiao commented Sep 6, 2017

liangchenye commented Sep 7, 2017

wking Sep 7, 2017

wking commented Sep 15, 2017

Mashimiao commented Sep 15, 2017

liangchenye commented Oct 9, 2017

liangchenye commented Oct 17, 2017

wking Oct 17, 2017

liangchenye Oct 17, 2017

wking Oct 17, 2017

wking Oct 17, 2017

wking Oct 17, 2017

liangchenye Oct 17, 2017

liangchenye commented Oct 18, 2017

zhouhao3 commented Oct 18, 2017 •

edited by caniszczyk

Loading

Mashimiao commented Oct 18, 2017 •

edited by caniszczyk

Loading

		@@ -41,6 +41,73 @@ const (
		// ReadonlyOnWindows represents the error code of readonly setting test on Windows
		ReadonlyOnWindows

		@@ -0,0 +1,31 @@
		// Package specerror implements runtime-spec-specific tooling for
		// tracking RFC 2119 violations.

rebase the specerror framework ; complete rfc code of config.md #458

rebase the specerror framework ; complete rfc code of config.md #458

Conversation

liangchenye commented Aug 31, 2017

liangchenye commented Aug 31, 2017

wking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liangchenye commented Sep 1, 2017

wking commented Sep 1, 2017 via email

liangchenye commented Sep 1, 2017

liangchenye commented Sep 2, 2017

liangchenye commented Sep 5, 2017

Mashimiao commented Sep 6, 2017

liangchenye commented Sep 7, 2017

Choose a reason for hiding this comment

wking commented Sep 15, 2017

Mashimiao commented Sep 15, 2017

liangchenye commented Oct 9, 2017

liangchenye commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liangchenye commented Oct 18, 2017

zhouhao3 commented Oct 18, 2017 • edited by caniszczyk Loading

Mashimiao commented Oct 18, 2017 • edited by caniszczyk Loading

zhouhao3 commented Oct 18, 2017 •

edited by caniszczyk

Loading

Mashimiao commented Oct 18, 2017 •

edited by caniszczyk

Loading