runtimetest: add mounts order validation

[Mashimiao]

According to mount orders in /proc/self/mountinfo to validate whether mounts are mounted in order.

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

[wking]

Possibly warn that the check is not implemented (like we currently do here)?

[wking]

(r)bind mounts are still not covered in the spec. My most recent attempt was opencontainers/runtime-spec#771, which I think should be reopened, but there is also an in-flight opencontainers/runtime-spec#878. Making runtime-tools decisions based on in-flight spec changes is dicey, but in both of those PRs the bind-ness of a mounts entry is based on options, not on type. So if you want to land special bind handling at all here, I suggest you look in specMount.Options.

[wking]

As a cheap local part of the in-flight #266 you could use configMount instead of specMount for this variable.

[Mashimiao]

@wking all updated.

[Mashimiao]

@opencontainers/runtime-tools-maintainers PTAL

[zhouhao3]

LGTM

[wking]

Not a big deal, because there are probably not going to be tons of options to iterate over, but you can break here.

[wking]

You could return a more informative message here if current was a*mountOrder.

[Mashimiao]

Sorry, I don't understand your comment very well. Can you give an example?

[wking]

Can you give an example?

I haven't compiled the following, but something like:

var current *mountOrder
for i, configMount := range spec.Mounts {
  mounts, ok := mountsMap[configMount.Destination]
  if !ok {
    // this error message could be better too, but I'm following validateMountsExist
    return fmt.Errorf("Expected mount %v does not exist", configMount)
  }
  for j, mount := range mounts {
    source := mount.Source
    for _, option := range configMount.Options {
      if option == "bind" || option == "rbind" {
        source = mount.Root
        break
      }
    }
    if source == configMount.Source {
      if current != nil && current.Order > mount.Order {
        return fmt.Errorf("mounts[%d] %q is mounted before %q", i, configMount.Destination, current.Destination)
      }
      current = &mount
      // in order to deal with dup mount elements
      mountsMap[configMount.Destination] = append(mountsMap[configMount.Destination][:j], mountsMap[configMount.Destination][j+1:]...)
      break
    }
  }
}

I'm also not clear on your dup-handling bit at the end. And we'd probably have better (or at least more consistent) matching if we used mountMatch here too (like we already do here).

[Mashimiao]

On 08/22/2017 06:26 AM, W. Trevor King wrote: I'm also not clear on your dup-handling bit at the end. And we'd probably have better (or at least more consistent) matching if we used |mountMatch| here too (loke we already do here <https://github.com/opencontainers/runtime-tools/blob/9e0e42dbf918070406a2a4a2e1476e7350ba9129/cmd/runtimetest/main.go#L619-L627>).

I don't think we need mountMatch here. Because validateMountsExist() is executed earlier, we can make sure all expected mountpoints exist. Then we continue to validate mount order. For two totally same mounts, it's hard to distinguish them. I think the only we can do is remove one in array when find such mountpoint and detect whether the other one is also mounted. As they are totally same, I think order doesn't make too much sense, we should ensure both of them are mounted.

[wking]

On Tue, Aug 22, 2017 at 01:51:20AM +0000, Ma Shimiao wrote: I don't think we need mountMatch here.

I think we do, because we don't want to match a mount of the wrong type (or whatever).

Because validateMountsExist() is executed earlier, we can make sure all expected mountpoints exist.

Is there a reason to keep the order and existence checks separate? If we roll them together, we could drop [1].

For two totally same mounts, it's hard to distinguish them. I think the only we can do is remove one in array when find such mountpoint and detect whether the other one is also mounted.

I'm fine with this. But mountMatch is also comparing types, so it's more strict about total-sameness than what you have in 3abb7af's validateMountsOrder. [1]: https://github.com/opencontainers/runtime-tools/pull/444/files#diff-6c88621051d8dcd99407582c2b76fd68R663

[Mashimiao]

On 08/23/2017 03:05 AM, W. Trevor King wrote: Is there a reason to keep the order and existence checks separate? If we roll them together, we could drop [1].

exists and order check are different target and put them into one function will make the function very long and complicated. Currently, for dup mounts, validateMountExist can only check it the mountpoint exist. `len(mounts)` works with `mountsMap[configMount.Destination] = append(mountsMap[configMount.Destination][:j], mountsMap[configMount.Destination][j+1:]...)` to check if each dup mounts are mounted. So, I don't think we can drop it.

[liangchenye]

LGTM

[zhouhao3]

LGTM