Skip to content
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

Evaluate and eliminate use of google.golang.org/api/compute/v0.alpha because it's 20M #76504

Open
dims opened this Issue Apr 12, 2019 · 16 comments

Comments

Projects
None yet
6 participants
@dims
Copy link
Member

dims commented Apr 12, 2019

One of the top offenders that contribute to the size of our binaries is the v0.alpha API used in the GCE provider

20M    ./k8s.io/kubernetes/vendor/google.golang.org/api/compute/v0.alpha.a

Can we please check if the alpha features have already gotten promoted to beta? and if we really need the alpha features?

Steps to find sizes of built packages.

go build -a -v ./cmd/hyperkube 2>&1 | grep vendor | grep -v vendor/k8s.io > built.txt
cat built.txt | xargs -L 1 -I {} go build -o "{}.a" {}
find . -type f -name *.a -exec du -h {} \; | sort -h | grep -v vendor/k8s.io

@dims dims added the kind/bug label Apr 12, 2019

@dims

This comment has been minimized.

Copy link
Member Author

dims commented Apr 12, 2019

@k8s-ci-robot k8s-ci-robot added sig/gcp and removed needs-sig labels Apr 12, 2019

@dims dims referenced this issue Apr 12, 2019

Open

Reduce sizes of binaries #76506

0 of 4 tasks complete

@liggitt liggitt added kind/cleanup and removed kind/bug labels Apr 12, 2019

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 12, 2019

Looks like most of the use is network related. Would be good to evaluate if the APIs we're using have graduated to beta and we can stop calling the alpha APIs

fyi @bowei @MrHohn

@MrHohn

This comment has been minimized.

Copy link
Member

MrHohn commented Apr 12, 2019

It may be possible that the existing alpha APIs have graduated to beta or GA, though wouldn't this come back once we have another alpha API integrated in future?

@dims

This comment has been minimized.

Copy link
Member Author

dims commented Apr 12, 2019

@MrHohn we should not be adding any more "new" stuff to in-tree cloud providers right? :)

cc @andrewsykim

@andrewsykim

This comment has been minimized.

Copy link
Member

andrewsykim commented Apr 12, 2019

Yes, as per https://github.com/kubernetes/kubernetes/tree/master/pkg/cloudprovider#deprecation-notice we shouldn't anticipate cloud providers in-tree to add new features/integrations. Should only be incremental changes to fix bugs.

@MrHohn

This comment has been minimized.

Copy link
Member

MrHohn commented Apr 12, 2019

Right, thanks for the info. So it feels like the real fix here is kubernetes/enhancements#668, after which we can remove all of google.golang.org/api/compute from k/k? Or is this causing immediate pain that we need to resolve sooner?

@andrewsykim

This comment has been minimized.

Copy link
Member

andrewsykim commented Apr 12, 2019

If google.golang.org/api/compute/v0.alpha is needed for existing features in the GCE provider then I don't think it would be reasonable to remove it until the out-of-tree provider is supported like you mentioned. I think the ask here is if the google.golang.org/api/compute/v0.alpha dependency can be removed if we are importing v1 versions of the SDK. I haven't look through the GCE compute SDK in a while so not sure if the v0.alpha import is required or can be replaced with beta or v1 versions.

@andrewsykim

This comment has been minimized.

Copy link
Member

andrewsykim commented Apr 12, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member

andrewsykim commented Apr 12, 2019

Seems like we create separate clients for alpha/beta versions of the client https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/gce/gce.go#L104-L105 can we go through usage and see if it's feasible to remove?

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 12, 2019

if it's a lot of work to tease out and remove, I'll settle for lumping it in with the rest of the "move out of tree" work, but it seemed worth checking, given it's the number 2 package in size

@andrewsykim

This comment has been minimized.

Copy link
Member

andrewsykim commented Apr 12, 2019

if it's a lot of work to tease out and remove, I'll settle for lumping it in with the rest of the "move out of tree" work, but it seemed worth checking, given it's the number 2 package in size

After doing a quick scan of the provider, it seems like it would be quite a bit of work to do this and would actually break compatibility if I'm not mistaken. Please correct me if I'm wrong @MrHohn

@MrHohn

This comment has been minimized.

Copy link
Member

MrHohn commented Apr 12, 2019

@andrewsykim Thanks for checking that. It may not break compatibility, though will require touching many places for switching this and need to make adjustment to the auto-generating wrapper we built sometimes before.. So need more testing to keep this safe.

Also did another check on the vendor --- it seems like github.com/GoogleCloudPlatform/k8s-cloud-provider itself also imports this Alpha API. So making change to k/k might not be sufficient to eliminate this?

$ git grep -in "google.golang.org/api/compute/v0.alpha" vendor/
vendor/BUILD:448:        "//vendor/google.golang.org/api/compute/v0.alpha:all-srcs",
vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/BUILD:23:        "//vendor/google.golang.org/api/compute/v0.alpha:go_default_library",
vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/gen.go:34:   alpha "google.golang.org/api/compute/v0.alpha"
vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta/BUILD:16:        "//vendor/google.golang.org/api/compute/v0.alpha:go_default_library",
vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta/meta.go:22:     alpha "google.golang.org/api/compute/v0.alpha"
vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta/method.go:66:   case "google.golang.org/api/compute/v0.alpha":
vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock/BUILD:13:        "//vendor/google.golang.org/api/compute/v0.alpha:go_default_library",
vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock/mock.go:36:     alpha "google.golang.org/api/compute/v0.alpha"
vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/op.go:25:    alpha "google.golang.org/api/compute/v0.alpha"
vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/service.go:25:       alpha "google.golang.org/api/compute/v0.alpha"
...
@bowei

This comment has been minimized.

Copy link
Member

bowei commented Apr 13, 2019

Even though this is a big chunk of code, if we are support features calling the alpha API, then it has to stay in some form or another.
Until we build cloud specific libraries, as Go has no dynamic linking, I don't think we can do much about the code size.
What is the target binary size we are trying to go to and why?

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 13, 2019

if we are support features calling the alpha API, then it has to stay in some form or another

No disagreement, just thought it was worth seeing if they had graduated to the beta lib and we could drop use of the alpha package, given the size impact

@bowei

This comment has been minimized.

Copy link
Member

bowei commented Apr 13, 2019

Support for alpha features is an ongoing thing (they are always being added). Even if we remove them today, they will be required almost immediately tomorrow. It will cause unnecessary code churn at this point.

@dims

This comment has been minimized.

Copy link
Member Author

dims commented Apr 14, 2019

@bowei aren't we supposed to move to external cloud providers and stop adding stuff in the in-tree one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.