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

Reduce the pressure on api server caused by daemonset related pod queries #5761

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

twilight327426371
Copy link
Contributor

Query pods related to daemonset. If select is not set, all pods in the current namespace of daemonset will be queried. If there are more pods in the current namespace, the pressure on the api server will increase greatly. Set the label when querying to reduce the number of query pods.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 13, 2021
@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

Merging #5761 (75714a0) into master (7021640) will decrease coverage by 0.92%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #5761      +/-   ##
==========================================
- Coverage   45.37%   44.44%   -0.93%     
==========================================
  Files         214      214              
  Lines       10244     9124    -1120     
  Branches      110      110              
==========================================
- Hits         4648     4055     -593     
+ Misses       5321     4794     -527     
  Partials      275      275              

@floreks
Copy link
Member

floreks commented Jan 14, 2021

IIRC we have intentionally not used this type of filtering as the only certain way to check if pod is controlled by deamon set is to do it based on the controller ref as it is done at the end of this method common.FilterPodsByControllerRef(daemonSet, podList.Items).

@maciaszczykm do you remember that case?

@maciaszczykm
Copy link
Member

More or less I think this was the case. Perhaps it has changed, but we would need to have some kind of docs to confirm that.

@twilight327426371
Copy link
Contributor Author

twilight327426371 commented Jan 15, 2021

IIRC we have intentionally not used this type of filtering as the only certain way to check if pod is controlled by deamon set is to do it based on the controller ref as it is done at the end of this method common.FilterPodsByControllerRef(daemonSet, podList.Items).

@maciaszczykm do you remember that case?

In the actual production process, frequent queries of damonset pod caused the k8s api server pressure to reach 70%,
Drop to 30% after putting on the label, You will encounter performance problems if you don’t find the label when searching. If a nameapce has thousands of pods, This is not to solve accuracy.

@maciaszczykm
Copy link
Member

But it may break accuracy if the label selector will be missing on the daemon set, isn't it?

@floreks
Copy link
Member

floreks commented Jan 15, 2021

The first and most important thing that we need to ensure is to always match correct pods with correct daemon sets. The second thing is the performance. If the first point would be somehow impacted by the performance improvement then we cannot allow that.

@twilight327426371
Copy link
Contributor Author

The first and most important thing that we need to ensure is to always match correct pods with correct daemon sets. The second thing is the performance. If the first point would be somehow impacted by the performance improvement then we cannot allow that.

Label filtering during query to reduce api server pressure,common.FilterPodsByControllerRef(daemonSet, podList.Items)
alos ensure is to always match correct pods with correct daemon sets.

@twilight327426371
Copy link
Contributor Author

The first and most important thing that we need to ensure is to always match correct pods with correct daemon sets. The second thing is the performance. If the first point would be somehow impacted by the performance improvement then we cannot allow that.

Label filtering during query to reduce api server pressure,common.FilterPodsByControllerRef(daemonSet, podList.Items)
alos ensure is to always match correct pods with correct daemon sets.

企业微信截图_16109345506120

@floreks
Copy link
Member

floreks commented Jan 18, 2021

Can you guarantee with 100% certainty that filtering by selector will not filter out a pod that does not match some selector but would be matched by controller ref?

@twilight327426371
Copy link
Contributor Author

Can you guarantee with 100% certainty that filtering by selector will not filter out a pod that does not match some selector but would be matched by controller ref?

Yes, we use the daemonset selector to match to ensure that the matched pod and controller ref match the same. If there are other pods configured with the same daemonset label, the controller ref shall prevail. I have created many in the system daemonset, can be 100% guaranteed to match the corresponding POD

@floreks
Copy link
Member

floreks commented Jan 18, 2021

I have done some tests and it seems that a selector is required when creating DaemonSet and it is always propagated to the created Pods. Updating the Pod itself forces it to get "disconnected" from the daemon set and it gets replcaed by a new Pod. It should be safe then to limit the search to matched pods only.

PS. Fix the static check.

@floreks
Copy link
Member

floreks commented Jan 21, 2021

@twilight327426371 can you fix the CI so we can merge?

@twilight327426371
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link
Contributor

@twilight327426371: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@twilight327426371
Copy link
Contributor Author

@floreks Please help retest

… api server caused by daemonset related pod queries
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 22, 2021
@twilight327426371
Copy link
Contributor Author

@floreks fix static check.

@floreks
Copy link
Member

floreks commented Jan 25, 2021

/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 Jan 25, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floreks, twilight327426371

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 Jan 25, 2021
@k8s-ci-robot k8s-ci-robot merged commit 38a40e8 into kubernetes:master Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants