[WIP] Add CRI-O support #1119
[WIP] Add CRI-O support #1119
Conversation
This adds support for using Spegel with CRI-O by implementing a new CRIoClient that accesses CRI-O's image content cache for P2P distribution. Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
sohankunkerkar
force-pushed
the
crio-spegel
branch
from
3973949 to
3ba8849
Compare
|
@phillebaba Could you take a look at it? |
phillebaba
left a comment
phillebaba
left a comment
There was a problem hiding this comment.
This is a good start to get CRIO support in Spegel. I have some questions regarding the implementation that seems like they add more complexity than needed. The main ones right now is the optional store and then the lack of the ns optional parameter. The latter is a bit tricky as it requires changes to CRIO.
I and thinking about if it is better to split this feature into a few PRs. The main things missing right now are integration tests for CRIO like the ones that we have with Containerd. I dont know if we have an option to run something similar to Kind but OpenShift that would allow tests. These things are important long term to ensure future stability.
| // | ||
| // Supports two formats: | ||
| // 1. Containerd: /v2/<repository>/manifests/<ref>?ns=<registry> | ||
| // 2. CRI-O: /v2/<registry>/<repository>/manifests/<ref> (registry embedded in path) |
There was a problem hiding this comment.
I would prefer if CRIO added the ns parameter to the request, as it is now merged in the OCI distribution spec. We need to discuss this more.
There was a problem hiding this comment.
Yeah, I have already raised a PR here to get some consensus.
| MirrorResolveTimeout time.Duration `arg:"--mirror-resolve-timeout,env:MIRROR_RESOLVE_TIMEOUT" default:"20ms" help:"Max duration spent finding a mirror."` | ||
| MirrorResolveRetries int `arg:"--mirror-resolve-retries,env:MIRROR_RESOLVE_RETRIES" default:"3" help:"Max amount of mirrors to attempt."` | ||
| DebugWebEnabled bool `arg:"--debug-web-enabled,env:DEBUG_WEB_ENABLED" default:"true" help:"When true enables debug web page."` | ||
| CRIOImageContentCacheDir string `arg:"--crio-image-content-cache-dir,env:CRIO_IMAGE_CONTENT_CACHE_DIR" default:"/var/lib/containers/storage/image-content-cache" help:"Path to CRI-O image content cache directory. If this path exists, CRI-O mode is used."` |
There was a problem hiding this comment.
Need to think about the future of the configuration here. Maybe it is better to rethink this properly.
|
|
||
| // CRIoClient accesses CRI-O's image content cache and containers/storage for P2P distribution. | ||
| // It serves both layer blobs (from image content cache) and manifests/configs (from containers/storage). | ||
| type CRIoClient struct { |
There was a problem hiding this comment.
Rename this to CRIO to match Containerd.
| } | ||
|
|
||
| // containers/storage is optional - it may not be accessible in nested container environments. | ||
| var store storage.Store |
There was a problem hiding this comment.
Why would we make the store optional? I have trouble understanding why Spegel would run in a nested container environment without access to the store?
There was a problem hiding this comment.
I will remove that comment. I think that's for the testing purpose.
2 participants
This adds support for using Spegel with CRI-O by implementing a new CRIoClient that accesses CRI-O's image content cache for P2P distribution.
There's a PR in-flight to add support for storing tar image content in CRI-O.