-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
WIP DNM support Path in OCI spec's LinuxDeviceCgroup #3609
Conversation
FIXME doc this Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Rename buildEmulator to emulatorFromRules, and move it to devices_emulator.go. 2. Reuse it from all the places that do the same thing. This is a preparation for the next commit. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This brings in initial support for specifying a device access rule using device path instead of (or in addition to) device type, major, and minor. In case the path is not specified, nothing is done. Otherwise, it is checked that the path exists and is an actual block or character device. In case the path is specified together with type/major/minor, a sanity check is performed to make sure that they match those of the actual device. In case the path is specified but type/major/minor isn't, they are filled it from the actual device. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
// Path to the device. | ||
Path string `json:"path"` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this will cause issues with upgrades.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this can't affect runc exec
, runc stop
etc, and for runc update
we currently skip devices entirely. Having said that, this is a valid concern as we might change runc update
to manage devices in the future.
What I wanted to achieve with this is to not have two fields named Path
as this makes the code less readable. Seems that's inevitable...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant more generally that I'm worried we'll have a repeat of the initProcessTime
issue (which ended up with Docker having some super dodgy patching code to work around the issue) -- state.json
is deserialised when loading container state, and if you upgrade runc such that the two JSON specs are no longer compatible you'll end up with errors after upgrading. Then again, I think that extra fields don't produce errors in json.Decode
?
Hi, @kolyshkin , I want to know when this PR will be completed and merged to solve the problem. |
OK, we have a better solution now, see #3842 |
FIXME doc this
Signed-off-by: Kir Kolyshkin kolyshkin@gmail.com