-
Notifications
You must be signed in to change notification settings - Fork 209
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
Proposal: Deprecate single-request monolithic blob uploads before 1.0 #234
Comments
@jdolitsky relevant to the PR @dmcgowan may have more historical context for this client authors: please let me know if you disagree with my assessment of SRMU use in the wild registry operators: please let me know if you disagree with this, if you do or do not support SRMU, and if you would support deprecation of SRMU |
If majority of clients do not use it, I have no issue with removing |
Please keep in mind that for a on-premise use case it is not unusual to have (for example) a private Github instance, a private Jenkins instance, a private registry and worker nodes to roll out deployments, all "internal" to a k8s environment. Thinking about this some more, if the use cases and tradeoffs are so different between cloud operators and on-premise, maybe it needs to be captured/reflected so in the dist-spec. Just a thought! |
@rchincha I'm not sure I follow -- can you explain how on-prem use cases relate to monolithic uploads? |
With regards to your earlier Backblaze comment why SRMU is bad. The main concern appears to be that without any way to indicate to the client as to where its (possibly large) upload can be routed, hurts the cloud operators to support the spec in its current form. How many customers actually do SRMU and why? But in use cases where it could be supported, perhaps it provides a better UX (response times, single cURL request, etc) - building/pushing images frequently, which happens when you have a growing team of developers and projects, and just for this situation UX trumps infra costs (on-premise) +1 for this version of the proposal At the very least, we should update the spec to say that clients should interpret 201 as "upload succeeded" and 202 as "proceed with upload at Location" |
I am only aware of one client that does this (by default). Not sure why, but I would guess because sending one HTTP request is easier than sending two 😄
Yeah, I think the curl example is the most compelling. Latency is a valid concern, but I think the redirect is usually a pretty small portion of the overall upload, and the trade-off for scalability seems pretty obvious most of the time. That's one reason I want to just move it to a "deprecated" place to discourage its use, rather than just delete it. My hope is that by documenting that clients need to handle a 202, clients will be able to fall back to non-SRMU for registries that don't support it (many). For registries that do support it, the client should be happy, but for registries that don't support it, they can just continue through the normal path without any (additional) overhead. |
Oh and I forgot to respond to the original point re: on-prem... The multi-request upload path allows for the registry to hand off ingress to a separate service, but doesn't require it. As a response to the POST that initiates a blob upload, the |
Relavent conformance test: https://github.com/opencontainers/distribution-spec/blob/master/conformance/02_push_test.go#L63 |
This sounds like the best behavior to me |
dropping from milestone since #239 merged |
Two points, if you keep it in the spec I would strongly warn about using it for too large entities and also mandate that Expect: Continue should be sent/supported, especially if the entities are larger than a few mb. |
background
It is possible to upload a blob in a single POST, per the spec, called a monolithic upload.
It is also possible to upload a blob via POST/PUT, which is also called a monolithic upload.
I'm talking about the first one. I call this "single request monolithic upload", which may be abbreviated as "SRMU" in the rest of this proposal.
I've put specific, actionable proposals in bold.
I can send a PR that implements my proposal, but I'd like to solicit some feedback first to see if anyone feels strongly opposed.
proposal
(Optional aside (3): We should have a deprecation document. At one point we deprecated catalog, but I can't seem to find reference to it anywhere after #178. It can be helpful to clearly document the differences, as in opencontainers/image-spec#817)
rationale
ambiguity
This API is not well defined, so it is unclear how clients should interpret the registry's response.
For cross-repo mounting, if the mount failed for any reason, the registry will return a 202 that indicates you should proceed with the upload as usual. If the mount succeeds, the registry will return a 201, and clients can skip the upload.
For SRMUs, we don't have such a distinction... from the current spec at HEAD:
See #230 (comment), where a test enforcing the spec (202 is allowed) would pass for a registry even though it doesn't support it.
At the very least, we should update the spec to say that clients should interpret 201 as "upload succeeded" and 202 as "proceed with upload at
Location
"This has the nice failure mode for registries that don't support SRMUs. If a registry doesn't bother to check the request body or
?digest
querystring parameter of POST requests, they will likely just return a 202 with aLocation
, as per a normal chunked upload. Clients that see this should probably assume that the registry does not support SRMU in this case, and proceed with a chunked upload or POST/PUT monolithic upload.lack of support
From #211 (comment)
Deprecation should not imply that registries that currently support this stop, so any clients that rely on a specific registry implementing this should still just work. I know of a single client that actually uses this upload mechanism, so there is very low cost to deprecating it. It's also a very easy fix, client-side, if a registry decided to stop supporting it.
I've just tried to perform this against Docker Hub, and it doesn't work. The upload POST response has a 202 status with a
Location
header, and the subsequent manifest PUT fails withMANIFEST_BLOB_UNKNOWN
, presumably because the SRMU failed.GCR does support this, but I'd rather it not.
(I have not tried other registries, feel free to pile on with support or lack of support on your favorite registry.)
cost
Also from #211 (comment)
For some more discussion of why the SRMU is bad, see this great post from backblaze.
Specifically relevant:
This is analogous to what we have in the registry spec. The registry is responsible for handling the blob ingress and cannot easily hand off that traffic to another service via the
Location
header.The text was updated successfully, but these errors were encountered: