image-rs: use index for layers store path

[squarti]

This PR associates an index for a given image layer and uses the index to build the layer store path. This reduces the path length significantly. The next available layer index is computed by reading the existent directories in the data directory. This is done during pull client initialization.

Fixes: #901

[mkulke]

awesome bug report, thanks. Can you describe in more detail what you are doing in your fix in the PR description?

[squarti]

A possible issue with the approach taken is that pull client cannot run in parallel since the layer index is managed in memory. Although, this already seems to be an issue since the meta store is only saved at the end of pull_image.

[Xynnn007]

Thanks for the fix. Btw I did not find any place that shows mount does not support data longer than 4096 bytes although the behavior shows this.

Another thing I worry about is synchronization issues. Now ImageClient can respond to multiple pull_image requests asynchronously. Each pull_image request will create a PullClient. Assuming multiple pull_image requests are initiated at the same time, it will be possible to obtain multiple identical or close layers_index, and overwriting might occur.

The previous method of using hash values ​​to name layers can avoid this risk assuming that the hashes do not collide. This is possible if an auto-incrementing number is used.

If number is used, do we need to consider using PullClient as part of ImageClient and perform operations such as async_pull_layers and pull_manifest exclusively and atomically?

[squarti]

Would moving the counter to ImageClient as above address these issues?

[ChengyuZhu6]

I'm thinking would it be possible to use the footprint instead of the full hash value as the layer store name? For example, changing from run/image-rs/layers/sha256_055008870f0b0eef21dc8f9be651f07c3f52fa33724d899f8d14bded0a4fa38d to run/image-rs/layers/sha256_055008870f0b?

[Xynnn007]

I'm thinking would it be possible to use the footprint instead of the full hash value as the layer store name? For example, changing from run/image-rs/layers/sha256_055008870f0b0eef21dc8f9be651f07c3f52fa33724d899f8d14bded0a4fa38d to run/image-rs/layers/sha256_055008870f0b?

I have considered it before, but considering that the probability of collision will increase exponentially after truncating the hash value, it may cause unexpected bugs in extreme cases. If we use the self-increasing index as proposed, layer collisions can be absolutely avoid.

[mkulke]

is there a way to write this code in a way that we can unit-test the construction of the layer storage paths? this would be good also for specifying what we are doing.

[Xynnn007]

LGTM. @mkulke PTAL

[ChengyuZhu6]

LGTM, thanks @squarti . While I'm concerned about the ci failure with ubuntu-24.04-arm.

[fitzthum]

CI Green. @mkulke are we good to merge this?

[mkulke]

at the moment we're just logging the errors instead of bailing out. since this is a sensitive area of the code, I would prefer if we would propagate the errors and fail on anything that we encounter and do not expect, i.e. get_layers_index() should return a Result<> since it's a fallible operation.

[mkulke]

thanks for accommodating, a few nits.

it's not great that we swallow the errors at the upper layers now, but apparently that is also what we do with the meta store. It's still an improvement because we don't get something undefined on malformed images, but a predictable and unusuable 0-ed out layerstore. if we unwrap we still need to log the errors though (see suggestion), otherwise it won't be fun to debug.