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

add options to configure k8s client qps/burst #28151

Merged
merged 8 commits into from
Nov 2, 2021
Merged

add options to configure k8s client qps/burst #28151

merged 8 commits into from
Nov 2, 2021

Conversation

newly12
Copy link
Contributor

@newly12 newly12 commented Sep 28, 2021 •
edited
Loading

Enhancement

What does this PR do?

add options to configure k8s client qps/burst

Why is it important?

Accelerate autodiscover pod event processing

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 28, 2021
@mergify
Copy link
Contributor

mergify bot commented Sep 28, 2021

This pull request does not have a backport label. Could you fix it @newly12? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Sep 28, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 28, 2021 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-11-02T10:49:05.107+0000

  • Duration: 21 min 59 sec

  • Commit: b7e2110

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

@jsoriano jsoriano added the Team:Integrations Label for the Integrations team label Oct 25, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 25, 2021
@jsoriano
Copy link
Member

/test

ChrsMark
ChrsMark reviewed Oct 26, 2021
View reviewed changes
metricbeat/module/kubernetes/util/kubernetes.go Outdated
@@ -83,7 +83,7 @@ func GetWatcher(base mb.BaseMetricSet, resource kubernetes.Resource, nodeScope b
return nil, nil
}

client, err := kubernetes.GetKubernetesClient(config.KubeConfig)
client, err := kubernetes.GetKubernetesClient(config.KubeConfig, nil)
Copy link
Member

Choose a reason for hiding this comment

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

Why we don't add the options here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good cache. Let me check how this should be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

ChrsMark
ChrsMark reviewed Oct 26, 2021
View reviewed changes
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Nice, thank you for working on this! I would consider it mergeable if we have changelogs for beats and agent as well as the respective documentation parts to expose the settings to the users :).

@newly12
Copy link
Contributor Author

newly12 commented Oct 29, 2021

@ChrsMark Thanks for checking. Docs are updated. I've moved qps/burst options to kube_client_options section in the config as well. PTAL.

ChrsMark
ChrsMark approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for adding this, really valuable!

@ChrsMark
Copy link
Member

/test

@ChrsMark
Copy link
Member

Btw, @newly12 would #28521 be needed after this one is merged?

@newly12
Copy link
Contributor Author

newly12 commented Nov 1, 2021

@ChrsMark yes I think we still like to have #28521 to allow to disable it as 1) we don't need deployment name to be added 2) API calls overhead.. feel free to let me know your thought on this

@ChrsMark
Copy link
Member

ChrsMark commented Nov 1, 2021

Cool @newly12 let's discuss about #28521 after this one is merged :) in general it sounds reasonable.

Regarding this PR, could you check the lint errors? Maybe you need some make update after the docs' additions :)

@ChrsMark ChrsMark self-assigned this Nov 1, 2021
@MichaelKatsoulis
Copy link
Contributor

/test

@ChrsMark
Copy link
Member

ChrsMark commented Nov 2, 2021

CI failure is unrelated, I will merge this one. Thank you @newly12 again!

@ChrsMark ChrsMark merged commit 9fc2f8e into elastic:master Nov 2, 2021
@newly12 newly12 deleted the kube_client_qps branch November 3, 2021 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrsMark ChrsMark ChrsMark approved these changes

@MichaelKatsoulis MichaelKatsoulis Awaiting requested review from MichaelKatsoulis

Assignees

@ChrsMark ChrsMark

Labels
backport-skip Skip notification from the automated backport with mergify Team:Integrations Label for the Integrations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Autodiscover could reach to k8s client's default rate limit
5 participants
@newly12 @elasticmachine @jsoriano @ChrsMark @MichaelKatsoulis