Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validate: modify the validation of mounts.type #397

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zhouhao3
Copy link

According to this PR revision.

Signed-off-by: zhouhao [email protected]

} else if OS == "windows" {
supportedTypes["ntfs"] = true
} else if OS == "solaris" {
supportedTypes["lofs"] = true
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the type of solaris support, I found the following sentence in the relevant documents:

Used to specify the FSType on which to operate. The FSType must be specified or must be determinable from /etc/vfstab, or by consulting /etc/default/fs or /etc/dfs/fstypes.

But I did not find these documents, only given examples given a support type.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct modification. You can refer to FSTYPE in mount(1) to determine solaris supports fs types.
Windows does not support type in mount, there is no need to output warning and we should accept this change after spec v1.0 released.

@Mashimiao Mashimiao added this to the v0.2.0 milestone Sep 15, 2017
@@ -337,11 +337,31 @@ func (v *Validator) CheckRlimits() (msgs []string) {
func supportedMountTypes(OS string, hostSpecific bool) (map[string]bool, error) {
supportedTypes := make(map[string]bool)

if OS != "linux" && OS != "windows" {
if OS != "linux" && OS != "solaris" {
logrus.Warnf("%v is not supported to check mount type", OS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeating @Mashimiao's earlier comment, since it still applies:

Windows does not support type in mount, there is no need to output warning and we should accept this change after spec v1.0 released.

type is not defined for Windows since opencontainers/runtime-spec#854 (which you site in the comment opening this PR). So configs which set it are taking advantage of the extensibility rules that allow them to do so. Individual runtimes may or may not act upon the extention, but it's still a valid config.

There may be users who are interested in knowing if a runtime MUST support a given config (e.g. if that config can be portably used with any OCI-compliant runtime). In that case, we would want to warn/error when we found extentions like this. More thoughts on that sort of thing in opencontainers/image-tools#66. However, neither runtime-tools nor image-tools currently provides switches for that sort of thing, so for the time being I'd rather silently ignore the type value (if any) in Windows configs.

@Mashimiao
Copy link

Mashimiao commented Sep 25, 2017

I‘m not familiar with solaris system, so I'm not sure if this is a right validation. But from here, It seems vfstab stores default mounted file systems not all supported file systems.
Any comments from @opencontainers/runtime-tools-maintainers ?

@Mashimiao Mashimiao modified the milestones: v0.2.0, v0.7.0 Sep 25, 2017
@wking
Copy link
Contributor

wking commented Sep 28, 2017

Why is this in v0.7.0? I don't see anything about “solve options conflict problem” here (I'd expect v0.7.0 to only contain #414, but it currently doesn't). This looks like part of implementing config validation, so it belonged in v0.2.0. And with v0.2.0 cut, it should fall back to v0.3.0.

@Mashimiao
Copy link

It's a validation about solaris but we are not familiar with it, at least I@liangchenye and I. We can't make sure whether it's right or not just from solaris's reference document. And I think we should perfect Linux related validations first. Other platform's may be later.
So, moving it to v0.7, just want to put it into low priority.
If anyone's familiar with solaris, you can review this PR. Though it's in v0.7, we can merge this before v0.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants