Add basic structure of bloom gateways #10782

chaudum · 2023-10-03T15:54:23Z

Summary

This pull requests adds the basic structure for the new bloom gateway component.

Adds new bloom-gateway target that runs with multiple instances joined by a ring
Adds a querier and client component on the index gateway to filter chunk refs
Adds the gRPC protobuf definitions for commication between index gateways and bloom gateways
Adds a store component used on the bloom gateways to query binary bloom files


			     Querier   Query Frontend
			        |           |
			................................... service boundary
			        |           |
			        +----+------+
			             |
			     indexgateway.Gateway**
			             |
			   bloomgateway.BloomQuerier
			             |
			   bloomgateway.GatewayClient
			             |
			  logproto.BloomGatewayClient
			             |
			................................... service boundary
			             |
			      bloomgateway.Gateway
			             |
			       bloomshipper.Store
			             |
			      bloomshipper.Shipper
			             |
                        bloomshipper.BloomFileClient**
			             |
			        ObjectClient**
			             |
			................................... service boundary
			             |
		         object storage

** not part of this PR

This PR still contains a lot of TODOs and possibilities for optimisations, which will be addressed in subsequent pull requests.

pkg/bloomgateway/bloomgateway.go

pkg/storage/bloom/bloom-shipper/bloom_shipper.go

pkg/storage/stores/shipper/bloomshipper/store.go

owen-d

Left some suggestions but approving 👍

pkg/bloomgateway/client.go

pkg/bloomgateway/querier.go

owen-d · 2023-10-04T16:46:39Z

pkg/bloomgateway/ringmanager.go

+
+// NewRingManager instantiates a new RingManager instance.
+// The other functions will assume the RingManager was instantiated through this function.
+func NewRingManager(mode ManagerMode, cfg Config, logger log.Logger, registerer prometheus.Registerer) (*RingManager, error) {


nit: probably a good thing to refactor into independent code. This looks to be repeated in both the scheduler & index-gw

owen-d · 2023-10-04T16:52:14Z

pkg/bloomgateway/sharding.go

+	BloomGatewayShardSize(tenantID string) int
+}
+
+type ShardingStrategy interface {


nit: this also looks copy+pasted from the index-gw, can't we reuse?

Yes, will consolidate both the ring manager scaffolding and the sharding strategy in a separate PR after merging this one.

pkg/logproto/bloomgateway.proto

poyzannur

very minor comments/questions. LGTM.

poyzannur · 2023-10-05T13:36:41Z

pkg/bloomgateway/bloomgateway.go

+			             |
+			      bloomshipper.Shipper
+			             |
+	     bloomshipper.BloomFileClient


drawing the flow like this is a really nice practice to follow, i'll adopt it. nit tab tab, alignment on this line :)

poyzannur · 2023-10-05T15:08:44Z

pkg/bloomgateway/sharding.go

+	// Since we set the shard size to replication factor if shard size is 0, this
+	// can only happen if both the shard size and the replication factor are set
+	// to 0.
+	if shardSize <= 0 {


can shardSize be negative? why not ==

shardSize is an int. And the config would allow setting a negative value.

poyzannur · 2023-10-05T15:12:45Z

pkg/storage/stores/shipper/bloomshipper/store.go

+type ForEachBlockCallback func(bq BlockQuerier) error
+
+type ReadShipper interface {
+	ForEachBlock(ctx context.Context, tenant string, from, through time.Time, fingerprints []uint64, callback ForEachBlockCallback) error
+}
+
+type WriteShipper interface {
+}
+
+type Shipper interface {
+	ReadShipper
+	WriteShipper


Does this block also have bloom-compactor in mind? otherwise why is WriteShipper needed?

I could remove it, because it's not needed for the purposes of this PR.

vlad-diachenko

Looks awesome. just one important comment that I believe we need to address before merging it

vlad-diachenko · 2023-10-05T18:07:44Z

pkg/bloomgateway/sharding.go

+		rs, err = subRing.Get(uint32(blockRef.FromFp), BlocksOwnerSync, bufDescs, bufHosts, bufZones)
+		if err != nil {
+			return nil, err
+		}
+		// Include the block only if it belongs to this bloom gateway shard.
+		if rs.Includes(s.instanceID) {
+			filteredBlockRefs = append(filteredBlockRefs, blockRef)
+			continue
+		}
+
+		rs, err = subRing.Get(uint32(blockRef.ThroughFp), BlocksOwnerSync, bufDescs, bufHosts, bufZones)
+		if err != nil {
+			return nil, err
+		}
+		// Include the block only if it belongs to this bloom gateway shard.
+		if rs.Includes(s.instanceID) {
+			filteredBlockRefs = append(filteredBlockRefs, blockRef)
+			continue
+		}


If we include the block by two factors: 1) if FromFp belongs to this instance 2) if ThroughFp belongs to this instance, it means that two separate bloomGw might own the same block, one by FromFp another one by ThroughFp .
We should check only FromFp , it's enough to identify the owner

It is actually OK that multiple instances own the same block, if the same block span into the key ranges from both instances.

pkg/bloomgateway/bloomgateway_test.go

pkg/bloomgateway/client.go

vlad-diachenko

Looks cool

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

on top of existing shipper code The existing bloom_shipper.Shipper will be renamed to bloom_shipper.BloomClient since it is mainly responsible for file handling from/to object storage and has a lower level interface than what the bloom gateway consumes. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Provide no-op implementation of the shipper to define boundary of this pull request. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

vlad-diachenko

looks awesome, but I found some cases when isOutsideRange()does not work

vlad-diachenko · 2023-10-16T12:21:04Z

pkg/storage/stores/shipper/bloomshipper/shipper.go


-	blockRefs, err := s.getActiveBlockRefs(ctx, tenantID, startTimestamp, endTimestamp, minFingerprint, maxFingerprint)
+	level.Debug(s.logger).Log("msg", "ForEachBlock", "tenant", tenantID, "from", from, "through", through, "fingerprints", fingerprints)


it might be thousands of fingerprints, lets skip printing it in the logs

vlad-diachenko · 2023-10-16T12:37:59Z

pkg/storage/stores/shipper/bloomshipper/shipper.go

+		}
+		return i
+	}
+	return len(s)


must return len(s) - 1 otherwise it will be index out of range error

It's actually possible that the index is out of range: in case the value v is greater than the last element of s.

vlad-diachenko · 2023-10-16T12:57:06Z

pkg/storage/stores/shipper/bloomshipper/shipper.go

+	idx := getPosition[[]uint64](fingerprints, b.MinFingerprint)
+	return b.MaxFingerprint < fingerprints[idx]


this check should give a true if no one item from fingerprints []uint64 is greater or equal to b.MinFingerprint and less or equal to b.MaxFingerprint.
however, this method returns false for the block b.MinFingerprint: 3 , b.MaxFingerprint: 5 if fingerprints []uint64 is :

[2] - should be true because the block does not cover this fingerprint

[6] - should be true because the block does not cover this fingerprint

could you please add more test cases to cover this method?

These cases are actually covered in line 163-166

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

vlad-diachenko

LGTM

**What this PR does / why we need it**: The errors below were introduced during adding initial service structure for BloomGateway #10782 and BloomCompactor #10748 components. ``` docker-loki-read-3 | panic: runtime error: invalid memory address or nil pointer dereference docker-loki-read-3 | [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9e6d90] docker-loki-read-3 | docker-loki-read-3 | goroutine 1 [running]: docker-loki-read-3 | github.com/grafana/dskit/kv.createClient({_, _}, {_, _}, {{{0x22f6128, 0xe}, {{0x0, 0x0}}, 0x4a817c800, 0x0, ...}, ...}, ...) docker-loki-read-3 | /src/loki/vendor/github.com/grafana/dskit/kv/client.go:158 +0x360 docker-loki-read-3 | github.com/grafana/dskit/kv.NewClient({{0x22ec444, 0xa}, {0x22ee95d, 0xb}, {{{0x22f6128, 0xe}, {{...}}, 0x4a817c800, 0x0, 0x3ff0000000000000, ...}, ...}, ...}, ...) ``` This PR initialies new components as part of MemberlistKV. **Which issue(s) this PR fixes**: Fixes #<issue number> **Special notes for your reviewer**: Tested on local via docker workflow **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [ ] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory.

### Summary This pull requests adds the basic structure for the new bloom gateway component. - Adds new `bloom-gateway` target that runs with multiple instances joined by a ring - Adds a querier and client component on the index gateway to filter chunk refs - Adds the gRPC protobuf definitions for commication between index gateways and bloom gateways - Adds a store component used on the bloom gateways to query binary bloom files ``` Querier Query Frontend | | ................................... service boundary | | +----+------+ | indexgateway.Gateway** | bloomgateway.BloomQuerier | bloomgateway.GatewayClient | logproto.BloomGatewayClient | ................................... service boundary | bloomgateway.Gateway | bloomshipper.Store | bloomshipper.Shipper | bloomshipper.BloomFileClient** | ObjectClient** | ................................... service boundary | object storage ** not part of this PR ``` This PR still contains a lot of TODOs and possibilities for optimisations, which will be addressed in subsequent pull requests. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

**What this PR does / why we need it**: The errors below were introduced during adding initial service structure for BloomGateway grafana#10782 and BloomCompactor grafana#10748 components. ``` docker-loki-read-3 | panic: runtime error: invalid memory address or nil pointer dereference docker-loki-read-3 | [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9e6d90] docker-loki-read-3 | docker-loki-read-3 | goroutine 1 [running]: docker-loki-read-3 | github.com/grafana/dskit/kv.createClient({_, _}, {_, _}, {{{0x22f6128, 0xe}, {{0x0, 0x0}}, 0x4a817c800, 0x0, ...}, ...}, ...) docker-loki-read-3 | /src/loki/vendor/github.com/grafana/dskit/kv/client.go:158 +0x360 docker-loki-read-3 | github.com/grafana/dskit/kv.NewClient({{0x22ec444, 0xa}, {0x22ee95d, 0xb}, {{{0x22f6128, 0xe}, {{...}}, 0x4a817c800, 0x0, 0x3ff0000000000000, ...}, ...}, ...}, ...) ``` This PR initialies new components as part of MemberlistKV. **Which issue(s) this PR fixes**: Fixes #<issue number> **Special notes for your reviewer**: Tested on local via docker workflow **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [ ] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory.

pull-request-size bot added the size/XXL label Oct 3, 2023

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Oct 3, 2023

chaudum commented Oct 3, 2023

View reviewed changes

pkg/bloomgateway/bloomgateway.go Outdated Show resolved Hide resolved

pkg/storage/bloom/bloom-shipper/bloom_shipper.go Outdated Show resolved Hide resolved

pkg/storage/stores/shipper/bloomshipper/store.go Outdated Show resolved Hide resolved

chaudum force-pushed the chaudum/bloom-gateway-structure branch from f415580 to 67647f4 Compare October 4, 2023 06:08

owen-d approved these changes Oct 4, 2023

View reviewed changes

chaudum force-pushed the chaudum/bloom-gateway-structure branch from 3ba9e8e to aec3d7b Compare October 5, 2023 11:30

chaudum changed the title ~~WIP: Add basic structure of bloom gateways~~ Add basic structure of bloom gateways Oct 5, 2023

poyzannur approved these changes Oct 5, 2023

View reviewed changes

vlad-diachenko requested changes Oct 5, 2023

View reviewed changes

chaudum mentioned this pull request Oct 6, 2023

Index gateway simple mode connection pool #10783

Merged

7 tasks

chaudum marked this pull request as ready for review October 6, 2023 06:30

chaudum requested a review from a team as a code owner October 6, 2023 06:30

kavirajk reviewed Oct 6, 2023

View reviewed changes

pkg/bloomgateway/bloomgateway_test.go Outdated Show resolved Hide resolved

pkg/bloomgateway/client.go Show resolved Hide resolved

vlad-diachenko approved these changes Oct 6, 2023

View reviewed changes

chaudum added 16 commits October 16, 2023 08:36

Add basic structure for bloom gateway component

a31fd4f

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Define protos for bloom gateway communication

d292bec

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Implement skeleton for bloom gateway client and sharding

512c52b

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Integrate bloom querier into index gateway

756b7bc

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

fixup! Implement skeleton for bloom gateway client and sharding

0bd6504

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Separate interfaces for read and write

55bb2f1

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Add test cases for bloom gateway

bb9e9d3

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Fix initBloomGateway initialisation function

9cb7e65

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Add basic test for bloom gateway client

f034f7b

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Clarify interface for BloomShipper

adf1504

Provide no-op implementation of the shipper to define boundary of this pull request. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Apply fmt-proto

2c07db8

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Add tests for bloom querier

3520f3d

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Remove unused code, fix linter warnings

fd83233

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

fixup! Remove unused code, fix linter warnings

4cf21ee

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Make encoding of FilterChunkRefsResponse more efficient

5b9958e

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

chaudum added 8 commits October 16, 2023 08:36

Fix sort order of fingerprints in bloom gateway querier

c92f117

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Add note regarding algorithm for grouping fingerprints by servers

79f4b74

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Add todo in bloom gateway querier

20cb111

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Fix import order

63e43e1

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

fixup! Fix import order

cef0679

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Remove unused WriteShipper interface

5889b00

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Expose client pool configuration correctly

e58e147

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Address review comments

796154a

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

chaudum force-pushed the chaudum/bloom-gateway-structure branch from 6daeb41 to 796154a Compare October 16, 2023 06:40

chaudum enabled auto-merge (squash) October 16, 2023 06:48

chaudum disabled auto-merge October 16, 2023 07:47

chaudum added 4 commits October 16, 2023 13:43

Adjust shipper API to accept list of fingerprints

e5de0fd

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Fix linter warning (stutter)

1489b64

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

fixup! Fix linter warning (stutter)

8a7e8cf

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Correctly pass bloom shipper config

26ad894

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

vlad-diachenko reviewed Oct 16, 2023

View reviewed changes

Add some more tests for helper function

6e18ed5

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

vlad-diachenko approved these changes Oct 16, 2023

View reviewed changes

chaudum enabled auto-merge (squash) October 16, 2023 13:51

chaudum merged commit b49b3ce into main Oct 16, 2023
3 checks passed

chaudum deleted the chaudum/bloom-gateway-structure branch October 16, 2023 14:00

poyzannur mentioned this pull request Oct 24, 2023

Fix ring start up errors #11015

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic structure of bloom gateways #10782

Add basic structure of bloom gateways #10782

chaudum commented Oct 3, 2023 •

edited

Loading

owen-d left a comment

owen-d Oct 4, 2023

owen-d Oct 4, 2023

chaudum Oct 5, 2023

poyzannur left a comment

poyzannur Oct 5, 2023

poyzannur Oct 5, 2023

chaudum Oct 6, 2023

poyzannur Oct 5, 2023

chaudum Oct 6, 2023

vlad-diachenko left a comment

vlad-diachenko Oct 5, 2023

chaudum Oct 6, 2023

vlad-diachenko left a comment

vlad-diachenko left a comment

vlad-diachenko Oct 16, 2023

vlad-diachenko Oct 16, 2023

chaudum Oct 16, 2023

vlad-diachenko Oct 16, 2023

chaudum Oct 16, 2023

vlad-diachenko left a comment


		blockRefs, err := s.getActiveBlockRefs(ctx, tenantID, startTimestamp, endTimestamp, minFingerprint, maxFingerprint)
		level.Debug(s.logger).Log("msg", "ForEachBlock", "tenant", tenantID, "from", from, "through", through, "fingerprints", fingerprints)

		idx := getPosition[[]uint64](fingerprints, b.MinFingerprint)
		return b.MaxFingerprint < fingerprints[idx]

Add basic structure of bloom gateways #10782

Add basic structure of bloom gateways #10782

Conversation

chaudum commented Oct 3, 2023 • edited Loading

Summary

owen-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poyzannur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-diachenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-diachenko left a comment

Choose a reason for hiding this comment

vlad-diachenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-diachenko left a comment

Choose a reason for hiding this comment

chaudum commented Oct 3, 2023 •

edited

Loading