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

storage: problematic dependency resolution with stats #11283

Closed
StephenWithPH opened this issue Dec 13, 2024 · 28 comments
Closed

storage: problematic dependency resolution with stats #11283

StephenWithPH opened this issue Dec 13, 2024 · 28 comments
Assignees
Labels
api: storage Issues related to the Cloud Storage API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@StephenWithPH
Copy link

Having upgraded from:

cloud.google.com/go/storage v1.46.0 -> cloud.google.com/go/storage v1.48.0, I get the following error:

<redacted>/go/pkg/mod/cloud.google.com/go/storage@v1.48.0/storage.go:264:25: undefined: stats.NewMetrics

This corresponds to:

Metrics: stats.NewMetrics("grpc.client.attempt.duration"),

... and I believe these two imports are in conflict:

"google.golang.org/grpc/experimental/stats"
"google.golang.org/grpc/stats/opentelemetry"

google.golang.org/grpc v1.67.2
google.golang.org/grpc/stats/opentelemetry v0.0.0-20240907200651-3ffb98b2c93a

I suspect that somehow, this NewMetrics is getting shadowed by https://pkg.go.dev/google.golang.org/grpc/stats/opentelemetry@v0.0.0-20241028142157-ada6787961b3 .

For a brief period, I was getting ambiguous import errors pointing at these two packages with go mod tidy. I'm no longer getting those errors, but I still fail to compile.

@StephenWithPH StephenWithPH added the triage me I really want to be triaged. label Dec 13, 2024
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Dec 13, 2024
@davidh16
Copy link

same issue here

@SadriShehu
Copy link

Facing the same issue as well.

@fasmat
Copy link

fasmat commented Dec 13, 2024

The problem is that google.golang.org/grpc/stats/opentelemetry is not its own module any more. This was changed between v1.68.1 and v1.69.0 of google.golang.org/grpc: grpc/grpc-go#7759

Upgrading to the latest google.golang.org/grpc package will now cause the same package to be imported from two different sources. Upgrading storage to use the latest google.golang.org/grpc and dropping the google.golang.org/grpc/stats/opentelemetry dependency should fix the issue.

@nikolaydubina
Copy link

same issue here. direct removal google.golang.org/grpc/stats/opentelemetry v0.0.0-20241028142157-ada6787961b3 // indirect from go.mod, makes build to fail

something has to be changed in cloud.google.com/go module

@fasmat
Copy link

fasmat commented Dec 13, 2024

I created a PR to update the GRPC dependency in all modules: #11284

@nikolaydubina
Copy link

I mean, that change is huge. not sure can be accepted

need someone from google to fix this

@fasmat
Copy link

fasmat commented Dec 13, 2024

Depends, the amount of code changed is minimal (I believe just a singe line of code). The rest is just replacing the current version of the dependency with the newest one and bumping Go everywhere where it's still 1.21 to 1.22 🙂 The new grpc dependency requires at least 1.22.

@nikolaydubina
Copy link

ok, updating everything and keeping cloud.google.com/go/storage v1.46.0 worked for me. will check back on this once google replies.

@dfawley
Copy link
Contributor

dfawley commented Dec 13, 2024

I believe the only (?) issue here is that grpc-go moved some of the symbols that cloud storage was relying upon from our experimental directory into a stable location, without leaving behind type aliases. If so, that's our mistake and we will fix it ASAP. PR grpc/grpc-go#7929 should address it, and we will do a patch release with it ASAP.

@tritone tritone added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. and removed triage me I really want to be triaged. labels Dec 13, 2024
@fasmat
Copy link

fasmat commented Dec 13, 2024

@dfawley I'm not sure this solves the issue. In v1.69.0 google.golang.org/grpc/stats/opentelemetry is a package of the google.golang.org/grpc module. In previous versions of google.golang.org/grpc it is its own module.

So you still have the problem where if you use both cloud.google.com/go/storage and google.golang.org/grpc as direct dependencies you cannot upgrade the later to v1.69.0 without breaking the dependency tree because of ambiguous imports regarding the opentelemetry package.

Example go.mod:

module testmodule

go 1.22

require (
	cloud.google.com/go/storage v1.48.0
	google.golang.org/grpc v1.69.0
)

Then call go mod tidy:

$ go mod tidy
go: testmodule/testpackage imports
        cloud.google.com/go/storage imports
        google.golang.org/grpc/stats/opentelemetry: ambiguous import: found package google.golang.org/grpc/stats/opentelemetry in multiple modules:
        google.golang.org/grpc v1.69.0 (/go/pkg/mod/google.golang.org/grpc@v1.69.0/stats/opentelemetry)
        google.golang.org/grpc/stats/opentelemetry v0.0.0-20240907200651-3ffb98b2c93a (/go/pkg/mod/google.golang.org/grpc/stats/opentelemetry@v0.0.0-20240907200651-3ffb98b2c93a)

The only solution seems to be to stick with an older version of google.golang.org/grpc until cloud.google.com/go/storage is updated.

@dfawley
Copy link
Contributor

dfawley commented Dec 13, 2024

@fasmat Thanks for confirming that the change above will not fully fix things. This document seems to indicate that such an operation is possible and valid, but it's missing details on how to perform the removal -- it just refers to the "adding a module" instructions and how to actually do it correctly is left as an exercise for the reader. In particular, splitting a module requires adding a dependency from the new module to a new version of the existing module, but when joining modules, I can't add a dependency to the removed module, since no module exists there anymore.

So for now I'm not sure how to properly remove a module and move its packages to its parent module. (If it is possible as the wiki suggests.) I'll keep working on this.

@tritone
Copy link
Contributor

tritone commented Dec 13, 2024

Hey all, apologies for the issue here, we are working to fix this ASAP.

There is a workaround that should unblock compilation and go get calls, which is to exclude the latest release of grpc-go from your go.mod.

exclude google.golang.org/grpc v1.69.0

Let me know if this resolves this issue.

@tritone tritone self-assigned this Dec 13, 2024
@gannett-ggreer
Copy link

@tritone

exclude google.golang.org/grpc v1.69.0

Let me know if this resolves this issue.

It avoids the dependency resolution failing at least, for the repositories I've tried.

The non-manual way is go mod edit --exclude=google.golang.org/grpc@v1.69.0.

@StevenACoffman
Copy link

StevenACoffman commented Dec 16, 2024

@tritone @zasweq Is it possible to just add the go.mod back for stats/opentelemetry that was removed in #7759 and cut a point release until cloud.google.com/go/storage is ready to upgrade to minimum version of Go 1.22 (and drop Go 1.21) in January?

Also, anything that imported the missing module will be unable to upgrade to gRPC-go 1.69:
https://pkg.go.dev/google.golang.org/grpc/stats/opentelemetry?tab=importedby

@dfawley
Copy link
Contributor

dfawley commented Dec 16, 2024

it possible to just add the go.mod back for stats/opentelemetry

My plan is to do the opposite. Pull the module into grpc-go v1.67.x and v1.68.x, so it's like it never existed. Then googleapis can upgrade its version to those and everything should work moving forward, too.

I was hoping we could actually find a way to do this that worked automagically, just like how you can split modules without any coordination. But no such approach seems possible, and https://go.dev/wiki/Modules#is-it-possible-to-remove-a-module-from-a-multi-module-repository should really be updated to say "no, not really" instead of "yes, just do the opposite of the above".

@StevenACoffman
Copy link

StevenACoffman commented Dec 17, 2024

@dfawley You are probably already aware, but I wanted to get confirmation that I understand the plan for my colleagues.

However:

So after you have merged #7935 and released grpc-go v1.67.3(?) , cloud.google.com/go/storage will need to update to that (not v1.68.x) and then cut a new release (maybe v1.50.0 since v1.49.0 release already has a PR for it and appears to be in progress). Then people can update to grpc-go v1.67.3(?) or cloud.google.com/go/storage v1.50.0(?) and not have an ambiguous update. In January, cloud.google.com/go/storage can update to Go 1.22 and updating to grpc-go v1.69.x+ becomes unblocked.

In the meantime, go mod edit --exclude=google.golang.org/grpc@v1.69.0 is the only workaround.

@codyoss
Copy link
Member

codyoss commented Dec 17, 2024

@StevenACoffman I believe you summarized this well, yes.

nader-ziada added a commit to cloudfoundry/bosh-cli that referenced this issue Dec 17, 2024
This reverts commit b3ce3b8.
and commit 9a78ef5.

There is an issue with the cloud.google.com/go/storage dependency
details here: googleapis/google-cloud-go#11283 (comment)
@tritone
Copy link
Contributor

tritone commented Dec 21, 2024

Okay, the latest release (storage v1.49.0) seems to fix the issue in my repro.

  • go get -u google.golang.org/grpc (can upgrade to latest release v1.69.2 or v1.67.3 for compatibility with go 1.21)
  • go get -u cloud.google.com/go/storage

You may also have to remove google.golang.org/grpc/stats/opentelemetry as an indirect dependency.

Let me know if this resolves the issue for you all or if there are still any problems.

Apologies for the inconvenience here.

tritone added a commit to tritone/storage-shared-benchmarking that referenced this issue Dec 21, 2024
This update fixes compilation issues caused by
googleapis/google-cloud-go#11283
BrennaEpp pushed a commit to googleapis/storage-shared-benchmarking that referenced this issue Dec 21, 2024
This update fixes compilation issues caused by
googleapis/google-cloud-go#11283
@StevenACoffman
Copy link

BTW, I had to exclude google.golang.org/grpc/stats/opentelemetry v0.0.0-20240907200651-3ffb98b2c93a:

go mod edit --exclude=google.golang.org/grpc/stats/opentelemetry@v0.0.0-20240907200651-3ffb98b2c93a
go get -u google.golang.org/grpc
go get -u cloud.google.com/go/storage

@tritone
Copy link
Contributor

tritone commented Dec 22, 2024

BTW, I had to exclude google.golang.org/grpc/stats/opentelemetry v0.0.0-20240907200651-3ffb98b2c93a:

go mod edit --exclude=google.golang.org/grpc/stats/opentelemetry@v0.0.0-20240907200651-3ffb98b2c93a
go get -u google.golang.org/grpc
go get -u cloud.google.com/go/storage

Yes, or manually deleting the indirect dep on google.golang.org/grpc/stats/opentelemetry from the go.mod should also do the trick.

@dfawley
Copy link
Contributor

dfawley commented Dec 23, 2024

Yes, or manually deleting the indirect dep on google.golang.org/grpc/stats/opentelemetry from the go.mod should also do the trick.

I believe go mod tidy should take care of this for you, no?

@tritone
Copy link
Contributor

tritone commented Dec 23, 2024

Yes, or manually deleting the indirect dep on google.golang.org/grpc/stats/opentelemetry from the go.mod should also do the trick.

I believe go mod tidy should take care of this for you, no?

Unfortunately in my repro, go mod tidy also didn't work until the indirect dep was manually removed.

@tritone
Copy link
Contributor

tritone commented Dec 23, 2024

Okay, the latest release (storage v1.49.0) seems to fix the issue in my repro.

  • go get -u google.golang.org/grpc (can upgrade to latest release v1.69.2 or v1.67.3 for compatibility with go 1.21)
  • go get -u cloud.google.com/go/storage

You may also have to remove google.golang.org/grpc/stats/opentelemetry as an indirect dependency.

Let me know if this resolves the issue for you all or if there are still any problems.

Apologies for the inconvenience here.

I haven't heard from anyone still experiencing this issue, so I'm going to close this out. Thanks for your patience all.

@tritone tritone closed this as completed Dec 23, 2024
@mathieu-lemay
Copy link

You have to be kidding me. While this issue indeed seems to be fixed, github.com/envoyproxy/go-control-plane decided to join the party!

$ go get -u cloud.google.com/go/storage
go: cloud.google.com/go/storage imports
        google.golang.org/grpc/xds/googledirectpath imports
        google.golang.org/grpc/internal/xds/bootstrap imports
        github.com/envoyproxy/go-control-plane/envoy/config/core/v3: ambiguous import: found package github.com/envoyproxy/go-control-plane/envoy/config/core/v3 in multiple modules:
        github.com/envoyproxy/go-control-plane v0.13.1 (/Users/6004148/.local/share/go/pkg/mod/github.com/envoyproxy/go-control-plane@v0.13.1/envoy/config/core/v3)
        github.com/envoyproxy/go-control-plane/envoy v1.32.2 (/Users/6004148/.local/share/go/pkg/mod/github.com/envoyproxy/go-control-plane/envoy@v1.32.2/config/core/v3)

@dfawley
Copy link
Contributor

dfawley commented Dec 23, 2024

Please file a bug in go-control-plane. That issue is unrelated.

envoyproxy/go-control-plane#714 probably didn't follow the right steps for splitting modules:

https://go.dev/wiki/Modules#is-it-possible-to-add-a-module-to-a-multi-module-repository

@tritone
Copy link
Contributor

tritone commented Dec 23, 2024

Ugh. I'll open a new issue here as well to track resolution, since it blocks updates for storage as well as other clients which have this as an indirect dependency (spanner, bigtable).

@tritone
Copy link
Contributor

tritone commented Dec 24, 2024

Follow along here: #11344

jcscottiii added a commit to GoogleChrome/webstatus.dev that referenced this issue Dec 30, 2024
This updates all of the go modules.

I had to manually run these workarounds for the first time before make go-update and make go-tidy would work:
- googleapis/google-cloud-go#11283 (comment)
- googleapis/google-cloud-go#11344
github-merge-queue bot pushed a commit to GoogleChrome/webstatus.dev that referenced this issue Dec 30, 2024
This updates all of the go modules.

I had to manually run these workarounds for the first time before make go-update and make go-tidy would work:
- googleapis/google-cloud-go#11283 (comment)
- googleapis/google-cloud-go#11344
github-merge-queue bot pushed a commit to GoogleChrome/webstatus.dev that referenced this issue Dec 30, 2024
This updates all of the go modules.

I had to manually run these workarounds for the first time before make go-update and make go-tidy would work:
- googleapis/google-cloud-go#11283 (comment)
- googleapis/google-cloud-go#11344
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.