validate: add sysctl check #124
Conversation
Mashimiao
force-pushed
the
add-linux-sysctl-check
branch
from
256bb6a to
d83899e
Compare
validate.go
Outdated
| logrus.Fatalf("Sysctl %v requires a new Network namespace to be specified as well", k) | ||
| } | ||
| if strings.HasPrefix(k, "fs.mqueue.") && !ipcExists { | ||
| logrus.Fatalf("Sysctl %v requires a new IPC namespace to be specified as well", k) |
There was a problem hiding this comment.
Does this also depend on having a /dev/mqueue that is specific to your IPC namespace? That would require a new mount namespace as well (see testing recommendations here). And /dev/mqueue is not one of the required filesystems, but testing for “will this config supply a new /dev/mqueue?” is getting a bit dicey (e.g. maybe the user intends to supply it on their own after create and before start). So I think we may want to require a new mount namespace here, but not bother about checking for a new /dev/mqueue.
There was a problem hiding this comment.
add new mount namespace detection is OK. will add it.
Mashimiao
force-pushed
the
add-linux-sysctl-check
branch
from
d83899e to
38b2406
Compare
validate.go
Outdated
| } else if spec.Linux.Namespaces[index].Type == rspec.UTSNamespace { | ||
| utsExists = true | ||
| } else if spec.Linux.Namespaces[index].Type == rspec.IPCNamespace { | ||
| ipcExists = true |
There was a problem hiding this comment.
These checks should probably skip namespaces where path is set (so they only pick up new namespaces). If tweaking existing namespaces is fine, there's no need for the “does the config declare a namespace” checks. And the current spec explicitly forbids tweaking non-new namespaces:
Also, when a path is specified, a runtime MUST assume that the setup for that particular namespace has already been done and error out if the config specifies anything else related to that namespace.
Although I'm happy to revisit that restriction.
There was a problem hiding this comment.
I don't think we should skip namespace where path is set.
That's true spec says runtime must assume path specified namespace has already been done.
But the target of validate is to help user to write a complete config file and all items can be applied correctly.
Some items in config file need to work with some namespace.
If some namespaces did not exist, those items would not make sense.
So check path specified namespace declared or not is also the business of validate, we should not skip it.
There was a problem hiding this comment.
On Mon, Jun 27, 2016 at 11:58:28PM -0700, Ma Shimiao wrote:
Some items in config file need to work with some namespace. If some
namespaces did not exist, those items would not make sense.
The container process will always be in some namespace (for each
type). Without a namespace entry, it will be in the runtime namespace
1. With a namespace entry that sets ‘path’, it will join some
existing namespace. With a namespace entry that does not set ‘path’,
it will create a new namespace. The current spec wording requires a
new namespace “if the config specifies anything else related to that
namespace”. I think that means you need an IPC namespace without
‘path’ and a mount namespace without ‘path’ before you can set an
‘fs.mqueue.…’ sysctl.
Mashimiao
force-pushed
the
add-linux-sysctl-check
branch
from
38b2406 to
cadf326
Compare
|
ping @mrunalp @liangchenye |
validate.go
Outdated
| logrus.Fatalf("namespace %v is invalid.", spec.Linux.Namespaces[index]) | ||
| } else if spec.Linux.Namespaces[index].Type == rspec.UTSNamespace { | ||
| utsExists = true | ||
| } else if spec.Linux.Namespaces[index].Type == rspec.IPCNamespace && len(spec.Linux.Namespaces[index].Path) <= 0 { |
There was a problem hiding this comment.
I suggest to use len(***Path) == 0 in all these empty string checks.
Mashimiao
force-pushed
the
add-linux-sysctl-check
branch
from
cadf326 to
2c32a42
Compare
|
2c32a42 LGTM |
validate.go
Outdated
| if !namespaceValid(spec.Linux.Namespaces[index]) { | ||
| logrus.Fatalf("namespace %v is invalid.", spec.Linux.Namespaces[index]) | ||
| } else if spec.Linux.Namespaces[index].Type == rspec.UTSNamespace { | ||
| utsExists = true |
There was a problem hiding this comment.
I think we want:
else if len(spec.Linux.Namespaces[index].Path) == 0 {
continue
}
as the first block after the validity check. That way the logic also applies to this UTS case, and we don't have to duplicate Path checks for each namespace below.
There was a problem hiding this comment.
On 07/06/2016 11:39 AM, W. Trevor King wrote:
I think we want:
|else if len(spec.Linux.Namespaces[index].Path) == 0 { continue } |
as the first block after the validity check. That way the logic #124 (comment) also applies to this UTS case, and we don't have to duplicate |Path| checks for each namespace below.
Yes, it really is. will fix.
Ma Shimiao
Development Dept.I
Nanjing Fujitsu Nanda Software Tech. Co., Ltd.(FNST)
Signed-off-by: Ma Shimiao <[email protected]>
Mashimiao
force-pushed
the
add-linux-sysctl-check
branch
from
2c32a42 to
a9ae71a
Compare
|
a9ae71a looks good to me :).
|
|
ping @mrunalp |
|
LGTM |
4 participants
Signed-off-by: Ma Shimiao [email protected]