Add device resource update support #3402
Conversation
|
This calls for test cases. |
| @@ -24,6 +24,9 @@ type Device struct { | |||
|
|
|||
| // Gid of the device. | |||
| Gid uint32 `json:"gid"` | |||
|
|
|||
| // Is Default Device | |||
| IsDefault bool `json:"is_default"` | |||
There was a problem hiding this comment.
This struct is stored on disk and so, on upgrade, any existing containers will have a JSON config that has ever device be "non-default". It seems that this flag is only used for an internal check (and the on-disk flag is never checked) -- in that case, this shouldn't be stored on the filesystem at all and you should hide this field. I'm not sure why we need this flag since we already have a list of the default devices (every item on that list is the default).
(As an aside, we really should have tests to make sure we never break this again -- Docker 17.06.1 was a complete nightmare. I suspect this wouldn't fail tests like that -- since it seems like nothing would actually break -- but it makes me a little worried.)
There was a problem hiding this comment.
It seems that this flag is only used for an internal check (and the on-disk flag is never checked)
Afaik, the on-disk flag is used when updating. CreateDefaultDevicesCgroups is now called from update.go (see updateCommand).
From what I remember this was necessary to "clear" allowed devices. Otherwise, the user would have to pass the default devices to get back to default state.
This struct is stored on disk and so, on upgrade, any existing containers will have a JSON config that has ever device be "non-default".
Hm, this is a problem, can we migrate the data?
There was a problem hiding this comment.
I've reviewed this again, the reason it is required is because default allowed devices get special treatment for via CreateCgroupConfig. To do this, we need to understand which devices have been added through spec, and which have been added from AllowedDevices. This used to be done in a single function createDevices. With commit 390ac2b de-duplicates this logic so it can be used for updates.
There is no need to store this flag in JSON, so I think we can use json:"-" which we use in other places too.
There was a problem hiding this comment.
It turns out, the is_default needs to be stored in the container state to successfully "re-duplicate" on update.
agners
force-pushed
the
main-add-device-support
branch
2 times, most recently
from
e160f2a to
edd7890
Compare
With CGroups v2, device CGroup rules update is not as straight forward since it requires ebpf rules. Currently, runc does support updating this ebpf rules, but when using the runc update command new device rules are not passed yet. A patched runc version (and hopefully future runc versions) support updating device resources, see: opencontainers/runc#3402 Use this (patched) runc capability to support Device CGroup rules updates with CGroups v2.
* Support Device CGroup rules update through runc With CGroups v2, device CGroup rules update is not as straight forward since it requires ebpf rules. Currently, runc does support updating this ebpf rules, but when using the runc update command new device rules are not passed yet. A patched runc version (and hopefully future runc versions) support updating device resources, see: opencontainers/runc#3402 Use this (patched) runc capability to support Device CGroup rules updates with CGroups v2. * Determine CGroup version at initialization time * Address go linter issues
|
Any thoughts or comments for this PR? |
Can you please squash in the intermediate changes so that it can be reviewed commit by commit. You had three commits originally, now it's 11. |
agners
force-pushed
the
main-add-device-support
branch
2 times, most recently
from
bab777f to
2ec946e
Compare
|
I've rebased and suqashed the commits. There are essentially the original three commits plus one for the integration tests. |
Signed-off-by: Stefan Agner <[email protected]>
Signed-off-by: Stefan Agner <[email protected]>
Add support to update Device Resources with runc update. Signed-off-by: Stefan Agner <[email protected]>
Signed-off-by: Stefan Agner <[email protected]>
agners
force-pushed
the
main-add-device-support
branch
from
2ec946e to
f17333d
Compare
|
Any thoughts on this PR? It is still a feature we are looking for. |
|
@kolyshkin is there a chance to get this feature moving forward? It is kinda road block for our users to adopt CGroupsV2 🙈 In CGroupsV1 we were able to update the device permission directly (which was hacky, granted, but it worked). However, this approach is much more sane and works for CGroupsV2 too, so we really like to have such support upstream. |
|
Is there anything missing on this commit to be pushed? It's still a blocker for HomeAssistant Supervised home-assistant/supervised-installer#372 |
Implement support for updating device resources to allow updating CGroup v1/v2 device permissions on container update.
Fixes: #3401