Skip to content

Conversation

SungJin1212
Copy link
Member

This PR introduces a user scanner to reduce the number of list calls to object storage.

Which issue(s) this PR fixes:
Fixes #6914

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@dosubot dosubot bot added component/alertmanager component/rules Bits & bobs todo with rules and alerts: the ruler, config service etc. labels Aug 29, 2025
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

I wonder if we should extract user scanner into a dedicated package? pkg/storage/tsdb might not be the best place.

@SungJin1212
Copy link
Member Author

SungJin1212 commented Aug 30, 2025

@yeya24
Yeah, agree. What about /pkg/usersscanner?

@yeya24
Copy link
Contributor

yeya24 commented Aug 30, 2025

Yeah, agree. What about /pkg/usersscanner?

I am thinking about /pkg/util/users which includes everything user related, not just user scanner.

@SungJin1212
Copy link
Member Author

SungJin1212 commented Aug 30, 2025

I am thinking about /pkg/util/users which includes everything user related, not just user scanner.

ok, it looks good.
I moved user-related files to /pkg/util/users, and relocated some test files to prevent cycle imports.
I renamed some files:
users_scanner_config.go -> scanner_config.go
updater.go -> index_updater.go

@SungJin1212 SungJin1212 force-pushed the Add-user-scanner-for-am-and-ruler branch 3 times, most recently from e8c616c to 04acdd5 Compare September 1, 2025 06:31
@SungJin1212 SungJin1212 marked this pull request as draft September 1, 2025 06:38
@SungJin1212 SungJin1212 force-pushed the Add-user-scanner-for-am-and-ruler branch 2 times, most recently from ee1808c to 15f2ccd Compare September 1, 2025 06:51
@danielblando
Copy link
Contributor

What is the difference for us between user and tenant? It is a bit weird to have /pkg/util/users/tenant/.

@SungJin1212
Copy link
Member Author

SungJin1212 commented Sep 3, 2025

@danielblando
I think tenant should be integrated into users, but I've left it like this for now due to a cyclic import. I'll look into a way to integrate it.

@SungJin1212 SungJin1212 force-pushed the Add-user-scanner-for-am-and-ruler branch from b0e778d to 22768c9 Compare September 3, 2025 11:17
@SungJin1212
Copy link
Member Author

@danielblando @yeya24
I've moved the tenant package to the users package. I also deprecated util.StringsContain and replaced it with slices.Contains to resolve a cyclic import.

@SungJin1212 SungJin1212 marked this pull request as ready for review September 4, 2025 01:25
@SungJin1212 SungJin1212 force-pushed the Add-user-scanner-for-am-and-ruler branch 2 times, most recently from fbfa1dd to 2953348 Compare September 10, 2025 01:59
}
}

func IsOneOfTheExpectedErrors(f ...objstore.IsOpFailureExpectedFunc) objstore.IsOpFailureExpectedFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this function is not part of the bucket package but in a generate pkg/util/errors? This seems specific to object store failures so I am not sure if it should be in the generic error file

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved IsOneOfTheExpectedErrors to the bucket package. It causes a cyclic import if it is in the tsdb package.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think of using only the list strategy and only exposing cache_ttl as config?

func NewBucketAlertStore(bkt objstore.InstrumentedBucket, userScannerCfg users.UsersScannerConfig, cfgProvider bucket.TenantConfigProvider, logger log.Logger, reg prometheus.Registerer) (*BucketAlertStore, error) {
alertBucket := bucket.NewPrefixedBucketClient(bkt, alertsPrefix)

usersScanner, err := users.NewScanner(userScannerCfg, alertBucket, logger, extprom.WrapRegistererWith(prometheus.Labels{"component": "alertmanager"}, reg))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we support user index based user scanner? Who will work as the user index updater for AM and Ruler bucket?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, the user index updater would also be necessary for Alertmanager and Ruler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, for compactor, we use Ring to choose one compactor pod to periodically update the user index in the storage bucket. I think we need the same thing for Ruler/AM as they are using different S3 buckets

Copy link
Member Author

Choose a reason for hiding this comment

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

How about adding clean_up_interval for the user index updater? Currently, it uses clean_up_interval in the Compactor.

cortex_testutil "github.com/cortexproject/cortex/pkg/storage/tsdb/testutil"
"github.com/cortexproject/cortex/pkg/util/services"
"github.com/cortexproject/cortex/pkg/util/test"
cotex_testutil "github.com/cortexproject/cortex/pkg/util/testutil"
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a typo.

@SungJin1212 SungJin1212 force-pushed the Add-user-scanner-for-am-and-ruler branch 3 times, most recently from f262ab1 to fd54015 Compare September 15, 2025 10:59
@SungJin1212
Copy link
Member Author

@yeya24
I introduced a flag clean_up_interval for the user index updater and added updaters to Alertmanager and Ruler.

@SungJin1212 SungJin1212 requested a review from yeya24 September 17, 2025 03:55
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
@SungJin1212 SungJin1212 force-pushed the Add-user-scanner-for-am-and-ruler branch from fc2f24c to 271165b Compare September 30, 2025 05:13
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/alertmanager component/rules Bits & bobs todo with rules and alerts: the ruler, config service etc. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support User Scanner in Rule and Alerts Bucket Store
3 participants