Remove deprecated RDT fields #278
Conversation
|
@marquiz, @klihub, I ran into problems on latest upstream runtime-spec and cdi mismatches when I was trying to build containerd with the latest runtime-spec. This change helped building it, but I'm not very familiar with cdi, I might miss something essential in this PR. If you have time, please have a look with even more suspicion than usually. 👀 D |
There was a problem hiding this comment.
Ach, CDI had those fields defined 🤦
Yes, those were removed from the runtime spec as they were really problematic and had no known runtime implementation. See opencontainers/runtime-spec#1287
Not sure how CDI should handle these. Just drop or filter out (or smth else) 🤔
|
@marquiz since you were the one to add the fields in the first place could you comment on whether these fields are used "in the wild"? If we think the risk of this is low we could drop the fields -- although this would required a spec version bump. Also, how would we ensure that a modified spec is compatible with the incoming OCI runtime spec version as we would not want to adjust this. |
I guess ideally/in principle we should deprecate those fields. So basically, remove the them from IntelRDT.toOCI() output, log a warning or an error if we load a CDI Spec file with either of those fields set to true, then in the next phase also refuse to load the whole CDI Spec if either is set. If we chose to do this, then additionally we should set the newly introduced Then again, according to a comment by @marquiz in the above linked PR no known runtime implementations impemented either of these fields so if any CDI Spec currently in use tries to set those it anyway does not work correctly. So maybe we could just drop them altogether, like it is done in this PR. @elezar @bart0sh WDYT would be the proper course of actions here ? |
@elezar According to @marquiz' original comment opencontainers/runtime-spec#1287 (comment), no known runtime implementation has ever implemented either of these fields. So it would seem to me that it should be safe to skip the deprecate-then-remove transitions and drop these in a single go. |
|
@askervin Can you do a |
I'd say the risk of somebody using those fields is very very low. There are no implementations for those and their specification was very vague/contradicting.
This might be the most proper way.
I would suggest not to do this to avoid further confusion and possible breakage in the future. People start to use and depend on these (for accident or whatever) and then if they're removed we brake them. |
askervin
force-pushed
the
5cP_runtimespec_rdtchange
branch
from
93eccac to
49dfbc7
Compare
cmd/cdi/go.mod
Outdated
| @@ -4,7 +4,7 @@ go 1.20 | |||
|
|
|||
| require ( | |||
| github.com/fsnotify/fsnotify v1.5.1 | |||
| github.com/opencontainers/runtime-spec v1.1.0 | |||
| github.com/opencontainers/runtime-spec v1.2.2-0.20250804081626-bfdffd548aa6 | |||
There was a problem hiding this comment.
If the new field is not yet in a new released spec version, then we could first remove the deprecated fields and then add the new field once a tag has been released.
|
I think the simplest solution is to remove the deprecated fields. It seems that the risk is relatively low that we have customers actually using this. Let's do this as a separate commit so that it's easier to track from a release notes / client perspective. We can add the new field in a follow-up before releasing a new spec version. |
|
@elezar, indeed, the new field is not yet released in runtime-spec. Removing old fields is not released either. So we are pretty well in sync. Anyway, if handling this change in two PRs makes still sense to you, I'll keep this as "drop deprecated fields" PR, and create new PR that adds the new field. How that sounds? |
@askervin @elezar Then IMO at least the PR for the new field should be kept as a draft and updated once a new runtime spec is tagged which has the new field. Removing the deprecated field could be merged right away IMO, though (as long as it does not change our runtime-spec dependency). |
Yes, let's drop the deprecated fields in a PR now (this should NOT have any dependency updates) and create a draft PR to be updated once a tagged spec is available for the new field. |
Drop IntelRdt.EnableCMT and EnableMBM to match runtime-spec versions after v1.2.1. Signed-off-by: Antti Kervinen <[email protected]>
askervin
force-pushed
the
5cP_runtimespec_rdtchange
branch
from
49dfbc7 to
3ebd90e
Compare
askervin
changed the title
Drop IntelRdt.EnableCMT and EnableMBM, and switch to EnableMonitoring to match upstream runtime-spec.