Looks way too hacky. Perhaps if we rename Set to Update, this will look less hacky.

Ideally, though, we should add something like a check that devices are already as they were set before, and skip the update in such a case.

well that is the problem. With the nvidia device plugin or runtime, it modifies the devices outside of the spec. Then if you call an update, it wipes it out. That wouldn't be too big of an issue but devices are not specified or can be changed on runc update

So maybe set SkipDevices right there in update.go (looks less hacky to me), and do not save it to the state file (as a "real" container started by runc can never has it set to true.

This is what I mean

diff --git a/libcontainer/configs/cgroup_linux.go b/libcontainer/configs/cgroup_linux.go
index 715ec1cb..e2472fe7 100644
--- a/libcontainer/configs/cgroup_linux.go
+++ b/libcontainer/configs/cgroup_linux.go
@@ -127,8 +127,8 @@ type Resources struct {
 
        // SkipDevices allows to skip configuring device permissions.
        // Used by e.g. kubelet while creating a parent cgroup (kubepods)
-       // common for many containers.
+       // common for many containers, and also by runc update.
        //
        // NOTE it is impossible to start a container which has this flag set.
-       SkipDevices bool `json:"skip_devices"`
+       SkipDevices bool `json:"-"`
 }
diff --git a/update.go b/update.go
index ce89de08..7d5fba34 100644
--- a/update.go
+++ b/update.go
@@ -329,6 +329,11 @@ other options are ignored.
                        config.IntelRdt.MemBwSchema = memBwSchema
                }
 
+               // XXX(kolyshkin@): currently "runc update" is unable to change
+               // device permissions, so add this to skip updating the device cgroup.
+               // This helps in case an extra plugin (nvidia GPU) applies some
+               // configuration on top of what runc does.
+               // Note this field is not saved into container's state.json.
+               config.Cgroups.SkipDevices = true
+
                return container.Set(config)
        },
 }

Still looks like a kludge, but at least it is in more proper place.

// Cc @cyphar

With the nvidia device plugin or runtime, it modifies the devices outside of the spec.

Would it make sense to accommodate this usecase in the spec?

@kolyshkin ok, that will work since we remove it from the state on disk. I could have refactored more but didn't want to cause a big mess for such a small change at this point with the 1.0 being finalized. I can make this change if you are good with it. Then after we have the 1.0 out the door, we can look at a better refactor without just hacking it in. 1.0 is more important at this point and keeping the code from churning.

@h-vetinari I think we will have to wait until the CDI work in finalized. Doing these side band operations is hacky but we don't have a better solution at the moment that works across various runtimes.

Opened #2994 based on the diff above.