Remove dependency on github.com/opencontainers/runtime-tools#316
Remove dependency on github.com/opencontainers/runtime-tools#316tianon-sso wants to merge 3 commits into
github.com/opencontainers/runtime-tools#316Conversation
The `generate` package was used in two places purely as a convenience wrapper over `*oci.Spec` field mutations -- none of its logic was load-bearing. Replace it with direct spec manipulation throughout. In `pkg/cdi/container-edits.go`, drop the `ocigen.Generator` value and operate on `*oci.Spec` directly in `Apply`. Extract small private helpers for the operations that have non-trivial logic: `addMultipleProcessEnv` (deduplicates by key), `removeDevice` and `removeMount` (find-and-splice), and `addProcessAdditionalGid` (append-if-absent). In `cmd/cdi/cmd/cdi-api.go`, replace the three-step `gen.NewFromSpec` / `ClearLinuxDevices` / `AddDevice` sequence in `collectCDIDevicesFromOCISpec` with a single partition loop over `spec.Linux.Devices`. Assisted-By: "claude my eyes right out" Signed-off-by: Tianon Gravi <tianon.gravi@docker.com>
|
Friendly ping - |
|
Sorry for the delay here. I'll have a look tomorrow. |
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
A first pass of this looks good. One question I would have is whether we wanted to add a local type that wrapps *oci.Spec like runtime-tools does to reduce the diff further, but I assume that it probably isn't worth the effort for the reduced feature set we have here.
@klihub what do you think?
Signed-off-by: Evan Lezar <elezar@nvidia.com>
| if specgen == nil || netDev == nil { | ||
| // addMultipleProcessEnv adds or replaces environment variables on the process, | ||
| // deduplicating by key. | ||
| func addMultipleProcessEnv(process *oci.Process, envs []string) { |
There was a problem hiding this comment.
One thing that I want to call out (thanks Codex). There is a subtle difference between the new code and the original runtime-tools implementation. In the previous implemenation, the entries in envCache were constucted using ENV=value strings when loading an OCI spec. This means that when updating envs by name from the CDI spec, we would not overwrite the initial envvar, but always append. (Subsequent CDI updates would append). This was likely a bug in runtime-tools.
Note that there is also a slight mismatch to the wording of the CDI specification which states that envvars are appended to the environment.
I do, however, think that the new behaviour is correct and should also remove ambiguity around dealing with multiple envvars. We may want to update the wording around evvars for the spec though.
| } | ||
| prefix := parts[0] + "=" | ||
| replaced := false | ||
| for i, e := range process.Env { |
There was a problem hiding this comment.
Implementing a simplified generator type locally will allow us to cache the envvars to improve performance for containers with many envvars. (Using envvars in CDI is not common, so it may not be worth it though).
| spec.Linux = &oci.Linux{} | ||
| } | ||
| removeDevice(spec, dev.Path) | ||
| spec.Linux.Devices = append(spec.Linux.Devices, dev) |
There was a problem hiding this comment.
Another inconsistency is that specgen.AddDevice(dev) would skip the addition if the spec had multiple matching devices to start with.
There was a problem hiding this comment.
Is that something you'd like me to update the PR for/with, or just something you want to note? 🙇
There was a problem hiding this comment.
It was initially just a note from my side, but what about adding an ensureDevice(*oci.Spec, *Device) function that handles all of this internally?
There was a problem hiding this comment.
(similarly for ensureMount where we follow the same pattern). Not blockers from my side though.
Signed-off-by: Tianon Gravi <tianon.gravi@docker.com>
@elezar I think that would be cleaner. |
| specgen := ocigen.NewFromSpec(spec) | ||
| if len(e.Env) > 0 { | ||
| specgen.AddMultipleProcessEnv(e.Env) | ||
| if spec.Process == nil { |
There was a problem hiding this comment.
Maybe this could be an ensureProcess() for consistency.
| specgenAddLinuxNetDevice(&specgen, dev.HostInterfaceName, (&LinuxNetDevice{dev}).toOCI()) | ||
| } | ||
| for _, dev := range e.NetDevices { | ||
| ensureLinuxNetDevices(spec) |
There was a problem hiding this comment.
If we kept the length check it would be more consistent and we could call ensureLinuxNetDevices() once in the if branch outside the for loop.
klihub
left a comment
klihub
left a comment
There was a problem hiding this comment.
I have a few minor nits.
@elezar I am fine with this if you and others consider eliminating our dependency on runtime-tools that important. I am not sure and I tend to think that a properly maintained runtime-tools/generate would be more useful than re-implementing the same, in this case arguably small, functionality multiple times wherever runtime-tools/generate could be used, but this is just my subjective opinion.
I agree that a well-maintained One thing to note is that at least for Both Podman and Cri-o have been using the dependency from the beginning. |
|
@tianon-sso thanks again for this change. We decided to go with #317 instead as it more clearly leaves open the option to switch back to |
3 participants
The
generatepackage was used in two places purely as a convenience wrapper over*oci.Specfield mutations -- none of its logic was load-bearing. Replace it with direct spec manipulation throughout.In
pkg/cdi/container-edits.go, drop theocigen.Generatorvalue and operate on*oci.Specdirectly inApply. Extract small private helpers for the operations that have non-trivial logic:addMultipleProcessEnv(deduplicates by key),removeDeviceandremoveMount(find-and-splice), andaddProcessAdditionalGid(append-if-absent).In
cmd/cdi/cmd/cdi-api.go, replace the three-stepgen.NewFromSpec/ClearLinuxDevices/AddDevicesequence incollectCDIDevicesFromOCISpecwith a single partition loop overspec.Linux.Devices.Assisted-By: "claude my eyes right out"