Add device resource update support#3402
Conversation
|
This calls for test cases. |
| 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.
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. |
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 <stefan@agner.ch>
Signed-off-by: Stefan Agner <stefan@agner.ch>
Add support to update Device Resources with runc update. Signed-off-by: Stefan Agner <stefan@agner.ch>
Signed-off-by: Stefan Agner <stefan@agner.ch>
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 |
The rebased patch series in original PR [1] that was used here after update to v1.2.5 were missing the last patch from the old patch series that changes the behavior to add/remove permissions incrementally instead of replacing them. This caused regression described in [2]. With the missing patch added, the permissions are added correctly for all new devices. [1] opencontainers/runc#3402 [2] home-assistant/operating-system#3915 Signed-off-by: Jan Čermák <sairon@sairon.cz>
The rebased patch series in original PR [1] that was used here after update to v1.2.5 were missing the last patch from the old patch series that changes the behavior to add/remove permissions incrementally instead of replacing them. This caused regression described in [2]. With the missing patch added, the permissions are added correctly for all new devices. [1] opencontainers/runc#3402 [2] home-assistant/operating-system#3915 Signed-off-by: Jan Čermák <sairon@sairon.cz>
Add support to update device resources through runc update --resources command. This is required by the Home Assistant OS-Agent to provide device permission updates for the system. Long-term a similar function should get added to runc upstream, so it can be used through directly calling runc or even the Docker API. Signed-off-by: Stefan Agner <stefan@agner.ch> [Jan: - rebased to patches from [1] - added patch for extended default allowed devices since v1.2.4 - addeed patch for incremental update missing in [1] ] [1] opencontainers/runc#3402 Signed-off-by: Jan Čermák <sairon@sairon.cz>
Add support to update device resources through runc update --resources command. This is required by the Home Assistant OS-Agent to provide device permission updates for the system. Long-term a similar function should get added to runc upstream, so it can be used through directly calling runc or even the Docker API. Signed-off-by: Stefan Agner <stefan@agner.ch> [Jan: - rebased to patches from [1] - added patch for extended default allowed devices since v1.2.4 - addeed patch for incremental update missing in [1] ] [1] opencontainers/runc#3402 Signed-off-by: Jan Čermák <sairon@sairon.cz>
Implement support for updating device resources to allow updating CGroup v1/v2 device permissions on container update.
Fixes: #3401