integration-cli: remove emptyfs#22
Conversation
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
| // It is important that this ID remain stable. If a code change causes | ||
| // it to be different, this is equivalent to a cache bust when pulling | ||
| // a legacy-format manifest. If the check at the end of this function | ||
| // fails, fix the difference in the image serialization instead of | ||
| // updating this hash. | ||
| imageTestID := "sha256:11f64303f0f7ffdc71f001788132bca5346831939a956e3e975c93267d89a16d" | ||
| id := inspectField(c, imageTest, "Id") |
There was a problem hiding this comment.
Yeah, agreed, this is the only bit that somewhat concerns me. Not sure what the exact impact would be (and if it matters). Perhaps @tianon or @tonistiigi have a better estimation to see if it's important.
There was a problem hiding this comment.
I believe the intent of this test was to validate that the full round-tripping from a pre-existing tarball (not created by the code-under-test) worked correctly and as expected, and that we didn't accidentally change any of the image configuration in the process, which I think is probably still valuable? Not entirely sure.
My "gut" reaction would be that we should have docker load still import the potentially non-platform-matching image (since that's what users expect from doing that on engines today), and that this test should instead be conditional on the containerd-snapshotter flag (switching from this older ID to whatever the new ID is) -- perhaps even changing the test to not only validate the manifest in that case, but to also then query containerd for the config blob checksum (which would still match this older sha256)?
Definitely on-board with changing all the other tests to no longer use emptyfs though. 👍
|
Closing in favor of #31 |
4 participants
I attempted to run integration tests with the containerd feature flag enabled and noticed that testing hangs after a failed attempt to load the
emptyfsimage.The image is created by the .ensure-emptyfs helper script - the problem is that it uses the old legacy v1 image format, which is not supported by the containerd and fails to import.
My initial reaction was to just change the script to create an emptyfs with a newer layout. It kinda works - there are few problems though:
docker loadimplementation makes the image impossible to load in some scenarios (in my case it doesn't load anything on M1 because it's arm64). I can set the architecture based on theuname -moutput, but it changes the configuration and so it hashes to a different hash than expected which impacts the image id.docker loadto import not-matching platform image, the image id is still not11f64303...because containerd doesn't derive image id form config content. In this case the image is stored asapplication/vnd.docker.distribution.manifest.v2+jsonwhich is addressed by the sha256 sum of the manifest content.I see that the emptyfs is a legacy of the ancient times when
scratchwas a fixed image and given that it's only used in some integration-cli tests I think we can just remove it completely and switch the tests to use some of the frozen images.I replaced it with busybox, because it's already widely used in the integration tests.
Also I'm not sure what really to do with the
TestInspectImagetest. It checks whether the id of the equivalent of the legacyscratchimage doesn't change for backwards compatibility - which sounds like something we should still maintain for backwards compatibility.. but maybe we don't need it?- What I did
Removed emptyfs which unblocks setup of integration tests with the containerd feature enabled