Skip to content

Distributor validation #5250

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Distributor validation #5250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
90a3c19
Fix validation error
dogukanteber Apr 3, 2023
7783d4e
Update the tests
dogukanteber Apr 5, 2023
a6aa085
Update CHANGELOG.md
dogukanteber Apr 5, 2023
f2ec611
Resolve merge conflict
dogukanteber May 30, 2023
6c2e50e
Fix Changelog
dogukanteber May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
* [ENHANCEMENT] Log: Avoid expensive log.Valuer evaluation for disallowed levels. #5297
* [ENHANCEMENT] Improving Performance on the API Gzip Handler. #5347
* [BUGFIX] Ruler: Validate if rule group can be safely converted back to rule group yaml from protobuf message #5265
* [BUGFIX] Querier: Convert gRPC `ResourceExhausted` status code from store gateway to 422 limit error. #5286
Copy link
Member

Choose a reason for hiding this comment

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

we need a separate PR for this. don't fix stuff that is not related to the PR

* [BUGFIX] Alertmanager: Route web-ui requests to the alertmanager distributor when sharding is enabled. #5293
* [BUGFIX] Storage: Bucket index updater should ignore meta not found for partial blocks. #5343
* [BUGFIX] Ring: Add JOINING state to read operation. #5346
* [BUGFIX] Compactor: Partial block with only visit marker should be deleted even there is no deletion marker. #5342
* [BUGFIX] Disable shuffle sharding and fix the error when `-distributor.ingestion-tenant-shard-size` is set to 0. #5189

## 1.15.1 2023-04-26

Expand Down
7 changes: 4 additions & 3 deletions pkg/distributor/distributor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ var (

// Validation errors.
errInvalidShardingStrategy = errors.New("invalid sharding strategy")
errInvalidTenantShardSize = errors.New("invalid tenant shard size, the value must be greater than 0")

// Distributor instance limits errors.
errTooManyInflightPushRequests = errors.New("too many inflight push requests in distributor")
Expand Down Expand Up @@ -176,8 +175,10 @@ func (cfg *Config) Validate(limits validation.Limits) error {
return errInvalidShardingStrategy
}

if cfg.ShardingStrategy == util.ShardingStrategyShuffle && limits.IngestionTenantShardSize <= 0 {
return errInvalidTenantShardSize
if limits.IngestionTenantShardSize <= 0 {
Copy link
Member

@friedrichg friedrichg Apr 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There could be an scenario where:

  1. Shuffle sharding is enabled for all tenants
  2. Ingestion tenant shard is zero for all tenants
  3. There is one specific tenant that has tenant shard bigger than zero

    cortex/pkg/util/validation/limits.go

    Line 56 in 2f436e8

    IngestionTenantShardSize int `yaml:"ingestion_tenant_shard_size" json:"ingestion_tenant_shard_size"`
  4. Shuffle sharding should be definitely enabled for that tenant.

Is that supported with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it does not. Let me check and make necessary changes

Copy link
Member

Choose a reason for hiding this comment

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

Let's make sure it is still supported

Copy link
Contributor Author

@dogukanteber dogukanteber May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I checked the case you mentioned @friedrichg and made sure it is still supported with this change.

Copy link
Member

Choose a reason for hiding this comment

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

We make sure things are supported adding tests. integration or unit test

cfg.ShardingStrategy = "default"
msg := fmt.Sprintf("ingestion_tenant_shard_size is given %d. Suffle sharding is disabled", limits.IngestionTenantShardSize)
level.Info(util_log.Logger).Log("msg", msg)
}

haHATrackerConfig := cfg.HATrackerConfig.ToHATrackerConfig()
Expand Down
4 changes: 2 additions & 2 deletions pkg/distributor/distributor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,14 @@ func TestConfig_Validate(t *testing.T) {
initLimits: func(_ *validation.Limits) {},
expected: errInvalidShardingStrategy,
},
"should fail if the default shard size is 0 on when sharding strategy = shuffle-sharding": {
"should pass if the default shard size is 0 on when sharding strategy = shuffle-sharding": {
initConfig: func(cfg *Config) {
cfg.ShardingStrategy = "shuffle-sharding"
},
initLimits: func(limits *validation.Limits) {
limits.IngestionTenantShardSize = 0
},
expected: errInvalidTenantShardSize,
expected: nil,
},
"should pass if the default shard size > 0 on when sharding strategy = shuffle-sharding": {
initConfig: func(cfg *Config) {
Expand Down