Batch tagging controller create/delete tags calls #1120

shiv-amz · 2025-03-21T22:54:48Z

/kind feature

What this PR does / why we need it:

Tagging controller tags/untags a single instance at a time. Added createTag/deleteTag batcher which can batche multiple api calls into a single call with multiple instances.
Batching is behind option tagging-controller-enable-batching flag.

Special notes for your reviewer:
We have a similar batching for other EC2 calls in Karpenter

Sample logs, tagging multiple instances. 
createTags.go:61] Batched create tags {
  Resources: [
    "i-09c32e7b40efd2a37",
    "i-0b8387fdf8ffb4e21",
    "i-035ccfa88750fc37e",
    "i-0f1f9fd038b4a8953",
    "i-0c97fbe1fb9ad5823"
  ],
  Tags: [{
      Key: "k8s.io/cloud-provider-aws",
      Value: "test-cluster"
    }]
}

deleteTags.go:76] Batched delete tags {
  Resources: [
    "i-01288200c4829f248",
    "i-0dfc2e29029a8d5ce",
    "i-026eb630a8ad49f7b",
    "i-0b4e6e3ef3612c4f5"
  ],
  Tags: [{
      Key: "k8s.io/cloud-provider-aws"
      Value: "test-cluster"
    }]
}

Does this PR introduce a user-facing change?:

Improve tagging controller performance by batching tag creation and deletion API calls. Batching can be enabled using the `--tagging-controller-enable-batching` flag.

k8s-ci-robot · 2025-03-21T22:54:55Z

This issue is currently awaiting triage.

If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 · 2025-03-21T22:54:57Z

Hi @shiv-amz. 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.

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.

cartermckinnon · 2025-03-25T17:18:13Z

pkg/controllers/options/tagging_controller.go

+	Resources      []string
+	RateLimit      float64
+	BurstLimit     int
+	EnableBatching bool


What's the rationale for making this configurable? If it's useful, a BatchSize config option seems more flexible for tuning.

Yes, we don't need the enable batching flag. I added this for my load testing, to easily compare results between batching versus single calls. I'll remove this in next rev.

cartermckinnon · 2025-03-25T17:19:12Z

pkg/controllers/tagging/tagging_controller.go

@@ -64,6 +65,7 @@ const (
 	taggingControllerLabelKey = "k8s.io/cloud-provider-aws"

 	maxRequeuingCount = 9
+	workerQueueCount  = 10


This seems like a useful config option, similar to --concurrent-node-syncs, --concurrent-service-syncs, etc.

cartermckinnon · 2025-03-25T17:20:59Z

pkg/controllers/tagging/tagging_controller.go

+		for i := int32(0); i < workerQueueCount; i++ {
+			go wait.Until(tc.work, tc.nodeMonitorPeriod, stopCh)
+		}


Why is parallelism of API calls gated by the batching-per-call option? these seem like 2 different concerns

They are related since, with single concurrent thread, batching api calls does not work. If all calls are made by single thread, they won't ever get batched. So to enable batching, parallel threads consuming from workqueue is required.

As pointed above, will make workerQueueCount as configurable, and remove the batching flag.

cartermckinnon · 2025-03-25T17:27:44Z

pkg/providers/v1/batcher/batcher.go

There's a lot going on here :)

Is there a simpler way we could implement this? Could a goroutine in the tagging controller poll multiple items off the queue (if they're available) and fire off the request? In Karpenter, it makes sense why we'd need a decoupled "batcher" because controller-runtime doesn't expose the underlying work queue to Reconcile, but we have some more flexibility in this controller

cartermckinnon · 2025-03-25T19:07:01Z

Chatted offline -- we'll split this up into 2 PR's, one that adds a simple parallelism mechanism (with a worker pool), and we'll keep iterating on the batching approach.

cartermckinnon · 2025-04-18T17:19:35Z

/ok-to-test

shiv-amz · 2025-04-18T17:45:24Z

/ok-to-test

pkg/providers/v1/batcher/createTags.go

pkg/providers/v1/batcher/batch_test.go

go.mod

pkg/providers/v1/createTagsBatch.go

pkg/providers/v1/create_tags_batch.go

pkg/providers/v1/delete_tags_batch.go

pkg/providers/v1/create_tags_batch.go

pkg/providers/v1/delete_tags_batch.go

cartermckinnon · 2025-04-22T19:01:35Z

/lgtm
/approve

@shiv-amz can you remove the usage of controller-runtime's logging framework in a followup? We should use the same logger used in the rest of the project

k8s-ci-robot · 2025-04-22T19:01:44Z

[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:

~~OWNERS~~ [cartermckinnon]

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

…1120-#1126-upstream-release-1.32 Automated cherry pick of #1116: feat: add tagging controller delays and work queue size #1120: Batch tagging controller create/delete tags calls #1126: Enable concurrent worker syncs for tagging controller

…1120-upstream-release-1.33 Automated cherry pick of #1116: feat: add tagging controller delays and work queue size #1120: Batch tagging controller create/delete tags calls

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 21, 2025

k8s-ci-robot requested review from dims and olemarkus March 21, 2025 22:54

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 21, 2025

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 21, 2025

shiv-amz marked this pull request as draft March 21, 2025 22:58

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 21, 2025

shiv-amz marked this pull request as ready for review March 22, 2025 00:55

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 22, 2025

cartermckinnon reviewed Mar 25, 2025

View reviewed changes

shiv-amz added 3 commits April 17, 2025 19:53

Batch tagging controller create/delete tags calls

65b83e3

Add delete tags logging

0a806e0

Remove enable batching flag

8fe956c

shiv-amz force-pushed the batcher branch from fb0ef6f to 8fe956c Compare April 18, 2025 03:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 18, 2025

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 18, 2025

Add comments for exported methods

a9e9b3d

Fix linter errors

6fe20ac

shiv-amz force-pushed the batcher branch from 4fe2488 to 6fe20ac Compare April 18, 2025 18:03

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 18, 2025

k8s-ci-robot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Apr 18, 2025

cartermckinnon reviewed Apr 18, 2025

View reviewed changes

pkg/providers/v1/batcher/createTags.go Outdated Show resolved Hide resolved

cartermckinnon reviewed Apr 18, 2025

View reviewed changes

pkg/providers/v1/batcher/batch_test.go Outdated Show resolved Hide resolved

cartermckinnon reviewed Apr 18, 2025

View reviewed changes

pkg/providers/v1/batcher/batch_test.go Show resolved Hide resolved

shiv-amz force-pushed the batcher branch from 4fe11f0 to bd269e2 Compare April 18, 2025 22:35

cartermckinnon reviewed Apr 18, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

pkg/providers/v1/createTagsBatch.go Outdated Show resolved Hide resolved

shiv-amz force-pushed the batcher branch 2 times, most recently from 82c48ea to c3da800 Compare April 18, 2025 23:05

cartermckinnon reviewed Apr 21, 2025

View reviewed changes

pkg/providers/v1/create_tags_batch.go Outdated Show resolved Hide resolved

pkg/providers/v1/delete_tags_batch.go Outdated Show resolved Hide resolved

shiv-amz force-pushed the batcher branch from c3da800 to aa593b8 Compare April 21, 2025 20:46

cartermckinnon reviewed Apr 22, 2025

View reviewed changes

pkg/providers/v1/create_tags_batch.go Outdated Show resolved Hide resolved

pkg/providers/v1/delete_tags_batch.go Outdated Show resolved Hide resolved

Address comments

0059675

shiv-amz force-pushed the batcher branch from aa593b8 to 0059675 Compare April 22, 2025 16:02

k8s-ci-robot assigned cartermckinnon Apr 22, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2025

k8s-ci-robot merged commit 8ab6997 into kubernetes:master Apr 22, 2025
11 checks passed

shvbsle mentioned this pull request Apr 28, 2025

Automated cherry pick of #1116: feat: add tagging controller delays and work queue size #1120: Batch tagging controller create/delete tags calls #1140

Merged

Batch tagging controller create/delete tags calls #1120

Batch tagging controller create/delete tags calls #1120

Uh oh!

Conversation

shiv-amz commented Mar 21, 2025 • edited by cartermckinnon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 21, 2025

Uh oh!

k8s-ci-robot commented Mar 21, 2025

Uh oh!

cartermckinnon Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

shiv-amz Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

cartermckinnon Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

cartermckinnon Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

shiv-amz Mar 25, 2025 • 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!

cartermckinnon Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

cartermckinnon commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cartermckinnon commented Apr 18, 2025

Uh oh!

shiv-amz commented Apr 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cartermckinnon commented Apr 22, 2025

Uh oh!

k8s-ci-robot commented Apr 22, 2025

Uh oh!

Uh oh!

Uh oh!

shiv-amz commented Mar 21, 2025 •

edited by cartermckinnon

Loading

shiv-amz Mar 25, 2025 •

edited

Loading

cartermckinnon commented Mar 25, 2025 •

edited

Loading