[Issue #325] Added retry count to abort workitem after a few failed retries by nguyenkndinh · Pull Request #334 · kubernetes/cloud-provider-aws

nguyenkndinh · 2022-04-05T23:48:20Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Added a logic to abort work item after a certain number of retries in case EC2 responds with non-retryable errors.
Added metrics collection to the controller.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
NONE

NONE

k8s-ci-robot · 2022-04-05T23:48:28Z

Hi @nguyenkndinh. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

saurav-agarwalla · 2022-04-06T22:37:52Z

 }

+const (
+	maxRequeuingCount = 10


Do we know what this translates into in terms of retry duration?

We are using the DefaultControllerRateLimiter, which uses the worst case of the 2 limiters:

ItemExponentialFailureRateLimiter, with a base delay of 5 milliseconds and max delay of 1000 seconds

BucketRateLimiter, with a limit of 10, and 100 burst.

If not counting BucketRateLimiter cause that's a different beast, the work items would have to wait anywhere from 50 milliseconds to 10000 seconds in retry time (excluding work time and queue wait time as they can differ). However, the real avg number is a bit more complicated depending on the number of items in the queue, if the item has been enqueued many times, which rate limiter was used. So, When to add an item back in the queue depends on either BucketRateLimiter or ItemExponentialFailureRateLimiter is selected as worst case. However, I wouldn't expect the worst case of 10000 to ever happens, though we need metrics data to fine tune this as it goes I assume.

nckturner · 2022-04-09T00:11:07Z

Can you squash your commits

nckturner · 2022-04-09T00:12:11Z

/ok-to-test

nguyenkndinh · 2022-04-09T00:17:45Z

/hold

saurav-agarwalla · 2022-05-03T20:01:09Z

+func recordWorkitemMetrics(actionName string, timeTaken float64, err error) {
+	if err != nil {
+		workItemError.With(metrics.Labels{"workqueue": actionName}).Inc()
+	}
+
+	workItemLatency.With(metrics.Labels{"workqueue": actionName}).Observe(timeTaken)
+}


Should we break this down into separate functions for count and latency metrics? Seems like at least for error counts, we're going to have zero latency all the time which doesn't seem desirable?

saurav-agarwalla · 2022-05-03T20:08:42Z

+
+func recordWorkitemMetrics(actionName string, timeTaken float64, err error) {
+	if err != nil {
+		workItemError.With(metrics.Labels{"workqueue": actionName}).Inc()


Have a question around what the dimensions for this metric is going to look like - are we able to query this metric using actionName as well? Because we seem to be passing strings with node names wherever we're calling this function so need to understand if that makes sense or if we need actionName to be somewhat static.

That's a great call out, as I'm based this change off of aws_metrics.go, I assume the dimensions for this are similar to this. I'll check in CW and see how they are being produced. However, I think you're right as the usage in aws.go is also somewhat static.

saurav-agarwalla · 2022-05-04T21:54:27Z

+var (
+	workItemLatency = metrics.NewHistogramVec(
+		&metrics.HistogramOpts{
+			Name:           "tagging_workitem_latency",


Have been looking at https://pkg.go.dev/github.com/prometheus/client_golang/prometheus for more info around how Prometheus handles these metrics and it looks like this is the metric name and the labels applied in the below functions are additional dimensions.

Keeping that in mind, can we rename these to cloudprovider_tagging_controller_work_item_latency and cloudprovider_tagging_controller_work_item_error instead to be more specific and to follow a similar pattern as the other metrics in this package?

saurav-agarwalla · 2022-05-04T22:00:22Z

+			}
+
+			klog.Errorf("error processing work item '%v': %s, requeuing count exceeded", workItem, err.Error())
+			recordWorkItemErrorMetrics("process_work_item")


I think we may want to track both total errors and errors after all retries are exhausted. Can you please emit two labels for this: total_errors and errors_after_retries_exhausted? You can emit total_errors inside the if above and errors_after_retries_exhausted outside it.

do you mean to count the number of total errors inside the if with a counter and the number of errors outside the if?

I think I understand it now but pls correct me if I was wrong, it is simply to add recordWorkItemErrorMetrics(TotalErrors) in the if and recordWorkItemErrorMetrics(ErrorsAfterRetriesExhausted) outside the if so that we can aggregate them later.

saurav-agarwalla · 2022-05-04T22:01:10Z

+}
+
+func recordWorkItemErrorMetrics(actionName string) {
+	workItemError.With(metrics.Labels{"workqueue": actionName}).Inc()


Can we change the label name to error_type instead and change the param name accordingly?

saurav-agarwalla · 2022-05-04T22:01:27Z

+}
+
+func recordWorkItemLatencyMetrics(actionName string, timeTaken float64) {
+	workItemLatency.With(metrics.Labels{"workqueue": actionName}).Observe(timeTaken)


Same for this. Can we change this to latency_type instead and change the param name accordingly?

saurav-agarwalla · 2022-05-04T22:02:30Z

+		} else {
+			klog.Infof("Finished processing %v", workItem)
+			timeTaken = time.Since(workItem.enqueueTime).Seconds()
+			recordWorkItemLatencyMetrics("process_work_item", timeTaken)


Can we change this to work_item_processing_time?

saurav-agarwalla · 2022-05-04T22:04:00Z

 		}

+		timeTaken := time.Since(workItem.enqueueTime).Seconds()
+		recordWorkItemLatencyMetrics("dequeue_work_item", timeTaken)


Can we change this to work_item_dequeuing_time?

saurav-agarwalla · 2022-05-04T22:04:30Z

-			utilruntime.HandleError(fmt.Errorf("expected workItem in workqueue but got %#v", obj))
+			err := fmt.Errorf("expected workItem in workqueue but got %#v", obj)
+			utilruntime.HandleError(err)
+			recordWorkItemErrorMetrics("dequeue_work_item")


I don't think we need to emit a latency metric here since this means that the workitem is invalid, right?

saurav-agarwalla · 2022-05-04T22:09:37Z

+	workItemLatency.With(metrics.Labels{"workqueue": actionName}).Observe(timeTaken)
+}
+
+func recordWorkItemErrorMetrics(actionName string) {


Can we add a label for instance_id in this function and take in the appropriate params? Might help to see if there are certain instances running into errors.

saurav-agarwalla

Looks good to me.

nckturner · 2022-05-06T17:48:50Z

+var (
+	workItemDuration = metrics.NewHistogramVec(
+		&metrics.HistogramOpts{
+			Name:           "cloudprovider_tagging_controller_work_item_duration_seconds",


One more thing for this metric name, to match the other metrics already present in aws_metrics.go we should start with the prefix: cloudprovider_aws so the metric should be cloudprovider_aws_tagging_controller_work_item_duration_seconds. Then other controllers can follow the same pattern of cloudprovider_aws_controller_name_metric_name.

nckturner · 2022-05-06T17:53:20Z

+
+	workItemError = metrics.NewCounterVec(
+		&metrics.CounterOpts{
+			Name:           "cloudprovider_tagging_controller_work_item_errors_total",


Suggested change

Name: "cloudprovider_tagging_controller_work_item_errors_total",

Name: "cloudprovider_aws_tagging_controller_work_item_errors_total",

nckturner · 2022-05-06T17:57:44Z


+const (
+	maxRequeuingCount           = 9
+	totalErrors                 = "total_errors"


Since some of these are metrics labels, I think it makes more sense to define those with the metrics they will be used with. And when you do that you can write a comment as to what it can be used for when querying.

nckturner · 2022-05-06T18:47:39Z

/lgtm
/approve

Added metrics

nguyenkndinh · 2022-05-06T19:02:04Z

/unhold

nckturner · 2022-05-06T19:07:41Z

/kind feature
/triage accepted

nckturner · 2022-05-06T19:08:02Z

/lgtm
/unhold

k8s-ci-robot · 2022-05-06T19:08:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nckturner, nguyenkndinh, saurav-agarwalla

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [nckturner]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…f-#308-#334-#387-upstream-release-1.23 Automated cherry pick of #308: Add tagging controller configuration #334: Stop retrying failed workitem after a certain amount of #387: Fix issues in tagging controller

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 5, 2022

k8s-ci-robot requested review from M00nF1sh and jaypipes April 5, 2022 23:48

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 5, 2022

saurav-agarwalla reviewed Apr 6, 2022

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 7, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 9, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2022

nguyenkndinh force-pushed the master branch 2 times, most recently from 6d70a64 to 030a5b6 Compare April 9, 2022 00:27

saurav-agarwalla reviewed May 3, 2022

View reviewed changes

saurav-agarwalla reviewed May 4, 2022

View reviewed changes

saurav-agarwalla approved these changes May 5, 2022

View reviewed changes

nckturner reviewed May 5, 2022

View reviewed changes

Comment thread pkg/controllers/tagging/metrics.go

saurav-agarwalla approved these changes May 5, 2022

View reviewed changes

nckturner reviewed May 6, 2022

View reviewed changes

Comment thread pkg/controllers/tagging/metrics.go Outdated

Comment thread pkg/controllers/tagging/tagging_controller.go

Comment thread pkg/controllers/tagging/tagging_controller.go Outdated

nguyenkndinh force-pushed the master branch from 46abef3 to d377bd5 Compare May 6, 2022 16:21

nguyenkndinh force-pushed the master branch from d377bd5 to 8462e2d Compare May 6, 2022 16:22

nckturner reviewed May 6, 2022

View reviewed changes

nckturner approved these changes May 6, 2022

View reviewed changes

k8s-ci-robot assigned nckturner May 6, 2022

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 6, 2022

Stop retrying failed workitem after a certain amount of retries

e7f9a06

Added metrics

nguyenkndinh force-pushed the master branch from 37e6008 to e7f9a06 Compare May 6, 2022 19:01

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2022

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 6, 2022

k8s-ci-robot merged commit 0780829 into kubernetes:master May 6, 2022

saurav-agarwalla mentioned this pull request May 26, 2022

Automated cherry pick of #308: Add tagging controller configuration #334: Stop retrying failed workitem after a certain amount of #387: Fix issues in tagging controller #393

Merged

	Name: "cloudprovider_tagging_controller_work_item_errors_total",
	Name: "cloudprovider_aws_tagging_controller_work_item_errors_total",

Conversation

nguyenkndinh commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguyenkndinh Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nckturner commented Apr 9, 2022

Uh oh!

nckturner commented Apr 9, 2022

Uh oh!

nguyenkndinh commented Apr 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguyenkndinh May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saurav-agarwalla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nckturner commented May 6, 2022

Uh oh!

nguyenkndinh commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nckturner commented May 6, 2022

Uh oh!

nckturner commented May 6, 2022

Uh oh!

k8s-ci-robot commented May 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

nguyenkndinh commented Apr 5, 2022 •

edited

Loading

nguyenkndinh Apr 7, 2022 •

edited

Loading

nguyenkndinh May 4, 2022 •

edited

Loading

nguyenkndinh commented May 6, 2022 •

edited

Loading