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

ci(deps): upgrade OpenTelemetry libraries to v1.19.0 / v0.45.0 #4318

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

milas
Copy link
Contributor

@milas milas commented Oct 10, 2023

Core OTel libs are now at v1.19.0, while contrib is at v0.45.0. The protocol has also reached 1.0.0.

Note that Jaeger exporter is frozen at v1.17.0, which is the last version it was supported by OpenTelemetry. BuildKit should likely begin the process of deprecating it as well, since OTLP is now the recommended solution (by Jaeger themselves), but that is not handled here.

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the dependencies that we share with containerd consistent with the version we are importing. Otherwise, it can become quite a mess for future changes or anyone who imports us. And we don't know if the proto/grpc changes break anything upstream.

@milas milas marked this pull request as draft October 11, 2023 01:42
@milas
Copy link
Contributor Author

milas commented Oct 11, 2023

Yeah, CI seems to indicate that it's going to be a problem. I don't see a PR over on the containerd side, so I'll open a similar one there and link it to this tomorrow

@sipsma
Copy link
Collaborator

sipsma commented Oct 17, 2023

Just FYI that there's now a CVE for these older versions of various otel deps: https://github.com/moby/buildkit/security/dependabot/22

I'm here because our dagger builds started failing security scans, but then after I upgraded our go.mod with the newer versions we started getting cannot merge resource due to conflicting Schema URL errors due to the mismatch between our dep versions and buildkit's older versions 😣

I'll workaround the error separately, not relevant here really I don't think; just chiming in because there may be some more urgency for upgrading these deps now (even if it takes a containerd upgrade also)

@milas @tonistiigi

@milas
Copy link
Contributor Author

milas commented Oct 17, 2023

containerd dependency upgrade:

@thaJeztah
Copy link
Member

This needs a rebase

@milas
Copy link
Contributor Author

milas commented Oct 26, 2023

milas and others added 2 commits November 28, 2023 13:12
Core OTel libs are now at v1.19.0, while contrib is at v0.45.0.
The protocol has also reached 1.0.0.

Note that Jaeger exporter is frozen at v1.17.0, which is the
last version it was supported by OpenTelemetry. BuildKit should
likely begin the process of deprecating it as well, since OTLP
is now the recommended solution (by Jaeger themselves), but that
is not handled here.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc
Copy link
Member

jedevc commented Nov 28, 2023

I've updated this PR to bump containerd to v1.7.9.

I've also fixed CI, I needed to bump semconv - https://github.com/moby/buildkit/pull/4318/files#diff-a75d83cef52595715014a0115a5f35c38d99e06551f033d3479e40aef05ed547R15. Super fun that the version number is actually in the code, and not in go.mod 🙃

@thaJeztah @milas @tonistiigi how does this seem to you? I do feel the lack of tests for tracing, so not 100% confident by myself, this is definitely something to sanity-check at some point in a followup. I think it's ready for review though.

@jedevc jedevc marked this pull request as ready for review November 28, 2023 13:18
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Member

I've also fixed CI, we also need to bump semconv - https://github.com/moby/buildkit/pull/4318/files#diff-a75d83cef52595715014a0115a5f35c38d99e06551f033d3479e40aef05ed547R15. Super fun that the version number is actually in the code, and not in go.mod 🙃

Yes, that's currently the issue we're running into in Moby; I have a PR pending there that also updates containerd + OTEL, it compiles, but it breaks integrating with BuildKit; moby/moby#46830

@gerhard
Copy link

gerhard commented Nov 28, 2023

Thanks @jedevc! Looking forward to this getting merged 💪

FWIW:

@jedevc jedevc merged commit c9ee849 into moby:master Nov 29, 2023
61 checks passed
@thaJeztah
Copy link
Member

@milas @jedevc do we need these changes in BuildKit v0.12 to make the moby side work again?

@jedevc
Copy link
Member

jedevc commented Nov 29, 2023

@thaJeztah the OTEL versions in releases of buildkit and moby need to match AFAIK - so v0.12 would only need to have this change if moby updates OTEL and keeps vendoring v0.12.

That said, for such a massive dependency update (why why is it such a pain) I don't particularly love the idea of putting that in a patch release. I'd be more tempted to say that v0.12 of buildkit shouldn't be used with the updated OTEL, and should instead need v0.13 (but that's more of a release process/scheduling question, that I'm not fully in the loop with).

What version of buildkit is v25.0 of moby expected to use?

@thaJeztah
Copy link
Member

Moby v25 is using BuildKit v0.12, but containerd 1.7 was updated to the new OTEL version, which means we are either stuck on an older version of containerd, or must update as well.

(and yes, OTEL having breaking changes with just about every release is a huge PITA)

@gerhard
Copy link

gerhard commented Nov 30, 2023

Thank you @jedevc 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants