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

runtimetest: add linux devices validation #211

Merged

Conversation

Mashimiao
Copy link

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

@@ -394,7 +450,7 @@ func validate(context *cli.Context) error {

linuxValidations := []validation{
validateDefaultFS,
validateDefaultDevices,
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to keep this (as long as the spec requires runtimes to supply those devices).

Copy link
Author

Choose a reason for hiding this comment

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

On 09/14/2016 01:38 AM, W. Trevor King wrote:

We want to keep this (as long as the spec requires runtimes to supply those devices).

validateDefaultDevices is not removed.
I have just merged it into validateLinuxDevices

Copy link
Author

Choose a reason for hiding this comment

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

un... I think separate them more reasonable, will update.

@Mashimiao Mashimiao force-pushed the runtime-test-linux-devices-validation branch from 3141370 to 3d60190 Compare September 14, 2016 01:25
@Mashimiao
Copy link
Author

@wking I think adding the caveat and rewriting runtimetest to not carry on instead of dying on errors in a follow-up PR is more reasonable.
Can we pass and add this validation first?

@wking
Copy link
Contributor

wking commented Sep 14, 2016

On Wed, Sep 14, 2016 at 03:07:57AM -0700, Ma Shimiao wrote:

@wking I think adding the caveat and rewriting runtimetest to not
carry on instead of dying on errors in a follow-up PR is more
reasonable. Can we pass and add this validation first?

Sure, although the one-liner I suggest in 1 seems easy enough to add
now ;).

if err != nil {
return err
}
fStat := fi.Sys().(*syscall.Stat_t)
Copy link
Contributor

Choose a reason for hiding this comment

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

This Stat with its type/major/minor checks (and probably the Stat existence check too) should get pulled out into it's own helper (func validateDevice(path string, device *rspec.Device) error?) so we can use it in validateDefaultDevices as well.

Copy link
Author

Choose a reason for hiding this comment

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

On 09/14/2016 11:46 PM, W. Trevor King wrote:

This |Stat| with its type/major/minor checks (and probably the |Stat| existence check too) should get pulled out into it's own helper (|func validateDevice(path string, device *rspec.Device) error|?) so we can use it in |validateDefaultDevices| as well.

added |Stat| existence check.
I think helper function is not needed yet, as default device validation does not need so complicated check

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@Mashimiao Mashimiao force-pushed the runtime-test-linux-devices-validation branch from 3d60190 to f0ae1dc Compare September 21, 2016 09:15
@wking
Copy link
Contributor

wking commented Sep 23, 2016

I still have some changes I'd like to make to this PR [1,2], but I
think f0ae1dc is a step forward for testing runtime device support at
all. I'd be happy to see this landed so I can file my further
suggestions and we can decide if they're useful too ;).

@mrunalp
Copy link
Contributor

mrunalp commented Sep 27, 2016

LGTM

@mrunalp mrunalp merged commit 7ab77aa into opencontainers:master Sep 27, 2016
wking pushed a commit to wking/ocitools-v2 that referenced this pull request Sep 28, 2016
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>

Backported to v1.0.0.rc1 from f0ae1dc opencontainers#211 (cherry-pick applied
cleanly).

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/ocitools-v2 that referenced this pull request Oct 26, 2016
As I recommended earlier [1], in the context of checking for all
configured devices.  An example would be:

  {
    ...
    "linux": {
      "devices": [
        {
          "path": "/dev/fuse",
          ...
        }
      ],
    },
    "hooks": {
      "prestart": [
        {
          "path": "/bin/rm",
          "args": ["rm", "/dev/fuse"]
        }
      }
    }
  }

where the resulting container (when created by a conformant runtime)
would not have the /dev/fuse entry runtimetest's linux.devices check
is looking for.

[1]: opencontainers#211 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/ocitools-v2 that referenced this pull request Oct 26, 2016
As I recommended earlier in the context of checking for all configured
devices [1].  An example would be:

  {
    ...
    "linux": {
      "devices": [
        {
          "path": "/dev/fuse",
          ...
        }
      ],
    },
    "hooks": {
      "prestart": [
        {
          "path": "/bin/rm",
          "args": ["rm", "/dev/fuse"]
        }
      }
    }
  }

where the resulting container (when created by a conformant runtime)
would not have the /dev/fuse entry runtimetest's linux.devices check
is looking for.

[1]: opencontainers#211 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@Mashimiao Mashimiao deleted the runtime-test-linux-devices-validation branch November 14, 2016 09:30
wking added a commit to wking/ocitools-v2 that referenced this pull request Nov 22, 2016
As I recommended earlier in the context of checking for all configured
devices [1].  An example would be:

  {
    ...
    "linux": {
      "devices": [
        {
          "path": "/dev/fuse",
          ...
        }
      ],
    },
    "hooks": {
      "prestart": [
        {
          "path": "/bin/rm",
          "args": ["rm", "/dev/fuse"]
        }
      }
    }
  }

where the resulting container (when created by a conformant runtime)
would not have the /dev/fuse entry runtimetest's linux.devices check
is looking for.

[1]: opencontainers#211 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/ocitools-v2 that referenced this pull request Nov 22, 2016
As I recommended earlier in the context of checking for all configured
devices [1].  An example would be:

  {
    ...
    "linux": {
      "devices": [
        {
          "path": "/dev/fuse",
          ...
        }
      ],
    },
    "hooks": {
      "prestart": [
        {
          "path": "/bin/rm",
          "args": ["rm", "/dev/fuse"]
        }
      }
    }
  }

where the resulting container (when created by a conformant runtime)
would not have the /dev/fuse entry runtimetest's linux.devices check
is looking for.

[1]: opencontainers#211 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/ocitools-v2 that referenced this pull request Nov 23, 2016
As I recommended earlier in the context of checking for all configured
devices [1].  An example would be:

  {
    ...
    "linux": {
      "devices": [
        {
          "path": "/dev/fuse",
          ...
        }
      ],
    },
    "hooks": {
      "prestart": [
        {
          "path": "/bin/rm",
          "args": ["rm", "/dev/fuse"]
        }
      }
    }
  }

where the resulting container (when created by a conformant runtime)
would not have the /dev/fuse entry runtimetest's linux.devices check
is looking for.

[1]: opencontainers#211 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
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