optimize tagging controller workqueue handling #1091
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
needs-kind
|
This issue is currently awaiting triage. If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the The 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
|
/hold give me a bit please to review this! |
k8s-ci-robot
added
the
do-not-merge/hold
|
/hold cancel Looks good to me @kmala but some questions in line. |
k8s-ci-robot
removed
the
do-not-merge/hold
| ) | ||
|
|
||
| var register sync.Once | ||
|
|
||
| var ( | ||
| workItemDuration = metrics.NewHistogramVec( |
There was a problem hiding this comment.
We should have an equivalent metric from the workqueue package itself via:
but please make sure we actually scrape that one @kmala !
There was a problem hiding this comment.
work queue metrics are available by default https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/component-base/metrics/prometheus/workqueue/metrics.go#L40 and i had verified the same the metrics endpoint of the CCM will provide the tagging controller metrics
There was a problem hiding this comment.
Yes we've had those metrics for a while because of the import above (you have to import it for these to be registered: https://github.com/kubernetes/kubernetes/blob/d06398aac3e378da0c95472cd39713998206e9ff/staging/src/k8s.io/component-base/metrics/prometheus/workqueue/metrics.go#L26)
But do we actually scrape those? I'm pretty sure we've been scraping this other metric that we're removing
There was a problem hiding this comment.
yes, these metrics are available and i had verified the same by querying the metrics endpoint.
what do you mean by scrape those?
| for _, resource := range tc.resources { | ||
| switch resource { | ||
| case opt.Instance: | ||
| err := tc.tagEc2Instance(node) | ||
| v1node, err := tc.nodeInformer.Lister().Get(node.name) |
There was a problem hiding this comment.
Do we want to try to capture all necessary information in taggingControllerNode itself to avoid this lookup?
There was a problem hiding this comment.
we need to have a lookup if we want to avoid having multiple items in workqueue since the labels can be different on different updates.
There was a problem hiding this comment.
The *Node should point to the same struct though, right? the watcher/lister should only have one copy of the Node in memory
k8s-ci-robot
added
release-note
| // if the struct has fields which are all comparable then the workqueue add will handle make sure multiple adds of the same object | ||
| // will only have one item in the workqueue. | ||
| item := workItem{ | ||
| name: node.GetName(), | ||
| providerID: node.Spec.ProviderID, | ||
| action: action, |
There was a problem hiding this comment.
The action func wasn't comparable, so this change allows multiple workitems for the same node, with different actions. Is that desirable? Why change action to a string?
There was a problem hiding this comment.
I'm thinking the same. Seems we're relying on queuing-order to determine the final state which boils down to a last-write-wins situation in the workqueue.
There was a problem hiding this comment.
so this change allows multiple workitems for the same node, with different actions. Is that desirable?
Before we are adding to workitems for every update if the add hasn't happened and there could be work item for the delete also. With this change we would only have maximum of 2 work items in the queue which should be the same behavior as before as as single or multiple add tag work items would mean the same behavior at the end.
the action to a string is made so that its comparable and will prevent duplicate updates to not add to the workqueue.
| requeuingCount int | ||
| enqueueTime time.Time |
There was a problem hiding this comment.
So these 2 fields were the crux of the issue, right? If a workItem already existed in the queue, it's enqueueTime would always be different, even if it's requeingCount happened to match (0).
There was a problem hiding this comment.
Correct me if I am wrong, the requeuingCount looks like there is a maximum retry. Are we good to remove it?
Is there a option to keep those fields and only compare the node identity fields when enqueue?
There was a problem hiding this comment.
we can get the requeue count from the workqueue itself and i changed the code to use that. So the functionality remains the same as before.
|
@ndbaker1 can you review this one? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cartermckinnon 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 |
k8s-ci-robot
added
the
approved
…stream-release-1.32 Automated cherry pick of #1091: optimize tagging controller workqueue handling
What type of PR is this?
/kind bug
What this PR does / why we need it:
The tagging controller can add the same node multiple times to the workqueue since the node is updated frequently by the kubelet if the add event for that node is not processed yet. This is because for the workqueue add we are not adding an object which is different for each event. When a large number of nodes are added at a time, the queue can become so big that it can take hours/days for all the events to be handled.
Removed the time and retry count from the workqueue object as they are provided by default by the workqueue library and made the workqueue object simple such that multiple add/updates to a node will only add one item to the workqueue.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: