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

[controller] Only list objects created by operator #222

Merged
merged 4 commits into from
Jun 9, 2020

Conversation

schallert
Copy link
Collaborator

Continuously listing every pod and statefulset in large clusters can
slow down the operator control loops significantly. We know every pod
and statefulset created by an M3DBCluster will have predetermined
labels, and can only list objects from the Kubernetes API with those
labels.

Continuously listing every pod and statefulset in large clusters can
slow down the operator control loops significantly. We know every pod
and statefulset created by an M3DBCluster will have predetermined
labels, and can only list objects from the Kubernetes API with those
labels.
@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #222 into master will increase coverage by 0.02%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
+ Coverage   75.50%   75.53%   +0.02%     
==========================================
  Files          30       30              
  Lines        2111     2113       +2     
==========================================
+ Hits         1594     1596       +2     
  Misses        391      391              
  Partials      126      126              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cac232c...cad0403. Read the comment docs.

@schallert schallert marked this pull request as ready for review June 9, 2020 17:58
@@ -53,7 +57,7 @@ import (

const (
// Informers will resync on this interval
_informerSyncDuration = time.Minute
_informerSyncDuration = 5 * time.Minute
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a fairly large increase, is it better to take a half measure to 2.5mins or something perhaps?

How often would we see a lost delivery do we think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's pretty rare for us to see a lost delivery. We originally had this at a higher value but reduced it to 1min because we thought we were missing events, but it turns out we were overwhelming the controller.

For reference, the default value in kubebuilder is something like 6h and I've never seen issues with code using that.

filteredInformerFactory kubeinformers.SharedInformerFactory
// The kube informer factory is still required to list nodes, as they don't
// have our label.
kubeInformerFactory kubeinformers.SharedInformerFactory
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a shared informer factory for the node lister?

I checked where nodeLister is used and it's only with .Get(..) (i.e. lookup a specific host) so would it be possible to just use the kube client for that rather than an informer factory which can be affected by the same issue we're solving here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we use the informer then we'll be notified of updates, and our Get(...) calls will just read from the cache if it's fresh. If we were to use the client directly it would put slightly more load on the API server.

Copy link
Collaborator

@robskillington robskillington 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 comments made without a whole lot of knowledge, so feel free to ignore and merge if they look like benign comments

@schallert schallert merged commit 53a05ca into master Jun 9, 2020
@schallert schallert deleted the schallert/fix_annotations branch June 9, 2020 19:30
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.

2 participants