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.

Sign up for GitHub

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

Jump to bottom

🐛 Refactor tests to drop hard otel dependency #2460

Merged
merged 1 commit into from
Aug 31, 2023
Merged

🐛 Refactor tests to drop hard otel dependency #2460

merged 1 commit into from
Aug 31, 2023

Conversation

howardjohn
Copy link
Contributor

Since v0.16.0 (#2407), there is an import for all users of various opentelemetry libraries. This is caused by manager test -> pkg/metrics/filters -> k8s.io/apiserver -> otel-go.

The issue is API server users a otel-go library from >1 year ago. This makes it impossible to import a modern otel-go library and a modern controller-runtime library together.

Go is unable to prune the dependency even though its only used in tests (TBH, this surprised me!). By moving the tests that use filters under the filters package, though, this drops the required dependency on otel-go. Users that import the filters package will use it, of course, but everyone does not need to.

So basically this refactors tests, but has user facing changes -- fewer dependencies are required to import controller-runtime core, allowing using the newer controller-runtime with newer otel-go.

In parallel I will attempt to get k/k to update their otel-go version, but this will take months to ship at the earliest. IMO, this is worth fixing in the meantime as a v0.16.x patch.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 29, 2023
@hugoShaka hugoShaka mentioned this pull request Aug 29, 2023
Closed
sbueringer
sbueringer reviewed Aug 30, 2023
View reviewed changes
pkg/metrics/filters/filters_test.go Outdated Show resolved Hide resolved
pkg/metrics/filters/filters_test.go Show resolved Hide resolved
pkg/metrics/filters/filter_suite_test.go Show resolved Hide resolved
@sbueringer
Copy link
Member

sbueringer commented Aug 30, 2023 •
edited
Loading

Go is unable to prune the dependency even though its only used in tests (TBH, this surprised me!)

Very interesting. I also didn't expect that and tried to implement the feature in a way that exactly something like this does not happen.

In parallel I will attempt to get k/k to update their otel-go version, but this will take months to ship at the earliest. IMO, this is worth fixing in the meantime as a v0.16.x patch.

That would be great, would be nice if you can link a k/k issue so we can subscribe.

IMO, this is worth fixing in the meantime as a v0.16.x patch.

Absolutely agree. Thx for reporting and fixing this so quickly. Let's get this PR merged, cherry-picked and released asap

@howardjohn howardjohn mentioned this pull request Aug 30, 2023
Merged
@howardjohn
Copy link
Contributor Author

Somehow https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.17.0 magically fixes this. I have no clue how, TBH. This is probably still desired anyways but not as important

@sbueringer
Copy link
Member

sbueringer commented Aug 31, 2023 •
edited
Loading

Somehow https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.17.0 magically fixes this. I have no clue how, TBH. This is probably still desired anyways but not as important

Thx for the info. I'm fine with going ahead with this PR regardless, should still be helpful to avoid problems with other versions. (and potential problems with future versions)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2023
@howardjohn
Refactor tests to drop hard otel dependency
d17751f 
Since v0.16.0 (kubernetes-sigs#2407), there is an import for all users of various
opentelemetry libraries. This is caused by manager test ->
pkg/metrics/filters -> k8s.io/apiserver -> otel-go.

The issue is API server users a otel-go library from >1 year ago. This
makes it impossible to import a modern otel-go library and a modern
controller-runtime library together.

Go is unable to prune the dependency *even though its only used in
tests* (TBH, this surprised me!). By moving the tests that use
`filters` under the `filters` package, though, this drops the required
dependency on otel-go. Users that import the `filters` package will use
it, of course, but everyone does not need to.

So basically this refactors tests, but has user facing changes -- fewer
dependencies are required to import controller-runtime core, allowing
using the newer controller-runtime with newer otel-go.

In parallel I will attempt to get k/k to update their otel-go version,
but this will take months to ship at the earliest. IMO, this is worth
fixing in the meantime as a v0.16.x patch.
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2023
@howardjohn
Copy link
Contributor Author

/retest

@sbueringer
Copy link
Member

Thank you!
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2023
@k8s-ci-robot k8s-ci-robot merged commit 4332e3a into kubernetes-sigs:main Aug 31, 2023
@sbueringer
Copy link
Member

/cherry-pick release-0.16

@k8s-infra-cherrypick-robot
Copy link

@sbueringer: #2460 failed to apply on top of branch "release-0.16":

Applying: Refactor tests to drop hard otel dependency
Using index info to reconstruct a base tree...
M	examples/scratch-env/go.sum
M	pkg/manager/manager_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/manager/manager_test.go
CONFLICT (content): Merge conflict in pkg/manager/manager_test.go
Auto-merging examples/scratch-env/go.sum
CONFLICT (content): Merge conflict in examples/scratch-env/go.sum
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Refactor tests to drop hard otel dependency
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sbueringer
Copy link
Member

@howardjohn Would be nice if you can open a cherry-pick for release-0.16. I'll merge & release after.

@howardjohn
Copy link
Contributor Author

@sbueringer opened #2465, thanks for the review+help

@sbueringer
Copy link
Member

You're welcome!

@sbueringer sbueringer mentioned this pull request Aug 31, 2023
Merged
@kcp-ci-bot kcp-ci-bot mentioned this pull request Feb 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@joelanford joelanford Awaiting requested review from joelanford

@vincepri vincepri Awaiting requested review from vincepri

Assignees

@sbueringer sbueringer

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@howardjohn @sbueringer @k8s-ci-robot @k8s-infra-cherrypick-robot