-
Notifications
You must be signed in to change notification settings - Fork 676
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
config.rootfs is superfluous and should be removed #743
Comments
(This is also a question phrased as a bug report. I'd like to know why we need the rootfs DiffIDs, and why it should be in the config object, as opposed to manifest (with the rest of the layer info). The rootfs inclusion in config was unexplained and surprising to me from reading the spec documents - and indeed the ChainID discussion seems to be an obsolete section, no longer referenced by the actual schema) |
Not only that, it is relatively expensive to compute (e.g. converting from Docker schema 1). |
After a lengthy irc discussion, I've learned:
The last point is the important one, and this trivial improvement is clearly not worth that level of churn. I expect this means this issue will sit on the pile for some time awaiting a larger, cumulative update to the config spec... (@stevvooe: let me know if I've summarised our long conversation incorrectly - I'll edit as needed) |
@anguslees Thanks for summarizing! I understand some things about this specification are non-ideal but we can't let that get in the way of working software. As more implementations migrate to being able to handle digest/mediatype resolution of container image components, this is a transition that will be easier to do in the future. |
I agree this adds an expense for diffID calculation, @mattmoor . Likely, a smoother next step could be relaxing the MUST on rootfs.type = layers, so that a future value and approach could be used. Regardless, for your ask of specifically demoting |
The suggestion is still to drop rootfs to ignored. The recognition above is that that will break existing clients and so requires some sort of deprecation/notification process.
I guess that depends on the scope of this GitHub project. If the point is to track issues with the spec that can be included in some future revision, then I guess it should stay open. If changes to the spec are managed through some other process and out of scope for GitHub issues, then yes, we should close it (and move to that other forum). |
@anguslees @stevvooe I cannot find any references to
could you please clarify what you mean? Sorry to revive this old issue |
@tianon I understand the idea of ChainID, I don't understand where the following was pointed out
|
There is a section in the spec that describes a "Layer ChainID". This ChainID is not referred to elsewhere in other specs, and afaict from much my earlier conversation is just a dockerd internal implementation historical point of interest. It serves no purpose in the standard, and I suggest this whole "Layer ChainID" section can be removed immediately - although it is obviously not worth publishing a new spec revision just for this minor textual cleanup. Note the above ChainID reference was a minor aside on this bug report. This bug report is that the entire config.rootfs section is unnecessary and (I propose) can be removed, if/when we publish the next version of this spec. At that point, the DiffID section (in addition to ChainID) can also be removed. |
I'm fine with removing ChainID, but not the config.rootfs section. That would result in a non-unique Image ID, breaking downstream consumers including Docker and Kubernetes. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
It's been a while since I looked at the specs in enough detail ... but is this still true in our new containerd world? I think these systems use manifest digest in all the important (exposed to the user) places, and if they use imageID internally somewhere, then it can (and arguably should!) be replaced with manifest digest or other reference. I agree we would need to make it known that config digest is no longer unique. I think that means that ImageID should also be removed from image-spec, when we go ahead with this hypothetical new spec revision. To be super-clear: yes, removing rootfs, and removing ImageID would be a breaking change, reserved for a future revision of image-spec - as mentioned earlier above. I am not proposing to sneak this into the existing version without a deprecation/notification process. |
See #1173 |
containerd still uses the rootfs section to identify the snapshots used by containers. The image ID is also used by the CRI plugin and there is value in keeping the image ID as referring to the single runnable image rather than to an index of multiple runnable images.
We should do this otherwise any innovative work to move away from the layer model will require having a "non-compliant" image config. The rootfs section should not be removed though, an alternative way to uniquely identify the container rootfs will still be relevant. |
Could an innovation away from the layer model use a different config media type? If so, the config.md wouldn't apply and the spec language for the manifest.md has a looser recommendation that gets away from the MUST language:
|
I would expect it to use a different media-type, but the main takeaway I had from the whole artefacts saga is that public registries are so strict about media-types (which we made up 🤔...) that any innovation requires abusing parts of the spec to sneak things past registries. Otherwise you won't be able to get things working "well enough" to start working on spec changes. Maybe I'm off-base, but that was my impression. 😩 |
I think config.rootfs effectively[*] duplicates the manifest.layers information available elsewhere, and thus config.rootfs can/should be removed from the image spec. This makes image generation slightly simpler, removes an extra checksum calculation step during unpacking, and makes the image config object portable across minor image versions (one more cacheable thing).
[*] The values are not strictly identical, but the DiffIDs can be easily calculated from the manifest layer tar.gz.
Specifically, I propose just demoting
rootfs
fromapplication/vnd.oci.image.config.v1+json
from "required" to "ignored", which will be backwards compatible.history
can remain as an optional annotation, although I note it appears highly out of place withoutrootfs
, and probably belongs in some layer or manifest metadata instead.If a runtime or other tool requires a value similar to the rootfs DiffID, this can be calculated during rendering from the image layers.
Note also that with this change, the sections discussing DiffID and ChainID calculation can be removed from the image config spec, since DiffID+ChainID are no longer relevant terms anywhere in the oci image spec (these concepts may be useful to runtimes, so perhaps might be moved there).
The text was updated successfully, but these errors were encountered: