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

chore(k8sclusterreceiver): 🔥 remove deprecated kubernetes API resources #26516

Merged
merged 17 commits into from
Sep 11, 2023
Merged

chore(k8sclusterreceiver): 🔥 remove deprecated kubernetes API resources #26516

merged 17 commits into from
Sep 11, 2023

Conversation

prashant-shahi
Copy link
Contributor

@prashant-shahi prashant-shahi commented Sep 7, 2023 •
edited
Loading

Description:
Remove support for deprecated Kubernetes API resources:

  • batch/v1beta1
  • autoscaling/v2beta2

This also resolves the issue with double reporting metrics for hpa and cronjob in certain k8s versions.

Link to tracking Issue(s):
#23612 #26551

Testing:
From source, built the custom image coolboi567/otelcontribcol:0.83.1 and tested in kubernetes cluster v1.25 as well as v1.27.

In K8s v1.25, we no longer see the warnings in the logs about usage of deprecated APIs.

Documentation:
N/A

@prashant-shahi prashant-shahi requested a review from a team September 7, 2023 18:11
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 7, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

dmitryax
dmitryax reviewed Sep 7, 2023
View reviewed changes
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

@prashant-shahi thank you for the PR. Can you please add a changelog entry?

receiver/k8sclusterreceiver/README.md Outdated Show resolved Hide resolved
@jinja2
Copy link
Contributor

jinja2 commented Sep 8, 2023

Can we drop support for the hpa and cronjob beta apis? I am not sure if we have a compatibility matrix with k8s versions. Hpa was promoted to stable in v1.23 and cronjob in v1.21. Upstream k8s currently maintains 1.25 to 1.28.

Below are EOL dates for the major managed clusters, all of which have no support for 1.22 (the last version which would need the hpa beta):

https://endoflife.date/google-kubernetes-engine
https://endoflife.date/azure-kubernetes-service
https://endoflife.date/amazon-eks

I think if we can commit to some k8s version compatibility window, it would make maintenance of k8s receivers simpler. A good place to start might be to keep up with the major managed cluster providers.

@dmitryax
Copy link
Member

dmitryax commented Sep 8, 2023 •
edited
Loading

@jinja2, that makes sense. It depends on the definition of "support" from the collector side. Should we support collecting metrics from the deprecated resource versions? I believe we should. In that case, dropping them now makes the supported window pretty narrow: 1.28, 1.27, 1.26 due to HPA beta removal in 1.26. I'm in favor of introducing this config option to provide a smoother transition. This will likely be needed going forward as well.

@prashant-shahi prashant-shahi changed the title feat(k8sclusterreceiver): ✨ introduce ignore_deprecated_resource config feat(k8sclusterreceiver): ✨ introduce ignore_deprecated_resources config Sep 8, 2023
@prashant-shahi
Copy link
Contributor Author

prashant-shahi commented Sep 8, 2023 •
edited
Loading

Can you please add a changelog entry?

Changelog has been updated.

dropping them now makes the supported window pretty narrow: 1.28, 1.27, 1.26 due to HPA beta removal in 1.26. I'm in favor of introducing this config option to provide a smoother transition.

Absolutely. Introducing this option can also help the folks who do not want to upgrade to newest K8s version but want to get rid of the deprecation warning.

Also, the usage of the deprecated K8s API causes the failure of managed clusters to auto-upgrade to newer k8s version (as reported in GKE and OpenShift). The ignore_deprecated_resources option helps in that case.

@jinja2
Copy link
Contributor

jinja2 commented Sep 8, 2023

I have started a new issue which provides more context on api versioning in k8s. The tldr is that we can use the v2 api for hpas which we created in the v2beta2 version format, starting from k8s v1.23. I don't think we should add this flag. Instead we should decide on our k8s version compatibility. Removing v2beta2 here still makes us compatible with latest - 5 versions of k8s which to me seems like a very reasonable support duration.

@prashant-shahi
Copy link
Contributor Author

prashant-shahi commented Sep 9, 2023 •
edited
Loading

@jinja2 Thanks for the detailed issue on the behaviour. It was really informative.

Updating the PR to remove the deprecated APIs instead of adding the flag.

@prashant-shahi prashant-shahi changed the title feat(k8sclusterreceiver): ✨ introduce ignore_deprecated_resources config chore(k8sclusterreceiver): 🔥 remove deprecated kubernetes API resources Sep 9, 2023
dmitryax
dmitryax reviewed Sep 11, 2023
View reviewed changes
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM other than the changelog issue

CHANGELOG.md Outdated Show resolved Hide resolved
dmitryax
dmitryax approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Thank you, @prashant-shahi!

TylerHelmuth
TylerHelmuth reviewed Sep 11, 2023
View reviewed changes
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

Based on this issue, we should update the README that this breaks <= 1.22 users right?

@prashant-shahi
Copy link
Contributor Author

prashant-shahi commented Sep 11, 2023 •
edited
Loading

Based on this issue, we should update the README that this breaks <= 1.22 users right?

@TylerHelmuth Include that in changelog? Yeah, sure.

However, I am not sure if we need to include it in README.
It's been around a year since the end of life for 1.22 support.

Ref: https://endoflife.date/kubernetes

@TylerHelmuth
Copy link
Member

@prashant-shahi if a user enabled this component in a k8s 1.22 cluster with the default values would the collector crash? If so I think we should call that out in the README since there are definitely still people using <= 1.22.

@jinja2
Copy link
Contributor

jinja2 commented Sep 11, 2023

@TylerHelmuth it won't crash, the receiver will start up ok but HPA metrics will stop. I think a user-facing changelog entry to call out dropped support for the api is good enough.

@dmitryax dmitryax merged commit 06dd7e2 into open-telemetry:main Sep 11, 2023
83 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 11, 2023
@prashant-shahi prashant-shahi deleted the chore/flag-autoscaling-v2beta2 branch September 11, 2023 19:22
@prashant-shahi
Copy link
Contributor Author

@dmitryax @jinja2 @TylerHelmuth Thanks for the thorough review and suggestions in the PR.

Can the related issues be closed now or after the next release?

@TylerHelmuth
Copy link
Member

@prashant-shahi you can close it now

@dmitryax
Copy link
Member

Thank you @prashant-shahi for resolving it!

@prashant-shahi
Copy link
Contributor Author

you can close it now

@TylerHelmuth I am unable to close them. Since the related issues were created by other folks.

@prashant-shahi prashant-shahi mentioned this pull request Oct 19, 2023
Closed
@jinja2 jinja2 mentioned this pull request Oct 20, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitryax dmitryax dmitryax approved these changes

@TylerHelmuth TylerHelmuth TylerHelmuth approved these changes

Assignees

@MovieStoreGuy MovieStoreGuy

Labels
receiver/k8scluster
Projects
None yet
Milestone
v0.85.0
Development

Successfully merging this pull request may close these issues.
5 participants
@prashant-shahi @jinja2 @dmitryax @TylerHelmuth @MovieStoreGuy