-
Notifications
You must be signed in to change notification settings - Fork 614
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
Add parameters to support change work queue #841
Add parameters to support change work queue #841
Conversation
tests seem unhappy since #834 merged |
node/podcontroller.go
Outdated
@@ -198,8 +217,9 @@ func NewPodController(cfg PodControllerConfig) (*PodController, error) { | |||
ready: make(chan struct{}), | |||
done: make(chan struct{}), | |||
recorder: cfg.EventRecorder, | |||
k8sQ: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "syncPodsFromKubernetes"), | |||
deletionQ: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "deletePodsFromKubernetes"), | |||
k8sQ: workqueue.NewNamedRateLimitingQueue(controllerRateLimiter(cfg.WorkQueueRetryQPS), "syncPodsFromKubernetes"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were the previous defaults 10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, it is defined 10 in the original client-go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, what do you recommend?
In our environment, we have changed it to 100 now to accelerate deleting pods.
tests should be fixed by #842 -- the problem is that K8s just release 1.17.7. This broke the validation script. |
109fa8a
to
b534925
Compare
btw, what do you recommend? |
Before this works, we should wait for the PR virtual-kubelet/node-cli#22 in node-cli to be merged |
@cwdsuzhou I'll hold off on the merge. But, this to me looks like it should work, and fallback to 10. |
Currently we do not expose anything about the queue, or even that we use a queue to the package API. This changes that. |
I wonder if it would be better to expose the queue to the config instead of the queue config. |
yes, this should work |
This is better. But I prefer to exposing the |
d7e3d4a
to
8df7338
Compare
8df7338
to
b8ac627
Compare
b8ac627
to
ca417d5
Compare
I will close virtual-kubelet/node-cli#22 and re-open a new one when this PR has been merged. |
@cpuguy83 Are you okay with this, or do you want to take a different approach of queue / queue configuration / queue configuration policy? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good. LGTM
Fix #840