Remove github.com/docker/distribution/registry/client package #1487

Luap99 · 2022-03-09T17:17:54Z

Using the github.com/docker/distribution/registry/client package will
import many huge prometheus dependencies, e.g.

github.com/prometheus/client_golang/prometheus/promhttp
github.com/prometheus/client_golang/prometheus
github.com/prometheus/procfs
and even more...

All of these dependencies are completely unused AFAICT but will still end
up in a binary because they are imported transitive.

github.com/docker/distribution/registry/client is only used to check
http errors so I think it makes sense to copy only the required code
into the docker package.

I vendored this commit into podman and it saves over 700KB in binary
size.

rhatdan · 2022-03-09T18:14:57Z

LGTM
@mtrmac @vrothberg PTAL

TomSweeneyRedHat · 2022-03-09T22:19:35Z

docker/errors.go

@@ -59,3 +63,144 @@ func registryHTTPResponseToError(res *http.Response) error {
 	}
 	return err
 }
+
+// Code below is taken from https://github.com/distribution/distribution/blob/a4d9db5a884b70be0c96dd6a7a9dbef4f2798c51/registry/client/errors.go


@vrothberg and @mtrmac 's call, but this almost seems like this new code should be in a separate file. I"m going back and forth on it.

I concur. It should be a separate file.

In a second commit, we should also unexport the API and symbols.

+1 to both, if this would end up happening.

vrothberg

Dropping 700k from the binary is sufficiently attractive to me.

We definitely need to add the tests as well but let's wait for @mtrmac's blessing before.

vrothberg · 2022-03-10T08:48:59Z

docker/errors.go

@@ -59,3 +63,144 @@ func registryHTTPResponseToError(res *http.Response) error {
 	}
 	return err
 }
+
+// Code below is taken from https://github.com/distribution/distribution/blob/a4d9db5a884b70be0c96dd6a7a9dbef4f2798c51/registry/client/errors.go


I concur. It should be a separate file.

In a second commit, we should also unexport the API and symbols.

mtrmac

As is, I’m not at all sure that this is worth it.

(None of the client.*Error values are explicitly documented API, but they are soft of an API in practice. Though that’s much more the case for github.com/docker/distribution/registry/api/errcode, with various uses in c/common, Buildah, Podman.)
Right now this would remove 700 kB, but soon https://github.com/sigstore/fulcio/blob/main/pkg/api/metrics.go would just drag it all back in, and we would we back where we started.

Basically I think we need to decide whether we are going to make no use of Prometheus (or only an opt-in use) a universal rule, and insist/contribute in upstreams to make that viable. Is that reasonable to aim for?

Alternatively, an (admittedly somewhat desperate) approach might be for the top-level users to use go.mod’s replace directive to just replace all of the Prometheus client with something dummy that does nothing and has no dependencies. (Compare prometheus/client_golang#196 .) That would allow us to drop that dependency chain, whether upstreams cooperate or not.

(Now, moving just the parser could be useful just because it would allow us to integrate better — registryHTTPResponseToError is just plumbing to interface with docker/distribution, and with the ability to modify HandleErrorResponse, most of that plumbing could almost entirely disappear. But that would be a different PR, with much more focus on interoperability/test coverage.)

mtrmac · 2022-03-10T14:26:58Z

docker/errors.go

@@ -59,3 +63,144 @@ func registryHTTPResponseToError(res *http.Response) error {
 	}
 	return err
 }
+
+// Code below is taken from https://github.com/distribution/distribution/blob/a4d9db5a884b70be0c96dd6a7a9dbef4f2798c51/registry/client/errors.go


+1 to both, if this would end up happening.

mtrmac · 2022-03-10T14:43:54Z

docker/errors_test.go

@@ -8,7 +8,6 @@ import (
 	"testing"


If we are going to copy the code, we should also copy registry/client/errors_test.go, or any other relevant tests from the original repo.

Luap99 · 2022-03-10T16:47:38Z

My goal is to reduce the podman binary size. As it looks there is really not much to do there, the prometheus code is large and we do not need it. Many dependencies are deeply wired and cannot be removed, this one seems like an easy way to safe space. Most of my other findings will only reduce size by < 100 KB. So I would prefer if we could get this in.

Right now this would remove 700 kB, but soon https://github.com/sigstore/fulcio/blob/main/pkg/api/metrics.go would just drag it all back in, and we would we back where we started.

Yeah that would be unfortunate.

(None of the client.*Error values are explicitly documented API, but they are soft of an API in practice. Though that’s much more the case for github.com/docker/distribution/registry/api/errcode, with various uses in c/common, Buildah, Podman.)

Do you fear that this change is breaking consumers?

Alternatively, an (admittedly somewhat desperate) approach might be for the top-level users to use go.mod’s replace directive to just replace all of the Prometheus client with something dummy that does nothing and has no dependencies. (Compare prometheus/client_golang#196 .) That would allow us to drop that dependency chain, whether upstreams cooperate or not.

I doubt that maintenance would be worth it. It would be just a workaround for the actual issue and it would still take some space for the stub interface.

I leave this up for the maintainers to decide if you want to go forward with this before I apply the requested changes.

mtrmac · 2022-03-10T16:58:30Z

Do you fear that this change is breaking consumers?

It’s a risk. I’d classify it as fairly low, at least Google can’t find any external users of the two error types.

vrothberg · 2022-03-11T09:35:27Z

Right now this would remove 700 kB, but soon https://github.com/sigstore/fulcio/blob/main/pkg/api/metrics.go would just drag it all back in, and we would we back where we started.

Great catch, @mtrmac! I don't think that it should block this PR here though. Taming the dependencies will never be finished, I guess, and we can tackle them as they come in. In case of fulcio, I am sure we can introduce a build-tag upstream to debloat - other callers may benefit as well.

mtrmac · 2022-03-11T16:56:39Z

I don't think that it should block this PR here though. Taming the dependencies will never be finished, I guess, and we can tackle them as they come in.

There’s a thin line between yak shaving an endless, but clearly useful work, and doing something that is ultimately known not to make a difference. I’m honestly not sure on which side we are here.

So I’m somewhat skeptical but not strongly opposed to this PR, and I’m fine with doing this if there is otherwise consensus that this is worth trying.

In case of fulcio, I am sure we can introduce a build-tag upstream to debloat - other callers may benefit as well.

Should we try to do that with docker/distribution = distribution/distribution as well, then? (Maybe even the same build tag?)

rhatdan · 2022-03-13T15:33:52Z

I think we should go forward with this PR, and open an issue with Docker.

vrothberg · 2022-03-14T09:44:28Z

I think we should go forward with this PR, and open an issue with Docker.

That seems contradictory to me.

Should we try to do that with docker/distribution = distribution/distribution as well, then? (Maybe even the same build tag?)

Sounds like a good idea to me. @Luap99, do you want to give it a shot?

Luap99 · 2022-03-14T13:35:14Z

Should we try to do that with docker/distribution = distribution/distribution as well, then? (Maybe even the same build tag?)

Sounds like a good idea to me. @Luap99, do you want to give it a shot?

I am not against trying this but size gained will be less, the current import path seems to be github.com/docker/distribution/registry/client -> github.com/docker/distribution/registry/storage/cache -> github.com/docker/distribution/metrics -> github.com/docker/go-metrics -> github.com/prometheus/client_golang/prometheus -> and more prometheus stuff

When we add a build tag to disable prometheus, we would only gain this one dependency. In theory the other packages in this list could import other big dependencies that we do not need. This is not be the case today but it could be in the future.

If we want to change upstream I think it would make more sense to split the error handling into a separate package which does not need further imports and we could import this package instead.

tyler92 · 2022-08-31T12:49:19Z

Is there any chance to merge this PR? I find it very useful too.

Luap99 · 2022-09-05T12:05:04Z

Right now this would remove 700 kB, but soon https://github.com/sigstore/fulcio/blob/main/pkg/api/metrics.go would just drag it all back in, and we would we back where we started.

@mtrmac Is the sigstore work fully merged?

I do not see any new references to github.com/prometheus/client_golang/prometheus so I think it would make sense to try to remove it from here.

I can also try to push distribution/distribution#3608 forward if this option would be preferred?

mtrmac · 2022-09-05T16:54:52Z

Right now this would remove 700 kB, but soon https://github.com/sigstore/fulcio/blob/main/pkg/api/metrics.go would just drag it all back in, and we would we back where we started.

@mtrmac Is the sigstore work fully merged?

The Fulcio part does not exist in c/image yet — but I have also removed that dependency in sigstore/fulcio#616 . So this specific concern no longer applies, and that turns this PR from ineffective to useful. (OTOH some more users of docker/distribution have been added. AFAICS those don’t include the registry/client subpackage right now.)

That makes this PR quite a bit more attractive to me. I read @vrothberg as being supportive of the idea from the start, so, let’s do it.

Could you address the earlier review feedback, please?

I can also try to push distribution/distribution#3608 forward if this option would be preferred?

It would be nice for the wider ecosystem, to spread the idea that simple clients should not have hard-coded dependencies on infrastructure for serving metrics over HTTP. Personally, I wouldn’t say that I prefer that PR any more — d/distribution releases have been very infrequent, and moving the HTTP parser in-package so that we don’t need to override its outcomes in docker/errors.go (as in #1299 ) could — eventually, after we have much more test coverage and confidence — result in a simpler implementation overall.

Luap99 · 2022-09-06T08:41:53Z

Could you address the earlier review feedback, please?

done

rhatdan · 2022-09-06T13:08:09Z

LGTM

mtrmac

LGTM other than that one nit, feel free to merge without another review round.

Thanks!

(FWIW, this removes about 3 % from Skopeo’s size on macOS.)

docker/distribution_error.go

Using the github.com/docker/distribution/registry/client package will import many huge prometheus dependencies, e.g. * github.com/prometheus/client_golang/prometheus/promhttp * github.com/prometheus/client_golang/prometheus * github.com/prometheus/procfs and even more... All of these dependencies are completely unused AFAICT but will still end up in a binary because they are imported transitive. github.com/docker/distribution/registry/client is only used to check http errors so I think it makes sense to copy only the required code into the docker package. I vendored this commit into podman and it saves over 700KB in binary size. Signed-off-by: Paul Holzinger <[email protected]>

mtrmac

LGTM. Thanks!

Luap99 force-pushed the remove-deps1 branch from 270a3b2 to 38af274 Compare March 9, 2022 17:25

TomSweeneyRedHat reviewed Mar 9, 2022

View reviewed changes

vrothberg reviewed Mar 10, 2022

View reviewed changes

mtrmac reviewed Mar 10, 2022

View reviewed changes

Luap99 mentioned this pull request Mar 24, 2022

Bump github.com/prometheus/client_golang to v1.11.1 containers/podman#13642

Merged

mtrmac mentioned this pull request Jul 12, 2022

Errors returned are not typed making it difficult to distinguish for approriate handling. #1110

Open

Luap99 force-pushed the remove-deps1 branch from 38af274 to 67d4297 Compare September 6, 2022 08:40

mtrmac reviewed Sep 6, 2022

View reviewed changes

docker/distribution_error.go Outdated Show resolved Hide resolved

Luap99 force-pushed the remove-deps1 branch from 67d4297 to 4911372 Compare September 6, 2022 16:41

mtrmac approved these changes Sep 6, 2022

View reviewed changes

mtrmac merged commit d2420cb into containers:main Sep 6, 2022

Luap99 deleted the remove-deps1 branch September 6, 2022 17:09

Luap99 mentioned this pull request Sep 9, 2022

Update buildah and c/common to latest containers/podman#15695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove github.com/docker/distribution/registry/client package #1487

Remove github.com/docker/distribution/registry/client package #1487

Luap99 commented Mar 9, 2022

rhatdan commented Mar 9, 2022

TomSweeneyRedHat Mar 9, 2022

vrothberg Mar 10, 2022

mtrmac Mar 10, 2022

vrothberg left a comment

vrothberg Mar 10, 2022

mtrmac left a comment

mtrmac Mar 10, 2022

mtrmac Mar 10, 2022

Luap99 commented Mar 10, 2022

mtrmac commented Mar 10, 2022

vrothberg commented Mar 11, 2022

mtrmac commented Mar 11, 2022 •

edited

Loading

rhatdan commented Mar 13, 2022

vrothberg commented Mar 14, 2022

Luap99 commented Mar 14, 2022

tyler92 commented Aug 31, 2022

Luap99 commented Sep 5, 2022

mtrmac commented Sep 5, 2022

Luap99 commented Sep 6, 2022

rhatdan commented Sep 6, 2022

mtrmac left a comment

mtrmac left a comment

Remove github.com/docker/distribution/registry/client package #1487

Remove github.com/docker/distribution/registry/client package #1487

Conversation

Luap99 commented Mar 9, 2022

rhatdan commented Mar 9, 2022

TomSweeneyRedHat Mar 9, 2022

Choose a reason for hiding this comment

vrothberg Mar 10, 2022

Choose a reason for hiding this comment

mtrmac Mar 10, 2022

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Mar 10, 2022

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Mar 10, 2022

Choose a reason for hiding this comment

mtrmac Mar 10, 2022

Choose a reason for hiding this comment

Luap99 commented Mar 10, 2022

mtrmac commented Mar 10, 2022

vrothberg commented Mar 11, 2022

mtrmac commented Mar 11, 2022 • edited Loading

rhatdan commented Mar 13, 2022

vrothberg commented Mar 14, 2022

Luap99 commented Mar 14, 2022

tyler92 commented Aug 31, 2022

Luap99 commented Sep 5, 2022

mtrmac commented Sep 5, 2022

Luap99 commented Sep 6, 2022

rhatdan commented Sep 6, 2022

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Mar 11, 2022 •

edited

Loading